fix doubly shell quoting args to spack spec (#29282)

* add test to verify fix works
* fix spec cflags/variants parsing test (breaking change)
* fix `spack spec` arg quoting issue
* add error report for deprecated cflags coalescing
* use .group(n) vs subscript regex group extraction for 3.5 compat
* add random test for untested functionality to pass codecov
* fix new test failure since rebase

lib/spack/spack/cmd/__init__.py

@@ -8,7 +8,10 @@
 import argparse
 import os
 import re
+import shlex
 import sys
+from textwrap import dedent
+from typing import List, Tuple
 
 import ruamel.yaml as yaml
 import six
@@ -147,6 +150,58 @@ def get_command(cmd_name):
     return getattr(get_module(cmd_name), pname)
 
 
+class _UnquotedFlags(object):
+    """Use a heuristic in `.extract()` to detect whether the user is trying to set
+    multiple flags like the docker ENV attribute allows (e.g. 'cflags=-Os -pipe').
+
+    If the heuristic finds a match (which can be checked with `__bool__()`), a warning
+    message explaining how to quote multiple flags correctly can be generated with
+    `.report()`.
+    """
+
+    flags_arg_pattern = re.compile(
+        r'^({0})=([^\'"].*)$'.format(
+            '|'.join(spack.spec.FlagMap.valid_compiler_flags()),
+        ))
+
+    def __init__(self, all_unquoted_flag_pairs):
+        # type: (List[Tuple[re.Match, str]]) -> None
+        self._flag_pairs = all_unquoted_flag_pairs
+
+    def __bool__(self):
+        # type: () -> bool
+        return bool(self._flag_pairs)
+
+    @classmethod
+    def extract(cls, sargs):
+        # type: (str) -> _UnquotedFlags
+        all_unquoted_flag_pairs = []  # type: List[Tuple[re.Match, str]]
+        prev_flags_arg = None
+        for arg in shlex.split(sargs):
+            if prev_flags_arg is not None:
+                all_unquoted_flag_pairs.append((prev_flags_arg, arg))
+            prev_flags_arg = cls.flags_arg_pattern.match(arg)
+        return cls(all_unquoted_flag_pairs)
+
+    def report(self):
+        # type: () -> str
+        single_errors = [
+            '({0}) {1} {2} => {3}'.format(
+                i + 1, match.group(0), next_arg,
+                '{0}="{1} {2}"'.format(match.group(1), match.group(2), next_arg),
+            )
+            for i, (match, next_arg) in enumerate(self._flag_pairs)
+        ]
+        return dedent("""\
+        Some compiler or linker flags were provided without quoting their arguments,
+        which now causes spack to try to parse the *next* argument as a spec component
+        such as a variant instead of an additional compiler or linker flag. If the
+        intent was to set multiple flags, try quoting them together as described below.
+
+        Possible flag quotation errors (with the correctly-quoted version after the =>):
+        {0}""").format('\n'.join(single_errors))
+
+
 def parse_specs(args, **kwargs):
     """Convenience function for parsing arguments from specs.  Handles common
        exceptions and dies if there are errors.
@@ -157,15 +212,28 @@ def parse_specs(args, **kwargs):
 
     sargs = args
     if not isinstance(args, six.string_types):
-        sargs = ' '.join(spack.util.string.quote(args))
+        sargs = ' '.join(args)
-    specs = spack.spec.parse(sargs)
+    unquoted_flags = _UnquotedFlags.extract(sargs)
-    for spec in specs:
-        if concretize:
-            spec.concretize(tests=tests)  # implies normalize
-        elif normalize:
-            spec.normalize(tests=tests)
 
-    return specs
+    try:
+        specs = spack.spec.parse(sargs)
+        for spec in specs:
+            if concretize:
+                spec.concretize(tests=tests)  # implies normalize
+            elif normalize:
+                spec.normalize(tests=tests)
+        return specs
+
+    except spack.error.SpecError as e:
+
+        msg = e.message
+        if e.long_message:
+            msg += e.long_message
+        if unquoted_flags:
+            msg += '\n\n'
+            msg += unquoted_flags.report()
+
+        raise spack.error.SpackError(msg)
 
 
 def matching_spec_from_env(spec):

lib/spack/spack/cmd/spec.py

@@ -80,7 +80,8 @@ def spec(parser, args):
     # Use command line specified specs, otherwise try to use environment specs.
     if args.specs:
         input_specs = spack.cmd.parse_specs(args.specs)
-        specs = [(s, s.concretized()) for s in input_specs]
+        concretized_specs = spack.cmd.parse_specs(args.specs, concretize=True)
+        specs = list(zip(input_specs, concretized_specs))
     else:
         env = ev.active_environment()
         if env:

lib/spack/spack/test/cmd/common/arguments.py

@@ -45,21 +45,22 @@ def test_negative_integers_not_allowed_for_parallel_jobs(job_parser):
     assert 'expected a positive integer' in str(exc_info.value)
 
 
-@pytest.mark.parametrize('specs,expected_variants,unexpected_variants', [
+@pytest.mark.parametrize('specs,cflags,negated_variants', [
-    (['coreutils', 'cflags=-O3 -g'], [], ['g']),
+    (['coreutils cflags="-O3 -g"'], ['-O3', '-g'], []),
-    (['coreutils', 'cflags=-O3', '-g'], ['g'], []),
+    (['coreutils', 'cflags=-O3 -g'], ['-O3'], ['g']),
+    (['coreutils', 'cflags=-O3', '-g'], ['-O3'], ['g']),
 ])
 @pytest.mark.regression('12951')
-def test_parse_spec_flags_with_spaces(
+def test_parse_spec_flags_with_spaces(specs, cflags, negated_variants):
-        specs, expected_variants, unexpected_variants
-):
     spec_list = spack.cmd.parse_specs(specs)
     assert len(spec_list) == 1
 
     s = spec_list.pop()
 
-    assert all(x not in s.variants for x in unexpected_variants)
+    assert s.compiler_flags['cflags'] == cflags
-    assert all(x in s.variants for x in expected_variants)
+    assert list(s.variants.keys()) == negated_variants
+    for v in negated_variants:
+        assert '~{0}'.format(v) in s
 
 
 @pytest.mark.usefixtures('config')

lib/spack/spack/test/cmd/spec.py

@@ -4,10 +4,12 @@
 # SPDX-License-Identifier: (Apache-2.0 OR MIT)
 
 import re
+from textwrap import dedent
 
 import pytest
 
 import spack.environment as ev
+import spack.error
 import spack.spec
 import spack.store
 from spack.main import SpackCommand, SpackCommandError
@@ -55,6 +57,54 @@ def test_spec_concretizer_args(mutable_config, mutable_database):
     assert h in output
 
 
+def test_spec_parse_dependency_variant_value():
+    """Verify that we can provide multiple key=value variants to multiple separate
+    packages within a spec string."""
+    output = spec('multivalue-variant fee=barbaz ^ a foobar=baz')
+
+    assert 'fee=barbaz' in output
+    assert 'foobar=baz' in output
+
+
+def test_spec_parse_cflags_quoting():
+    """Verify that compiler flags can be provided to a spec from the command line."""
+    output = spec('--yaml', 'gcc cflags="-Os -pipe" cxxflags="-flto -Os"')
+    gh_flagged = spack.spec.Spec.from_yaml(output)
+
+    assert ['-Os', '-pipe'] == gh_flagged.compiler_flags['cflags']
+    assert ['-flto', '-Os'] == gh_flagged.compiler_flags['cxxflags']
+
+
+def test_spec_parse_unquoted_flags_report():
+    """Verify that a useful error message is produced if unquoted compiler flags are
+    provided."""
+    # This should fail during parsing, since /usr/include is interpreted as a spec hash.
+    with pytest.raises(spack.error.SpackError) as cm:
+        # We don't try to figure out how many following args were intended to be part of
+        # cflags, we just explain how to fix it for the immediate next arg.
+        spec('gcc cflags=-Os -pipe -other-arg-that-gets-ignored cflags=-I /usr/include')
+    # Verify that the generated error message is nicely formatted.
+    assert str(cm.value) == dedent('''\
+    No installed spec matches the hash: 'usr'
+
+    Some compiler or linker flags were provided without quoting their arguments,
+    which now causes spack to try to parse the *next* argument as a spec component
+    such as a variant instead of an additional compiler or linker flag. If the
+    intent was to set multiple flags, try quoting them together as described below.
+
+    Possible flag quotation errors (with the correctly-quoted version after the =>):
+    (1) cflags=-Os -pipe => cflags="-Os -pipe"
+    (2) cflags=-I /usr/include => cflags="-I /usr/include"''')
+
+    # Verify that the same unquoted cflags report is generated in the error message even
+    # if it fails during concretization, not just during parsing.
+    with pytest.raises(spack.error.SpackError) as cm:
+        spec('gcc cflags=-Os -pipe')
+    cm = str(cm.value)
+    assert cm.startswith('trying to set variant "pipe" in package "gcc", but the package has no such variant [happened during concretization of gcc cflags="-Os" ~pipe]')  # noqa: E501
+    assert cm.endswith('(1) cflags=-Os -pipe => cflags="-Os -pipe"')
+
+
 def test_spec_yaml():
     output = spec('--yaml', 'mpileaks')
 
@@ -125,14 +175,14 @@ def test_spec_returncode():
 
 
 def test_spec_parse_error():
-    with pytest.raises(spack.spec.SpecParseError) as e:
+    with pytest.raises(spack.error.SpackError) as e:
         spec("1.15:")
 
     # make sure the error is formatted properly
     error_msg = """\
     1.15:
         ^"""
-    assert error_msg in e.value.long_message
+    assert error_msg in str(e.value)
 
 
 def test_env_aware_spec(mutable_mock_env_path):

lib/spack/spack/test/spec_semantics.py

@@ -15,6 +15,7 @@
     SpecFormatSigilError,
     SpecFormatStringError,
     UnconstrainableDependencySpecError,
+    UnsupportedCompilerError,
 )
 from spack.variant import (
     InvalidVariantValueError,
@@ -1320,3 +1321,8 @@ def test_concretize_partial_old_dag_hash_spec(mock_packages, config):
 
     # make sure package hash is NOT recomputed
     assert not getattr(spec["dt-diamond-bottom"], '_package_hash', None)
+
+
+def test_unsupported_compiler():
+    with pytest.raises(UnsupportedCompilerError):
+        Spec('gcc%fake-compiler').validate_or_raise()