fix doubly shell quoting args to `spack spec` by cosmicexplorer · Pull Request #29282 · spack/spack

cosmicexplorer · 2022-03-02T05:27:08Z

Problem

The following command fails:

$ spack spec 'emacs toolkit=gtk ^ gcc languages=jit' 
==> Error: invalid values for variant "toolkit" in package "emacs": ['gtk ^ gcc languages=jit']

But this appears to be a valid spec when parsed directly:

$ spack python
Spack version 0.17.1
Python 3.10.2, Linux x86_64
>>> from spack.spec import Spec
>>> print(Spec('emacs toolkit=gtk ^ gcc languages=jit').to_yaml())
spec:
  _meta:
    version: 2
  nodes:
  - name: emacs
    versions:
    - ':'
    parameters:
      toolkit:
      - gtk
    concrete: false
    dependencies:
    - name: gcc
      hash: ku4hvzuxvhxqv6b7qonr2ic4lsxsilxt
      type: []
    hash: n3yengnkkhcyuhjw2sehq6u7tnushgau
  - name: gcc
    versions:
    - ':'
    parameters:
      languages:
      - jit
    concrete: false
    hash: ku4hvzuxvhxqv6b7qonr2ic4lsxsilxt

Solution

Avoid shell quoting arguments to spack spec multiple times before joining them.

Result

$ spack spec 'emacs toolkit=gtk ^ gcc languages=jit'
Input spec
--------------------------------
emacs toolkit=gtk
    ^gcc languages=jit

Concretized
--------------------------------
emacs@master%[email protected]~X+native~tls toolkit=gtk arch=linux-alpine3-zen3
    ^[email protected]%[email protected] arch=linux-alpine3-zen3
    ^[email protected]%[email protected] arch=linux-alpine3-zen3
    ^[email protected]%[email protected]~binutils+bootstrap~graphite~nvptx~piclibs+strip languages=jit arch=linux-alpine3-zen3
... (truncated)

haampie · 2022-03-02T09:03:53Z

Can you check the failing tests @cosmicexplorer? To me this change looks sensible, we shouldn't rely on the shell splitting arguments, but just pass a string with whitespaces to the parser.

becker33

@trws I think this addresses your question we discussed recently

becker33 · 2022-03-02T17:41:44Z

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

+def test_spec_parse_arg_quoting():
+    """Assert that certain spec constructions work when provided as arguments."""
+    # 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
+


I believe that this test succeeds even without the changes in this PR, can you double-check on that?

You have truly fantastic timing @cosmicexplorer. Would you mind testing:

spack spec ‘[email protected] cflags=“-Os -pipe” cxxflags=“-Os -pipe”

spack spec ‘[email protected] cflags=-Os -pipe cxxflags=-Os -pipe
and see what comes out?

This should definitely help, but I think we may still have an over-eager glob on options with a VAL token.

@becker33:

I believe that this test succeeds even without the changes in this PR, can you double-check on that?

I orchestrated the commits so that the test is added before the fix, and this test indeed fails without the fix:

spack.variant.InvalidVariantValueError: invalid values for variant "fee" in package "multivalue-variant": ['barbaz ^ a foobar=baz']

@trws:

> spack spec '[email protected] cflags="-Os -pipe" cxxflags="-Os -pipe"' ... [email protected]%[email protected] cflags="-Os -pipe" cxxflags="-Os -pipe" ~binutils+bootstrap~graphite~nvptx~piclibs~strip languages=c,c++,fortran patches=0689c26,27e9829,7112abb,f5643d9,f73de87 arch=linux-alpine3-zen3 ... > spack spec '[email protected] cflags=-Os -pipe cxxflags=-Os -pipe' ==> Error: Cannot specify variant "pipe" twice

Regarding the second command line with cflags=-Os -pipe: that one is currently accepted by spack on develop, so while I think this change should still be made, it may break some users. How should we notify users about this?

