patch directive : fixed retrieval from urls ( fixes #1584 ) (#2039)

* patch directive : fixed retrieval from urls fixes #1584

 - add support for 'gz' archives
 - fixed bugs with URL patches
 - updated nwchem

* patch directive : added checksum to UrlPatch

 - refactored classes in patch.py
 - updated nwchem

* patch directive : added caching

5 files changed, 94 insertions, 40 deletions

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/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