From 1ed695dca790c5e5e1a262ed974fb0fa043e71df Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Tue, 12 Oct 2021 22:11:07 +0200 Subject: Improve error messages for bootstrap download failures (#26599) --- lib/spack/spack/binary_distribution.py | 81 ++++++++++++++++++++++++++-------- lib/spack/spack/bootstrap.py | 2 +- lib/spack/spack/ci.py | 5 ++- lib/spack/spack/cmd/buildcache.py | 12 ++++- lib/spack/spack/test/bindist.py | 23 ++++++++++ 5 files changed, 100 insertions(+), 23 deletions(-) diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index c8e866ff14..c976dc80fb 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/lib/spack/spack/binary_distribution.py @@ -47,6 +47,25 @@ _build_cache_relative_path = 'build_cache' _build_cache_keys_relative_path = '_pgp' +class FetchCacheError(Exception): + """Error thrown when fetching the cache failed, usually a composite error list.""" + def __init__(self, errors): + if not isinstance(errors, list): + raise TypeError("Expected a list of errors") + self.errors = errors + if len(errors) > 1: + msg = " Error {0}: {1}: {2}" + self.message = "Multiple errors during fetching:\n" + self.message += "\n".join(( + msg.format(i + 1, err.__class__.__name__, str(err)) + for (i, err) in enumerate(errors) + )) + else: + err = errors[0] + self.message = "{0}: {1}".format(err.__class__.__name__, str(err)) + super(FetchCacheError, self).__init__(self.message) + + class BinaryCacheIndex(object): """ The BinaryCacheIndex tracks what specs are available on (usually remote) @@ -293,14 +312,22 @@ class BinaryCacheIndex(object): # Otherwise the concrete spec cache should not need to be updated at # all. + fetch_errors = [] + all_methods_failed = True + for cached_mirror_url in self._local_index_cache: cache_entry = self._local_index_cache[cached_mirror_url] cached_index_hash = cache_entry['index_hash'] cached_index_path = cache_entry['index_path'] if cached_mirror_url in configured_mirror_urls: # May need to fetch the index and update the local caches - needs_regen = self._fetch_and_cache_index( - cached_mirror_url, expect_hash=cached_index_hash) + try: + needs_regen = self._fetch_and_cache_index( + cached_mirror_url, expect_hash=cached_index_hash) + all_methods_failed = False + except FetchCacheError as fetch_error: + needs_regen = False + fetch_errors.extend(fetch_error.errors) # The need to regenerate implies a need to clear as well. spec_cache_clear_needed |= needs_regen spec_cache_regenerate_needed |= needs_regen @@ -327,7 +354,12 @@ class BinaryCacheIndex(object): for mirror_url in configured_mirror_urls: if mirror_url not in self._local_index_cache: # Need to fetch the index and update the local caches - needs_regen = self._fetch_and_cache_index(mirror_url) + try: + needs_regen = self._fetch_and_cache_index(mirror_url) + all_methods_failed = False + except FetchCacheError as fetch_error: + fetch_errors.extend(fetch_error.errors) + needs_regen = False # Generally speaking, a new mirror wouldn't imply the need to # clear the spec cache, so leave it as is. if needs_regen: @@ -335,7 +367,9 @@ class BinaryCacheIndex(object): self._write_local_index_cache() - if spec_cache_regenerate_needed: + if all_methods_failed: + raise FetchCacheError(fetch_errors) + elif spec_cache_regenerate_needed: self.regenerate_spec_cache(clear_existing=spec_cache_clear_needed) def _fetch_and_cache_index(self, mirror_url, expect_hash=None): @@ -354,6 +388,8 @@ class BinaryCacheIndex(object): True if this function thinks the concrete spec cache, ``_mirrors_for_spec``, should be regenerated. Returns False otherwise. + Throws: + FetchCacheError: a composite exception. """ index_fetch_url = url_util.join( mirror_url, _build_cache_relative_path, 'index.json') @@ -363,14 +399,19 @@ class BinaryCacheIndex(object): old_cache_key = None fetched_hash = None + errors = [] + # Fetch the hash first so we can check if we actually need to fetch # the index itself. try: _, _, fs = web_util.read_from_url(hash_fetch_url) fetched_hash = codecs.getreader('utf-8')(fs).read() except (URLError, web_util.SpackWebError) as url_err: - tty.debug('Unable to read index hash {0}'.format( - hash_fetch_url), url_err, 1) + errors.append( + RuntimeError("Unable to read index hash {0} due to {1}: {2}".format( + hash_fetch_url, url_err.__class__.__name__, str(url_err) + )) + ) # The only case where we'll skip attempting to fetch the buildcache # index from the mirror is when we already have a hash for this @@ -397,24 +438,23 @@ class BinaryCacheIndex(object): _, _, fs = web_util.read_from_url(index_fetch_url) index_object_str = codecs.getreader('utf-8')(fs).read() except (URLError, web_util.SpackWebError) as url_err: - tty.debug('Unable to read index {0}'.format(index_fetch_url), - url_err, 1) - # We failed to fetch the index, even though we decided it was - # necessary. However, regenerating the spec cache won't produce - # anything different than what it has already, so return False. - return False + errors.append( + RuntimeError("Unable to read index {0} due to {1}: {2}".format( + index_fetch_url, url_err.__class__.__name__, str(url_err) + )) + ) + raise FetchCacheError(errors) locally_computed_hash = compute_hash(index_object_str) if fetched_hash is not None and locally_computed_hash != fetched_hash: - msg_tmpl = ('Computed hash ({0}) did not match remote ({1}), ' - 'indicating error in index transmission') - tty.error(msg_tmpl.format(locally_computed_hash, expect_hash)) + msg = ('Computed hash ({0}) did not match remote ({1}), ' + 'indicating error in index transmission').format( + locally_computed_hash, expect_hash) + errors.append(RuntimeError(msg)) # We somehow got an index that doesn't match the remote one, maybe - # the next time we try we'll be successful. Regardless, we're not - # updating our index cache with this, so don't regenerate the spec - # cache either. - return False + # the next time we try we'll be successful. + raise FetchCacheError(errors) url_hash = compute_hash(mirror_url) @@ -1559,6 +1599,9 @@ def update_cache_and_get_specs(): possible, so this method will also attempt to initialize and update the local index cache (essentially a no-op if it has been done already and nothing has changed on the configured mirrors.) + + Throws: + FetchCacheError """ binary_index.update() return binary_index.get_all_built_specs() diff --git a/lib/spack/spack/bootstrap.py b/lib/spack/spack/bootstrap.py index 3fad57b5ae..0bdf49c5e8 100644 --- a/lib/spack/spack/bootstrap.py +++ b/lib/spack/spack/bootstrap.py @@ -218,7 +218,7 @@ class _BuildcacheBootstrapper(object): index = spack.binary_distribution.update_cache_and_get_specs() if not index: - raise RuntimeError("could not populate the binary index") + raise RuntimeError("The binary index is empty") for item in data['verified']: candidate_spec = item['spec'] diff --git a/lib/spack/spack/ci.py b/lib/spack/spack/ci.py index 32b5da451a..733b5f3384 100644 --- a/lib/spack/spack/ci.py +++ b/lib/spack/spack/ci.py @@ -668,7 +668,10 @@ def generate_gitlab_ci_yaml(env, print_summary, output_file, # Speed up staging by first fetching binary indices from all mirrors # (including the per-PR mirror we may have just added above). - bindist.binary_index.update() + try: + bindist.binary_index.update() + except bindist.FetchCacheError as e: + tty.error(e) staged_phases = {} try: diff --git a/lib/spack/spack/cmd/buildcache.py b/lib/spack/spack/cmd/buildcache.py index 889c93d690..13b0158ff4 100644 --- a/lib/spack/spack/cmd/buildcache.py +++ b/lib/spack/spack/cmd/buildcache.py @@ -334,7 +334,11 @@ def match_downloaded_specs(pkgs, allow_multiple_matches=False, force=False, specs_from_cli = [] has_errors = False - specs = bindist.update_cache_and_get_specs() + try: + specs = bindist.update_cache_and_get_specs() + except bindist.FetchCacheError as e: + tty.error(e) + if not other_arch: arch = spack.spec.Spec.default_arch() specs = [s for s in specs if s.satisfies(arch)] @@ -560,7 +564,11 @@ def install_tarball(spec, args): def listspecs(args): """list binary packages available from mirrors""" - specs = bindist.update_cache_and_get_specs() + try: + specs = bindist.update_cache_and_get_specs() + except bindist.FetchCacheError as e: + tty.error(e) + if not args.allarch: arch = spack.spec.Spec.default_arch() specs = [s for s in specs if s.satisfies(arch)] diff --git a/lib/spack/spack/test/bindist.py b/lib/spack/spack/test/bindist.py index abaa0905d0..dbcf0d8ea6 100644 --- a/lib/spack/spack/test/bindist.py +++ b/lib/spack/spack/test/bindist.py @@ -666,3 +666,26 @@ def test_update_index_fix_deps(monkeypatch, tmpdir, mutable_config): # Make sure the full hash of b in a's spec json matches the new value assert(a_prime[b.name].full_hash() == new_b_full_hash) + + +def test_FetchCacheError_only_accepts_lists_of_errors(): + with pytest.raises(TypeError, match="list"): + bindist.FetchCacheError("error") + + +def test_FetchCacheError_pretty_printing_multiple(): + e = bindist.FetchCacheError([RuntimeError("Oops!"), TypeError("Trouble!")]) + str_e = str(e) + print("'" + str_e + "'") + assert "Multiple errors" in str_e + assert "Error 1: RuntimeError: Oops!" in str_e + assert "Error 2: TypeError: Trouble!" in str_e + assert str_e.rstrip() == str_e + + +def test_FetchCacheError_pretty_printing_single(): + e = bindist.FetchCacheError([RuntimeError("Oops!")]) + str_e = str(e) + assert "Multiple errors" not in str_e + assert "RuntimeError: Oops!" in str_e + assert str_e.rstrip() == str_e -- cgit v1.2.3-70-g09d2