diff options
| -rw-r--r-- | lib/spack/spack/cmd/__init__.py | 84 | ||||
| -rw-r--r-- | lib/spack/spack/cmd/spec.py | 3 | ||||
| -rw-r--r-- | lib/spack/spack/test/cmd/common/arguments.py | 17 | ||||
| -rw-r--r-- | lib/spack/spack/test/cmd/spec.py | 54 | ||||
| -rw-r--r-- | lib/spack/spack/test/spec_semantics.py | 6 |
5 files changed, 145 insertions, 19 deletions
diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py index ff6c3d7bf4..608db2776b 100644 --- a/lib/spack/spack/cmd/__init__.py +++ b/lib/spack/spack/cmd/__init__.py @@ -8,7 +8,10 @@ from __future__ import print_function 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)) - specs = spack.spec.parse(sargs) - for spec in specs: - if concretize: - spec.concretize(tests=tests) # implies normalize - elif normalize: - spec.normalize(tests=tests) + sargs = ' '.join(args) + unquoted_flags = _UnquotedFlags.extract(sargs) - 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): diff --git a/lib/spack/spack/cmd/spec.py b/lib/spack/spack/cmd/spec.py index 74a47d154d..1e5b213fe4 100644 --- a/lib/spack/spack/cmd/spec.py +++ b/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: diff --git a/lib/spack/spack/test/cmd/common/arguments.py b/lib/spack/spack/test/cmd/common/arguments.py index 5f1299a94a..c527cc074a 100644 --- a/lib/spack/spack/test/cmd/common/arguments.py +++ b/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', [ - (['coreutils', 'cflags=-O3 -g'], [], ['g']), - (['coreutils', 'cflags=-O3', '-g'], ['g'], []), +@pytest.mark.parametrize('specs,cflags,negated_variants', [ + (['coreutils cflags="-O3 -g"'], ['-O3', '-g'], []), + (['coreutils', 'cflags=-O3 -g'], ['-O3'], ['g']), + (['coreutils', 'cflags=-O3', '-g'], ['-O3'], ['g']), ]) @pytest.mark.regression('12951') -def test_parse_spec_flags_with_spaces( - specs, expected_variants, unexpected_variants -): +def test_parse_spec_flags_with_spaces(specs, cflags, negated_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 all(x in s.variants for x in expected_variants) + assert s.compiler_flags['cflags'] == cflags + assert list(s.variants.keys()) == negated_variants + for v in negated_variants: + assert '~{0}'.format(v) in s @pytest.mark.usefixtures('config') diff --git a/lib/spack/spack/test/cmd/spec.py b/lib/spack/spack/test/cmd/spec.py index 42f4c7a59a..e406fabe26 100644 --- a/lib/spack/spack/test/cmd/spec.py +++ b/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): diff --git a/lib/spack/spack/test/spec_semantics.py b/lib/spack/spack/test/spec_semantics.py index edc31e4f09..db1f68f396 100644 --- a/lib/spack/spack/test/spec_semantics.py +++ b/lib/spack/spack/test/spec_semantics.py @@ -15,6 +15,7 @@ from spack.spec import ( 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() |