@cosmicexplorer FYI

@tgamblin This partially or mostly fixes #35756 and #31214.

This now works correctly:

spack spec zlib cflags='-g -O2'

This now correctly fails:

spack spec zlib cflags=\"-g  -O2\"

However, The Machine(*) finds one odd case that's still not correct.
The following should set cflags to empty and then fail because +foo
is not a legal variant for zlib.

$ spack spec zlib cflags=  +foo     
Input spec
--------------------------------
 -   zlib cflags='+foo' 

Concretized
--------------------------------
 -   [email protected]%[email protected] cflags='+foo' +optimize+pic+shared build_system=makefile arch=linux-rhel8-broadwell
 -       ^[email protected]%[email protected]~guile build_system=generic arch=linux-rhel8-broadwell

We see from the input spec that the error happens very early. The
shell passes 4 arguments to the spack command: spec, zlib,
cflags= and +foo. That much is the shell, spack has no control
over this. (Not without adding more quotes to the command line.)

But early on, something in spack is incorrectly combining args 3+4
into cflags='+foo', and from there the error is propagated on.

Worse yet, spack spec zlib cflags= -pic is legal and should set cflags
to empty and turn pic off. But again, -pic is incorrectly pulled into cflags.

$ ./spack spec zlib cflags=  -pic
Input spec
--------------------------------
 -   zlib cflags=-pic 

Concretized
--------------------------------
 -   [email protected]%[email protected] cflags=-pic +optimize+pic+shared build_system=makefile arch=linux-rhel8-broadwell
 -       ^[email protected]%[email protected]~guile build_system=generic arch=linux-rhel8-broadwell

But be careful, remember cflags=\"-g -O2\" should be an error.

(*) Sometimes, I like to think of my alter-ego as "The Big, Bad
Counter-Example Machine." I can find a counter-example to anything.
Do not taunt The Machine. :-)

@mwkrentel: historically we have allowed spaces on either side of the = for variants and flags. So the args come in, the current heuristic doesn’t notice cflags= because it thinks there should be a value, then the whole thing gets joined with spaces and sent to the parser, which is ok with it.

I think we should fix the CLI case, because the user could’ve typed:

spack spec zlib cflags=“” -pic

that’s easy enough — we just match any argv element that starts with name= regardless of value. It actually simplifies the heuristic.

What about the “whole thing is a string” case? i.e.:

spack spec ‘zlib cflags=  -pic’

That’s legal at the moment. -pic becomes part of the flags because the whitespace isn’t meaningful to the parser. This works the way you want (just like if you wrote it on the CLI)

spack spec ‘zlib cflags=“” -pic’

I’m hesitant to disallow spaces around = b/c we’ve always allowed it. Thoughts?

