From 527ff860f01de34e34f8c7be8e5682745504d774 Mon Sep 17 00:00:00 2001 From: Todd Gamblin Date: Sun, 16 Dec 2018 23:17:23 -0800 Subject: [PATCH] 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 --- lib/spack/spack/directives.py | 13 +- lib/spack/spack/package.py | 52 +++++-- lib/spack/spack/patch.py | 137 +++++++++--------- lib/spack/spack/test/cmd/env.py | 2 +- lib/spack/spack/test/install.py | 5 +- lib/spack/spack/test/patch.py | 5 +- .../builtin/packages/catalyst/package.py | 6 +- 7 files changed, 119 insertions(+), 101 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 @@ class OpenMpi(Package): 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 _execute_patch(pkg_or_dep): # 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 @@ def _decorator(func): 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 @@ def possible_dependencies( 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 @@ def content_hash(self, content=None): 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 @@ def build_log_path(self): 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: 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) + + +def apply_patch(stage, patch_path, level=1, working_dir='.'): + """Apply the patch at patch_path to code in the stage. Args: - pkg: a valid package object, or a Dependency object. + 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 '.') """ - if isinstance(pkg, spack.dependency.Dependency): - pkg = pkg.pkg - m = inspect.getmodule(pkg) - return os.path.abspath(m.__file__) + 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) class Patch(object): - """Base class to describe a patch that needs to be applied to some - expanded source code. + """Base class for patches. + + Defines the interface (basically just ``apply()``, at the moment) and + common variables. """ - - @staticmethod - def create(pkg, path_or_url, level=1, working_dir=".", **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 (default 1) - working_dir (str): dir to change to before applying (default '.') - - 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) - 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 + + # path needs to be set by subclasses before calling self.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.") - 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) + def __init__(self, path, level, working_dir): + super(FilePatch, self).__init__(path, 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)) + if not os.path.isfile(path): + raise NoSuchPatchError("No such patch: %s" % path) + self.path = path self._sha256 = None @property @@ -106,21 +102,20 @@ def sha256(self): 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 + def __init__(self, url, level, working_dir, **kwargs): + super(UrlPatch, self).__init__(url, level, working_dir) - 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') + 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) - if 'sha256' not in kwargs: - raise PatchDirectiveError("URL patches require a sha256 checksum") 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 @@ def mock_stage(tmpdir, monkeypatch): 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 @@ 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 @@ def patch(self): 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):