patches: add a per-repository patch index

- this fixes a bug where if we save a concretized sug-DAG where a package
  had been patched by a dependent, and the dependent was not in the DAG,
  we would not read in all patches correctly.

- Rather than looking up patches in the DAG, we look them up globally
  from an index created from the entire repository.

- The patch cache is a bit tricky for several reasons:

  - we have to cache information from packages, specifically, the patch
    level and working directory.

  - FilePatches need to know which package owns them, so that they can
    figure out where the patch lives.  The repo can change locations from
    run to run, so we have to store relative paths and restore them when
    the cache is reloaded.

  - Patch files can change underneath the cache, because repo indexes
    only update on package changes.  We currently punt on this -- there
    are stub methods for needs_update() that will need to check patch
    files when packages are loaded.  There isn't an easy way to do this
    at global indexing time without making the FastPackageChecker a lot
    slower.  This is TBD for a future commit.

  - Currently, the same patch can only be used one way in a package. That
    is, if it appears twice with different level/working_dir settings,
    bad things will happen.  There's no package that current uses the
    same patch two different ways, so we've punted on this as well, but
    we may need to fix this in the future by moving a lot of the metdata
    (level, working dir) to the spec, and *only* caching sha256sums in
    the PatchCache.  That would require some much more complicated tweaks
    to the Spec, so we're holding off on that til later.

- This required patches to be refactored somewhat -- the difference
  between a UrlPatch and a FilePatch is still not particularly clean.

8 files changed, 339 insertions, 116 deletions

diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 57492f5c1c..5b2bf652ed 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -395,7 +395,7 @@ def patch(url_or_filename, level=1, when=None, working_dir=".", **kwargs):
     certain conditions (e.g. a particular version).
 
     Args:
-        url_or_filename (str): url or filename of the patch
+        url_or_filename (str): url or relative filename of the patch
         level (int): patch level (as in the patch shell command)
         when (Spec): optional anonymous spec that specifies when to apply
             the patch
@@ -417,13 +417,18 @@ def patch(url_or_filename, level=1, when=None, working_dir=".", **kwargs):
         # patch to the existing list.
         cur_patches = pkg_or_dep.patches.setdefault(when_spec, [])
 
-        # if pkg_or_dep is a Dependency, make it a Package
         pkg = pkg_or_dep
         if isinstance(pkg, Dependency):
             pkg = pkg.pkg
 