If the issue was only the CLI and the shell, then my answer would be:

   The shell has precise rules for quoting.
   Trust the shell to do quoting the way it does,
   and trust your users to write correct shell quoting.
   (That is, don't try to out-think the rules.)

I'm guessing the real problem here is that you also have to parse
config files and you want to allow a more relaxed syntax in the config
files (something that a human would understand), BUT you're parsing
both with the same set of rules. Is that right?

Is it possible to separate these two cases?
Maybe put a preprocessing step for config files that implements
the heuristic rules that isn't used for CLI?

Note: the shell treats these two lines as identical:

spack spec zlib cflags= +pic
spack spec zlib cflags="" +pic

So, if you're trying to tweak the first case to do what you think the
user meant to write, then you can never get both cases correct.

I don't know what to suggest. I think I need to know more about what
syntax you want to allow in a config file and how it gets parsed. Is
there a grammar for this?

Playing standardese guy for a second, has spack intentionally supported spaces around = or is it an implementation detail? I ask because all the language I can find in the documentation, in help --spec and everywhere else, all says "if it contains spaces, it must be quoted" or similar. All examples are also bound correctly, so I would have called ‘zlib cflags= -pic’ parsing as zlib cflags=-pic a parser error without reading this conversation.

Changing it might technically be a breaking change in the software, but I've yet to find anywhere it would be a change in the documented API.

@trws we actually did a similar breaking change in #40344, see the bottom of the pr description too.

I don't see the point of allowing white space around it either, and wouldn't consider it very breaking, cause it's pretty ambiguous.

I also agree with @mwkrentel in that I know how shell quoting works, I find any tricks that Spack does with argv only confusing, I'd rather just have spack parse ' '.join(argv) as a string.

@haampie

I'd rather just have spack parse ' '.join(argv) as a string.

I bet you don't want that at all, or at least no user I know wants that, because:

spack spec zlib cflags="-O3 -g" +bar

will parse as:

sys.argv           == ["spack", "spec", "zlib", "cflags=-O3 -g", "+bar"]
" ".join(sys.argv) == "spack spec zlib cflags=-O3 -g +bar"

And the resulting spec is:

zlib~g+bar cflags="-O3"

This is exactly what this PR is intended to solve.

I think when I tried to do something akin to this before I literally quoted every single argument before joining them, such that we could get some of this, but it caused problems with a whole lot of tests. The problem is we accept both "spec ~quoted +just because='I felt like it' ^meh" (which is what we get in configs but almost never the CLI) and "spec ~" "quoted" "+" "just" "because=I felt like it" ^ meh with no way to differentiate, and have for a very long time, so either we end up with heuristics like this (which this one seems to work quite well), we tighten the syntax so we can break the ambiguities, or we need a way to explicitly say "break this spec string apart"/"join these pieces together into a spec" to get the rest of the way around it. If we could require something like "each argument must be a complete token" then it gets substantially simpler since each piece could be validated by itself and horrible abominations like "gcc ~" 'bootstrap cflags=-O3 +' binutils could actually be diagnosed. When we looked at it last time, the consensus seemed to be that we were more willing to keep with this pattern than to break one or the other. This seems to be the best balance I've seen down that road.

If we wanted to make it no longer ambiguous, my preference would be to actually require each argument to be a "complete" component. Rather like gcc doesn't accept '-' std =c++ '14' we could require each argument to be one or more complete components, with or without extra spaces. That would mean gcc%clang@whatever+bootstrap would still be valid, but gcc% clang would not for example. Which honestly would be a good thing IMO, the fact that binutils as an argument might either be another spec, or it might be a variant depending on the last character of another argument feels really kinda icky...

@mwkrentel @trws: see if the current version is sufficient. Tests should be passing now.

I have fixed the case @mwkrentel brought up, specifically the case where you pass either of these on the CLI:

spack spec zlib cflags=  +foo
spack spec zlib cflags=""  +foo

Both of those result in the same argv, and they will both result in the spec zlib+foo. If you want to see all the special cases I could come up with, look at the test_cli_spec_roundtrip test here, as it pretty much enumerates them. If you can recommend tweaks to make some of them less special, then I'd be interested in hearing those.

On disallowing spaces:

@trws @haampie:

has spack intentionally supported spaces around = or is it an implementation detail?

I intentionally supported it from the start, because I don't think (or at least did not think) that the grammar should be sensitive to whitespace. In practice, the grammar is sensitive to whitespace. e.g., you can't put spaces around commas in multi-valued variant values, and we disallowed spaces in version ranges in #40344.

So, I'm not unsympathetic to disallowing spaces around flags and variants, but I also don't think it matters much. I went ahead and implemented it, and the only changes that it introduces are the following:

1. These two tests now fail to parse (as expected):

   @1.2:   develop   = foo
   @1.2:develop   = foo
   

2. Spaces are preserved in quoted variant values on the CLI, where they were not before:

   +        # These flags are assumed to be quoted by the shell, but the space doesn't matter because
   +        # flags are space-separated.
            (["zlib", "ldflags= +pic"], "zlib ldflags='+pic'"),
            (["ldflags= +pic"], "ldflags='+pic'"),
   -        # Ensure same results with variants (not flag names)
   -        (["zlib", "foo= +pic"], "zlib foo='+pic'"),
   -        (["foo= +pic"], "foo='+pic'"),
   +        # If the name is not a flag name, the space is preserved verbatim, because variant values
   +        # are comma-separated.
   +        (["zlib", "foo= +pic"], "zlib foo=' +pic'"),
   +        (["foo= +pic"], "foo=' +pic'"),

3. These two weirdly quoted values now result in an error where they would've gobbled up extra stuff before:

   -        ([" ldflags= +pic"], "ldflags='+pic'"),
   -        ([" ldflags=", "+pic"], "ldflags='+pic'"),
   +        ([" ldflags= +pic"], SpecTokenizationError),
   +        ([" ldflags=", "+pic"], SpecTokenizationError),

This is because the real problem isn't that we allow or disallow spaces around = and ==; it's that what you get from the CLI is fundamentally ambiguous and different from what you get out of a text file -- you cannot recover the quoting (or lack of it) that the user types on the CLI.

There is really only one parse that you might care about here - namely if a user accidentally wrote:

Spec("zlib cflags= +pic")

+pic is going to get gobbled. On the CLI, if you wrote the same thing, it would get you zlib+pic.

So to decide whether you want to disallow spaces, the question is really whether breaking any users who were using spaces (we don't know how many were) is worth returning an error (probably a better outcome) in case 3 above.

Regardless of whether you allow spaces, you need the heuristic I introduced here to determine whether you should consider a key-value pair on the CLI to be quoted or not. Without that I think you're stuck with what I consider a far greater evil, which is that you have to write one of these on the CLI:

zlib cflags=\"-O2 -g\"    # unforgivable madness
zlib cflags='"-O2 -g"'    # redundant stupidity

Both of those are anti-user monstrosities while, with the fix I added above for @mwkrentel, disallowing spaces really only affects the corner cases.

@mwkrentel:

I'm guessing the real problem here is that you also have to parse
config files and you want to allow a more relaxed syntax in the config
files (something that a human would understand), BUT you're parsing
both with the same set of rules. Is that right?

Yes. The grammar was born on the CLI (look at parser.py for the details) but it's grown, and there is no way to make both worlds do the same things once you start allowing quoting. It's fundamentally ambiguous.

Is it possible to separate these two cases?
Maybe put a preprocessing step for config files that implements
the heuristic rules that isn't used for CLI?

This PR essentially does the reverse -- which I think is more understandable. Basically there is one heuristic: if any arg in argv starts with name= or name==, assume the rest of arg is a quoted value. That's it. Add the quotes (with proper escaping) and parse the result.

I think this results in the most consistency possible (see above for the corner cases) between the two worlds. Nearly anything you write on the CLI will end up being parsed the same as if you wrote it in a file, and vice versa. I think the only meaningful corner cases at this point are:

cflags=\"-O2 -g\"   # works in a file (but why would you write it?) and fails on the CLI
cflags="" +pic      # works the same in both places
cflags= +pic        # works on the CLI, fails in a file (can't distinguish from above on CLI)

And you can make anything above behave as though it was in a file by quoting and starting with space:

' cflags=\"-O2 -g\"'
' cflags="" +pic'
' cflags= +pic'

But I don't know why anyone would write these -- maybe if they were writing a bash script or something that accumulates specs in a string. In Python there are much better ways to do these things, so I think it's really confined to CLI corner cases.

@trws @tgamblin

If we wanted to make it no longer ambiguous, my preference would be to actually require each argument to be a "complete" component.

For what is worth the initial implementation of #34151 was exactly like ☝️ but was changed per #34151 (comment) and preceding discussion in the PR to get 100% backward compatibility.

I'd be 👍 if we want to go back and disallow @ 2.3 % gcc + foo etc.

I'd be 👍 if we want to go back and disallow @ 2.3 % gcc + foo etc.

Why though? We have packages in mainline (look at mumps) that use this. 2/3 of users have private package repositories. I can't imagine that this isn't pointlessly breaking something.

Again, the spaces allowed in the grammar aren't the real issue here. They're not what is preventing us from writing cflags="-O2 -g" on the CLI.

I'm dropping myself from the discussion cause I feel like I'm a minority, but not before pointing out a few things anyways:

1. Isn't the motivating example spack install lammps ldflags='-Wl,-rpath=$ORIGIN/lmp_libs' somewhat flawed? ldflags=-Wl,-rpath=$ORIGIN/lmp_libs as a literal string can be made to parse, it's unnecessary to require quotes?

2. The argv Spack receives doesn't help with the intended cflags argv. If you want to break up cflags you need different syntax, like spack install x 'cflag=-DVERSION="version 1.2.3"' cflag=-O3 (ref -Xlinker / -Wl,). That said, you can't specify that one in a config file at all, right?

3. Anybody who has run a command over ssh knows the issue of double quoting, and everyone coding in C is used to gcc -DSTRING=\"ABC\", I don't think it's really necessary to "solve" quoting issues in Spack, imo it makes it only hard to reason about if Spack is trying to be helpful.

1. You either need to single-quote ‘$ORIGIN’, or you need to escape the $ to make the shell ignore it.

2. With this PR, you can write:

   spack install x cflags=‘-DVERSION="version 1.2.3" -O3’
   

   or

   spack install x cflags=‘-DVERSION="version 1.2.3"’ cflags=-O3
   

   Both seem clear to me, but the first one is what I would instinctually type; the second one I might use if I wanted to propagate the -O (with ==) but not the -D.

   You can write both in a YAML file, with or without the outer quotes.

3. I don’t see where in this instance the “helpfulness” is getting in the way. This PR allows you to put quotes around space-separated or single args when needed, without the footgun we currently have, namely that this fails:

   spack install foo cflags=“-O2 -g”    # foo~g cflags=-O2
   

   I’m used to escaping quotes in C, yes, but not on the CLI, where quoting as above a) works and b) is expected by users.

