Merge branch 'develop' of https://github.com/LLNL/spack into features/install_with_phases_rebase

10 files changed, 146 insertions, 65 deletions

diff --git a/lib/spack/docs/packaging_guide.rst b/lib/spack/docs/packaging_guide.rst
index efd4c459a2..bf5763f4f8 100644
--- a/lib/spack/docs/packaging_guide.rst
+++ b/lib/spack/docs/packaging_guide.rst
@@ -526,32 +526,57 @@ in the package.  For example, Spack is smart enough to download
 version ``8.2.1.`` of the ``Foo`` package above from
 ``http://example.com/foo-8.2.1.tar.gz``.
 
-If spack *cannot* extrapolate the URL from the ``url`` field by
-default, you can write your own URL generation algorithm in place of
-the ``url`` declaration.  For example:
+If the URL is particularly complicated or changes based on the release,
+you can override the default URL generation algorithm by defining your
+own ``url_for_version()`` function. For example, the developers of HDF5
+keep changing the archive layout, so the ``url_for_version()`` function
+looks like:
+
+.. literalinclude:: ../../../var/spack/repos/builtin/packages/hdf5/package.py
+   :pyobject: Hdf5.url_for_version
+
+With the use of this ``url_for_version()``, Spack knows to download HDF5 ``1.8.16``
+from ``http://www.hdfgroup.org/ftp/HDF5/releases/hdf5-1.8.16/src/hdf5-1.8.16.tar.gz``
+but download HDF5 ``1.10.0`` from ``http://www.hdfgroup.org/ftp/HDF5/releases/hdf5-1.10/hdf5-1.10.0/src/hdf5-1.10.0.tar.gz``.
+
+You'll notice that HDF5's ``url_for_version()`` function makes use of a special
+``Version`` function called ``up_to()``. When you call ``version.up_to(2)`` on a
+version like ``1.10.0``, it returns ``1.10``. ``version.up_to(1)`` would return
+``1``. This can be very useful for packages that place all ``X.Y.*`` versions in
+a single directory and then places all ``X.Y.Z`` versions in a subdirectory.
+
+There are a few ``Version`` properties you should be aware of. We generally
+prefer numeric versions to be separated by dots for uniformity, but not all
+tarballs are named that way. For example, ``icu4c`` separates its major and minor
+versions with underscores, like ``icu4c-57_1-src.tgz``. The value ``57_1`` can be
+obtained with the use of the ``version.underscored`` property. Note that Python
+properties don't need parentheses. There are other separator properties as well:
+
+===================  ======
+Property             Result
+===================  ======
+version.dotted       1.2.3
+version.dashed       1-2-3
+version.underscored  1_2_3
+version.joined       123
+===================  ======
 
-.. code-block:: python
-   :linenos:
+.. note::
 
-   class Foo(Package):
-       version('8.2.1', '4136d7b4c04df68b686570afa26988ac')
-       ...
-       def url_for_version(self, version):
-           return 'http://example.com/version_%s/foo-%s.tar.gz' \
-               % (version, version)
-       ...
+   Python properties don't need parentheses. ``version.dashed`` is correct.
+   ``version.dashed()`` is incorrect.
 
-If a URL cannot be derived systematically, you can add an explicit URL
-for a particular version:
+If a URL cannot be derived systematically, or there is a special URL for one
+of its versions, you can add an explicit URL for a particular version:
 
 .. code-block:: python
 
    version('8.2.1', '4136d7b4c04df68b686570afa26988ac',
            url='http://example.com/foo-8.2.1-special-version.tar.gz')
 
-For the URL above, you might have to add an explicit URL because the
-version can't simply be substituted in the original ``url`` to
-construct the new one for ``8.2.1``.
+This is common for Python packages that download from PyPi. Since newer
+download URLs often contain a unique hash for each version, there is no
+way to guess the URL systematically.
 
 When you supply a custom URL for a version, Spack uses that URL
 *verbatim* and does not perform extrapolation.
diff --git a/lib/spack/env/cc b/lib/spack/env/cc
index 60e24979c8..84a17abf20 100755
--- a/lib/spack/env/cc
+++ b/lib/spack/env/cc
@@ -317,13 +317,6 @@ case "$mode" in
         args=("${args[@]}" ${SPACK_LDLIBS[@]}) ;;
 esac
 
-#
-# Unset pesky environment variables that could affect build sanity.
-#
-unset LD_LIBRARY_PATH
-unset LD_RUN_PATH
-unset DYLD_LIBRARY_PATH
-
 full_command=("$command" "${args[@]}")
 
 # In test command mode, write out full command for Spack tests.
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 9c9e9e10ff..2351e2bfc9 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -95,7 +95,11 @@ class DefaultConcretizer(object):
                                      not b.external and b.external_module):
                 # We're choosing between different providers, so
                 # maintain order from provider sort
