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.

Sign up for GitHub

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

catch download failures better in init, re: #522 #523

Merged
merged 2 commits into from
Aug 1, 2017
Merged

catch download failures better in init, re: #522 #523

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0fd102c
catch download failures better in init, re: #522
tfoote Jul 12, 2017
1cd8b85
Making error message much more verbose.
tfoote Jul 28, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions src/rosdep2/main.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@

from . import create_default_installer_context, get_default_installer
from . import __version__
from .core import RosdepInternalError, InstallFailed, UnsupportedOs, InvalidData, CachePermissionError
from .core import RosdepInternalError, InstallFailed, UnsupportedOs, InvalidData, CachePermissionError, DownloadFailure
from .installers import normalize_uninstalled_to_list
from .installers import RosdepInstaller
from .lookup import RosdepLookup, ResolutionError
Expand Down Expand Up @@ -517,6 +517,10 @@ def command_init(options):
except URLError as e:
print("ERROR: cannot download default sources list from:\n%s\nWebsite may be down."%(DEFAULT_SOURCES_LIST_URL))
return 4
except DownloadFailure as e:
print("ERROR: cannot download default sources list from:\n%s\nWebsite may be down."%(DEFAULT_SOURCES_LIST_URL))
print(e)
return 4
# reuse path variable for error message
path = get_sources_list_dir()
old_umask = os.umask(0o022)
Expand Down Expand Up @@ -878,6 +882,3 @@ def command_fix_permissions(options):
_command_no_args = ['update', 'init', 'db', 'fix-permissions']

_commands = command_handlers.keys()



10 changes: 7 additions & 3 deletions src/rosdep2/sources_list.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -309,7 +309,8 @@ def download_default_sources_list(url=DEFAULT_SOURCES_LIST_URL):

:param url: override URL of default sources list file
:return: raw sources list data, ``str``
:raises: :exc:`InvalidData`
:raises: :exc:`DownloadFailure` If data cannot be
retrieved (e.g. 404, bad YAML format, server down).
:raises: :exc:`urllib2.URLError` If data cannot be
retrieved (e.g. 404, server down).
"""
Expand All @@ -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 from %s : empty contents" % url)
# parse just for validation
parse_sources_data(data)
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))
Copy link
Member

Choose a reason for hiding this comment

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

Since this error has nothing to do with the download but when parsing the data I am not sure this exception type is appropriate.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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..."

Copy link
Member Author

Choose a reason for hiding this comment

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

I've updated the message to be more explicit about the potential causes.

return data

def parse_sources_data(data, origin='<string>', model=None):
Expand Down