-        cur_patches.append(spack.patch.create(
-            pkg, url_or_filename, level, working_dir, **kwargs))
+        if '://' in url_or_filename:
+            patch = spack.patch.UrlPatch(
+                pkg, url_or_filename, level, working_dir, **kwargs)
+        else:
+            patch = spack.patch.FilePatch(
+                pkg, url_or_filename, level, working_dir)
+
+        cur_patches.append(patch)
 
     return _execute_patch
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 7df500039e..4262ac14aa 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -223,6 +223,11 @@ class PackageMeta(
             namespace = namespace[len(prefix):]
         return namespace
 
+    @property
+    def fullname(self):
+        """Name of this package, including the namespace"""
+        return '%s.%s' % (self.namespace, self.name)
+
 
 def run_before(*phases):
     """Registers a method of a package to be run before a given phase"""
@@ -568,6 +573,11 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         return type(self).namespace
 
     @property
+    def fullname(self):
+        """Name of this package, including namespace: namespace.name."""
+        return type(self).fullname
+
+    @property
     def global_license_dir(self):
         """Returns the directory where global license files for all
            packages are stored."""
@@ -968,40 +978,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         if not os.listdir(self.stage.path):
             raise FetchError("Archive was empty for %s" % self.name)
 
-    @classmethod
-    def lookup_patch(cls, sha256):
-        """Look up a patch associated with this package by its sha256 sum.
-
-        Args:
-            sha256 (str): sha256 sum of the patch to look up
-
-        Returns:
-            (Patch): ``Patch`` object with the given hash, or ``None`` if
-                not found.
-
-        To do the lookup, we build an index lazily.  This allows us to
-        avoid computing a sha256 for *every* patch and on every package
-        load.  With lazy hashing, we only compute hashes on lookup, which
-        usually happens at build time.
-
-        """
-        if cls._patches_by_hash is None:
-            cls._patches_by_hash = {}
-
-            # Add patches from the class
-            for cond, patch_list in cls.patches.items():
-                for patch in patch_list:
-                    cls._patches_by_hash[patch.sha256] = patch
-
-            # and patches on dependencies
-            for name, conditions in cls.dependencies.items():
-                for cond, dependency in conditions.items():
-                    for pcond, patch_list in dependency.patches.items():
-                        for patch in patch_list:
-                            cls._patches_by_hash[patch.sha256] = patch
-
-        return cls._patches_by_hash.get(sha256, None)
-
     def do_patch(self):
         """Applies patches if they haven't been applied already."""
         if not self.spec.concrete:
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index 69949564c5..ec52a3705d 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -3,41 +3,24 @@
 #
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
+import abc
+import hashlib
 import os
 import os.path
-import hashlib
+import six
 
 import llnl.util.filesystem
+import llnl.util.lang
 
 import spack.error
 import spack.fetch_strategy as fs
+import spack.repo
 import spack.stage
-from spack.util.crypto import checksum, Checker
+import spack.util.spack_json as sjson
 
-from spack.util.executable import which
 from spack.util.compression import allowed_archive
-
-
-def create(pkg, path_or_url, level=1, working_dir=".", **kwargs):
-    """Make either a FilePatch or a UrlPatch, depending on arguments.
-
-      Args:
-          pkg: package that needs to be patched
-          path_or_url: path or url where the patch is found
-          level: patch level (default 1)
-          working_dir (str): relative path within the package stage;
-              change to this before before applying (default '.')
-
-      Returns:
-          (Patch): a patch object on which ``apply(stage)`` can be called
-    """
-    # Check if we are dealing with a URL (which will be fetched)
-    if '://' in path_or_url:
-        return UrlPatch(path_or_url, level, working_dir, **kwargs)
-
-    # If not, it's a file patch, which is stored within the repo directory.
-    patch_path = os.path.join(pkg.package_dir, path_or_url)
-    return FilePatch(patch_path, level, working_dir)
+from spack.util.crypto import checksum, Checker
+from spack.util.executable import which
 
 
 def apply_patch(stage, patch_path, level=1, working_dir='.'):
@@ -58,54 +41,96 @@ def apply_patch(stage, patch_path, level=1, working_dir='.'):
               '-d', working_dir)
 
 
+@six.add_metaclass(abc.ABCMeta)
 class Patch(object):
     """Base class for patches.
 
-    Defines the interface (basically just ``apply()``, at the moment) and
-    common variables.
+    Arguments:
+        pkg (str): the package that owns the patch
+
+    The owning package is not necessarily the package to apply the patch
+    to -- in the case where a dependent package patches its dependency,
+    it is the dependent's fullname.
+
     """
-    def __init__(self, path_or_url, level, working_dir):
+    def __init__(self, pkg, path_or_url, level, working_dir):
         # validate 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.owner = pkg.fullname
         self.path_or_url = path_or_url  # needed for debug output
         self.level = level
         self.working_dir = working_dir
 
-        # path needs to be set by subclasses before calling self.apply()
-        self.path = None
-
+    @abc.abstractmethod
     def apply(self, stage):
-        """Apply this patch to code in a stage."""
-        assert self.path, "self.path must be set before Patch.apply()"
-        apply_patch(stage, self.path, self.level, self.working_dir)
+        """Apply a patch to source in a stage.
+
+        Arguments:
+            stage (spack.stage.Stage): stage where source code lives
+        """
 
+    def to_dict(self):
+        """Partial dictionary -- subclases should add to this."""
+        return {
+            'owner': self.owner,
+            'sha256': self.sha256,
+            'level': self.level,
+            'working_dir': self.working_dir,
+        }
 
-class FilePatch(Patch):
-    """Describes a patch that is retrieved from a file in the repository"""
-    def __init__(self, path, level, working_dir):
-        super(FilePatch, self).__init__(path, level, working_dir)
 
-        if not os.path.isfile(path):
-            raise NoSuchPatchError("No such patch: %s" % path)
-        self.path = path
+class FilePatch(Patch):
+    """Describes a patch that is retrieved from a file in the repository.
+
+    Arguments:
+        pkg (str): the class object for the package that owns the patch
+        relative_path (str): path to patch, relative to the repository
+            directory for a package.
+        level (int): level to pass to patch command
+        working_dir (str): path within the source directory where patch
+            should be applied
+    """
+    def __init__(self, pkg, relative_path, level, working_dir):
+        self.relative_path = relative_path
+        self.path = os.path.join(pkg.package_dir, self.relative_path)
+        super(FilePatch, self).__init__(pkg, self.path, level, working_dir)
         self._sha256 = None
 
