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

Hint against =-separated match specs #2232

Merged
merged 6 commits into from
Jan 27, 2025

Conversation

jaimergp
Copy link
Member

@jaimergp jaimergp commented Jan 26, 2025

Checklist

  • Added a news entry
  • Regenerated schema JSON if schema altered (python conda_smithy/schema.py)

@h-vetinari raised this issue in conda-build (conda/conda-build#5571), which led us to realizing that conda-build assumes all specs in a meta.yaml are space separated. However, this is not checked and =-separated specs are happily accepted. The problem is that they bypass some checks like this one about incompatible hashes because they are not a "three field matchspec".

I don't think we can forbid this and error out in conda-build, but at least we should hint against it to inform folks about this possible footgun. The repodata.json entries should only contain three-field matchspecs anyway (for backwards compat).

@jaimergp jaimergp marked this pull request as ready for review January 26, 2025 20:06
@jaimergp jaimergp requested a review from a team as a code owner January 26, 2025 20:06
Copy link
Member

@h-vetinari h-vetinari left a comment

Choose a reason for hiding this comment

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

I'm extremely grateful that you debugged this! At the same time I'm against hinting on this and would prefer fixing the footgun in conda-build.

But perhaps I'm fighting a losing battle here - rattler-build has all but forbidden = separators, but I feel very strongly that in terms of legibility

package <version> <build>

is much, much worse than

package =<version>=<build>

To my mind, the difference between

tzdata 2025*   # version constraint only
tzdata 2025 *  # version and build constraint

is unacceptably small for how different the two conditions are semantically. Similarly for

blas 2* mkl  # works 
blad 2 *mkl  # fails 

Moreover, one cannot use the space-delimited variant for installing something into an environment on the CLI (at least without quoting, which isn't obvious to add).

OTOH, I admit that I didn't appreciate that there were any differences between using space and = as delimiters, which I find distressing and would like to get rid of completely. I guess this would be something for conda/ceps#82...

Copy link
Member

@beckermr beckermr left a comment

Choose a reason for hiding this comment

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

Do we want to handle cases where people put items in brackets too? IDK what conda-build does with that.

@beckermr
Copy link
Member

@h-vetinari, a conda-smithy PR is not a place to debate how conda syntax should be written. Please do that as a CEP.

Since we cannot forbid this behavior from conda-build without declaring the correct behavior as a CEP, we are left with only a few options:

  1. Pass a CEP as a community and then put the change in conda-build using a full deprecation cycle.
  2. hinting to help people through the confusion
  3. doing nothing

I guess from your request for changes, you want item 1?

FWIW, I think 2 is fine for now and having a smithy hint to help people won't prevent us from doing standards work as a community later.

@h-vetinari
Copy link
Member

h-vetinari commented Jan 26, 2025

a conda-smithy PR is not a place to debate how conda syntax should be written.

We're talking about hinting on already-legal syntax though? My preference would be to fix the bugs (possibly with deprecation cycles where edge cases change behaviour), rather than forbid a syntax that conda-build supports.

As I mentioned I recognise that I'll likely be overruled on this, but while I will adapt to your point 2 if it happens, I still think it's the wrong direction, not least because the bug in conda/conda-build#5571 needed two edge cases to trigger (jinja confusion leading to mismatched outputs internally, as well as the divergent handling between and =). I don't see it as a pressing enough issue to hint against the syntax (which is in widespread use across conda-forge since many years), but I'm of course in favour of working through these topics through the CEP process.

@jaimergp
Copy link
Member Author

jaimergp commented Jan 27, 2025

My preference would be to fix the bugs (possibly with deprecation cycles where edge cases change behaviour), rather than forbid a syntax that conda-build supports.

The amount of .split() and .split(" ") results is concerning because the assumption is too widespread. There's also the problem that some of these are to pre-process unrendered specs, so they cannot be parsed as MatchSpec objects at that point. Space-splitting is easy but we can't reliably do =-splitting because of its meaning in version constraints (e.g. how to split python=3.9=*cpython* vs python>=3.9,<=3.11=*cpython*).

My impression is that it would take too much effort to fix, assuming that's even possible. Instead, I suggest we stick to the subset of the syntax that is known to work. I don't like space separation for the same reasons you mention, but there are 200+ recipes potentially running into the footguns we have identified.

@h-vetinari h-vetinari dismissed their stale review January 27, 2025 09:55

Sigh, I wish your argument were less compelling. If it's really that broken though, it should just be phased out within conda itself. We shouldn't leave these broken windows unfixed.

@beckermr beckermr merged commit 058670d into conda-forge:main Jan 27, 2025
2 checks passed
@h-vetinari
Copy link
Member

In a twist of heavy irony, the blas feedstock breaks when removing equal signs, see conda-forge/blas-feedstock#132

@beckermr
Copy link
Member

What breaks?

@h-vetinari
Copy link
Member

Look at the CI. main branch with = is fine, removing it leads to

Mismatching package: libblas (id 27_h706a439_openblas); dep: libblas 3.9.0 27*_openblas; consumer package: libcblas
Mismatching package: libblas (id 27_h706a439_openblas); dep: libblas 3.9.0 27*_openblas; consumer package: libcblas
Mismatching package: blas-devel (id 27_h6ab8cb0_openblas); dep: blas-devel 3.9.0 27*_openblas; consumer package: blas
Mismatching package: blas-devel (id 27_h6ab8cb0_openblas); dep: blas-devel 3.9.0 27*_openblas; consumer package: blas

@jaimergp
Copy link
Member Author

We'll need conda/conda-build#5600, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants