Skip to content

Remove .99 when possible#26422

Merged
sethrj merged 1 commit intospack:developfrom
haampie:fix/no-more-99
Oct 3, 2021
Merged

Remove .99 when possible#26422
sethrj merged 1 commit intospack:developfrom
haampie:fix/no-more-99

Conversation

@haampie
Copy link
Copy Markdown
Member

@haampie haampie commented Oct 2, 2021 •
edited
Loading

After #26402 we can drop almost all .999's

Actually many .999's were redundant even without that pr.

I've glanced over most changes and verified there's no x:x, x.y:x.y, x.y.z:x.y.z using these kinds of regexes (\d+\.\d+):\1["', ]

@haampie haampie force-pushed the fix/no-more-99 branch 3 times, most recently from de7c078 to b113484 Compare October 2, 2021 13:00
sethrj
sethrj reviewed Oct 3, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@sethrj sethrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Github is having a little trouble with this many changed files, but I just have a question about the .0 files. I think you can also expand the scope lightly to replace some of the old and new N.0:N constraints to N:N which I think looks better.

var/spack/repos/builtin/packages/petsc/package.py
depends_on('[email protected]:6.1.99~int64', when='@3.10.3:3.12.99+superlu-dist+mpi~int64')
depends_on('[email protected]:6.1.99+int64', when='@3.10.3:3.12.99+superlu-dist+mpi+int64')
depends_on('[email protected]:5.1.3~int64', when='@3.7.0:3.7+superlu-dist+mpi~int64')
depends_on('[email protected]:5.1.3+int64', when='@3.7.0:3.7+superlu-dist+mpi+int64')
Copy link
Copy Markdown
Contributor

@sethrj sethrj Oct 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the introduction of .0 here is necessary unless you think N.0:N is clearer than N:N (I personally think it's ungainly).

@sethrj sethrj self-assigned this Oct 3, 2021
@sethrj sethrj added the versions label Oct 3, 2021
@haampie
Copy link
Copy Markdown
Member Author

haampie commented Oct 3, 2021

@sethrj unfortunately x:x is not supported by Spack right now :(. It's OK to create a VersionRange(x, x), but Spack simplifies it to Version(x) in some code paths, which behaves in other code paths as a concrete version and elsehwere as a range. Fixing that bug requires some work...

@sethrj
Copy link
Copy Markdown
Contributor

sethrj commented Oct 3, 2021

Yuck! OK, thanks for the clarification. My check of the changes was more than a spot check but less than reviewing every change. The E4S pipeline passes, and I'd say let's get this in here before more merge conflicts develop. And the :N.99 is of course something we'll have to watch out for in future code reviews :)

@sethrj sethrj merged commit b9e7255 into spack:develop Oct 3, 2021
@sethrj
Copy link
Copy Markdown
Contributor

sethrj commented Oct 3, 2021

Ahh right, the x:x problem is already in an open PR of yours 😄 #26208

@haampie haampie deleted the fix/no-more-99 branch October 3, 2021 13:36
@tldahlgren tldahlgren mentioned this pull request Oct 7, 2021
Closed
2 tasks
@cosmicexplorer cosmicexplorer mentioned this pull request Oct 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sethrj sethrj sethrj approved these changes

Assignees

@sethrj sethrj

Labels

versions

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@haampie @sethrj