+    def apply(self, stage):
+        if not os.path.isfile(self.path):
+            raise NoSuchPatchError("No such patch: %s" % self.path)
+        apply_patch(stage, self.path, self.level, self.working_dir)
+
     @property
     def sha256(self):
         if self._sha256 is None:
             self._sha256 = checksum(hashlib.sha256, self.path)
         return self._sha256
 
+    def to_dict(self):
+        return llnl.util.lang.union_dicts(
+            super(FilePatch, self).to_dict(),
+            {'relative_path': self.relative_path})
+
 
 class UrlPatch(Patch):
-    """Describes a patch that is retrieved from a URL"""
-    def __init__(self, url, level, working_dir, **kwargs):
-        super(UrlPatch, self).__init__(url, level, working_dir)
+    """Describes a patch that is retrieved from a URL.
+
+    Arguments:
+        pkg (str): the package that owns the patch
+        url (str): URL where the patch can be fetched
+        level (int): level to pass to patch command
+        working_dir (str): path within the source directory where patch
+            should be applied
+    """
+    def __init__(self, pkg, url, level=1, working_dir='.', **kwargs):
+        super(UrlPatch, self).__init__(pkg, url, level, working_dir)
 
         self.url = url
+        self.path = None  # this is set in apply
 
         self.archive_sha256 = kwargs.get('archive_sha256')
         if allowed_archive(self.url) and not self.archive_sha256:
@@ -153,6 +178,7 @@ class UrlPatch(Patch):
                     raise NoSuchPatchError(
                         "Patch failed to download: %s" % self.url)
 
+            # set this here so that path is accessible after
             self.path = os.path.join(root, files.pop())
 
             if not os.path.isfile(self.path):
@@ -168,7 +194,170 @@ class UrlPatch(Patch):
                         "sha256 checksum failed for %s" % self.path,
                         "Expected %s but got %s" % (self.sha256, checker.sum))
 
