From 802dc4a03a4bb86cf42ff7f60915c1dabbbe31a9 Mon Sep 17 00:00:00 2001 From: Michael Kuhn Date: Wed, 2 Jan 2019 20:44:50 +0100 Subject: patch: split up fetch and clean into separate methods (#10150) "mirror create" was invoking a package's do_patch method in order to retrieve and archive URL patches. If a package implements a "patch" method, this is also called as part of do_patch; this failed when the package-specific implementation referred to environment variables that are only available at the time the package is built (e.g. "spack_cc"). This change introduces fetch and clean methods for patches. They are no-ops for FilePatch but perform the appropriate actions for UrlPatch. This allows "mirror create" to invoke do_fetch, which does not call the package's patch method. --- lib/spack/spack/mirror.py | 2 +- lib/spack/spack/package.py | 6 +++ lib/spack/spack/patch.py | 105 +++++++++++++++++++++++------------------- lib/spack/spack/spec.py | 6 ++- lib/spack/spack/test/patch.py | 2 + 5 files changed, 71 insertions(+), 50 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/mirror.py b/lib/spack/spack/mirror.py index fb4ffa365d..06868853cf 100644 --- a/lib/spack/spack/mirror.py +++ b/lib/spack/spack/mirror.py @@ -212,7 +212,7 @@ def create(path, specs, **kwargs): def add_single_spec(spec, mirror_root, categories, **kwargs): tty.msg("Adding package {pkg} to mirror".format(pkg=spec.format("$_$@"))) try: - spec.package.do_patch() + spec.package.do_fetch() spec.package.do_clean() except Exception as e: diff --git a/lib/spack/spack/package.py b/lib/spack/spack/package.py index d543539d9a..d262e71df2 100644 --- a/lib/spack/spack/package.py +++ b/lib/spack/spack/package.py @@ -967,6 +967,9 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)): self.stage.cache_local() + for patch in self.spec.patches: + patch.fetch(self.stage) + def do_stage(self, mirror_only=False): """Unpacks and expands the fetched tarball.""" if not self.spec.concrete: @@ -2078,6 +2081,9 @@ class PackageBase(with_metaclass(PackageMeta, PackageViewMixin, object)): def do_clean(self): """Removes the package's build stage and source tarball.""" + for patch in self.spec.patches: + patch.clean() + self.stage.destroy() def format_doc(self, **kwargs): diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py index d59723ba33..9895fde4e1 100644 --- a/lib/spack/spack/patch.py +++ b/lib/spack/spack/patch.py @@ -3,11 +3,9 @@ # # SPDX-License-Identifier: (Apache-2.0 OR MIT) -import abc import hashlib import os import os.path -import six import llnl.util.filesystem import llnl.util.lang @@ -41,7 +39,6 @@ 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. @@ -61,16 +58,33 @@ class Patch(object): # Attributes shared by all patch subclasses self.owner = pkg.fullname self.path_or_url = path_or_url # needed for debug output + self.path = None # must be set before apply() self.level = level self.working_dir = working_dir - @abc.abstractmethod + def fetch(self, stage): + """Fetch the patch in case of a UrlPatch + + Args: + stage: stage for the package that needs to be patched + """ + + def clean(self): + """Clean up the patch stage in case of a UrlPatch""" + def apply(self, stage): """Apply a patch to source in a stage. Arguments: stage (spack.stage.Stage): stage where source code lives """ + assert self.path, ( + "Path for patch not set in apply: %s" % self.path_or_url) + + 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) def to_dict(self): """Partial dictionary -- subclases should add to this.""" @@ -95,15 +109,11 @@ class FilePatch(Patch): """ 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) + abs_path = os.path.join(pkg.package_dir, self.relative_path) + super(FilePatch, self).__init__(pkg, abs_path, level, working_dir) + self.path = abs_path 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: @@ -130,7 +140,6 @@ class UrlPatch(Patch): 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: @@ -142,9 +151,8 @@ class UrlPatch(Patch): if not self.sha256: raise PatchDirectiveError("URL patches require a sha256 checksum") - def apply(self, stage): - """Retrieve the patch in a temporary stage, computes - self.path and calls `super().apply(stage)` + def fetch(self, stage): + """Retrieve the patch in a temporary stage and compute self.path Args: stage: stage for the package that needs to be patched @@ -159,42 +167,43 @@ class UrlPatch(Patch): 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() + self.stage = spack.stage.Stage(fetcher, mirror_path=mirror) + self.stage.create() + self.stage.fetch() + self.stage.check() + self.stage.cache_local() + + root = self.stage.path + if self.archive_sha256: + self.stage.expand_archive() + root = self.stage.source_path - root = patch_stage.path + files = os.listdir(root) + if not files: if self.archive_sha256: - patch_stage.expand_archive() - root = patch_stage.source_path - - files = os.listdir(root) - if not files: - if self.archive_sha256: - raise NoSuchPatchError( - "Archive was empty: %s" % self.url) - else: - 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): raise NoSuchPatchError( - "Archive %s contains no patch file!" % self.url) - - # for a compressed archive, Need to check the patch sha256 again - # and the patch is in a directory, not in the same place - if self.archive_sha256 and spack.config.get('config:checksum'): - checker = Checker(self.sha256) - if not checker.check(self.path): - raise fs.ChecksumError( - "sha256 checksum failed for %s" % self.path, - "Expected %s but got %s" % (self.sha256, checker.sum)) - - apply_patch(stage, self.path, self.level, self.working_dir) + "Archive was empty: %s" % self.url) + else: + raise NoSuchPatchError( + "Patch failed to download: %s" % self.url) + + self.path = os.path.join(root, files.pop()) + + if not os.path.isfile(self.path): + raise NoSuchPatchError( + "Archive %s contains no patch file!" % self.url) + + # for a compressed archive, Need to check the patch sha256 again + # and the patch is in a directory, not in the same place + if self.archive_sha256 and spack.config.get('config:checksum'): + checker = Checker(self.sha256) + if not checker.check(self.path): + raise fs.ChecksumError( + "sha256 checksum failed for %s" % self.path, + "Expected %s but got %s" % (self.sha256, checker.sum)) + + def clean(self): + self.stage.destroy() def to_dict(self): data = super(UrlPatch, self).to_dict() diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index 380766b245..1ff2fb4677 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -92,7 +92,7 @@ from six import iteritems from llnl.util.filesystem import find_headers, find_libraries, is_exe from llnl.util.lang import key_ordering, HashableMap, ObjectWrapper, dedupe -from llnl.util.lang import check_kwargs +from llnl.util.lang import check_kwargs, memoized from llnl.util.tty.color import cwrite, colorize, cescape, get_color_when import spack.architecture @@ -2665,6 +2665,7 @@ class Spec(object): return [spec for spec in self.traverse() if spec.virtual] @property + @memoized def patches(self): """Return patch objects for any patch sha256 sums on this Spec. @@ -2674,6 +2675,9 @@ class Spec(object): TODO: this only checks in the package; it doesn't resurrect old patches from install directories, but it probably should. """ + if not self.concrete: + raise SpecError("Spec is not concrete: " + str(self)) + if 'patches' not in self.variants: return [] diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py index bec47d7288..f06d9fd7d5 100644 --- a/lib/spack/spack/test/patch.py +++ b/lib/spack/spack/test/patch.py @@ -82,7 +82,9 @@ first line third line """) # apply the patch and compare files + patch.fetch(stage) patch.apply(stage) + patch.clean() with working_dir(stage.source_path): assert filecmp.cmp('foo.txt', 'foo-expected.txt') -- cgit v1.2.3-60-g2f50