Additionally, we can verify the issue from #29282 (comment) is fixed with the --yaml output:

> spack spec --yaml 'gh cflags="-Os -pipe" cxxflags="-Os -pipe"' | yq ... cflags: - -Os - -pipe ... > git checkout develop > spack spec --yaml 'gh cflags="-Os -pipe" cxxflags="-Os -pipe"' | yq ... cflags: - '"-Os' - -pipe" cxxflags="-Os - -pipe" ...

So it looks like that would absolutely be a good one to include as a new test.

trws · 2022-03-02T23:32:41Z

Absolutely fabulous:

~/spack fix-dep-kv-flags *1 ?3                                                                                                                                  ✘ 2 scogland1@chimera
❯ spack spec 'gh cflags="-Os -pipe" cxxflags="-Os -pipe"'
Input spec
--------------------------------
gh cflags="-Os -pipe" cxxflags="-Os -pipe"

This would be a good one to include as a test, because it was most definitely broken before this.

trws · 2022-03-04T03:23:07Z

Can you check the failing tests @cosmicexplorer? To me this change looks sensible, we shouldn't rely on the shell splitting arguments, but just pass a string with whitespaces to the parser.

The test I see breaking looks very much like a broken test to me. It relies on meh cflags=-u -g parsing as meh cflags=“-u -g”, which would be somewhat like how Dockerfiles parse the ENV attribute, but that’s a much trickier parse, with a whole lot more corner cases in it.

cosmicexplorer · 2022-05-09T19:44:26Z

@trws:

The test I see breaking looks very much like a broken test to me. It relies on meh cflags=-u -g parsing as meh cflags="-u -g", which would be somewhat like how Dockerfiles parse the ENV attribute, but that’s a much trickier parse, with a whole lot more corner cases in it.

I fixed test_parse_spec_flags_with_spaces() just now, but this implies another decision will have to be made:

Regarding the second command line with cflags=-Os -pipe: that one is currently accepted by spack on develop, so while I think this change should still be made, it may break some users. How should we notify users about this?

Since this change is actually necessary in order to e.g. write spack spec 'emacs toolkit=gtk ^ gcc languages=jit', I agree that we emphatically should make this breaking change in order to enable that type of command line to be written at all (while command lines relying on cflags=-Os -pipe to work without quotes can just add quotes).

EDIT: I just realized we should be able to actually shlex spack spec cli args and look for cases where a -* argument follows a *flags=... argument, and print out a warning if detected. I'll look into doing that so we can hopefully avoid making anyone too confused.

cosmicexplorer · 2022-05-09T21:37:18Z

Ok, just generated a really nice error report, which also has a test added for it in af00a25. It looks like:

> spack spec '[email protected] cflags=-Os -pipe'     
==> Error: trying to set variant "pipe" in package "gcc", but the package has no such variant [happened during concretization of [email protected] cflags="-Os" ~pipe]

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"

For multiple such cases (probably unlikely, but necessary in order to ensure that the arguments causing the error are represented in the error report):

> spack spec '[email protected] cflags=-Os -pipe cflags=-I /usr/include'
==> Error: 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"

trws · 2022-05-09T22:35:42Z

That looks really good to me.

cosmicexplorer · 2022-05-10T05:39:23Z

