Hotfix: maintain patch order while fixing hash

fixes #5587

In trying to preserve patch ordering, #5476 made equality inconsistent
for the added 'patches' variant. This commit maintains the original
weak ordering of patch applications while preserving consistency of
comparisons. The ordering DOES NOT enter the hashing mechanism. It's
supposed to be a hotfix, while we think of a cleaner and more-permanent
solution.

2 files changed, 29 insertions, 23 deletions

diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index d919aeecb7..8d5a1ec8df 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1812,10 +1812,12 @@ class Spec(object):
                     for patch in patch_list:
                         patches.append(patch.sha256)
             if patches:
-                # Special-case: keeps variant values unique but ordered.
-                s.variants['patches'] = MultiValuedVariant('patches', ())
-                mvar = s.variants['patches']
-                mvar._value = mvar._original_value = tuple(dedupe(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
 
         # Apply patches required on dependencies by depends_on(..., patch=...)
         for dspec in self.traverse_edges(deptype=all,
@@ -1832,15 +1834,13 @@ class Spec(object):
                             for patch in patch_list:
                                 patches.append(patch.sha256)
             if patches:
-                # note that we use a special multi-valued variant and
-                # keep the patches ordered.
-                if 'patches' not in dspec.spec.variants:
-                    mvar = MultiValuedVariant('patches', ())
-                    dspec.spec.variants['patches'] = mvar
-                else:
-                    mvar = dspec.spec.variants['patches']
-                mvar._value = mvar._original_value = tuple(
-                    dedupe(list(mvar._value) + patches))
+                mvar = dspec.spec.variants.setdefault(
+                    'patches', MultiValuedVariant('patches', ())
+                )
+                mvar.value = mvar.value + tuple(patches)
+                # FIXME: Monkey patches mvar to store patches order
+                l = getattr(mvar, '_patches_in_order_of_appearance', [])
+                mvar._patches_in_order_of_appearance = dedupe(l + patches)
 
         for s in self.traverse():
             if s.external_module:
@@ -2523,7 +2523,11 @@ class Spec(object):
             return []
 
         patches = []
-        for sha256 in self.variants['patches'].value:
+
+        # FIXME: The private attribute below 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.lookup_patch(sha256)
             if patch:
                 patches.append(patch)
diff --git a/lib/spack/spack/test/patch.py b/lib/spack/spack/test/patch.py
index 0a91a0847c..41c4ef0621 100644
--- a/lib/spack/spack/test/patch.py
+++ b/lib/spack/spack/test/patch.py
@@ -99,9 +99,11 @@ def test_patch_in_spec(builtin_mock, config):
     spec.concretize()
     assert 'patches' in list(spec.variants.keys())
 
-    # foo, bar, baz
-    assert (('b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c',
-             '7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730',
+    # Here the order is bar, foo, baz. Note that MV variants order
+    # lexicographically based on the hash, not on the position of the
+    # patch directive.
+    assert (('7d865e959b2466918c9863afca942d0fb89d7c9ac0c99bafc3749504ded97730',
+             'b5bb9d8014a0f9b1d61e21e796d78dccdf1352f23cd32812f4850b878ae4944c',
              'bf07a7fbb825fc0aae7bf4a1177b2b31fcf8a3feeaf7092761e18c859ee52a9c') ==
             spec.variants['patches'].value)
 
@@ -131,8 +133,8 @@ def test_multiple_patched_dependencies(builtin_mock, config):
     # URL patches
     assert 'patches' in list(spec['fake'].variants.keys())
     # urlpatch.patch, urlpatch.patch.gz
-    assert (('abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234',
-             '1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd') ==
+    assert (('1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd',
+             'abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234') ==
             spec['fake'].variants['patches'].value)
 
 
@@ -159,8 +161,8 @@ def test_conditional_patched_dependencies(builtin_mock, config):
     # URL patches
     assert 'patches' in list(spec['fake'].variants.keys())
     # urlpatch.patch, urlpatch.patch.gz
-    assert (('abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234',
-             '1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd') ==
+    assert (('1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd',
+             'abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234') ==
             spec['fake'].variants['patches'].value)
 
 
@@ -187,6 +189,6 @@ def test_conditional_patched_deps_with_conditions(builtin_mock, config):
     # URL patches
     assert 'patches' in list(spec['fake'].variants.keys())
     # urlpatch.patch, urlpatch.patch.gz
-    assert (('abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234',
-             '1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd') ==
+    assert (('1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd',
+             'abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234abcd1234') ==
             spec['fake'].variants['patches'].value)