patches: clean up patch.py, directives, and package class properties

- cleanup patch.py:
  - make patch.py constructors more understandable
  - loosen coupling of patch.py with package

- in Package: make package_dir, module, and namespace class properties

  - These were previously instance properties and couldn't be called from
    directives, e.g. in patch.create()

  - make them class properties so that they can be used in class definition

  - also add some instance properties to delegate to class properties so
    that prior usage on Package objects still works

7 files changed, 121 insertions, 103 deletions

diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py
index 9125d55e9a..57492f5c1c 100644
--- a/lib/spack/spack/directives.py
+++ b/lib/spack/spack/directives.py
@@ -36,12 +36,12 @@ from six import string_types
 import llnl.util.lang
 
 import spack.error
+import spack.patch
 import spack.spec
 import spack.url
 import spack.variant
 from spack.dependency import Dependency, default_deptype, canonical_deptype
 from spack.fetch_strategy import from_kwargs
-from spack.patch import Patch
 from spack.resource import Resource
 from spack.version import Version
 
@@ -416,9 +416,14 @@ def patch(url_or_filename, level=1, when=None, working_dir=".", **kwargs):
         # if this spec is identical to some other, then append this
         # patch to the existing list.
         cur_patches = pkg_or_dep.patches.setdefault(when_spec, [])
-        cur_patches.append(
-            Patch.create(pkg_or_dep, url_or_filename, level,
-                         working_dir, **kwargs))
+
+        # 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))
 
     return _execute_patch
 
diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py
index 279ed89d04..7df500039e 100644
--- a/lib/spack/spack/package.py
+++ b/lib/spack/spack/package.py
@@ -200,6 +200,29 @@ class PackageMeta(
             return func
         return _decorator
 
+    @property
+    def package_dir(self):
+        """Directory where the package.py file lives."""
+        return os.path.abspath(os.path.dirname(self.module.__file__))
+
+    @property
+    def module(self):
+        """Module object (not just the name) that this package is defined in.
+
+        We use this to add variables to package modules.  This makes
+        install() methods easier to write (e.g., can call configure())
+        """
+        return __import__(self.__module__, fromlist=[self.__name__])
+
+    @property
+    def namespace(self):
+        """Spack namespace for the package, which identifies its repo."""
+        namespace, dot, module = self.__module__.rpartition('.')
+        prefix = '%s.' % spack.repo.repo_namespace
+        if namespace.startswith(prefix):
+            namespace = namespace[len(prefix):]
+        return namespace
+
 
 def run_before(*phases):
     """Registers a method of a package to be run before a given phase"""
@@ -527,10 +550,22 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
 
         return visited
 
+    # package_dir and module are *class* properties (see PackageMeta),
+    # but to make them work on instances we need these defs as well.
     @property
     def package_dir(self):
-        """Return the directory where the package.py file lives."""
-        return os.path.abspath(os.path.dirname(self.module.__file__))
+        """Directory where the package.py file lives."""
+        return type(self).package_dir
+
+    @property
+    def module(self):
+        """Module object that this package is defined in."""
+        return type(self).module
+
+    @property
+    def namespace(self):
+        """Spack namespace for the package, which identifies its repo."""
+        return type(self).namespace
 
     @property
     def global_license_dir(self):
@@ -1081,11 +1116,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
             hashlib.sha256(bytes().join(
                 sorted(hash_content))).digest()).lower()
 
-    @property
-    def namespace(self):
-        namespace, dot, module = self.__module__.rpartition('.')
-        return namespace
-
     def do_fake_install(self):
         """Make a fake install directory containing fake executables,
         headers, and libraries."""
@@ -1724,14 +1754,6 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)):
         else:
             return os.path.join(self.stage.source_path, 'spack-build.out')
 
-    @property
-    def module(self):
-        """Use this to add variables to the class's module's scope.
-           This lets us use custom syntax in the install method.
-        """
-        return __import__(self.__class__.__module__,
-                          fromlist=[self.__class__.__name__])
-
     @classmethod
     def inject_flags(cls, name, flags):
         """
diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py
index cbab403f20..69949564c5 100644
--- a/lib/spack/spack/patch.py
+++ b/lib/spack/spack/patch.py
@@ -5,96 +5,92 @@
 
 import os
 import os.path
-import inspect
 import hashlib
 
+import llnl.util.filesystem
+
 import spack.error
 import spack.fetch_strategy as fs
 import spack.stage
 from spack.util.crypto import checksum, Checker
-from llnl.util.filesystem import working_dir
+
 from spack.util.executable import which
 from spack.util.compression import allowed_archive
 
 
-def absolute_path_for_package(pkg):
-    """Returns the absolute path to the ``package.py`` file implementing
-    the recipe for the package passed as argument.
+def create(pkg, path_or_url, level=1, working_dir=".", **kwargs):
+    """Make either a FilePatch or a UrlPatch, depending on arguments.
 
-    Args:
-        pkg: a valid package object, or a Dependency object.
+      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
     """
