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

repo: catch error updating the package cache #2079

Merged
merged 4 commits into from
Apr 25, 2018

Conversation

kalikiana
Copy link
Contributor

@kalikiana kalikiana commented Apr 16, 2018

  • Have you followed the guidelines for contributing?
  • Have you signed the CLA?
  • If this is a bugfix. Have you checked that there is a bug report open for the issue you are trying to fix on bug reports?
  • If this is a new feature. Have you discussed the design on the forum?
  • Have you successfully run ./runtests.sh static?
  • Have you successfully run ./runtests.sh unit?

This PR handles errors when updating the package cache, such as a hash mismatch or unresolved hostnames. The apt API doesn't distinguish these, so unfortunately we can't be very specific here but need to rely on the output we get before the error is raised.
If the error is a hashsum mismatch, the index will be cleared and Snapcraft will continue.

Fixes: #2029 #2030
Fixes: LP: #1747715

The following tests are added/ modified:

  • tests.unit.test_errors
  • To verify CacheUpdateFailedError
  • tests.unit.repo.test_deb.UbuntuTestCase.test_cache_update_failed
  • To verify that an apt.cache.FetchFailedException is handled as a proper error.
  • tests.unit.repo.test_deb.UbuntuTestCase.test_cache_hashsum_mismatch
  • To verify that a hashsum error will be caught and proceed after clearing the index.

I locally ran the tests:

Manual test steps:

  • cd tests/integration/snaps/go-hello
  • Add a stage package:
    stages-packages: [appstream]
  • Modify /etc/apt/sources.list so that it can't update successfully, such as by adding an invalid URL.
  • snapcraft
  • Observe the error "Failed to update the package cache: Some files could not be downloaded. Check that the sources on your host are configured correctly.".

try:
apt_cache.update(fetch_progress=self.progress,
sources_list=sources_list_file)
except apt.cache.FetchFailedException:
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is the current exception message? We should consider printing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can't assume that there is a message. If there is one, like in the linked bug, the message might look like so:

E:Failed to fetch copy:/home/alan/.cache/snapcraft/stage-packages/apt/a810ed86fa085532f20ab86eb7554a1ff527e7f8f46e49c343abc3d7c4d141ae0c39e374361c05f4561cf2aec6689aac/var/lib/apt/lists/partial/security.ubuntu.com_ubuntu_dists_xenial-security_multiverse_dep11_icons-64x64.tar.gz Hash Sum mismatch, E:Failed to fetch copy:/home/alan/.cache/snapcraft/stage-packages/apt/a810ed86fa085532f20ab86eb7554a1ff527e7f8f46e49c343abc3d7c4d141ae0c39e374361c05f4561cf2aec6689aac/var/lib/apt/lists/partial/gb.archive.ubuntu.com_ubuntu_dists_xenial-backports_main_dep11_icons-64x64.tar.gz Hash Sum mismatch, E:Some index files failed to download. They have been ignored, or old ones used instead.

To my mind this doesn't seem very user-friendly. In the same situation, the printed output before that will have Err http://security.ubuntu.com/ubuntu xenial-security/multiverse DEP-11 128x128 Icons Hash Sum mismatch at the end, so I'd rather be looking at that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the messages to the error (if they're present). And the index will be cleared if it's a hashsum mismatch (regex) so Snapcraft can continue as normal after that, meaning ideally the error should not be surfacing to the user if that was the issue.

@kyrofa
Copy link
Contributor

kyrofa commented Apr 20, 2018

Note that this PR fixes #2029.

Copy link
Contributor

@mvo5 mvo5 left a comment

Choose a reason for hiding this comment

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

👍 with one small suggestion

@@ -54,6 +55,8 @@
'''
_GEOIP_SERVER = "http://geoip.ubuntu.com/lookup"
_library_list = dict() # type: Dict[str, Set[str]]
_HASHSUM_MISMATCH_PATTERN = re.compile(
r'(E:Failed to fetch copy.+Hash Sum mismatch)+')
Copy link
Contributor

Choose a reason for hiding this comment

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

You can probably omit the "copy" here, this refers to the internal helper that generated the error and it might change in the future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Thanks for the review!

Copy link
Collaborator

@sergiusens sergiusens left a comment

Choose a reason for hiding this comment

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

Thanks @mvo5 for the assist

@sergiusens sergiusens merged commit d879b2e into canonical:master Apr 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants