-
-
Notifications
You must be signed in to change notification settings - Fork 14
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
Update version/build constraints for rattler-build compat #85
Conversation
Update the version strings not to use redundant `=`, and to match build strings as a separate field rather than appended to the version via `=`. This is necessary for feedstocks built using rattler-build to be able to depend on libabseil. Fixes conda-forge#84
…onda-forge-pinning 2024.12.13.14.31.10
Hi! This is the friendly automated conda-forge-linting service. I just wanted to let you know that I linted all conda-recipes in your PR ( I do have some suggestions for making it better though... For recipe/meta.yaml:
This message was generated by GitHub Actions workflow run https://github.com/conda-forge/conda-forge-webservices/actions/runs/12319662098. Examine the logs at this URL for more detail. |
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'm sorry, I don't want to block your work, but I disagree fundamentally with this. Rattler needs to be able to handle =
signs, not only because they're valid specs, but more importantly, because we really should not encourage syntax that depends on significant whitespace (e.g. libabseil *cxx{{ cxx_standard }}*
vs. libabseil * cxx{{ cxx_standard }}*
).
The build string syntax is horrible (c.f. conda/conda#11053), and I do not want to make it even more horrible by relying on significant whitespace.
Filed as prefix-dev/rattler-build#1265. |
Bas has a longer explanatory comment here: https://conda-forge.zulipchat.com/#narrow/channel/457337-general/topic/rattler-build.20.22strict.22.20matchspec.20parsing/near/488975665 |
From @baszalmstra's comment:
I fail to see any ambiguity here. Having an equal sign after the version means whatever comes after is qualifying the build string. The ambiguity is much worse IMO between
IOW, the equals-variant is much less ambiguous visually. |
A fix is coming: prefix-dev/rattler-build#1271 - will be released this week. |
We fixed the issue in 0.33.0 with the constraints coming from upstream, ie. this fix here is not needed anymore. The parsing of the input is now much stricter though and doesn't allow for
If you want something visually appealing, you can use |
This feels like a big hammer for very little gain, i.e. it'll break lots of recipes, and there's basically no packages with variable version depth (e.g. 1.2 and 1.2.1) where the distinction would make sense, much less that someone would then want to exactly pin to 1.2 rather than 1.2.*. Yes, it's more correct and more expressive. But in terms of our breakage budget, the benefit doesn't seem justified. Hopefully you have good error messages.
Can you explain to me like I'm 5 what aspect is confusing with this syntax? |
It uses an equal sign which usually means complete equality. |
Regarding breakage - very few packages have this syntax (foo 1.2). Most already use a range. The error message is very clear. |
Which it does, unless you use wildcards, which are a very explicit choice about which parts do not need to match. Are you saying that |
Your example was The first part is And then why is it So yes, we now require a real operator that is not confusing. If you can come up with a different, better looking separator character than whitespace or equal, we can discuss it! :) |
Could be Or |
I never said I liked it, but it's still better than
But it is used for equality (of the build string). Yes, I would like to get away from these kind of build-string shenanigans completely1, but On top of that, even if you personally dislike it, it's widely deployed, which in itself puts a massive burden of proof on any changes to this. If - as you seem to suggest - we're going to start requiring new syntax to be supported by all installers, then let's write a CEP (for something between conda/conda#11053 and your Footnotes
|
It's only not allowed in recipes as input specs (e..g in build/host/run). A normal conda-build spec with whitespaces is what is emitted at the end into the repodata.json / index.json. |
The |
I get that, but so far you haven't managed to make a convincing argument why this should be the case. You say "it's not really equality", but it uses the same mechanism as version comparison with wildcards does:
The fact that it's used for the backend syntax doesn't mean we have to pessimize the user-facing one. IMO
Yes, and it has all the same buildstring-matching & -overload problems, just within |
But =1.2 means 1.2.* :) |
I'm on board with cleaning up the version-range issues (which are almost fully orthogonal here). My point is:
should be equivalent to
If you remove the version-range confusion, then the build strings are not confusing at all. |
If that means anything, bash also uses |
Closing since rattler-build has been fixed. |
Checklist
0
(if the version changed)conda-smithy
(Use the phrase@conda-forge-admin, please rerender
in a comment in this PR for automated rerendering)Update the version strings not to use redundant
=
, and to match build strings as a separate field rather than appended to the version via=
. This is necessary for feedstocks built using rattler-build to be able to depend on libabseil.Fixes #84