-    if isinstance(pkg, spack.dependency.Dependency):
-        pkg = pkg.pkg
-    m = inspect.getmodule(pkg)
-    return os.path.abspath(m.__file__)
+    # 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)
 
-class Patch(object):
-    """Base class to describe a patch that needs to be applied to some
-    expanded source code.
-    """
 
-    @staticmethod
-    def create(pkg, path_or_url, level=1, working_dir=".", **kwargs):
-        """
-        Factory method that creates an instance of some class derived from
-        Patch
+def apply_patch(stage, patch_path, level=1, working_dir='.'):
+    """Apply the patch at patch_path to code in the stage.
 
-        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): dir to change to before applying (default '.')
+    Args:
+        stage (spack.stage.Stage): stage with code that will be patched
+        patch_path (str): filesystem location for the patch to apply
+        level (int, optional): patch level (default 1)
+        working_dir (str): relative path *within* the stage to change to
+            (default '.')
+    """
+    patch = which("patch", required=True)
+    with llnl.util.filesystem.working_dir(stage.source_path):
+        patch('-s',
+              '-p', str(level),
+              '-i', patch_path,
+              '-d', working_dir)
 
-        Returns:
-            instance of some Patch class
-        """
-        # Check if we are dealing with a URL
-        if '://' in path_or_url:
-            return UrlPatch(path_or_url, level, working_dir, **kwargs)
-        # Assume patches are stored in the repository
-        return FilePatch(pkg, path_or_url, level, working_dir)
 
+class Patch(object):
+    """Base class for patches.
+
+    Defines the interface (basically just ``apply()``, at the moment) and
+    common variables.
+    """
     def __init__(self, path_or_url, level, working_dir):
-        # Check on level (must be an integer > 0)
+        # 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.path_or_url = path_or_url
+        self.path_or_url = path_or_url  # needed for debug output
         self.level = level
         self.working_dir = working_dir
-        # 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.")
+        # path needs to be set by subclasses before calling self.apply()
+        self.path = None
 
     def apply(self, 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
-        """
-        patch = which("patch", required=True)
-        with working_dir(stage.source_path):
-            # Use -N to allow the same patches to be applied multiple times.
-            patch('-s', '-p', str(self.level), '-i', self.path,
-                  "-d", self.working_dir)
+        """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)
 
 
 class FilePatch(Patch):
     """Describes a patch that is retrieved from a file in the repository"""
-    def __init__(self, pkg, path_or_url, level, working_dir):
-        super(FilePatch, self).__init__(path_or_url, level, working_dir)
-
-        pkg_dir = os.path.dirname(absolute_path_for_package(pkg))
-        self.path = os.path.join(pkg_dir, path_or_url)
-        if not os.path.isfile(self.path):
-            raise NoSuchPatchError(
-                "No such patch for package %s: %s" % (pkg.name, self.path))
+    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
         self._sha256 = None
 
     @property
@@ -106,21 +102,20 @@ class FilePatch(Patch):
 
 class UrlPatch(Patch):
     """Describes a patch that is retrieved from a URL"""
-    def __init__(self, path_or_url, level, working_dir, **kwargs):
-        super(UrlPatch, self).__init__(path_or_url, level, working_dir)
-        self.url = path_or_url
-
-        self.archive_sha256 = None
-        if allowed_archive(self.url):
-            if 'archive_sha256' not in kwargs:
-                raise PatchDirectiveError(
-                    "Compressed patches require 'archive_sha256' "
-                    "and patch 'sha256' attributes: %s" % self.url)
-            self.archive_sha256 = kwargs.get('archive_sha256')
-
-        if 'sha256' not in kwargs:
-            raise PatchDirectiveError("URL patches require a sha256 checksum")
+    def __init__(self, url, level, working_dir, **kwargs):
+        super(UrlPatch, self).__init__(url, level, working_dir)
+
+        self.url = url
+
+        self.archive_sha256 = kwargs.get('archive_sha256')
+        if allowed_archive(self.url) and not self.archive_sha256:
+            raise PatchDirectiveError(
+                "Compressed patches require 'archive_sha256' "
+                "and patch 'sha256' attributes: %s" % self.url)
+
         self.sha256 = kwargs.get('sha256')