-                return candidates.index(a) - candidates.index(b)
+                index_of_a = next(i for i in range(0, len(candidates))
+                                  if a.satisfies(candidates[i]))
+                index_of_b = next(i for i in range(0, len(candidates))
+                                  if b.satisfies(candidates[i]))
+                return index_of_a - index_of_b
 
             result = cmp_specs(a, b)
             if result != 0:
diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index dda9fb32d8..9cf00334a8 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -259,7 +259,7 @@ def provides(pkg, *specs, **kwargs):
 
 
 @directive('patches')
-def patch(pkg, url_or_filename, level=1, when=None):
+def patch(pkg, url_or_filename, level=1, when=None, **kwargs):
     """Packages can declare patches to apply to source.  You can
        optionally provide a when spec to indicate that a particular
        patch should only be applied when the package's spec meets
@@ -271,7 +271,7 @@ def patch(pkg, url_or_filename, level=1, when=None):
     cur_patches = pkg.patches.setdefault(when_spec, [])
     # if this spec is identical to some other, then append this
     # patch to the existing list.
-    cur_patches.append(Patch(pkg, url_or_filename, level))
+    cur_patches.append(Patch.create(pkg, url_or_filename, level, **kwargs))
 
 
 @directive('variants')
diff --git a/lib/spack/spack/fetch_strategy.py b/lib/spack/spack/fetch_strategy.py
index 2becf9ed04..4374587250 100644
--- a/lib/spack/spack/fetch_strategy.py
+++ b/lib/spack/spack/fetch_strategy.py
@@ -286,6 +286,8 @@ class URLFetchStrategy(FetchStrategy):
                 "URLFetchStrategy couldn't find archive file",
                 "Failed on expand() for URL %s" % self.url)
 
+        if not self.extension:
+            self.extension = extension(self.archive_file)
         decompress = decompressor_for(self.archive_file, self.extension)
 
         # Expand all tarballs in their own directory to contain
@@ -313,7 +315,8 @@ class URLFetchStrategy(FetchStrategy):
                     shutil.move(os.path.join(tarball_container, f),
                                 os.path.join(self.stage.path, f))
                 os.rmdir(tarball_container)
-
+        if not files:
+            os.rmdir(tarball_container)
         # Set the wd back to the stage when done.
         self.stage.chdir()
 
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index 0bd9f5d29d..ee83748319 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -24,62 +24,106 @@
 ##############################################################################
 import os
 
-from llnl.util.filesystem import join_path
-
 import spack
-import spack.stage
 import spack.error
+import spack.stage
+import spack.fetch_strategy as fs
 
+from llnl.util.filesystem import join_path
 from spack.util.executable import which
 
-# Patch tool for patching archives.
-_patch = which("patch", required=True)
-
 
 class Patch(object):
-    """This class describes a patch to be applied to some expanded
-       source code."""
+    """Base class to describe a patch that needs to be applied to some
+    expanded source code.
+    """
+
+    @staticmethod
+    def create(pkg, path_or_url, level, **kwargs):
+        """
+        Factory method that creates an instance of some class derived from
+        Patch
+
+        Args:
+            pkg: package that needs to be patched
+            path_or_url: path or url where the patch is found
+            level: patch level
+
+        Returns:
+            instance of some Patch class
+        """
+        # Check if we are dealing with a URL
+        if '://' in path_or_url:
+            return UrlPatch(pkg, path_or_url, level, **kwargs)
+        # Assume patches are stored in the repository
+        return FilePatch(pkg, path_or_url, level)
 
     def __init__(self, pkg, path_or_url, level):
-        self.pkg_name = pkg.name
+        # Check on level (must be an integer > 0)
+        if not isinstance(level, int) or not level >= 0:
+            raise ValueError("Patch level needs to be a non-negative integer.")
+        # Attributes shared by all patch subclasses
         self.path_or_url = path_or_url
-        self.path = None
-        self.url = None
         self.level = level
+        # self.path needs to be computed by derived classes
+        # before a call to apply
+        self.path = None
 
         if not isinstance(self.level, int) or not self.level >= 0:
             raise ValueError("Patch level needs to be a non-negative integer.")
 
-        if '://' in path_or_url:
-            self.url = path_or_url
-        else:
-            pkg_dir = spack.repo.dirname_for_package_name(self.pkg_name)
-            self.path = join_path(pkg_dir, path_or_url)
-            if not os.path.isfile(self.path):
-                raise NoSuchPatchFileError(pkg_name, self.path)
-
     def apply(self, stage):
-        """Fetch this patch, if necessary, and apply it to the source
-           code in the supplied stage.
+        """Apply the patch at self.path to the source code in the
+        supplied stage
+
+        Args:
+            stage: stage for the package that needs to be patched
         """
         stage.chdir_to_source()