-            super(UrlPatch, self).apply(stage)
+            apply_patch(stage, self.path, self.level, self.working_dir)
+
+    def to_dict(self):
+        data = super(UrlPatch, self).to_dict()
+        data['url'] = self.url
+        if self.archive_sha256:
+            data['archive_sha256'] = self.archive_sha256
+        return data
+
+
+def from_dict(dictionary):
+    """Create a patch from json dictionary."""
+    owner = dictionary.get('owner')
+    if 'owner' not in dictionary:
+        raise ValueError('Invalid patch dictionary: %s' % dictionary)
+    pkg = spack.repo.get(owner)
+
+    if 'url' in dictionary:
+        return UrlPatch(
+            pkg,
+            dictionary['url'],
+            dictionary['level'],
+            dictionary['working_dir'],
+            sha256=dictionary['sha256'],
+            archive_sha256=dictionary.get('archive_sha256'))
+
+    elif 'relative_path' in dictionary:
+        patch = FilePatch(
+            pkg,
+            dictionary['relative_path'],
+            dictionary['level'],
+            dictionary['working_dir'])
+
+        # If the patch in the repo changes, we cannot get it back, so we
+        # just check it and fail here.
+        # TODO: handle this more gracefully.
+        sha256 = dictionary['sha256']
+        checker = Checker(sha256)
+        if not checker.check(patch.path):
+            raise fs.ChecksumError(
+                "sha256 checksum failed for %s" % patch.path,
+                "Expected %s but got %s" % (sha256, checker.sum),
+                "Patch may have changed since concretization.")
+        return patch
+    else:
+        raise ValueError("Invalid patch dictionary: %s" % dictionary)
+
+
+class PatchCache(object):
+    """Index of patches used in a repository, by sha256 hash.
+
+    This allows us to look up patches without loading all packages.  It's
+    also needed to properly implement dependency patching, as need a way
+    to look up patches that come from packages not in the Spec sub-DAG.
+
+    The patch index is structured like this in a file (this is YAML, but
+    we write JSON)::
+
+        patches:
+            sha256:
+                namespace1.package1:
+                    <patch json>
+                namespace2.package2:
+                    <patch json>
+                ... etc. ...
+
+    """
+    def __init__(self, data=None):
+        if data is None:
+            self.index = {}
+        else:
+            if 'patches' not in data:
+                raise IndexError('invalid patch index; try `spack clean -m`')
+            self.index = data['patches']
+
+    @classmethod
+    def from_json(cls, stream):
+        return PatchCache(sjson.load(stream))
+
+    def to_json(self, stream):
+        sjson.dump({'patches': self.index}, stream)
+
+    def patch_for_package(self, sha256, pkg):
+        """Look up a patch in the index and build a patch object for it.
+
+        Arguments:
+            sha256 (str): sha256 hash to look up
+            pkg (spack.package.Package): Package object to get patch for.
+
+        We build patch objects lazily because building them requires that
+        we have information about the package's location in its repo.
+
+        """
+        sha_index = self.index.get(sha256)
+        if not sha_index:
+            raise NoSuchPatchError(
+                "Couldn't find patch with sha256: %s" % sha256)
+
+        patch_dict = sha_index.get(pkg.fullname)
+        if not patch_dict:
+            raise NoSuchPatchError(
+                "Couldn't find patch for package %s with sha256: %s"
+                % (pkg.fullname, sha256))
+
+        # add the sha256 back (we take it out on write to save space,
+        # because it's the index key)
+        patch_dict = dict(patch_dict)
+        patch_dict['sha256'] = sha256
+        return from_dict(patch_dict)
+
+    def update_package(self, pkg_fullname):
+        # remove this package from any patch entries that reference it.
+        empty = []
+        for sha256, package_to_patch in self.index.items():
+            remove = []
+            for fullname, patch_dict in package_to_patch.items():
+                if patch_dict['owner'] == pkg_fullname:
+                    remove.append(fullname)
+
+            for fullname in remove:
+                package_to_patch.pop(fullname)
+
+            if not package_to_patch:
+                empty.append(sha256)
+
+        # remove any entries that are now empty
+        for sha256 in empty:
+            del self.index[sha256]
+
+        # update the index with per-package patch indexes
+        pkg = spack.repo.get(pkg_fullname)
+        partial_index = self._index_patches(pkg)
+        for sha256, package_to_patch in partial_index.items():
+            p2p = self.index.setdefault(sha256, {})
+            p2p.update(package_to_patch)
+
+    def update(self, other):
+        """Update this cache with the contents of another."""
+        for sha256, package_to_patch in other.index.items():
+            p2p = self.index.setdefault(sha256, {})
+            p2p.update(package_to_patch)
+
+    @staticmethod
+    def _index_patches(pkg_class):
+        index = {}
+
+        # Add patches from the class
+        for cond, patch_list in pkg_class.patches.items():
+            for patch in patch_list:
+                patch_dict = patch.to_dict()
+                patch_dict.pop('sha256')  # save some space
+                index[patch.sha256] = {pkg_class.fullname: patch_dict}
+
+        # and patches on dependencies
+        for name, conditions in pkg_class.dependencies.items():
+            for cond, dependency in conditions.items():
+                for pcond, patch_list in dependency.patches.items():
+                    for patch in patch_list:
+                        dspec = spack.repo.get(dependency.spec.name)
+                        patch_dict = patch.to_dict()
+                        patch_dict.pop('sha256')  # save some space
+                        index[patch.sha256] = {dspec.fullname: patch_dict}
+
+        return index
 
 
 class NoSuchPatchError(spack.error.SpackError):
diff --git a/lib/spack/spack/repo.py b/lib/spack/spack/repo.py
index 663edf2bf8..cc76c08416 100644
--- a/lib/spack/spack/repo.py
+++ b/lib/spack/spack/repo.py
@@ -13,7 +13,6 @@ import sys
 import inspect
 import re
 import traceback
-import json
 from contextlib import contextmanager
 from six import string_types, add_metaclass
 
@@ -33,7 +32,9 @@ from llnl.util.filesystem import mkdirp, install
 import spack.config
 import spack.caches
 import spack.error
+import spack.patch
 import spack.spec
+import spack.util.spack_json as sjson
 import spack.util.imp as simp
 from spack.provider_index import ProviderIndex
 from spack.util.path import canonicalize_path