+        if not self.sha256:
+            raise PatchDirectiveError("URL patches require a sha256 checksum")
 
     def apply(self, stage):
         """Retrieve the patch in a temporary stage, computes
diff --git a/lib/spack/spack/test/cmd/env.py b/lib/spack/spack/test/cmd/env.py
index 7085373392..965f16caa2 100644
--- a/lib/spack/spack/test/cmd/env.py
+++ b/lib/spack/spack/test/cmd/env.py
@@ -232,7 +232,7 @@ def test_env_repo():
 
     package = e.repo.get('mpileaks')
     assert package.name == 'mpileaks'
-    assert package.namespace == 'spack.pkg.builtin.mock'
+    assert package.namespace == 'builtin.mock'
 
 
 def test_user_removed_spec():
diff --git a/lib/spack/spack/test/install.py b/lib/spack/spack/test/install.py
index a63c149d3d..94b7ff51ff 100644
--- a/lib/spack/spack/test/install.py
+++ b/lib/spack/spack/test/install.py
@@ -6,6 +6,7 @@
 import os
 import pytest
 
+import spack.patch
 import spack.repo
 import spack.store
 from spack.spec import Spec
@@ -96,14 +97,12 @@ def test_partial_install_delete_prefix_and_stage(install_mockery, mock_fetch):
 
 
 def test_dont_add_patches_to_installed_package(install_mockery, mock_fetch):
-    import sys
     dependency = Spec('dependency-install')
     dependency.concretize()
     dependency.package.do_install()
 
     dependency.package.patches['dependency-install'] = [
-        sys.modules['spack.patch'].Patch.create(
-            None, 'file://fake.patch', sha256='unused-hash')]
+        spack.patch.create(None, 'file://fake.patch', sha256='unused-hash')]
 
     dependency_hash = dependency.dag_hash()
     dependent = Spec('dependent-install ^/' + dependency_hash)
diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py
index e606c9113a..724924edf3 100644
--- a/lib/spack/spack/test/patch.py
+++ b/lib/spack/spack/test/patch.py
@@ -4,12 +4,12 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import os
-import sys
 import filecmp
 import pytest
 
 from llnl.util.filesystem import working_dir, mkdirp
 
+import spack.patch
 import spack.paths
 import spack.util.compression
 from spack.util.executable import Executable
@@ -41,8 +41,7 @@ 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
-    m = sys.modules['spack.patch']
-    patch = m.Patch.create(
+    patch = spack.patch.create(
         None, url, sha256=sha256, archive_sha256=archive_sha256)
 
     # make a stage
diff --git a/var/spack/repos/builtin/packages/catalyst/package.py b/var/spack/repos/builtin/packages/catalyst/package.py
index 03f9e4dbcf..01983c8687 100644
--- a/var/spack/repos/builtin/packages/catalyst/package.py
+++ b/var/spack/repos/builtin/packages/catalyst/package.py
@@ -7,7 +7,6 @@ from spack import *
 import os
 import subprocess
 import llnl.util.tty as tty
-from spack.patch import absolute_path_for_package
 
 
 class Catalyst(CMakePackage):
@@ -18,7 +17,7 @@ class Catalyst(CMakePackage):
     homepage = 'http://www.paraview.org'
     url      = "http://www.paraview.org/files/v5.5/ParaView-v5.5.2.tar.gz"
     _urlfmt_gz = 'http://www.paraview.org/files/v{0}/ParaView-v{1}{2}.tar.gz'
-    _urlfmt_xz = 'http://www.paraview.org/files/v{0}/ParaView-v{1}{2}.tar.xz' 
+    _urlfmt_xz = 'http://www.paraview.org/files/v{0}/ParaView-v{1}{2}.tar.xz'
 
     version('5.5.2', '7eb93c31a1e5deb7098c3b4275e53a4a')
     version('5.5.1', 'a7d92a45837b67c3371006cc45163277')
@@ -52,11 +51,10 @@ class Catalyst(CMakePackage):
         at the package dir to the source code in
         root_cmakelists_dir."""
         patch_name = 'vtkm-catalyst-pv551.patch'
-        pkg_dir = os.path.dirname(absolute_path_for_package(self))
         patch = which("patch", required=True)
         with working_dir(self.root_cmakelists_dir):
             patch('-s', '-p', '1', '-i',
-                  join_path(pkg_dir, patch_name),
+                  join_path(self.package_dir, patch_name),
                   "-d", '.')
 
     def url_for_version(self, version):