(Performance nitpick) There seems to be a slight slowdown when parsing key/value pairs:

we can recover most of it with this diff:

diff --git a/lib/spack/spack/cmd/__init__.py b/lib/spack/spack/cmd/__init__.py
index cb4083ed8f..58c4de3469 100644
--- a/lib/spack/spack/cmd/__init__.py
+++ b/lib/spack/spack/cmd/__init__.py
@@ -154,7 +154,7 @@ def quote_kvp(string: str) -> str:
     or ``name==``, and we assume the rest of the argument is the value. This covers the
     common cases of passign flags, e.g., ``cflags="-O2 -g"`` on the command line.
     """
-    match = re.match(spack.parser.SPLIT_KVP, string)
+    match = spack.parser.SPLIT_KVP.match(string)
     if not match:
         return string
 
diff --git a/lib/spack/spack/parser.py b/lib/spack/spack/parser.py
index d8d3deef3c..60a0ec8c38 100644
--- a/lib/spack/spack/parser.py
+++ b/lib/spack/spack/parser.py
@@ -100,7 +100,7 @@
 VALUE = r"(?:[a-zA-Z_0-9\-+\*.,:=\~\/\\]+)"
 
 #: Anything that matches this *will* be conservatively quoted in Spack output
-NO_QUOTES_NEEDED = r"^[a-zA-Z0-9,/_.-]+$"
+NO_QUOTES_NEEDED = re.compile(r"^[a-zA-Z0-9,/_.-]+$")
 
 #: quoted values can be *anything* in between quotes, including escaped quotes.
 QUOTED_VALUE = r"(?:'(?:[^']|(?<=\\)')*'|\"(?:[^\"]|(?<=\\)\")*\")"
@@ -110,15 +110,15 @@
 VERSION_LIST = rf"(?:{VERSION_RANGE}|{VERSION})(?:\s*,\s*(?:{VERSION_RANGE}|{VERSION}))*"
 
 #: Regex with groups to use for splitting (optionally propagated) key-value pairs
-SPLIT_KVP = rf"^({NAME})(==?)(.*)$"
+SPLIT_KVP = re.compile(rf"^({NAME})(==?)(.*)$")
 
 #: Regex to strip quotes. Group 2 will be the unquoted string.
-STRIP_QUOTES = r"^(['\"])(.*)\1$"
+STRIP_QUOTES = re.compile(r"^(['\"])(.*)\1$")
 
 
 def strip_quotes_and_unescape(string: str) -> str:
     """Remove surrounding single or double quotes from string, if present."""
-    match = re.match(STRIP_QUOTES, string)
+    match = STRIP_QUOTES.match(string)
     if not match:
         return string
 
@@ -140,7 +140,7 @@ def quote_if_needed(value: str) -> str:
     ``"``, and control codes.
 
     """