+        # Use -N to allow the same patches to be applied multiple times.
+        _patch = which("patch", required=True)
+        _patch('-s', '-p', str(self.level), '-i', self.path)
+
+
+class FilePatch(Patch):
+    """Describes a patch that is retrieved from a file in the repository"""
+    def __init__(self, pkg, path_or_url, level):
+        super(FilePatch, self).__init__(pkg, path_or_url, level)
 
-        patch_stage = None
-        try:
-            if self.url:
-                # use an anonymous stage to fetch the patch if it is a URL
-                patch_stage = spack.stage.Stage(self.url)
-                patch_stage.fetch()
-                patch_file = patch_stage.archive_file
-            else:
-                patch_file = self.path
-
-            # Use -N to allow the same patches to be applied multiple times.
-            _patch('-s', '-p', str(self.level), '-i', patch_file)
-
-        finally:
-            if patch_stage:
-                patch_stage.destroy()
+        pkg_dir = spack.repo.dirname_for_package_name(pkg.name)
+        self.path = join_path(pkg_dir, path_or_url)
+        if not os.path.isfile(self.path):
+            raise NoSuchPatchFileError(pkg.name, self.path)
+
+
+class UrlPatch(Patch):
+    """Describes a patch that is retrieved from a URL"""
+    def __init__(self, pkg, path_or_url, level, **kwargs):
+        super(UrlPatch, self).__init__(pkg, path_or_url, level)
+        self.url = path_or_url
+        self.md5 = kwargs.get('md5')
+
+    def apply(self, stage):
+        """Retrieve the patch in a temporary stage, computes
+        self.path and calls `super().apply(stage)`
+
+        Args:
+            stage: stage for the package that needs to be patched
+        """
+        fetcher = fs.URLFetchStrategy(self.url, digest=self.md5)
+        mirror = join_path(
+            os.path.dirname(stage.mirror_path),
+            os.path.basename(self.url)
+        )
+        with spack.stage.Stage(fetcher, mirror_path=mirror) as patch_stage:
+            patch_stage.fetch()
+            patch_stage.check()
+            patch_stage.cache_local()
+            patch_stage.expand_archive()
+            self.path = os.path.abspath(
+                os.listdir(patch_stage.path).pop()
+            )
+            super(UrlPatch, self).apply(stage)
 
 
 class NoSuchPatchFileError(spack.error.SpackError):
diff --git a/lib/spack/spack/stage.py b/lib/spack/spack/stage.py
index c0dfbba987..7e6b543799 100644
--- a/lib/spack/spack/stage.py
+++ b/lib/spack/spack/stage.py
@@ -545,6 +545,10 @@ class StageComposite:
     def archive_file(self):
         return self[0].archive_file
 
+    @property
+    def mirror_path(self):
+        return self[0].mirror_path
+
 
 class DIYStage(object):
     """Simple class that allows any directory to be a spack stage."""
diff --git a/lib/spack/spack/test/versions.py b/lib/spack/spack/test/versions.py
index 9b4dc29f35..c1d427783c 100644
--- a/lib/spack/spack/test/versions.py
+++ b/lib/spack/spack/test/versions.py
@@ -392,6 +392,7 @@ class VersionsTest(unittest.TestCase):
             self.assertEqual(v.dotted, '1.2.3')
             self.assertEqual(v.dashed, '1-2-3')
             self.assertEqual(v.underscored, '1_2_3')
+            self.assertEqual(v.joined, '123')
 
     def test_repr_and_str(self):
 
diff --git a/lib/spack/spack/util/compression.py b/lib/spack/spack/util/compression.py
index 982a02d021..806465dc4e 100644
--- a/lib/spack/spack/util/compression.py
+++ b/lib/spack/spack/util/compression.py
@@ -46,6 +46,9 @@ def decompressor_for(path, extension=None):
             path.endswith('.zip')):
         unzip = which('unzip', required=True)
         return unzip
+    if extension and re.match(r'gz', extension):
+        gunzip = which('gunzip', required=True)
+        return gunzip
     tar = which('tar', required=True)
     tar.add_default_arg('-xf')
     return tar
diff --git a/lib/spack/spack/version.py b/lib/spack/spack/version.py
index 67a22f4660..0d68a709e8 100644
--- a/lib/spack/spack/version.py
+++ b/lib/spack/spack/version.py
@@ -146,6 +146,10 @@ class Version(object):
     def dashed(self):
         return '-'.join(str(x) for x in self.version)
 
+    @property
+    def joined(self):
+        return ''.join(str(x) for x in self.version)
+
     def up_to(self, index):
         """Return a version string up to the specified component, exclusive.
            e.g., if this is 10.8.2, self.up_to(2) will return '10.8'.