From 63c341037033f683221a7ede9d6b24e86ddf0951 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Fri, 21 Apr 2017 15:36:15 -0700 Subject: Fix checksumming in Python3; add more fetch tests (#3941) * Checksum code wasn't opening binary files as binary. - Fixes Python 3 issue where files are opened as unicode text by default, and decoding fails for binary blobs. * Simplify fetch test parametrization. * - add tests for URL fetching and checksumming. - fix coverage on interface functions in FetchStrategy superclass - add some extra crypto tests. --- lib/spack/spack/fetch_strategy.py | 38 +++++++++++----- lib/spack/spack/test/conftest.py | 13 ++++-- lib/spack/spack/test/git_fetch.py | 16 ++----- lib/spack/spack/test/hg_fetch.py | 16 ++----- lib/spack/spack/test/svn_fetch.py | 16 ++----- lib/spack/spack/test/url_fetch.py | 93 +++++++++++++++++++++++++++++++++++++++ lib/spack/spack/util/crypto.py | 18 ++++---- 7 files changed, 151 insertions(+), 59 deletions(-) create mode 100644 lib/spack/spack/test/url_fetch.py (limited to 'lib') diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py index 855b2f9379..7cafeb296d 100644 --- a/lib/spack/spack/fetch_strategy.py +++ b/lib/spack/spack/fetch_strategy.py @@ -103,27 +103,42 @@ class FetchStrategy(with_metaclass(FSMeta, object)): # Subclasses need to implement these methods def fetch(self): - pass # Return True on success, False on fail. + """Fetch source code archive or repo. + + Returns: + bool: True on success, False on failure. + """ def check(self): - pass # Do checksum. + """Checksum the archive fetched by this FetchStrategy.""" def expand(self): - pass # Expand archive. + """Expand the downloaded archive.""" def reset(self): - pass # Revert to freshly downloaded state. + """Revert to freshly downloaded state. + + For archive files, this may just re-expand the archive. + """ def archive(self, destination): - pass # Used to create tarball for mirror. + """Create an archive of the downloaded data for a mirror. + + For downloaded files, this should preserve the checksum of the + original file. For repositories, it should just create an + expandable tarball out of the downloaded repository. + """ @property def cachable(self): - """Return whether the fetcher is capable of caching the - resource it retrieves. This generally is determined by - whether the resource is identifiably associated with a - specific package version.""" - pass + """Whether fetcher is capable of caching the resource it retrieves. + + This generally is determined by whether the resource is + identifiably associated with a specific package version. + + Returns: + bool: True if can cache, False otherwise. + """ def __str__(self): # Should be human readable URL. return "FetchStrategy.__str___" @@ -162,7 +177,8 @@ class URLFetchStrategy(FetchStrategy): if not self.url: self.url = url - self.digest = kwargs.get('md5', None) + self.digest = next((kwargs[h] for h in crypto.hashes if h in kwargs), + None) if not self.digest: self.digest = digest diff --git a/lib/spack/spack/test/conftest.py b/lib/spack/spack/test/conftest.py index fc1d6ecec2..796d95a0cf 100644 --- a/lib/spack/spack/test/conftest.py +++ b/lib/spack/spack/test/conftest.py @@ -300,6 +300,7 @@ def mock_archive(): repo_name = 'mock-archive-repo' tmpdir.ensure(repo_name, dir=True) repodir = tmpdir.join(repo_name) + # Create the configure script configure_path = str(tmpdir.join(repo_name, 'configure')) with open(configure_path, 'w') as f: @@ -315,15 +316,21 @@ def mock_archive(): "EOF\n" ) os.chmod(configure_path, 0o755) + # Archive it current = tmpdir.chdir() archive_name = '{0}.tar.gz'.format(repo_name) tar('-czf', archive_name, repo_name) current.chdir() - Archive = collections.namedtuple('Archive', ['url', 'path']) - url = 'file://' + str(tmpdir.join(archive_name)) + Archive = collections.namedtuple('Archive', + ['url', 'path', 'archive_file']) + archive_file = str(tmpdir.join(archive_name)) + # Return the url - yield Archive(url=url, path=str(repodir)) + yield Archive( + url=('file://' + archive_file), + archive_file=archive_file, + path=str(repodir)) stage.destroy() diff --git a/lib/spack/spack/test/git_fetch.py b/lib/spack/spack/test/git_fetch.py index ef531ef65f..34f74df84f 100644 --- a/lib/spack/spack/test/git_fetch.py +++ b/lib/spack/spack/test/git_fetch.py @@ -31,18 +31,8 @@ from spack.spec import Spec from spack.version import ver -@pytest.fixture(params=['master', 'branch', 'tag', 'commit']) -def type_of_test(request): - """Returns one of the test type available for the mock_git_repository""" - return request.param - - -@pytest.fixture(params=[True, False]) -def secure(request): - """Attempt both secure and insecure fetching""" - return request.param - - +@pytest.mark.parametrize("type_of_test", ['master', 'branch', 'tag', 'commit']) +@pytest.mark.parametrize("secure", [True, False]) def test_fetch( type_of_test, secure, @@ -62,11 +52,13 @@ def test_fetch( # Retrieve the right test parameters t = mock_git_repository.checks[type_of_test] h = mock_git_repository.hash + # Construct the package under test spec = Spec('git-test') spec.concretize() pkg = spack.repo.get(spec, new=True) pkg.versions[ver('git')] = t.args + # Enter the stage directory and check some properties with pkg.stage: try: diff --git a/lib/spack/spack/test/hg_fetch.py b/lib/spack/spack/test/hg_fetch.py index 29a6eef561..96a23a1c53 100644 --- a/lib/spack/spack/test/hg_fetch.py +++ b/lib/spack/spack/test/hg_fetch.py @@ -31,18 +31,8 @@ from spack.spec import Spec from spack.version import ver -@pytest.fixture(params=['default', 'rev0']) -def type_of_test(request): - """Returns one of the test type available for the mock_hg_repository""" - return request.param - - -@pytest.fixture(params=[True, False]) -def secure(request): - """Attempt both secure and insecure fetching""" - return request.param - - +@pytest.mark.parametrize("type_of_test", ['default', 'rev0']) +@pytest.mark.parametrize("secure", [True, False]) def test_fetch( type_of_test, secure, @@ -62,11 +52,13 @@ def test_fetch( # Retrieve the right test parameters t = mock_hg_repository.checks[type_of_test] h = mock_hg_repository.hash + # Construct the package under test spec = Spec('hg-test') spec.concretize() pkg = spack.repo.get(spec, new=True) pkg.versions[ver('hg')] = t.args + # Enter the stage directory and check some properties with pkg.stage: try: diff --git a/lib/spack/spack/test/svn_fetch.py b/lib/spack/spack/test/svn_fetch.py index 69d675fe3c..2ffedee7db 100644 --- a/lib/spack/spack/test/svn_fetch.py +++ b/lib/spack/spack/test/svn_fetch.py @@ -31,18 +31,8 @@ from spack.spec import Spec from spack.version import ver -@pytest.fixture(params=['default', 'rev0']) -def type_of_test(request): - """Returns one of the test type available for the mock_svn_repository""" - return request.param - - -@pytest.fixture(params=[True, False]) -def secure(request): - """Attempt both secure and insecure fetching""" - return request.param - - +@pytest.mark.parametrize("type_of_test", ['default', 'rev0']) +@pytest.mark.parametrize("secure", [True, False]) def test_fetch( type_of_test, secure, @@ -62,11 +52,13 @@ def test_fetch( # Retrieve the right test parameters t = mock_svn_repository.checks[type_of_test] h = mock_svn_repository.hash + # Construct the package under test spec = Spec('svn-test') spec.concretize() pkg = spack.repo.get(spec, new=True) pkg.versions[ver('svn')] = t.args + # Enter the stage directory and check some properties with pkg.stage: try: diff --git a/lib/spack/spack/test/url_fetch.py b/lib/spack/spack/test/url_fetch.py new file mode 100644 index 0000000000..c9299f45a1 --- /dev/null +++ b/lib/spack/spack/test/url_fetch.py @@ -0,0 +1,93 @@ +############################################################################## +# Copyright (c) 2013-2016, Lawrence Livermore National Security, LLC. +# Produced at the Lawrence Livermore National Laboratory. +# +# This file is part of Spack. +# Created by Todd Gamblin, tgamblin@llnl.gov, All rights reserved. +# LLNL-CODE-647188 +# +# For details, see https://github.com/llnl/spack +# Please also see the LICENSE file for our notice and the LGPL. +# +# This program is free software; you can redistribute it and/or modify +# it under the terms of the GNU Lesser General Public License (as +# published by the Free Software Foundation) version 2.1, February 1999. +# +# This program is distributed in the hope that it will be useful, but +# WITHOUT ANY WARRANTY; without even the IMPLIED WARRANTY OF +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the terms and +# conditions of the GNU Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this program; if not, write to the Free Software +# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA +############################################################################## +import os +import pytest + +from llnl.util.filesystem import * + +import spack +from spack.spec import Spec +from spack.version import ver +import spack.util.crypto as crypto + + +@pytest.fixture(params=list(crypto.hashes.keys())) +def checksum_type(request): + return request.param + + +@pytest.mark.parametrize('secure', [True, False]) +def test_fetch( + mock_archive, + secure, + checksum_type, + config, + refresh_builtin_mock +): + """Fetch an archive and make sure we can checksum it.""" + mock_archive.url + mock_archive.path + + algo = crypto.hashes[checksum_type]() + with open(mock_archive.archive_file, 'rb') as f: + algo.update(f.read()) + checksum = algo.hexdigest() + + # Get a spec and tweak the test package with new chcecksum params + spec = Spec('url-test') + spec.concretize() + + pkg = spack.repo.get('url-test', new=True) + pkg.url = mock_archive.url + pkg.versions[ver('test')] = {checksum_type: checksum, 'url': pkg.url} + pkg.spec = spec + + # Enter the stage directory and check some properties + with pkg.stage: + try: + spack.insecure = secure + pkg.do_stage() + finally: + spack.insecure = False + + assert os.path.exists('configure') + assert is_exe('configure') + + with open('configure') as f: + contents = f.read() + assert contents.startswith('#!/bin/sh') + assert 'echo Building...' in contents + + +def test_hash_detection(checksum_type): + algo = crypto.hashes[checksum_type]() + h = 'f' * (algo.digest_size * 2) # hex -> bytes + checker = crypto.Checker(h) + assert checker.hash_name == checksum_type + + +def test_unknown_hash(checksum_type): + with pytest.raises(ValueError): + crypto.Checker('a') diff --git a/lib/spack/spack/util/crypto.py b/lib/spack/spack/util/crypto.py index 2965168056..f0d48702a1 100644 --- a/lib/spack/spack/util/crypto.py +++ b/lib/spack/spack/util/crypto.py @@ -26,16 +26,16 @@ import sys import hashlib """Set of acceptable hashes that Spack will use.""" -_acceptable_hashes = [ - hashlib.md5, - hashlib.sha1, - hashlib.sha224, - hashlib.sha256, - hashlib.sha384, - hashlib.sha512] +hashes = dict((h, getattr(hashlib, h)) for h in [ + 'md5', + 'sha1', + 'sha224', + 'sha256', + 'sha384', + 'sha512']) """Index for looking up hasher for a digest.""" -_size_to_hash = dict((h().digest_size, h) for h in _acceptable_hashes) +_size_to_hash = dict((h().digest_size, h) for h in hashes.values()) def checksum(hashlib_algo, filename, **kwargs): @@ -44,7 +44,7 @@ def checksum(hashlib_algo, filename, **kwargs): """ block_size = kwargs.get('block_size', 2**20) hasher = hashlib_algo() - with open(filename) as file: + with open(filename, 'rb') as file: while True: data = file.read(block_size) if not data: -- cgit v1.2.3-60-g2f50