Skip to content

Conversation

@DEKHTIARJonathan
Copy link
Member

This PR adds a command variantlib make_variant to manually add variant information "post build" to a wheel.

You can see it as "repackaging a wheel as a variant after a build".

This will be very useful to allow everybody to quickly transform any wheel into a variant without having to modify absolutely anything else.

variantlib make_variant \
    -f pep_xxx_wheel_variants-1.0.0-py3-none-any.whl \
    -o . 
    -p fictional_tech::quantum::drive \
    -p fictional_hw::platform::TARS

After discussing with @warsawnv we thought that this was a critical feature to support initially (as build backends gradually rollout support) and for testing.

This PR also include a "optional extra" called user - Just because this command adds a feature on wheel which will not be useful in most cases (like for pip and I'm not sure how if pip and other would like very much having to bring wheel), we can change that part later if we think it's not a big deal to add wheel as a core dependency.

This PR uses #33

@DEKHTIARJonathan DEKHTIARJonathan force-pushed the commands branch 2 times, most recently from 02f7a23 to 40fe15e Compare April 15, 2025 17:47

with tempfile.TemporaryDirectory() as _tmpdir:
tempdir = pathlib.Path(_tmpdir)
wheel_unpack(input_filepath, tempdir)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think we really need to unpack it. Unfortunately, it looks like zipfile lacks proper API to update files in place or copy compressed members (python/cpython#125718), but at least we could avoid writing files to disk and potentially altering their metadata in the process.

I.e. basically open both archives, iterate over members: if not metadata, copy between archives as-is; if metadata, read, alter and write. It will probably even be simpler than the current logic, which seems to make unnecessary changes to the archive (probably because it was adapted from the command altering tags, which we don't do here).

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't disagree though it's a little complicated for a first iteration - feel free to experiment and optimize ;)

DEKHTIARJonathan and others added 5 commits April 15, 2025 15:13
Co-authored-by: Michał Górny <mgorny@gentoo.org>
Co-authored-by: Michał Górny <mgorny@gentoo.org>
Co-authored-by: Michał Górny <mgorny@gentoo.org>
Co-authored-by: Michał Górny <mgorny@gentoo.org>
@DEKHTIARJonathan
Copy link
Member Author

Good enough for a V1 of the command - merging so that a few people can start playing with it.
@mgorny feel free to adjust & optimize the command further :)

@DEKHTIARJonathan DEKHTIARJonathan merged commit 4d0c025 into main Apr 15, 2025
16 checks passed
@DEKHTIARJonathan DEKHTIARJonathan deleted the commands branch April 15, 2025 19:34
Copy link

@warsawnv warsawnv left a comment

Choose a reason for hiding this comment

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

Thanks for writing this! I think this may end up being a very common way to create variant wheels.

Comments given; I hope I'm not being too nitpicky!


# Input Validation
variant_hash_pattern = rf"^[a-fA-F0-9]{{{VARIANT_HASH_LEN}}}$"
if not re.match(variant_hash_pattern, variant_hash):

Choose a reason for hiding this comment

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

If wheel_variant_pack() is an internal API, then you control the variant_hash and this could probably be an assertion.

raise whl_pck.WheelError(
f"Multiple .dist-info directories found in {directory}"
)
if not dist_info_dirs:

Choose a reason for hiding this comment

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

Since you're checking the length of a list, and you're explicitly calling len() above, it's probably best to also do so here, e.g. if len(dist_info_dirs) == 0:

tags: list[str] = info.get_all("Tag", [])
existing_build_number = info.get("Build")

if not tags:

Choose a reason for hiding this comment

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

Another place where it's generally better to use if len(tags) == 0:

I generally prefer to be as explicit about the truthiness test as possible. E.g. if you know you're testing the length of a sequence, use len. If you are testing whether something is None or not, use if foo is None. Only when the truthiness or falseiness can be any number of things (e.g. None, the empty string, or a zero length sequence) do I typically use a generic if foo or if not foo. YMMV!

if not tags:
raise whl_pck.WheelError(
f"No tags present in {dist_info_dir}/WHEEL; cannot determine target "
f"wheel filename"

Choose a reason for hiding this comment

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

I'm kind of surprised you didn't get a linter warning here, since the line 78 string doesn't need to be an f-string.

del info["Build"]
if build_number:
info["Build"] = build_number
name_version += "-" + build_number

Choose a reason for hiding this comment

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

This one could be an f-string: name_version += f"-{build_number}"

]

[project.optional-dependencies]
user = [

Choose a reason for hiding this comment

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

What do you think about using cli (or commands) for the optional instead?

Minimal changes tried to be applied to make it work with the Variant Hash.
:param directory: The unpacked wheel directory
:param dest_dir: Destination directory (defaults to the current directory)

Choose a reason for hiding this comment

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

Does it? Or is it required?

if not (metadata_f := distinfo_dir / "METADATA").exists():
raise FileNotFoundError(metadata_f)

with metadata_f.open(mode="r+") as file:

Choose a reason for hiding this comment

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

Do you think it would be better to write a new file and move it into place atomically? E.g. copy lines from METADATA into METADATA.new, then os.rename() the latter to the former. The question I have is, what if this loop errors out or is interrupted in the middle of it? You can probably also avoid the seek() back to the beginning and the truncate().

You probably still need a try/except that deletes the .new file and cleans up after itself, reraising the original exception, but at least then you can still recover back to the original state if something unexpected happens.

METADATA_VARIANT_HASH_HEADER = "Variant-hash"
METADATA_VARIANT_PROPERTY_HEADER = "Variant"

WHEEL_NAME_VALIDATION_REGEX = re.compile(

Choose a reason for hiding this comment

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

A multi-line regex string with comments would help with readability here.

Copy link
Collaborator

Choose a reason for hiding this comment

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

In general, I agree here but the regex is copied (and slightly modified) from pip (and we already have it repeated in too many places).

@DEKHTIARJonathan
Copy link
Member Author

Thanks a lot @warsawnv for your comments. Many of the comments you made are actually directly targeting the original implementation from wheel.cli.pack. I'm gonna check tomorrow what is the original code (which I don't want to modify) and which one my code (and I can improve).

I believe in the future some of that code may go inside wheel itself. Let's see.

In any case thanks for the comments, I'll check what I can improve and I'll open a new PR with these changes.

DEKHTIARJonathan added a commit that referenced this pull request May 2, 2025
* [Command] `generate-index-json` updated. According to #35

* Review Fixes
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