-    if re.match(spack.parser.NO_QUOTES_NEEDED, value):
+    if NO_QUOTES_NEEDED.match(value):
         return value
 
     return json.dumps(value) if "'" in value else f"'{value}'"
@@ -456,14 +456,14 @@ def parse(
                 )
 
             elif self.ctx.accept(TokenType.KEY_VALUE_PAIR):
-                match = re.match(SPLIT_KVP, self.ctx.current_token.value)
+                match = SPLIT_KVP.match(self.ctx.current_token.value)
                 assert match, "SPLIT_KVP and KEY_VALUE_PAIR do not agree."
 
                 name, delim, value = match.groups()
                 initial_spec._add_flag(name, strip_quotes_and_unescape(value), propagate=False)
 
             elif self.ctx.accept(TokenType.PROPAGATED_KEY_VALUE_PAIR):
-                match = re.match(SPLIT_KVP, self.ctx.current_token.value)
+                match = SPLIT_KVP.match(self.ctx.current_token.value)
                 assert match, "SPLIT_KVP and PROPAGATED_KEY_VALUE_PAIR do not agree."
 
                 name, delim, value = match.groups()

After the diff the results are:

@tgamblin Returning to these two examples for a minute:

spack spec zlib 'cflags=-g -O'      (works from CLI)
spack spec zlib ' cflags=-g -O'     (fails on CLI)

