From da50816127706fe470b6896f5ad852f23861bb6f Mon Sep 17 00:00:00 2001 From: Harmen Stoppels Date: Sat, 18 Feb 2023 19:22:48 +0100 Subject: [PATCH] Improve error handling in buildcache downloads (#35568) The checksum exception was not detailed enough and not reraised when using cache only, resulting in useless error messages. Now it dumps the file path, expected hash, computed hash, and the downloaded file summary. --- lib/spack/llnl/util/filesystem.py | 25 ++++++++++++++++ lib/spack/spack/binary_distribution.py | 30 ++++++++++++++------ lib/spack/spack/installer.py | 18 ++++++------ lib/spack/spack/test/llnl/util/filesystem.py | 10 +++++++ 4 files changed, 66 insertions(+), 17 deletions(-) diff --git a/lib/spack/llnl/util/filesystem.py b/lib/spack/llnl/util/filesystem.py index aece52f8436..c7a207d5d18 100644 --- a/lib/spack/llnl/util/filesystem.py +++ b/lib/spack/llnl/util/filesystem.py @@ -2589,3 +2589,28 @@ def temporary_dir(*args, **kwargs): yield tmp_dir finally: remove_directory_contents(tmp_dir) + + +def filesummary(path, print_bytes=16): + """Create a small summary of the given file. Does not error + when file does not exist. + + Args: + print_bytes (int): Number of bytes to print from start/end of file + + Returns: + Tuple of size and byte string containing first n .. last n bytes. + Size is 0 if file cannot be read.""" + try: + n = print_bytes + with open(path, "rb") as f: + size = os.fstat(f.fileno()).st_size + if size <= 2 * n: + short_contents = f.read(2 * n) + else: + short_contents = f.read(n) + f.seek(-n, 2) + short_contents += b"..." + f.read(n) + return size, short_contents + except OSError: + return 0, b"" diff --git a/lib/spack/spack/binary_distribution.py b/lib/spack/spack/binary_distribution.py index 8ceeeea7386..03af286f0b1 100644 --- a/lib/spack/spack/binary_distribution.py +++ b/lib/spack/spack/binary_distribution.py @@ -36,6 +36,7 @@ import spack.relocate as relocate import spack.repo import spack.store +import spack.util.crypto import spack.util.file_cache as file_cache import spack.util.gpg import spack.util.spack_json as sjson @@ -603,7 +604,12 @@ class NoChecksumException(spack.error.SpackError): Raised if file fails checksum verification. """ - pass + def __init__(self, path, size, contents, algorithm, expected, computed): + super(NoChecksumException, self).__init__( + "{} checksum failed for {}".format(algorithm, path), + "Expected {} but got {}. " + "File size = {} bytes. Contents = {!r}".format(expected, computed, size, contents), + ) class NewLayoutException(spack.error.SpackError): @@ -1861,14 +1867,15 @@ def _extract_inner_tarball(spec, filename, extract_to, unsigned, remote_checksum raise UnsignedPackageException( "To install unsigned packages, use the --no-check-signature option." ) - # get the sha256 checksum of the tarball + + # compute the sha256 checksum of the tarball local_checksum = checksum_tarball(tarfile_path) + expected = remote_checksum["hash"] # if the checksums don't match don't install - if local_checksum != remote_checksum["hash"]: - raise NoChecksumException( - "Package tarball failed checksum verification.\n" "It cannot be installed." - ) + if local_checksum != expected: + size, contents = fsys.filesummary(tarfile_path) + raise NoChecksumException(tarfile_path, size, contents, "sha256", expected, local_checksum) return tarfile_path @@ -1928,12 +1935,14 @@ def extract_tarball(spec, download_result, allow_root=False, unsigned=False, for # compute the sha256 checksum of the tarball local_checksum = checksum_tarball(tarfile_path) + expected = bchecksum["hash"] # if the checksums don't match don't install - if local_checksum != bchecksum["hash"]: + if local_checksum != expected: + size, contents = fsys.filesummary(tarfile_path) _delete_staged_downloads(download_result) raise NoChecksumException( - "Package tarball failed checksum verification.\n" "It cannot be installed." + tarfile_path, size, contents, "sha256", expected, local_checksum ) new_relative_prefix = str(os.path.relpath(spec.prefix, spack.store.layout.root)) @@ -2024,8 +2033,11 @@ def install_root_node(spec, allow_root, unsigned=False, force=False, sha256=None tarball_path = download_result["tarball_stage"].save_filename msg = msg.format(tarball_path, sha256) if not checker.check(tarball_path): + size, contents = fsys.filesummary(tarball_path) _delete_staged_downloads(download_result) - raise spack.binary_distribution.NoChecksumException(msg) + raise NoChecksumException( + tarball_path, size, contents, checker.hash_name, sha256, checker.sum + ) tty.debug("Verified SHA256 checksum of the build cache") # don't print long padded paths while extracting/relocating binaries diff --git a/lib/spack/spack/installer.py b/lib/spack/spack/installer.py index d95abf3ac23..99a83079ebd 100644 --- a/lib/spack/spack/installer.py +++ b/lib/spack/spack/installer.py @@ -1773,14 +1773,16 @@ def install(self): raise except binary_distribution.NoChecksumException as exc: - if not task.cache_only: - # Checking hash on downloaded binary failed. - err = "Failed to install {0} from binary cache due to {1}:" - err += " Requeueing to install from source." - tty.error(err.format(pkg.name, str(exc))) - task.use_cache = False - self._requeue_task(task) - continue + if task.cache_only: + raise + + # Checking hash on downloaded binary failed. + err = "Failed to install {0} from binary cache due to {1}:" + err += " Requeueing to install from source." + tty.error(err.format(pkg.name, str(exc))) + task.use_cache = False + self._requeue_task(task) + continue except (Exception, SystemExit) as exc: self._update_failed(task, True, exc) diff --git a/lib/spack/spack/test/llnl/util/filesystem.py b/lib/spack/spack/test/llnl/util/filesystem.py index 4950558db6e..24950b827e3 100644 --- a/lib/spack/spack/test/llnl/util/filesystem.py +++ b/lib/spack/spack/test/llnl/util/filesystem.py @@ -903,3 +903,13 @@ def test_remove_linked_tree_doesnt_change_file_permission(tmpdir, initial_mode): fs.remove_linked_tree(str(file_instead_of_dir)) final_stat = os.stat(str(file_instead_of_dir)) assert final_stat == initial_stat + + +def test_filesummary(tmpdir): + p = str(tmpdir.join("xyz")) + with open(p, "wb") as f: + f.write(b"abcdefghijklmnopqrstuvwxyz") + + assert fs.filesummary(p, print_bytes=8) == (26, b"abcdefgh...stuvwxyz") + assert fs.filesummary(p, print_bytes=13) == (26, b"abcdefghijklmnopqrstuvwxyz") + assert fs.filesummary(p, print_bytes=100) == (26, b"abcdefghijklmnopqrstuvwxyz")