Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

support strict version inequalities in specs #20258

Closed
wants to merge 9 commits into from

Conversation

cosmicexplorer
Copy link
Contributor

@cosmicexplorer cosmicexplorer commented Dec 6, 2020

Problem

This is phase 1 of proposed extensions for the spec syntax: see #20256 (comment):

  1. Support pip requirements.txt version comparators and wildcards:
    • <=,=>, and == already exist.
    • ==9.2.* => >=9.2,<9.3 (reduce to subproblem)
    • !=9.2.0 => <9.2.0,>9.2.0 (reduce to subproblem)
    • imprecise:
      • <9.2.0 => :9.1.999
      • 9.2.0 => 9.2.0.0.0.1

      • Spack is unable to represent strict inequalities (<,>) without this feature.
    • Breakage: none.

Solution

  • Extend the Spec parser to process all of the above new inequality notations.
  • Extend the Lexer to allow for more than 2 modes (this is useful setup for any further work on the spec syntax).
  • Extend VersionRange a bit to support the notion of "including the left/right endpoint", which is used to ensure __contains__ and __lt__ still work on the new edge cases.
  • Add testing.

Result

@:!3 and @3!: should let users avoid needing to type out the .999.999 or .0.0.0.1 suffixes (which I personally find difficult to maintain and ultimately incorrect).

@cosmicexplorer cosmicexplorer added documentation Improvements or additions to documentation specs proposal versions concretizer-use-case interesting dependency hierarchy that would challenge the current concretizer labels Dec 6, 2020
@cosmicexplorer cosmicexplorer force-pushed the spec-parse-inequalities branch 2 times, most recently from fea0a30 to bf46beb Compare December 6, 2020 15:50
@haampie
Copy link
Member

haampie commented Dec 6, 2020

I would say *, > and < are not a great choice for the command line

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Dec 6, 2020

I would say *, > and < are not a great choice for the command line

That is a great point! I think that especially with zsh so many symbols like ^ need to be quoted anyway to avoid this weird filename expansion mechanism it has, that i generally start wrapping spack specs in single or double quotes once they get longer than name@version.

Do you think the concern about copying shell metacharacters still apply if we (optionally?) restricted this syntax to yaml files? That loses the benefit of having a nicer syntax to play around with interactively, though. Hmmm.

I think there's an easier way to resolve this issue. I think since we already had to take the time to lower each new operator into the equivalent "canonical" spec form (i.e. <1.2.3 becomes :!1.2.3, which was also an operator newly introduced in this PR), and it's already confusing to me when I type spack install x@<3 and then I get it printing out the "canonicalized" x@:!3 for all of its output, that maintaining the pip-style facade seems to make no sense, for the reasons you described.

So I think the way I'll approach this is to probably to first try removing all of the new pip-like comparison operators. In particular, supporting the x@1.2.* syntax was pretty annoying, since it had to add a lot of logic acting on Version, and then propagate that up to VersionRange somehow.

I'm pretty sure I'll be able to remove all of the ones you mentioned (*,<,>) from this change. I also think being able to revert the .* version stuff will make this change much less intrusive and easier to review.

TODO:

  • Remove all of the pip-style (<,>,==,!=) version comparators (this will remove maybe 50-100 lines of code in the lexer and parser :D)
    • Remove .* support from classmethods on VersionRange to get another 50-100 lines back.
  • Ensure the spack-style analogues to the missing comparators (@:1.2,@1.2:,@1.2:!1.3, @!3, @3, @:!2, @2!:, @1!:!2) are sufficiently tested.
  • Make the rest of the CI pass!

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Dec 8, 2020

I realized I had incorrectly assumed that "x's endpoint value is infinite, y's is finite" implies "x < y", which is only true at the left endpoint. I fixed this in _VersionEndpoint, and am now seeing either that or something else causing a failure to concretize the spec mpileaks%gcc ^ elfutils@0.170, which is used as a smoke test in share/spack/qa/run-unit-tests. Looking into this.

@vsoch
Copy link
Member

vsoch commented Dec 23, 2020

hey @cosmicexplorer ! We had really good discussion on slack, and I think this issue would be a first good one to start helping with. So before we get holiday-ed away, I want to see how I can start helping here. The goal of this PR seems fairly straight forward - to give users more freedom to specify ranges of versions with a new syntax, and it looks like it needs a rebase to fix conflicts (I don't have write access so I cannot) but after that, what specific bullet / thing could I look at first?

To give you some context for my helping, I won't be officially doing so until early February of next year, but I want to start familiarizing with Spack so it's not so slow then. To set your expectation, this means that this first round of learning will likely be slower than come next year.

@sethrj
Copy link
Contributor

sethrj commented Jan 7, 2021

Yes! I too loathe the 2.99.999.99999.9999999999 workaround and find the spack versioning surprising and unpythonic.

@haampie
Copy link
Member

haampie commented Feb 9, 2021

Hi @cosmicexplorer, what's the status of this PR?

@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Feb 11, 2021

@haampie as of about 5 minutes ago I realized I need the SpecLexer changes from this PR in order to complete #21538. So I'm planning to rebase this and implement the changes today and hope to get this reviewable.

make lex errors nicer to read

move spec version scanning into its own lexeme

fix version regex

undo version regex

make things work much more
…rison

- need to add the test described above
- fails other runs from `share/spack/qa/run-unit-tests` too
@cosmicexplorer
Copy link
Contributor Author

cosmicexplorer commented Mar 2, 2021

@haampie looking at this now, it seems some basic tests like "does !2 contain 3" and "does !2 contain 2" were not added. I learned this by adding them and seeing them fail -- I am going to fix that behavior now and the test should hopefully make it easier for you/everyone else to review.

@cosmicexplorer
Copy link
Contributor Author

Still working on this, but no longer have access to push to spack/spack, so will open up a new draft PR! I've made great progress in reducing the implementation logic, but have run into another conundrum, where Version('1.6') < Version('1.6.5'), but Version('1.6') is not also greater than Version('1.6.5'). I now believe this is incorrect, since a Version('1.6') can resolve to e.g. Version('1.6.8'), which is clearly greater than Version('1.6.5'). I will attempt to succinctly describe why this is now a concern in the new PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
concretizer-use-case interesting dependency hierarchy that would challenge the current concretizer documentation Improvements or additions to documentation proposal specs versions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants