Parser: fix ambiguity with whitespace in version ranges by haampie · Pull Request #40344 · spack/spack

haampie · 2023-10-06T08:35:43Z

This is a breaking change, but imho develop is broken, so it's rather a bugfix.

Allowing white space around : in version ranges introduces an ambiguity:

a@1: b

parses as a@1:b but should really be parsed as two separate specs a@1: and b.

With white space disallowed around : in ranges, the ambiguity is resolved.

White space is still allowed around the , separator in version lists, since there we unambiguously require a next version/range token anyways, and in fact the syntax with whitespaces around , is used in a bunch of packages.

Apart from that, there's this rather odd, supported syntax:

a@1:b = c

It's parsed as [a, @1:, b=c]. Apparently this syntax is relied on in exactly one package -- I think disallowing that would have made sense if it wasn't relied on... I've left this as is, and added an additional test for this syntax.

Allowing white space introduces an ambiguity: ``` spack spec a@1: b ``` parses as `a@1:b` but should be parsed as two separate specs `a@1:` and `b`

alalazo

LGTM. Only a minor request.

lib/spack/spack/test/spec_syntax.py

lib/spack/spack/parser.py

This reverts commit 96f4e98.

tgamblin · 2023-10-07T22:22:10Z

Apparently this syntax is relied on in exactly one package

Which package relies on this?

haampie · 2023-10-08T06:45:03Z

spack/var/spack/repos/builtin/packages/py-wasabi/package.py

Lines 19 to 23 in e2a7f2e

    
           depends_on("py-setuptools", type="build") 
        
           depends_on( 
        
               "py-typing-extensions@3.7.4.1:4.4", type=("build", "run"), when="@1.1.2:^python@:3.7" 
        
           ) 
        
           depends_on("py-colorama@0.4.6:", type=("build", "run"), when="@1.1.2:platform=windows")

alalazo

LGTM, but let's wait for at least another ✔️ for the breaking change.

becker33

Can you add this detail to the comment at the top of lib/spack/spack/parser.py? I think that's the only relevant documentation to update.

tgamblin

Since this is breaking anyway I don't think it's unreasonable to not support the py-wasabi case. We generally require a space to differentiate key-value pairs in other places... I don't see why we shouldn't also require it after a version range.

so, disallow:

a@1:b = c

and require this space:

a@1: b = c
    ^

Or do folks object?

haampie · 2023-10-09T21:43:10Z

These are all valid:

@1:^python
@:2, 4%gcc
build_system = makefile%gcc

Maybe % and ^ are visually good enough separators, but to me it feels like it's all not very consistent, and disallowing a@1:b=c should also mean disallowing all of the above, and that will certainly make it more likely that user repos break

All whitespace in verison specifiers should've been banned from the start ;p

tgamblin · 2023-10-09T22:29:57Z

I don’t think it’s inconsistent. In other places we change context based on sigils, and require space for kvp’s (which start with an identifier). This is just (I think?) the only place where there is a word boundary because of a colon at the end of the thing before a kvp.

OTOH, if there is no need to disable support for it (does it introduce ambiguity or just weirdness?) then it could stay like it is.

haampie · 2023-10-10T05:56:09Z

It's not ambiguous, @1:a=b cannot be parsed as [@1:a, =b]

haampie · 2023-10-10T07:02:29Z

Can you add this detail to the comment at the top of lib/spack/spack/parser.py? I think that's the only relevant documentation to update.

What do you want to add exactly? Those high-level rules are generally unclear about use of white space, as I read them they're even more strict than what we allow, but it's hard to follow since it's not valid EBNF. I think the implementation + tests are the only correct documentation

tgamblin · 2023-10-19T18:35:35Z

@haampie: I think my concerns are addressed by the conversation but we're still waiting on @becker33's.

haampie · 2023-10-30T08:41:46Z

Ping @becker33

haampie · 2023-11-01T08:03:02Z

Ping @becker33

2 approvals should be enough

Allowing white space around `:` in version ranges introduces an ambiguity: ``` a@1: b ``` parses as `a@1:b` but should really be parsed as two separate specs `a@1:` and `b`. With white space disallowed around `:` in ranges, the ambiguity is resolved.

disallow white space in version ranges

563925d

Allowing white space introduces an ambiguity: ``` spack spec a@1: b ``` parses as `a@1:b` but should be parsed as two separate specs `a@1:` and `b`

spackbot-app bot added core PR affects Spack core functionality tests General test capability(ies) labels Oct 6, 2023

Improve spec parser whitespace in version tests

fb43645

haampie force-pushed the fix/parser-ambiguity branch from 848aeb9 to fb43645 Compare October 6, 2023 08:47

alalazo requested changes Oct 6, 2023

View reviewed changes

lib/spack/spack/test/spec_syntax.py Show resolved Hide resolved

tldahlgren assigned alalazo Oct 6, 2023

restore some whitespace related tests

d9002cd

haampie commented Oct 7, 2023

View reviewed changes

lib/spack/spack/parser.py Outdated Show resolved Hide resolved

haampie added 2 commits October 7, 2023 10:54

Also deal with x:a=b unclarity

96f4e98

Revert "Also deal with x:a=b unclarity"

55ab8ca

This reverts commit 96f4e98.

haampie force-pushed the fix/parser-ambiguity branch from d0529a2 to 55ab8ca Compare October 7, 2023 08:56

haampie added 3 commits October 7, 2023 11:04

simplify regex

cdab89e

Add a test for @1.2:develop = x

63f344b

Use non-capturing groups, as they are not used

1e6cce1

haampie mentioned this pull request Oct 7, 2023

parser: use non-capturing groups #40373

Merged

alalazo approved these changes Oct 9, 2023

View reviewed changes

alalazo assigned tgamblin Oct 9, 2023

haampie assigned becker33 Oct 9, 2023

becker33 previously requested changes Oct 9, 2023

View reviewed changes

tgamblin requested changes Oct 9, 2023

View reviewed changes

haampie requested review from becker33 and tgamblin October 12, 2023 10:40

haampie added this to the v0.21.0 milestone Oct 17, 2023

tgamblin approved these changes Oct 19, 2023

View reviewed changes

alalazo approved these changes Nov 1, 2023

View reviewed changes

haampie merged commit ac976a4 into spack:develop Nov 1, 2023

haampie deleted the fix/parser-ambiguity branch November 1, 2023 08:09

haampie mentioned this pull request Dec 11, 2023

Improve parsing of quoted flags and variants in specs #41529

Merged

4 tasks

Conversation

haampie commented Oct 6, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

tgamblin commented Oct 7, 2023

Uh oh!

haampie commented Oct 8, 2023

Uh oh!

alalazo left a comment

Choose a reason for hiding this comment

Uh oh!

becker33 left a comment

Choose a reason for hiding this comment

Uh oh!

tgamblin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

haampie commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Oct 9, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

haampie commented Oct 10, 2023

Uh oh!

haampie commented Oct 10, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tgamblin commented Oct 19, 2023

Uh oh!

haampie commented Oct 30, 2023

Uh oh!

haampie commented Nov 1, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

haampie commented Oct 6, 2023 •

edited

Loading

tgamblin left a comment •

edited

Loading

haampie commented Oct 9, 2023 •

edited

Loading

tgamblin commented Oct 9, 2023 •

edited

Loading

haampie commented Oct 10, 2023 •

edited

Loading