I'm still wondering if these two cases should behave differently.
Although I probably wouldn't write the second, I wouldn't really
expect it to behave differently. Also, as we've mentioned, it's not
that hard for a space to slip in from a script.

Suppose you stripped leading whitespace from every argv token.
Whould that break anything?

@mwkrentel:

Suppose you stripped leading whitespace from every argv token.
Would that break anything?

It would basically mean that you have no way to write an anonymous spec with multiple variant values in a single string. Specs don't have to have a name; you can start with a variant. If your script generates specs all in one string, and also quotes things properly internally, you probably don't want our heuristic. Consider:

spack spec 'cflags="-Dfoo=bar" +baz'

Our heuristic says "hey, that starts with r"^{IDENTIFIER}==?" so I'll consider it to be quoted on the command line. So you get an anonymous spec with only cflags set: cflags='"-Dfoo=bar" +baz'.

vs:

spack spec ' cflags="-Dfoo=bar" +baz'

The initial space gives you a way to always force the string to be parsed with its own internal quoting, and avoid our heuristic. Here you get the anonymous spec cflags="-Dfoo=bar" +baz.

The space was intended as an escape hatch for people building up specs in strings with their own quoting, and stripping it (or modifying the heuristic) removes the ability to do this.

That's unfortunate. You've convinced me that it's complicated. :-(

spack spec ' cflags="-Dfoo=bar" +baz'

The initial space gives you a way to always force the string to be parsed with its own internal quoting, and avoid our heuristic. Here you get the anonymous spec cflags="-Dfoo=bar" +baz.

The space was intended as an escape hatch for people building up specs in strings with their own quoting, and stripping it (or modifying the heuristic) removes the ability to do this.

This tempts me as a follow-on to see if a recursive regex to match nested quotes would be better or worse. It would handle that case, but I'm not sure it would really be better, since now the corner case would be if the quotes appear somewhere not touching the equals sign or the beginning of the argument, which might be even more surprising.

This tempts me as a follow-on to see if a recursive regex to match nested quotes would be better or worse. It would handle that case, but I'm not sure it would really be better, since now the corner case would be if the quotes appear somewhere not touching the equals sign or the beginning of the argument, which might be even more surprising.

@becker33 and I talked about this and opted for very simple heuristic over "try real hard to analyze internal quoting". Currently the rules are:

• if arg starts with name=: consider the whole arg to be enclosed with quotes from the CLI
• else: parse the arg without adding any quoting

I cannot think of a simpler heuristic, and I agree that analyzing internal quotes has the potential to make the heuristic very complicated. I also think that it would be very odd for someone to start a string with ' ' unless they meant to, and very rare for someone to want to parse an anonymous spec with additional args on the CLI. I don't think people are going to hit the corner cases very often, if at all.

Consider that this issue only comes up if you do something like:

"cflags='-Dfoo' other args here"

instead of

cflags='-Dfoo' other args here

I find it very hard to believe anyone would type the first one. If they are in the habit of quoting all specs, in most cases the spec is going to start with a name:

"zlib cflags='-Dfoo' other args here"
zlib cflags='-Dfoo' other args here

Both of those parse the same. It is really only the anonymous spec case that is affected by this, and only if they accidentally cram some extra args in quotes with it. So, again: rare, and a lesser evil.

If you are assembling specs in strings in the shell (i.e., if you are writing a really complicated bash script that you should be writing in python instead), we will document that you can fix this by starting specs with ' ', which IMO is a simple and understandable fix to bash people who are used to quotation madness.

I think you have a choice between:

1. "make shell quoting work like people expect" and
2. "save the very obscure shell script writers from having to think about this one weird but remarkably consistent trick"

Let me repeat: safe harbor. What I need is a way to write a given
spec in a straightforward way where I'm confident the parser will do
what I want.

Remember, this started with spack spec zlib cflags='-g -O'
which currently fails on CLI in develop.

I missed @alalazo's suggestion in #41529 (comment), but added it in #41657.

Should we backport this to v0.21.1?

Should we backport this to v0.21.1?

I don't think so, b/c it is a breaking change -- \" will no longer work when spread across unquoted args.