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.

8 files changed, 186 insertions, 59 deletions

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:
diff --git a/var/spack/repos/builtin.mock/packages/url-test/package.py b/var/spack/repos/builtin.mock/packages/url-test/package.py
new file mode 100644
index 0000000000..a1f9af7d7d
--- /dev/null
+++ b/var/spack/repos/builtin.mock/packages/url-test/package.py
@@ -0,0 +1,35 @@
+##############################################################################
+# Copyright (c) 2013-2017, 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
+##############################################################################
+from spack import *
+
+
+class UrlTest(Package):
+    """Mock package that fetches from a URL."""
+    homepage = "http://www.url-fetch-example.com"
+
+    version('test', url='to-be-filled-in-by-test')
+
+    def install(self, spec, prefix):
+        pass