@@ -190,11 +191,11 @@ class TagIndex(Mapping):
         self._tag_dict = collections.defaultdict(list)
 
     def to_json(self, stream):
-        json.dump({'tags': self._tag_dict}, stream)
+        sjson.dump({'tags': self._tag_dict}, stream)
 
     @staticmethod
     def from_json(stream):
-        d = json.load(stream)
+        d = sjson.load(stream)
 
         r = TagIndex()
 
@@ -242,6 +243,22 @@ class Indexer(object):
     def _create(self):
         """Create an empty index and return it."""
 
+    def needs_update(self, pkg):
+        """Whether an update is needed when the package file hasn't changed.
+
+        Returns:
+            (bool): ``True`` if this package needs its index
+                updated, ``False`` otherwise.
+
+        We already automatically update indexes when package files
+        change, but other files (like patches) may change underneath the
+        package file. This method can be used to check additional
+        package-specific files whenever they're loaded, to tell the
+        RepoIndex to update the index *just* for that package.
+
+        """
+        return False
+
     @abc.abstractmethod
     def read(self, stream):
         """Read this index from a provided file object."""
@@ -286,6 +303,28 @@ class ProviderIndexer(Indexer):
         self.index.to_json(stream)
 
 
+class PatchIndexer(Indexer):
+    """Lifecycle methods for patch cache."""
+    def _create(self):
+        return spack.patch.PatchCache()
+
+    def needs_update():
+        # TODO: patches can change under a package and we should handle
+        # TODO: it, but we currently punt. This should be refactored to
+        # TODO: check whether patches changed each time a package loads,
+        # TODO: tell the RepoIndex to reindex them.
+        return False
+
+    def read(self, stream):
+        self.index = spack.patch.PatchCache.from_json(stream)
+
+    def write(self, stream):
+        self.index.to_json(stream)
+
+    def update(self, pkg_fullname):
+        self.index.update_package(pkg_fullname)
+
+
 class RepoIndex(object):
     """Container class that manages a set of Indexers for a Repo.
 
@@ -403,6 +442,7 @@ class RepoPath(object):
 
         self._all_package_names = None
         self._provider_index = None
+        self._patch_index = None
 
         # Add each repo to this path.
         for repo in repos:
@@ -501,6 +541,16 @@ class RepoPath(object):
 
         return self._provider_index
 
+    @property
+    def patch_index(self):
+        """Merged PatchIndex from all Repos in the RepoPath."""
+        if self._patch_index is None:
+            self._patch_index = spack.patch.PatchCache()
+            for repo in reversed(self.repos):
+                self._patch_index.update(repo.patch_index)
+
+        return self._patch_index
+
     @_autospec
     def providers_for(self, vpkg_spec):
         providers = self.provider_index.providers_for(vpkg_spec)
@@ -870,15 +920,14 @@ class Repo(object):
                 "Repository %s does not contain package %s."
                 % (self.namespace, spec.fullname))
 
-        # Install any patch files needed by packages.
+        # Install patch files needed by the package.
         mkdirp(path)
-        for spec, patches in spec.package.patches.items():
-            for patch in patches:
-                if patch.path:
-                    if os.path.exists(patch.path):
-                        install(patch.path, path)
-                    else:
-                        tty.warn("Patch file did not exist: %s" % patch.path)
+        for patch in spec.patches:
+            if patch.path:
+                if os.path.exists(patch.path):
+                    install(patch.path, path)
+                else:
+                    tty.warn("Patch file did not exist: %s" % patch.path)
 
         # Install the package.py file itself.
         install(self.filename_for_package_name(spec), path)
@@ -894,6 +943,7 @@ class Repo(object):
             self._repo_index = RepoIndex(self._pkg_checker, self.namespace)
             self._repo_index.add_indexer('providers', ProviderIndexer())
             self._repo_index.add_indexer('tags', TagIndexer())
+            self._repo_index.add_indexer('patches', PatchIndexer())
         return self._repo_index
 
     @property
@@ -906,6 +956,11 @@ class Repo(object):
         """Index of tags and which packages they're defined on."""
         return self.index['tags']
 
