From a6511fbafcf16dd60d424e9d31b667339857628a Mon Sep 17 00:00:00 2001 From: Peter Scheibel Date: Thu, 28 Mar 2019 11:25:44 -0700 Subject: Consistent patch ordering (#10879) * preserve the order in which patches are applied by packages (in spite of grouping them by 'when') * add tests confirming patch order --- lib/spack/spack/directives.py | 16 +++++++++++---- lib/spack/spack/patch.py | 9 +++++++-- lib/spack/spack/spec.py | 47 +++++++++++++++++++++++++++---------------- lib/spack/spack/test/patch.py | 27 +++++++++++++++++++++++++ 4 files changed, 76 insertions(+), 23 deletions(-) (limited to 'lib') diff --git a/lib/spack/spack/directives.py b/lib/spack/spack/directives.py index 08df20ae88..26f43171b8 100644 --- a/lib/spack/spack/directives.py +++ b/lib/spack/spack/directives.py @@ -51,6 +51,8 @@ __all__ = [] #: These are variant names used by Spack internally; packages can't use them reserved_names = ['patches'] +_patch_order_index = 0 + class DirectiveMeta(type): """Flushes the directives that were temporarily stored in the staging @@ -267,8 +269,8 @@ def _depends_on(pkg, spec, when=None, type=default_deptype, patches=None): patches = [patches] # auto-call patch() directive on any strings in patch list - patches = [patch(p) if isinstance(p, string_types) - else p for p in patches] + patches = [patch(p) if isinstance(p, string_types) else p + for p in patches] assert all(callable(p) for p in patches) # this is where we actually add the dependency to this package @@ -422,12 +424,18 @@ def patch(url_or_filename, level=1, when=None, working_dir=".", **kwargs): if isinstance(pkg, Dependency): pkg = pkg.pkg + global _patch_order_index + ordering_key = (pkg.name, _patch_order_index) + _patch_order_index += 1 + if '://' in url_or_filename: patch = spack.patch.UrlPatch( - pkg, url_or_filename, level, working_dir, **kwargs) + pkg, url_or_filename, level, working_dir, + ordering_key=ordering_key, **kwargs) else: patch = spack.patch.FilePatch( - pkg, url_or_filename, level, working_dir) + pkg, url_or_filename, level, working_dir, + ordering_key=ordering_key) cur_patches.append(patch) diff --git a/lib/spack/spack/patch.py b/lib/spack/spack/patch.py index 9895fde4e1..ef03c54f45 100644 --- a/lib/spack/spack/patch.py +++ b/lib/spack/spack/patch.py @@ -107,12 +107,14 @@ class FilePatch(Patch): working_dir (str): path within the source directory where patch should be applied """ - def __init__(self, pkg, relative_path, level, working_dir): + def __init__(self, pkg, relative_path, level, working_dir, + ordering_key=None): self.relative_path = relative_path 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 + self.ordering_key = ordering_key @property def sha256(self): @@ -136,11 +138,14 @@ class UrlPatch(Patch): working_dir (str): path within the source directory where patch should be applied """ - def __init__(self, pkg, url, level=1, working_dir='.', **kwargs): + def __init__(self, pkg, url, level=1, working_dir='.', ordering_key=None, + **kwargs): super(UrlPatch, self).__init__(pkg, url, level, working_dir) self.url = url + self.ordering_key = ordering_key + self.archive_sha256 = kwargs.get('archive_sha256') if allowed_archive(self.url) and not self.archive_sha256: raise PatchDirectiveError( diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py index e74d38ea5e..ce5a536c6e 100644 --- a/lib/spack/spack/spec.py +++ b/lib/spack/spack/spec.py @@ -94,6 +94,7 @@ 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, memoized from llnl.util.tty.color import cwrite, colorize, cescape, get_color_when +import llnl.util.tty as tty import spack.architecture import spack.compiler @@ -1918,6 +1919,9 @@ class Spec(object): raise InvalidDependencyError( self.name + " does not depend on " + comma_or(extra)) + # This dictionary will store object IDs rather than Specs as keys + # since the Spec __hash__ will change as patches are added to them + spec_to_patches = {} for s in self.traverse(): # After concretizing, assign namespaces to anything left. # Note that this doesn't count as a "change". The repository @@ -1938,16 +1942,12 @@ class Spec(object): for cond, patch_list in s.package_class.patches.items(): if s.satisfies(cond): for patch in patch_list: - patches.append(patch.sha256) + patches.append(patch) if patches: - mvar = s.variants.setdefault( - 'patches', MultiValuedVariant('patches', ()) - ) - mvar.value = patches - # FIXME: Monkey patches mvar to store patches order - mvar._patches_in_order_of_appearance = patches + spec_to_patches[id(s)] = patches - # Apply patches required on dependencies by depends_on(..., patch=...) + # Also record all patches required on dependencies by + # depends_on(..., patch=...) for dspec in self.traverse_edges(deptype=all, cover='edges', root=False): pkg_deps = dspec.parent.package_class.dependencies @@ -1963,16 +1963,29 @@ class Spec(object): for pcond, patch_list in dependency.patches.items(): if dspec.spec.satisfies(pcond): for patch in patch_list: - patches.append(patch.sha256) + patches.append(patch) if patches: - mvar = dspec.spec.variants.setdefault( - 'patches', MultiValuedVariant('patches', ()) - ) - mvar.value = mvar.value + tuple(patches) - # FIXME: Monkey patches mvar to store patches order - p = getattr(mvar, '_patches_in_order_of_appearance', []) - mvar._patches_in_order_of_appearance = list( - dedupe(p + patches)) + all_patches = spec_to_patches.setdefault(id(dspec.spec), []) + all_patches.extend(patches) + + for spec in self.traverse(): + if id(spec) not in spec_to_patches: + continue + + patches = list(dedupe(spec_to_patches[id(spec)])) + mvar = spec.variants.setdefault( + 'patches', MultiValuedVariant('patches', ()) + ) + mvar.value = tuple(p.sha256 for p in patches) + # FIXME: Monkey patches mvar to store patches order + full_order_keys = list(tuple(p.ordering_key) + (p.sha256,) for p + in patches) + ordered_hashes = sorted(full_order_keys) + tty.debug("Ordered hashes [{0}]: ".format(spec.name) + + ', '.join('/'.join(str(e) for e in t) + for t in ordered_hashes)) + mvar._patches_in_order_of_appearance = list( + t[-1] for t in ordered_hashes) for s in self.traverse(): if s.external_module and not s.external_path: diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py index f06d9fd7d5..a62ebb8599 100644 --- a/lib/spack/spack/test/patch.py +++ b/lib/spack/spack/test/patch.py @@ -104,6 +104,33 @@ def test_patch_in_spec(mock_packages, config): baz_sha256) == spec.variants['patches'].value) + assert ((foo_sha256, bar_sha256, baz_sha256) == + tuple(spec.variants['patches']._patches_in_order_of_appearance)) + + +def test_patch_order(mock_packages, config): + spec = Spec('dep-diamond-patch-top') + spec.concretize() + + mid2_sha256 = 'mid21234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234' # noqa: E501 + mid1_sha256 = '0b62284961dab49887e31319843431ee5b037382ac02c4fe436955abef11f094' # noqa: E501 + top_sha256 = 'f7de2947c64cb6435e15fb2bef359d1ed5f6356b2aebb7b20535e3772904e6db' # noqa: E501 + + dep = spec['patch'] + patch_order = dep.variants['patches']._patches_in_order_of_appearance + # 'mid2' comes after 'mid1' alphabetically + # 'top' comes after 'mid1'/'mid2' alphabetically + # 'patch' comes last of all specs in the dag, alphabetically, so the + # patches of 'patch' to itself are applied last. The patches applied by + # 'patch' are ordered based on their appearance in the package.py file + expected_order = ( + mid1_sha256, + mid2_sha256, + top_sha256, + foo_sha256, bar_sha256, baz_sha256) + + assert expected_order == tuple(patch_order) + def test_nested_directives(mock_packages): """Ensure pkg data structures are set up properly by nested directives.""" -- cgit v1.2.3-70-g09d2