From 8c458a856f8c15e69d3ddc96728e0751042e29b6 Mon Sep 17 00:00:00 2001
From: scheibelp <scheibel1@llnl.gov>
Date: Fri, 10 Nov 2017 16:33:50 -0800
Subject: Group flags and values separated by space (#6169)

Fixes #6154

For compiler options which set values using the syntax "-flag value"
(with a space between the flag and the flag's value) the flag and
value were treated as separate and reordered. This updates Spack's
logic to treat the flag and value as a single unit, even if there is
a space between them. It assumes that all flags are prefixed with
"-" and that all flag values are not.
---
 lib/spack/spack/compiler.py        | 23 ++++++++++++++++++++++-
 lib/spack/spack/concretize.py      |  3 +++
 lib/spack/spack/spec.py            |  2 +-
 lib/spack/spack/test/compilers.py  | 20 ++++++++++++++++++++
 lib/spack/spack/test/concretize.py |  6 ++++++
 5 files changed, 52 insertions(+), 2 deletions(-)

diff --git a/lib/spack/spack/compiler.py b/lib/spack/spack/compiler.py
index 80489d4150..e19601116f 100644
--- a/lib/spack/spack/compiler.py
+++ b/lib/spack/spack/compiler.py
@@ -65,6 +65,27 @@ def dumpversion(compiler_path):
     return get_compiler_version(compiler_path, '-dumpversion')
 
 
+def tokenize_flags(flags_str):
+    """Given a compiler flag specification as a string, this returns a list
+       where the entries are the flags. For compiler options which set values
+       using the syntax "-flag value", this function groups flags and their
+       values together. Any token not preceded by a "-" is considered the
+       value of a prior flag."""
+    tokens = flags_str.split()
+    if not tokens:
+        return []
+    flag = tokens[0]
+    flags = []
+    for token in tokens[1:]:
+        if not token.startswith('-'):
+            flag += ' ' + token
+        else:
+            flags.append(flag)
+            flag = token
+    flags.append(flag)
+    return flags
+
+
 class Compiler(object):
     """This class encapsulates a Spack "compiler", which includes C,
        C++, and Fortran compilers.  Subclasses should implement
@@ -147,7 +168,7 @@ class Compiler(object):
         for flag in spack.spec.FlagMap.valid_compiler_flags():
             value = kwargs.get(flag, None)
             if value is not None:
-                self.flags[flag] = value.split()
+                self.flags[flag] = tokenize_flags(value)
 
     @property
     def version(self):
diff --git a/lib/spack/spack/concretize.py b/lib/spack/spack/concretize.py
index 7add40fb81..3b1e83cb5b 100644
--- a/lib/spack/spack/concretize.py
+++ b/lib/spack/spack/concretize.py
@@ -366,6 +366,9 @@ class DefaultConcretizer(object):
                 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)
                     ret = True
             except StopIteration:
diff --git a/lib/spack/spack/spec.py b/lib/spack/spack/spec.py
index 0bfef3de3a..e29c1bed3c 100644
--- a/lib/spack/spack/spec.py
+++ b/lib/spack/spack/spec.py
@@ -1122,7 +1122,7 @@ class Spec(object):
             self._set_architecture(target=value)
         elif name in valid_flags:
             assert(self.compiler_flags is not None)
-            self.compiler_flags[name] = value.split()
+            self.compiler_flags[name] = spack.compiler.tokenize_flags(value)
         else:
             # FIXME:
             # All other flags represent variants. 'foo=true' and 'foo=false'
diff --git a/lib/spack/spack/test/compilers.py b/lib/spack/spack/test/compilers.py
index 92ceea0a1b..d172d56638 100644
--- a/lib/spack/spack/test/compilers.py
+++ b/lib/spack/spack/test/compilers.py
@@ -47,3 +47,23 @@ class TestCompilers(object):
         filtered = [x for x in all_compilers if str(x.spec) == 'clang@3.3']
         filtered = [x for x in filtered if x.operating_system == 'SuSE11']
         assert len(filtered) == 1
+
+
+def test_compiler_flags_from_config_are_grouped():
+    compiler_entry = {
+        'spec': 'intel@17.0.2',
+        'operating_system': 'foo-os',
+        'paths': {
+            'cc': 'cc-path',
+            'cxx': 'cxx-path',
+            'fc': None,
+            'f77': None
+        },
+        'flags': {
+            'cflags': '-O0 -foo-flag foo-val'
+        },
+        'modules': None
+    }
+
+    compiler = compilers.compiler_from_config_entry(compiler_entry)
+    assert any(x == '-foo-flag foo-val' for x in compiler.flags['cflags'])
diff --git a/lib/spack/spack/test/concretize.py b/lib/spack/spack/test/concretize.py
index 7dab240673..f0d5ae131f 100644
--- a/lib/spack/spack/test/concretize.py
+++ b/lib/spack/spack/test/concretize.py
@@ -203,6 +203,12 @@ class TestConcretize(object):
         assert set(client.compiler_flags['fflags']) == set(['-O0'])
         assert not set(cmake.compiler_flags['fflags'])
 
+    def test_compiler_flags_from_user_are_grouped(self):
+        spec = Spec('a%gcc cflags="-O -foo-flag foo-val" platform=test')
+        spec.concretize()
+        cflags = spec.compiler_flags['cflags']
+        assert any(x == '-foo-flag foo-val' for x in cflags)
+
     def concretize_multi_provider(self):
         s = Spec('mpileaks ^multi-provider-mpi@3.0')
         s.concretize()
-- 
cgit v1.2.3-70-g09d2