Skip to content

Support custom variant labels #108

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

Merged
merged 12 commits into from
Jun 30, 2025
Merged

Support custom variant labels #108

merged 12 commits into from
Jun 30, 2025

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented Jun 25, 2025

Add support for a more generic concept of "variant labels" in filenames, to replace narrower "variant hashes". The label is still the hash by default, but the wheel builder can override it to any alphanumeric string up to 8 characters and underscores, providing a more readable description of the specific variant.

Variant label is the last component of the variant wheel filename, in place of the variant hash. It is also used as the key in variants.json files.

  • VariantDistInfo now permits modifying variant_label property (formerly variant_hash) to change the label.
  • make-variant command accepts a new --variant-label= option.
  • get_variant_hashes_by_priority() is renamed to get_variants_by_priority(), and now returns variant labels rather than hashes.
  • make_variant_dist_info() accepts an optional variant_label that can be used to override the label.

Add initial support for using wheels with non-variant hash labels.
This mostly means replacing "variant hash" with "variant label"
in a bunch of places, updating the regex and removing validation where
we permit a different label.
@mgorny mgorny marked this pull request as ready for review June 26, 2025 13:51
mgorny added 2 commits June 26, 2025 15:52
Rename the `get_variant_hashes_by_priority()` function
to `get_variants_by_priority()`, and update them to return variant
labels rather than hashes.  For the time being, we just use a reverse
map from hashes to labels -- but it may be cleaner to start storing
labels in `VariantDescription`.
@mgorny mgorny changed the title Initial support for custom variant labels Support custom variant labels Jun 26, 2025
@DEKHTIARJonathan
Copy link
Member

DEKHTIARJonathan commented Jun 30, 2025

Looking at the resulting filenames - it's really weird:

tests/artifacts/test-package/dist/test_package-0-py3-none-any-5d8be4b9.whl
tests/artifacts/test-package/dist/test_package-0-py3-none-any-60567bd9.whl
tests/artifacts/test-package/dist/test_package-0-py3-none-any-fbe82642.whl
tests/artifacts/test-package/dist/test_package-0-py3-none-any-bar.whl
tests/artifacts/test-package/dist/test_package-0-py3-none-any-foo.whl

I wonder if @konstin doesn't make a really good point about "separators".

Maybe one of these (or something else) is "safer":

  • tests/artifacts/test-package/dist/test_package-0-py3-none-any#foo.whl
  • tests/artifacts/test-package/dist/test_package-0-py3-none-any-#foo.whl
  • tests/artifacts/test-package/dist/test_package-0-py3-none-any-!foo.whl
  • tests/artifacts/test-package/dist/test_package-0-py3-none-any+foo.whl

The current "filename" allows VERY weird stuff (it's not weird because pypy - it's weird because it looks like a platform tag)

  • tests/artifacts/test-package/dist/test_package-0-py3-none-any-pypy.whl
  • tests/artifacts/test-package/dist/test_package-0-py3-none-any-python2.whl
  • tests/artifacts/test-package/dist/test_package-0-py3-none-any-manylinux.whl
  • ...

Now that these strings "(can) have a meaning" I find it very disturbing that the separator is a normal -.
It becomes pretty hard to distinguish in 1/4sec variant or not variant.

The hash use to provide immediate visual feedback - immediately giving a visual cue "this is not a normal wheel".
I feel that the current "filename format" doesn't allow a strong enough visual difference to flag -manylinux is not platform tag, it's a variant name.

Sorry to not have made this comment earlier - it really just "stroke me now"

Copy link
Member

@DEKHTIARJonathan DEKHTIARJonathan left a comment

Choose a reason for hiding this comment

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

Aside of my comment on filename - I really like this PR. Well made I like the approach and really doesn't change much of the internal mechanic.

Good job - let's wrap up the discussion the filename and we can merge this

@mgorny
Copy link
Contributor Author

mgorny commented Jun 30, 2025

I don't think "people using weird names" is a valid argument. Mind you, the following is also a valid wheel:

foo-1.2.3-py3.win64-none.linux-any.py36.whl

Going out of our way to prevent people from doing confusing things is not a good design goal. Changing the separator means that variant wheels will be wrongly taken by older pip versions which goes against our initial goals.

@DEKHTIARJonathan
Copy link
Member

DEKHTIARJonathan commented Jun 30, 2025

Changing the separator means that variant wheels will be wrongly taken by older pip versions which goes against our initial goals.

Would it ? It wouldn't fail the regex ? Because that's how I implemented Variants a while back.
The first design used -#abcd1234

@mgorny
Copy link
Contributor Author

mgorny commented Jun 30, 2025

Changing the separator means that variant wheels will be wrongly taken by older pip versions which goes against our initial goals.

Would it ? It wouldn't fail the regex ? Because that's how I implemented Variants a while back. The first design used -#abcd1234

Yes, the regex is literally [^\s-]+?. The only reason the original solution worked is that it used the fact that the build field is required to start with an integer.

@mgorny
Copy link
Contributor Author

mgorny commented Jun 30, 2025

Also note that Poetry's regexp is even terser — it's [^-]+, so - is really the only separator that works. We could add some extra character after that, but it feels like making the filename longer for a corner case of a corner case.

In the end, we are supposed to provide tools and guidance. We are not supposed to force people to use these tools "our way".

@DEKHTIARJonathan
Copy link
Member

DEKHTIARJonathan commented Jun 30, 2025

Alright well once you're done removing null variant label you can merge ;)

Do you mind adding a test that verify that:

  • you can't make-variant a null variant with a label
  • generation of the variants.json fails if the null variant has a label
  • same when using the top level APIs => verify the input variants.json

"Trust but verify"

@mgorny
Copy link
Contributor Author

mgorny commented Jun 30, 2025

  • generation of the variants.json fails if the null variant has a label

That's going to be messy, since the tools aren't supposed to permit you to create such an artifact. I'd rather not add a backdoor to facilitate tests, or provide artifacts that can't be regenerated trivially.

@mgorny
Copy link
Contributor Author

mgorny commented Jun 30, 2025

Done for make-variant and variants.json parsing. The latter implies that all API should reject it now, as well as generate-index-json.

@mgorny mgorny requested a review from DEKHTIARJonathan June 30, 2025 13:23
@DEKHTIARJonathan DEKHTIARJonathan merged commit 3fcb8cc into dev Jun 30, 2025
44 checks passed
@DEKHTIARJonathan DEKHTIARJonathan deleted the custom-label branch June 30, 2025 14:49
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.

2 participants