Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
retry: bugfix: package requirements with git commits (#35057) #36347
retry: bugfix: package requirements with git commits (#35057) #36347
Changes from 4 commits
acb4e87
bfb5a4f
c92938f
5370b83
fc46812
63e24c1
a6da605
ec5aed6
d5102d8
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tgamblin @scheibelp Sorry if I came late to reviewing this, and previously stopped at reading the title. My understanding was that this PR allowed unknown git versions to be used in package requirements, which is 👍 .
Reading the implementation (while resolving conflicts in other PRs) it seems to me that this PR makes any non-existing version in package requirements an allowed one. I think this is a bug. For example:
With the previous behavior on develop we would get:
which, despite the error message that can be improved, is what I expect by a requirement. This PR made it such that:
which in my opinion is wrong, since no version 2.0 exists for
zlib
. Was that on purpose? Shouldn't we restrict this logic to only git versions?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the user says it must be
zlib@2.0
then I think that's what it should be: if you sayspack spec zlib@2.0
on the command line then that's what we get (and Spack tries to find 2.0 from zlib's download link).If nothing else, right now if a user says
require: foo@hash=1.5
and1.5
doesn't exist in the package.py, then we want Spack to still allow it. I think it's consistent to extend that to all versions mandated in the config.Printing a warning message might be in order if the package can't be fetched, but otherwise I would say this is the desired behavior. If you want to prevent declaration of number versions that don't exist (e.g. when not associated with a git hash), then that would work for the use cases I created it for, but would be inconsistent (for the reasons outlined above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's inconsistent somehow. If the user says
+shared
we don't allow ashared
variant where it doesn't exist. I think the only case where popping a non-git version into existence make sense are externals, since it's software already installed - and somebody put the information on its version inpackages.yaml
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a git version, so I'm not questioning that. It's really "regular" versions that shouldn't be added in my opinion to the
alloweddeclared ones if they appear in requirements.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I do
doto make sure, are you also arguing for not doing that anymore?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When we define an environment, we also validate that the concretization output matches all of the versions we specify. So that should, in theory, catch the fact that the defined default in packages doesn't match what we get.
A warning would be nice though, that way we see the discrepancy faster.
Also, having packages define default versions that are undefined in the package causes concretization errors. The version doesn't end up in the concretization facts, version checks in
when
clauses all fail for that version.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@alalazo
To be clear, that was the behavior in Spack prior to this PR: putting versions in the preferences defines them as possible versions (i.e. if
2.0
is a version preference,spack spec zlib
will give youzlib@2.0
).Not to say you can't undo thatYou can undo that, but it will take (slightly more) than reverting parts of this PR to get what you want.Therefore
the "previous behavior" doesn't seem to be a valid given your own stated desires. I suggest outlining the things you want to be true (e.g. as done in #36347 (comment)) and then we can make them true (ignoring however they were behaving in the past).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, sorry if the reply was confusing but:
Also, see this issue we got today #36574 which is related to the discussion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is related but FWIW if you check out 9a93f22 (the commit before this was merged) you can still reproduce #36574. In some sense, my goal will be to extend this to do the opposite of what was decided there (for
hash=number
versions at least).Preventing users from introducing new non-hash versions in preferences will not conflict with this PR or the users I know of that would benefit from it, so I only oppose it on account of it being inconsistent (I am willing to be overridden on that).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I said this because I assumed that package requirements would have the same problem, but they don't: #36589; therefore that issue is not related to this PR (or at least not caused by it).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very bug prone. If no version is imposed by the user, it's looked up and something is assigned. So you can have
v1 = abcdef=1.2.3
andv2 = abcdef
for whichv1.satisfies(v2)
butv2 < v1
(as it causes a lookup ofref_version
).I maintain that we should just remove this lookup nonsense, it's just introducing too many edge cases, it's too opaque, and generally useless.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd rather have a centralized conversation on this (if there is such a conversation, can you link to it here?).
In this particular branch of the
if
statement, neither version is supplied by the user. So we are comparing things likefoo@abcdef...
andfoo@abcdef...
, they are equal if the git IDs are equal as presented by the user.There is a bug if the lookups would render to the same git ID: (e.g. a commit and a tag) e.g.
foo@abcdef...
andfoo@git.tag1
(iftag1
resolves toabcdef...
), although that's separate from what you mention here.