Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

download / registry requests silently fail #1104

Merged
merged 2 commits into from
Jul 10, 2017
Merged

Conversation

MartinNowak
Copy link
Member

@MartinNowak MartinNowak commented Mar 29, 2017

Took me an extra hour of debugging why our builds fails ;).
At least for search the exception that libcurl couldn't be loaded is silently ignored,

return null;
. But I'm also seeing Root package assert_writeln_magic references unknown package libdparse with no further indication of the underlying problem.

Based on v1.1.0, but still seems to be present.

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Mar 29, 2017
- making errors like 'unknown package' actually understandable
@MartinNowak MartinNowak changed the base branch from master to stable March 29, 2017 10:35
MartinNowak added a commit to MartinNowak/dub that referenced this pull request Mar 29, 2017
- making errors like 'unknown package' actually understandable
Copy link
Member

@PetarKirov PetarKirov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@PetarKirov
Copy link
Member

There many occurrences of catch (Exception e) in DUB's codebase and we should eventually get rid of them, but for now this should help explain some silent failures.

@MartinNowak
Copy link
Member Author

Seems to require a bit more work https://travis-ci.org/dlang/dub/jobs/216294639#L724

@wilzbach
Copy link
Member

I am observing "X references unknown package Y" very often on AppVeyor, e.g.
https://ci.appveyor.com/project/greenify/dcd/build/1.0.13/job/6uw39f0u185qjeun (and the regarding discussion), so more logging is very welcome and obviously this LGTM!

Seems to require a bit more work

Hmm I see:

/home/travis/build/dlang/dub/test/issue616-describe-vs-generate-commands.sh: logError: command not found
Script failure.

But this function is properly defined here:

https://github.com/dlang/dub/blob/stable/test/issue616-describe-vs-generate-commands.sh
https://github.com/dlang/dub/blob/master/test/run-unittest.sh

(I tried restarting Travis)

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 5, 2017
- making errors like 'unknown package' actually understandable
@dlang-bot
Copy link
Collaborator

Thanks for your pull request, @MartinNowak!

/++
Exception thrown on HTTP request failures, e.g. 404 Not Found.
+/
static if (__VERSION__ <= 2075) class HTTPStatusException : CurlException
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will there be a HTTPStatusException in 2.076.0?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@s-ludwig
Copy link
Member

s-ludwig commented Jul 6, 2017

Have you tried to use this with a second (empty) registry? I didn't go through the individual errors in detail, but it could very well be that this could lead to lots of bogus error messages when a package exists in one registry but not in another.

@MartinNowak
Copy link
Member Author

Looks OK to me.
Basically we report any non-404 error on any of the provided registry, even if sth. succeeds on a different registry. So it is transparent for people if e.g. their configured registry isn't used and dub falls back to the standard registry.

MartinNowak added a commit to MartinNowak/dub that referenced this pull request Jul 7, 2017
- making errors like 'unknown package' or 'no package found' actually understandable
@MartinNowak
Copy link
Member Author

MartinNowak commented Jul 7, 2017

I've changed this to only handle 404's in getMetadata, everything else relies on that method and checks for a null JSON as return value.

@s-ludwig
Copy link
Member

s-ludwig commented Jul 9, 2017

LGTM, but it should be retargeted to master to get some room for testing.

@MartinNowak MartinNowak changed the base branch from stable to master July 10, 2017 00:29
- making errors like 'unknown package' or 'no package found' actually understandable
- and throw any other errors
- also see dlang/phobos#5551 for HTTPStatusException
@MartinNowak
Copy link
Member Author

Done

@s-ludwig
Copy link
Member

@MartinNowak if you have mange access here (BTW, would sometimes be useful if I had), you'll have to override the merge due to the dmd-beta failure.

@s-ludwig
Copy link
Member

Restarted with the latest DMD beta.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants