Injected flags have a consistent order (#9960)

Fixes #9908

Spack was assembling flags in a manner that could in different
orderings for repeated concretizations of the same spec and config

4 files changed, 30 insertions, 16 deletions

diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 4b12588c56..5460233b84 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -380,13 +380,12 @@ class Concretizer(object):
                                if (compiler_match(p) and
                                    (p is not spec) and
                                    flag in p.compiler_flags))
-                nearest_flags = set(nearest.compiler_flags.get(flag, []))
-                flags = set(spec.compiler_flags.get(flag, []))
-                if (nearest_flags - flags):
-                    # TODO: these set operations may reorder the flags, which
-                    # for some orders of flags can be invalid. See:
-                    # https://github.com/spack/spack/issues/6154#issuecomment-342365573
-                    spec.compiler_flags[flag] = list(nearest_flags | flags)
+                nearest_flags = nearest.compiler_flags.get(flag, [])
+                flags = spec.compiler_flags.get(flag, [])
+                if set(nearest_flags) - set(flags):
+                    spec.compiler_flags[flag] = list(
+                        llnl.util.lang.dedupe(nearest_flags + flags)
+                    )
                     ret = True
             except StopIteration:
                 pass
@@ -402,10 +401,12 @@ class Concretizer(object):
                 raise
             return ret
         for flag in compiler.flags:
-            config_flags = set(compiler.flags.get(flag, []))
-            flags = set(spec.compiler_flags.get(flag, []))
-            spec.compiler_flags[flag] = list(config_flags | flags)
-            if (config_flags - flags):
+            config_flags = compiler.flags.get(flag, [])
+            flags = spec.compiler_flags.get(flag, [])
+            spec.compiler_flags[flag] = list(
+                llnl.util.lang.dedupe(config_flags + flags)
+            )
+            if set(config_flags) - set(flags):
                 ret = True
 
         return ret
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index d0574794b2..d38fe5ce74 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -179,9 +179,9 @@ class TestConcretize(object):
                       ' ^cmake %clang@3.5 platform=test os=fe target=fe')
         client.concretize()
         cmake = client['cmake']
-        assert set(client.compiler_flags['cflags']) == set(['-O0'])
+        assert set(client.compiler_flags['cflags']) == set(['-O0', '-g'])
         assert set(cmake.compiler_flags['cflags']) == set(['-O3'])
-        assert set(client.compiler_flags['fflags']) == set(['-O0'])
+        assert set(client.compiler_flags['fflags']) == set(['-O0', '-g'])
         assert not set(cmake.compiler_flags['fflags'])
 
     def test_architecture_inheritance(self):
diff --git a/lib/spack/spack/test/data/compilers.yaml b/lib/spack/spack/test/data/compilers.yaml
index ebba6a601d..d7092945d5 100644
--- a/lib/spack/spack/test/data/compilers.yaml
+++ b/lib/spack/spack/test/data/compilers.yaml
@@ -98,9 +98,9 @@ compilers:
       f77: /path/to/gfortran472
       fc: /path/to/gfortran472
     flags:
-      cflags: -O0
-      cxxflags: -O0
-      fflags: -O0
+      cflags: -O0 -g
+      cxxflags: -O0 -g
+      fflags: -O0 -g
     modules: 'None'
 - compiler:
     spec: clang@3.5
diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py
index d24c38fafc..eda3bd5ee3 100644
--- a/lib/spack/spack/test/spec_semantics.py
+++ b/lib/spack/spack/test/spec_semantics.py
@@ -743,3 +743,16 @@ class TestSpecSematics(object):
             expected = getattr(arch, prop, "")
             actual = spec.format(named_str)
             assert str(expected) == actual
+
+    @pytest.mark.regression('9908')
+    def test_spec_flags_maintain_order(self):
+        # Spack was assembling flags in a manner that could result in
+        # different orderings for repeated concretizations of the same
+        # spec and config
+        spec_str = 'libelf %gcc@4.7.2 os=redhat6'
+        for _ in range(25):
+            s = Spec(spec_str).concretized()
+            assert all(
+                s.compiler_flags[x] == ['-O0', '-g']
+                for x in ('cflags', 'cxxflags', 'fflags')
+            )