+    @property
+    def patch_index(self):
+        """Index of patches and packages they're defined on."""
+        return self.index['patches']
+
     @_autospec
     def providers_for(self, vpkg_spec):
         providers = self.provider_index.providers_for(vpkg_spec)
@@ -1200,6 +1255,10 @@ class UnknownEntityError(RepoError):
     """Raised when we encounter a package spack doesn't have."""
 
 
+class IndexError(RepoError):
+    """Raised when there's an error with an index."""
+
+
 class UnknownPackageError(UnknownEntityError):
     """Raised when we encounter a package spack doesn't have."""
 
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 5563b94e3f..dee2f2e338 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -2677,24 +2677,16 @@ class Spec(object):
         if 'patches' not in self.variants:
             return []
 
-        patches = []
-
-        # FIXME: The private attribute below is attached after
+        # FIXME: _patches_in_order_of_appearance is attached after
         # FIXME: concretization to store the order of patches somewhere.
         # FIXME: Needs to be refactored in a cleaner way.
-        for sha256 in self.variants['patches']._patches_in_order_of_appearance:
-            patch = self.package_class.lookup_patch(sha256)
-            if patch:
-                patches.append(patch)
-                continue
 
-            # if not found in this package, check immediate dependents
-            # for dependency patches
-            for dep_spec in self._dependents.values():
-                patch = dep_spec.parent.package_class.lookup_patch(sha256)
-
-                if patch:
-                    patches.append(patch)
+        # translate patch sha256sums to patch objects by consulting the index
+        patches = []
+        for sha256 in self.variants['patches']._patches_in_order_of_appearance:
+            index = spack.repo.path.patch_index
+            patch = index.patch_for_package(sha256, self.package)
+            patches.append(patch)
 
         return patches
 
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index 94b7ff51ff..85c860131a 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -101,13 +101,14 @@ def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
     dependency.concretize()
     dependency.package.do_install()
 
-    dependency.package.patches['dependency-install'] = [
-        spack.patch.create(None, 'file://fake.patch', sha256='unused-hash')]
-
     dependency_hash = dependency.dag_hash()
     dependent = Spec('dependent-install ^/' + dependency_hash)
     dependent.concretize()
 
+    dependency.package.patches['dependency-install'] = [
+        spack.patch.UrlPatch(
+            dependent.package, 'file://fake.patch', sha256='unused-hash')]
+
     assert dependent['dependency-install'] == dependency
 
 
diff --git a/lib/spack/spack/test/mirror.py b/lib/spack/spack/test/mirror.py
index e2ddb85c97..a29454b4bd 100644
--- a/lib/spack/spack/test/mirror.py
+++ b/lib/spack/spack/test/mirror.py
@@ -160,7 +160,7 @@ def test_mirror_with_url_patches(mock_packages, config, monkeypatch):
         with open(os.path.join(expanded_path, 'test.patch'), 'w'):
             pass
 
-    def successful_apply(_class, stage):
+    def successful_apply(*args, **kwargs):
         pass
 
     with Stage('spack-mirror-test') as stage:
@@ -170,7 +170,7 @@ def test_mirror_with_url_patches(mock_packages, config, monkeypatch):
                             successful_fetch)
         monkeypatch.setattr(spack.fetch_strategy.URLFetchStrategy,
                             'expand', successful_expand)
-        monkeypatch.setattr(spack.patch.Patch, 'apply', successful_apply)
+        monkeypatch.setattr(spack.patch, 'apply_patch', successful_apply)
         monkeypatch.setattr(spack.caches.MirrorCache, 'store', record_store)
 
         with spack.config.override('config:checksum', False):
diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py
index 724924edf3..061d12b3bc 100644
--- a/lib/spack/spack/test/patch.py
+++ b/lib/spack/spack/test/patch.py
@@ -41,8 +41,9 @@ data_path = os.path.join(spack.paths.test_path, 'data', 'patch')
 def test_url_patch(mock_stage, filename, sha256, archive_sha256):
     # Make a patch object
     url = 'file://' + filename
-    patch = spack.patch.create(
-        None, url, sha256=sha256, archive_sha256=archive_sha256)
+    pkg = spack.repo.get('patch')
+    patch = spack.patch.UrlPatch(
+        pkg, url, sha256=sha256, archive_sha256=archive_sha256)
 
     # make a stage
     with Stage(url) as stage:  # TODO: url isn't used; maybe refactor Stage