catch download failures better in init, re: #522 #523
Conversation
src/rosdep2/sources_list.py
Outdated
| @@ -320,9 +321,12 @@ def download_default_sources_list(url=DEFAULT_SOURCES_LIST_URL): | |||
| data = f.read().decode() | |||
| f.close() | |||
| if not data: | |||
| raise RuntimeError("cannot download defaults file: empty contents") | |||
| raise DownloadFailure("cannot download defaults file: empty contents") | |||
There was a problem hiding this comment.
It might be more helpful if the exception raised would contain the url for which the problem occured.
src/rosdep2/sources_list.py
Outdated
| try: | ||
| parse_sources_data(data) | ||
| except InvalidData as e: | ||
| raise DownloadFailure("Failed to download valid rosdep sources from %s Contents were:{{{%s}}} Parsing error: %s" % (url, data, e)) |
There was a problem hiding this comment.
Since this error has nothing to do with the download but when parsing the data I am not sure this exception type is appropriate.
There was a problem hiding this comment.
Of the available exceptions this looked the most appropriate to me. If there's one you deem more appropriate I'm happy to switch.
Although the error may be unrelated to the actual transport, if the downloaded data is invalid or corrupted the place to look is at the network or source. This is following the same pattern as the exceptions in the function download_rosdep_data where data downloaded failing validation is considered a failed download. And the DownloadFailed exception is the one that makes sense to add urls to me.
There was a problem hiding this comment.
I kind of agree here, since we could conceivably accidentally make malformed sources and then this error would be incorrectly pointing people to problems with their network rather than pointing them at us.
That being said, I think this is unlikely and most of the time it will be due to network errors. So I'm +0 on adding another error for this case.
@dirk-thomas / @tfoote can you guys pick a new error type or make another argument for leaving it as-is?
There was a problem hiding this comment.
I kind of agree here, since we could conceivably accidentally make malformed sources and then this error would be incorrectly pointing people to problems with their network rather than pointing them at us.
If we are worried about malformed upstream sources, we should add CI in the upstream repo. If it is broken for everyone, people will find the right place to report it even when the error message says its a download failure. I'm +1 on @tfoote's change as is. Or maybe we could rephrase slightly like: "Failed to download and validate rosdep sources from..."
There was a problem hiding this comment.
I've updated the message to be more explicit about the potential causes.
|
@tfoote do you have time to look at the conflicts on this branch? |
|
rebased and resolved merge conflict |
src/rosdep2/sources_list.py
Outdated
| try: | ||
| parse_sources_data(data) | ||
| except InvalidData as e: | ||
| raise DownloadFailure("Failed to download valid rosdep sources from %s Contents were:{{{%s}}} Parsing error: %s" % (url, data, e)) |
There was a problem hiding this comment.
I kind of agree here, since we could conceivably accidentally make malformed sources and then this error would be incorrectly pointing people to problems with their network rather than pointing them at us.
If we are worried about malformed upstream sources, we should add CI in the upstream repo. If it is broken for everyone, people will find the right place to report it even when the error message says its a download failure. I'm +1 on @tfoote's change as is. Or maybe we could rephrase slightly like: "Failed to download and validate rosdep sources from..."
I think this will provide a better experience and not tell people that this problem is an internal rosdep issue.