-
Notifications
You must be signed in to change notification settings - Fork 150
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
feat: adding tags CLI interface #422
Conversation
b204460
to
226635e
Compare
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #422 +/- ##
==========================================
+ Coverage 68.67% 72.73% +4.06%
==========================================
Files 12 13 +1
Lines 964 1060 +96
==========================================
+ Hits 662 771 +109
+ Misses 302 289 -13
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report in Codecov by Sentry. |
5fdc937
to
deb2a78
Compare
@mayeut could you take a look? Maybe we could even try it out in cmake/ninja/clang-format or some other package? |
Try using pypa/wheel#422
It seemed ok at first glance so I tried it on the ninja workflow. Permission bits are lost in the unpack/repack process: https://github.com/scikit-build/ninja-python-distributions/pull/84/checks?check_run_id=3847629425 |
Isn't that a bug that I should fix for unpack / repack? I'll try adding permissions checking to the tests early next week. (edit: delayed, will get to this in about a week (mid November), don't mean to lick the cookie, so if someone else has time to work on this sooner, let me know!) Edit: Probably after my scikit-build proposal is finished, Dec 8 or so. |
Try using pypa/wheel#422
deb2a78
to
c96e0fc
Compare
(Including #431 for now, to keep the linters happy) |
(I'm working on a major rewrite of the patch but like having the diff here, so I might rebase or retrigger later) |
0310ba6
to
de5b0e9
Compare
This is now passing (except for #433) in the ninja workflow. It now uses a custom zipfile copy that keeps the permission bits, and doesn't have to work around temp directories, pack and unpack output hiding, etc. Should be ready for an initial review. I'll add docs after that. |
521c8a8
to
44e5e7d
Compare
This code will have to be adjusted once I merge the patch that makes wheel use |
That's fine, it still needs docs and a few things. Does this generally look like a good direction? My thought is is still needs to read the line ending from the existing file rather than hard coding it. Also, there are several asserts that are untested, mostly just watching for malformed name vs. tags. I was thinking testing them would require generating new malformed test wheels, but actually just renaming the current one without changing the tags would do just fine, I think. Not having easy testing was one reason I avoided supporting misnamed wheels, but honestly, using this to "fix" a renamed file might be really useful - we could always go from the internal tags, and compute the final filename from that - so using it without "changes" would "fix" a badly renamed file (one that doesn't match the internal tags). |
2860149
to
c1fbf45
Compare
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've had a play with this locally and it seems to work great! It would be great to get this released as a 'correct' way for users to manipulate tags, which we see quite often in the community.
Yeah, this is probably ripe for merging. I'm juggling multiple projects and am currently on vacation, but I'll handle this PR when I get back. |
36c7778
to
c9c1639
Compare
usage: wheel tags [-h] [--remove] [--python-tag TAG] [--abi-tag TAG] [--platform-tag TAG] [--build NUMBER] [wheel ...] Make a new wheel with given tags. Any tags unspecified will remain the same. Separate multiple tags with a dot. Starting with a dot will append to the existing tags. The original file will remain unless --remove is given. The output file(s) will be displayed on stdout. positional arguments: wheel Existing wheel(s) to retag options: -h, --help Show this help message and exit --remove Remove the original files, keeping only the renamed ones --python-tag TAG Specify an interpreter tag(s) --abi-tag TAG Specify an ABI tag(s) --platform-tag TAG Specify a platform tag(s) --build NUMBER Specify a build number
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
Co-authored-by: Joe Rickerby <joerick@mac.com>
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
c9c1639
to
5b9dd22
Compare
Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com>
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.
Looks good enough.
Thanks! |
0.40 is out with this feature (just in time for π day! ;) ). Docs at https://wheel.readthedocs.io/en/stable/reference/wheel_tags.html. |
Try using pypa/wheel#422
* Try wheel cli Try using pypa/wheel#422 * Update pyproject.toml * Update requirements-repair.txt * chore: use the recently released wheel 0.40 Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> --------- Signed-off-by: Henry Schreiner <henryschreineriii@gmail.com> Co-authored-by: Henry Schreiner <HenrySchreinerIII@gmail.com>
A first attempt at #407. Usage:
Implementation notes:
Reused unpack and pack. Currently just redirecting the output to stderr, but would probably be cleaner to hide it completely. The output from this command (to stdout) is exactly the renamed wheels, for easy piping.Tag:
in the metadata, and assertions are thrown if there's a mismatch. Could change.Only "unit" testing the function, rather than the argparse wrapper, in line with the other tests.(Added more tests)