gitlab ci had a spurious failure to start the docker daemon and said to try again (https://gitlab.spack.io/spack/spack/-/jobs/2481179), so just force pushed to kick it off again.

trws · 2022-05-10T21:39:01Z

Looks like some github action unit test issues also?

cosmicexplorer · 2022-05-11T04:23:00Z

Ok, the unit test failure on 3.5 appears to be because subscripting re.Match objects was introduced in 3.6, so that was switched to use .group(n) instead. The coverage changes reported by codecov don't correspond to any code I've actually changed, and coverage has only increased for every file in the "diff" section, so I'm inclined to wait for it to re-run and see if it still complains.

cosmicexplorer · 2022-05-11T12:38:44Z

codecov/project failed again, so I pulled develop and tried again, and it failed but with an even smaller reported decrease in coverage, so I just added a test for something that wasn't covered and hoping that allows this to finally pass. Incredibly frustrated with the complete lack of indication as to why a diff produced a change in coverage.

trws · 2022-05-12T00:17:25Z

If this set passes with only the codecov failure, we can merge. Codecov is no longer set as required because of spurious failure issues.

cosmicexplorer · 2022-05-12T03:45:57Z

Looks like it passed!

tgamblin · 2022-06-11T19:35:23Z

lib/spack/spack/cmd/__init__.py

-        elif normalize:
-            spec.normalize(tests=tests)
+        sargs = ' '.join(args)
+    unquoted_flags = _UnquotedFlags.extract(sargs)


I'd move this inside the except block (just before if unquoted_flags:), as it's not needed except when there's an error.

* 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

This PR does three things: - [x] Allow any character to appear in the quoted values of variants and flags. - [x] Allow easier passing of quoted flags on the command line, e.g. `cflags="-O2 -g"` - [x] Handle quoting better in spec output, using single quotes around double quotes and vice versa. This PR is motivated by ORNL's [tips for launching at scale on Frontier](https://docs.olcf.ornl.gov/systems/frontier_user_guide.html#tips-for-launching-at-scale). ORNL recommends using `sbcast --send-libs` to broadcast executables and their libraries to compute nodes when running large jobs (e.g., 80k ranks). For an executable named `exe`, `sbcast --send-libs` stores the needed libraries in a directory alongside the executable called `exe_libs`. ORNL recommends pointing `LD_LIBRARY_PATH` at that directory so that `exe` will find the local libraries and not overwhelm the filesystem. Spack's `RPATHs` break this because they're considered with higher precedence than `LD_LIBRARY_PATH`. So Spack applications will still end up hitting the shared filesystem when searching for libraries. We can avoid this by injecting some `ldflags` in to the build, e.g., if were were going to launch, say, `LAMMPS` at scale, we could add another `RPATH` specifically for use with `sbcast`: spack install lammps ldflags='-Wl,-rpath=$ORIGIN/lmp_libs' This will put the `lmp_libs` directory alongside `LAMMPS`'s `lmp` executable first in the `RPATH`, so it will be searched before any directories on the shared filesystem. Before this PR, the command above would've errored out for two reasons: 1. `$` wasn't an allowed character in our spec parser. 2. You would've had to double quote the flags to get them to pass through correctly: spack install lammps ldflags='"-Wl,-rpath=$ORIGIN/lmp_libs"' This is ugly and I don't think many users will easily figure it out. The behavior was added in #29282, and it improved parsing of specs passed as a single string, e.g.: spack install 'lammps ldflags="-Wl,-rpath=$ORIGIN/lmp_libs"' but a lot of users are naturally going to try to quote arguments *directly* on the command line, without quoting their entire spec. needed to quote them, but the error message could be confusing to users. In particular, if you wrote `cflags="-O2 -g"` on the command line, it would break the flags up, warn, and tell you that you could fix the issue by writing `cflags="-O2 -g"` even though you just wrote that. It's telling you to *quote* that value, but the user has to know to double quote. There are only two places where we allow arbitrary quoted strings in specs: flags and variant values, so this PR adds a simpler heuristic to the CLI parser: if an argument in `sys.argv` starts with `name=...`, then we assume the whole argument is quoted. This means you can write: spack install bzip2 cflags="-O2 -g" directly on the command line, without multiple levels of quoting. This also works: spack install 'bzip2 cflags="-O2 -g"' The only place where this heuristic runs into ambiguity is if you attempt to pass anonymous specs that start with `name=...` as one large string. e.g., this will be interpreted as one large flag value: spack find 'cflags="-O2 -g" ~bar +baz' This sets `cflags` to `"-O2 -g" ~bar +baz`, which is likely not what you wanted. You can fix this easily by either removing the quotes: spack find cflags="-O2 -g" ~bar +baz Or by adding a space at the start, which has the same effect: spack find ' cflags="-O2 -g" ~bar +baz' You may wonder why we don't just look for quotes inside of flag arguments, and the reason is that you *might* want them there. If you are passing arguments like: spack install zlib cppflags="-D DEBUG_MSG1='quick fox' -D DEBUG_MSG2='lazy dog'" You *need* the quotes there. So we've opted for one potentially confusing, but easily fixed outcome vs. limiting what you can put in your quoted strings. In addition to being more lenient about characters accepted in quoted strings, this PR fixes up spec formatting a bit. If a variant or flag value has double quotes in it, we'll print it with single quotes, and vice versa. If a variant or flag value has both types of quotes in it, we'll surround it with single quotes and escape internal single quotes. Examples: zlib cflags='-D FOO="bar"' zlib cflags="-D FOO='bar'" zlib cflags='-D FOO=\'bar\' BAR="baz"'

This PR does several things: - [x] Allow any character to appear in the quoted values of variants and flags. - [x] Allow easier passing of quoted flags on the command line, e.g. `cflags="-O2 -g"`. - [x] Handle quoting better in spec output, using single quotes around double quotes and vice versa. - [x] Disallow spaces around `=` and `==` when parsing variants and flags. ## Motivation This PR is motivated by the issues above and by ORNL's [tips for launching at scale on Frontier](https://docs.olcf.ornl.gov/systems/frontier_user_guide.html#tips-for-launching-at-scale). ORNL recommends using `sbcast --send-libs` to broadcast executables and their libraries to compute nodes when running large jobs (e.g., 80k ranks). For an executable named `exe`, `sbcast --send-libs` stores the needed libraries in a directory alongside the executable called `exe_libs`. ORNL recommends pointing `LD_LIBRARY_PATH` at that directory so that `exe` will find the local libraries and not overwhelm the filesystem. There are other ways to mitigate this problem: * You could build with `RUNPATH` using `spack config add config:shared_linking:type:runpath`, which would make `LD_LIBRARY_PATH` take precedence over Spack's `RUNPATHs`. I don't recommend this one because `RUNPATH` can cause many other things to go wrong. * You could use `spack config add config:shared_linking:bind:true`, added in #31948, which will greatly reduce the filesystem load for large jobs by pointing `DT_NEEDED` entries in ELF *directly* at the needed `.so` files instead of relying on `RPATH` search via soname. I have not experimented with this at 80,000 ranks, but it should help quite a bit. * You could use [Spindle](https://github.com/hpc/Spindle) (as LLNL does on its machines) which should transparently fix this without any changes to your executable and without any need to use `sbcast` or other tools. But we want to support the `sbcast` use case as well. ## `sbcast` and Spack Spack's `RPATHs` break the `sbcast` fix because they're considered with higher precedence than `LD_LIBRARY_PATH`. So Spack applications will still end up hitting the shared filesystem when searching for libraries. We can avoid this by injecting some `ldflags` in to the build, e.g., if were were going to launch, say, `LAMMPS` at scale, we could add another `RPATH` specifically for use with `sbcast`: spack install lammps ldflags='-Wl,-rpath=$ORIGIN/lmp_libs' This will put the `lmp_libs` directory alongside `LAMMPS`'s `lmp` executable first in the `RPATH`, so it will be searched before any directories on the shared filesystem. ## Issues with quoting Before this PR, the command above would've errored out for two reasons: 1. `$` wasn't an allowed character in our spec parser. 2. You would've had to double quote the flags to get them to pass through correctly: spack install lammps ldflags='"-Wl,-rpath=$ORIGIN/lmp_libs"' This is ugly and I don't think many users will easily figure it out. The behavior was added in #29282, and it improved parsing of specs passed as a single string, e.g.: spack install 'lammps ldflags="-Wl,-rpath=$ORIGIN/lmp_libs"' but a lot of users are naturally going to try to quote arguments *directly* on the command line, without quoting their entire spec. #29282 used a heuristic to detect unquoted flags and warn the user, but the warning could be confusing. In particular, if you wrote `cflags="-O2 -g"` on the command line, it would break the flags up, warn, and tell you that you could fix the issue by writing `cflags="-O2 -g"` even though you just wrote that. It's telling you to *quote* that value, but the user has to know to double quote. ## New heuristic for quoted arguments from the CLI There are only two places where we allow arbitrary quoted strings in specs: flags and variant values, so this PR adds a simpler heuristic to the CLI parser: if an argument in `sys.argv` starts with `name=...`, then we assume the whole argument is quoted. This means you can write: spack install bzip2 cflags="-O2 -g" directly on the command line, without multiple levels of quoting. This also works: spack install 'bzip2 cflags="-O2 -g"' The only place where this heuristic runs into ambiguity is if you attempt to pass anonymous specs that start with `name=...` as one large string. e.g., this will be interpreted as one large flag value: spack find 'cflags="-O2 -g" ~bar +baz' This sets `cflags` to `"-O2 -g" ~bar +baz`, which is likely not what you wanted. You can fix this easily by either removing the quotes: spack find cflags="-O2 -g" ~bar +baz Or by adding a space at the start, which has the same effect: spack find ' cflags="-O2 -g" ~bar +baz' You may wonder why we don't just look for quotes inside of flag arguments, and the reason is that you *might* want them there. If you are passing arguments like: spack install zlib cppflags="-D DEBUG_MSG1='quick fox' -D DEBUG_MSG2='lazy dog'" You *need* the quotes there. So we've opted for one potentially confusing, but easily fixed outcome vs. limiting what you can put in your quoted strings. ## Quotes in formatted spec output In addition to being more lenient about characters accepted in quoted strings, this PR fixes up spec formatting a bit. We now format quoted strings in specs with single quotes, unless the string has a single quote in it, in which case we JSON-escape the string (i.e., we add `\` before `"` and `\`). zlib cflags='-D FOO="bar"' zlib cflags="-D FOO='bar'" zlib cflags="-D FOO='bar' BAR=\"baz\""

This PR does several things: - [x] Allow any character to appear in the quoted values of variants and flags. - [x] Allow easier passing of quoted flags on the command line, e.g. `cflags="-O2 -g"`. - [x] Handle quoting better in spec output, using single quotes around double quotes and vice versa. - [x] Disallow spaces around `=` and `==` when parsing variants and flags. ## Motivation This PR is motivated by the issues above and by ORNL's [tips for launching at scale on Frontier](https://docs.olcf.ornl.gov/systems/frontier_user_guide.html#tips-for-launching-at-scale). ORNL recommends using `sbcast --send-libs` to broadcast executables and their libraries to compute nodes when running large jobs (e.g., 80k ranks). For an executable named `exe`, `sbcast --send-libs` stores the needed libraries in a directory alongside the executable called `exe_libs`. ORNL recommends pointing `LD_LIBRARY_PATH` at that directory so that `exe` will find the local libraries and not overwhelm the filesystem. There are other ways to mitigate this problem: * You could build with `RUNPATH` using `spack config add config:shared_linking:type:runpath`, which would make `LD_LIBRARY_PATH` take precedence over Spack's `RUNPATHs`. I don't recommend this one because `RUNPATH` can cause many other things to go wrong. * You could use `spack config add config:shared_linking:bind:true`, added in spack#31948, which will greatly reduce the filesystem load for large jobs by pointing `DT_NEEDED` entries in ELF *directly* at the needed `.so` files instead of relying on `RPATH` search via soname. I have not experimented with this at 80,000 ranks, but it should help quite a bit. * You could use [Spindle](https://github.com/hpc/Spindle) (as LLNL does on its machines) which should transparently fix this without any changes to your executable and without any need to use `sbcast` or other tools. But we want to support the `sbcast` use case as well. ## `sbcast` and Spack Spack's `RPATHs` break the `sbcast` fix because they're considered with higher precedence than `LD_LIBRARY_PATH`. So Spack applications will still end up hitting the shared filesystem when searching for libraries. We can avoid this by injecting some `ldflags` in to the build, e.g., if were were going to launch, say, `LAMMPS` at scale, we could add another `RPATH` specifically for use with `sbcast`: spack install lammps ldflags='-Wl,-rpath=$ORIGIN/lmp_libs' This will put the `lmp_libs` directory alongside `LAMMPS`'s `lmp` executable first in the `RPATH`, so it will be searched before any directories on the shared filesystem. ## Issues with quoting Before this PR, the command above would've errored out for two reasons: 1. `$` wasn't an allowed character in our spec parser. 2. You would've had to double quote the flags to get them to pass through correctly: spack install lammps ldflags='"-Wl,-rpath=$ORIGIN/lmp_libs"' This is ugly and I don't think many users will easily figure it out. The behavior was added in spack#29282, and it improved parsing of specs passed as a single string, e.g.: spack install 'lammps ldflags="-Wl,-rpath=$ORIGIN/lmp_libs"' but a lot of users are naturally going to try to quote arguments *directly* on the command line, without quoting their entire spec. spack#29282 used a heuristic to detect unquoted flags and warn the user, but the warning could be confusing. In particular, if you wrote `cflags="-O2 -g"` on the command line, it would break the flags up, warn, and tell you that you could fix the issue by writing `cflags="-O2 -g"` even though you just wrote that. It's telling you to *quote* that value, but the user has to know to double quote. ## New heuristic for quoted arguments from the CLI There are only two places where we allow arbitrary quoted strings in specs: flags and variant values, so this PR adds a simpler heuristic to the CLI parser: if an argument in `sys.argv` starts with `name=...`, then we assume the whole argument is quoted. This means you can write: spack install bzip2 cflags="-O2 -g" directly on the command line, without multiple levels of quoting. This also works: spack install 'bzip2 cflags="-O2 -g"' The only place where this heuristic runs into ambiguity is if you attempt to pass anonymous specs that start with `name=...` as one large string. e.g., this will be interpreted as one large flag value: spack find 'cflags="-O2 -g" ~bar +baz' This sets `cflags` to `"-O2 -g" ~bar +baz`, which is likely not what you wanted. You can fix this easily by either removing the quotes: spack find cflags="-O2 -g" ~bar +baz Or by adding a space at the start, which has the same effect: spack find ' cflags="-O2 -g" ~bar +baz' You may wonder why we don't just look for quotes inside of flag arguments, and the reason is that you *might* want them there. If you are passing arguments like: spack install zlib cppflags="-D DEBUG_MSG1='quick fox' -D DEBUG_MSG2='lazy dog'" You *need* the quotes there. So we've opted for one potentially confusing, but easily fixed outcome vs. limiting what you can put in your quoted strings. ## Quotes in formatted spec output In addition to being more lenient about characters accepted in quoted strings, this PR fixes up spec formatting a bit. We now format quoted strings in specs with single quotes, unless the string has a single quote in it, in which case we JSON-escape the string (i.e., we add `\` before `"` and `\`). zlib cflags='-D FOO="bar"' zlib cflags="-D FOO='bar'" zlib cflags="-D FOO='bar' BAR=\"baz\""

* 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

spackbot-app bot added commands tests General test capability(ies) labels Mar 2, 2022

cosmicexplorer requested review from becker33, tgamblin and tldahlgren March 2, 2022 05:28

cosmicexplorer force-pushed the fix-dep-kv-flags branch from ed6ecca to 3c34a6b Compare March 2, 2022 06:04

cosmicexplorer changed the title ~~fix double shell quoting args to spack spec~~ fix doubly shell quoting args to spack spec Mar 2, 2022

cosmicexplorer mentioned this pull request Mar 2, 2022

[WIP] parsing variant value assignments across dependencies in specs #29098

Closed

2 tasks

becker33 reviewed Mar 2, 2022

View reviewed changes

cosmicexplorer force-pushed the fix-dep-kv-flags branch 2 times, most recently from 1ac8f9b to 333cf7e Compare May 9, 2022 19:40

cosmicexplorer force-pushed the fix-dep-kv-flags branch from d79ca03 to af00a25 Compare May 9, 2022 21:49

alalazo assigned trws May 10, 2022

cosmicexplorer force-pushed the fix-dep-kv-flags branch from af00a25 to dfdecb8 Compare May 10, 2022 05:38

cosmicexplorer force-pushed the fix-dep-kv-flags branch 2 times, most recently from c6a442b to f807d35 Compare May 11, 2022 12:36

cosmicexplorer force-pushed the fix-dep-kv-flags branch from f807d35 to 5a4d9c2 Compare May 11, 2022 21:34

trws previously approved these changes May 12, 2022

View reviewed changes

trws requested a review from becker33 May 12, 2022 19:06

cosmicexplorer added 6 commits June 10, 2022 22:22

add test to verify fix works

1fff891

fix spec cflags/variants parsing test (breaking change)

ed1bbcf

fix spack spec arg quoting issue

532b640

add error report for deprecated cflags coalescing

3411f2c

use .group(n) vs subscript regex group extraction for 3.5 compat

3ae75cc

add random test for untested functionality to pass codecov

8971312

cosmicexplorer dismissed trws’s stale review via 8971312 June 11, 2022 02:26

cosmicexplorer force-pushed the fix-dep-kv-flags branch from 5a4d9c2 to 8971312 Compare June 11, 2022 02:26

fix new test failure since rebase

3aa5e0c

tgamblin reviewed Jun 11, 2022

View reviewed changes

tgamblin approved these changes Jun 11, 2022

View reviewed changes

tgamblin merged commit 0c7fd9b into spack:develop Jun 11, 2022

alalazo mentioned this pull request Jun 13, 2022

Backports v0.18.1 #31049

Merged

33 tasks

adrienbernede mentioned this pull request Jun 21, 2022

Spack spec fails with "complex" flags string even if quoted #31214

Closed

3 tasks

tgamblin mentioned this pull request Dec 10, 2023

Improve parsing of quoted flags and variants in specs #41529

Merged

4 tasks

Conversation

cosmicexplorer commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Solution

Result

Uh oh!

haampie commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

becker33 Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

trws Mar 2, 2022

Choose a reason for hiding this comment

Uh oh!

cosmicexplorer May 9, 2022

Choose a reason for hiding this comment

Uh oh!

trws commented Mar 2, 2022

Uh oh!

trws commented Mar 4, 2022

Uh oh!

cosmicexplorer commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cosmicexplorer commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trws commented May 9, 2022

Uh oh!

cosmicexplorer commented May 10, 2022

Uh oh!

trws commented May 10, 2022

Uh oh!

cosmicexplorer commented May 11, 2022

Uh oh!

cosmicexplorer commented May 11, 2022

Uh oh!

trws commented May 12, 2022

Uh oh!

cosmicexplorer commented May 12, 2022

Uh oh!

tgamblin Jun 11, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

cosmicexplorer commented Mar 2, 2022 •

edited

Loading

haampie commented Mar 2, 2022 •

edited

Loading

cosmicexplorer commented May 9, 2022 •

edited

Loading

cosmicexplorer commented May 9, 2022 •

edited

Loading