-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Handle compatible libc-based manylinux/musllinux platform tags #10894
base: main
Are you sure you want to change the base?
Conversation
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.
Thanks for filing this! I have two major blocking concerns with this PR:
- The news fragment isn't sufficiently clear about the fact that this only affects cases when certain flags are passed. Further, it'd be nice to use :pep: inline role to reference PEPs or, even better, use a textual description of what the PEP represents in the news fragment.
- The implementation uses a private function from packaging.tags, which I don't like the idea of doing.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@q0w I appreciate the work you're putting toward this but could you clarify what implementation approach you're taking here? It looks like you've basically copied over code from packaging.tags after I mentioned that it's a no-go to use packaging.tag's internals -- both are equally bad and I'm not sure how you're expecting this PR to behave. |
This comment was marked as resolved.
This comment was marked as resolved.
8088326
to
106692f
Compare
This comment was marked as resolved.
This comment was marked as resolved.
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.
Logic looks right to me, although the news fragment definitely needs to better describe what users should expect from this change. Also the implementation can be slightly improved, I think.
An alternative to this is to go the route of requiring users to specify the entire environment: #10050 (comment) |
Is there anything blocking this from being merged? |
Personally I think this needs some better text explaining the scope of the feature, likely somewhere in documentation. |
I am not good at writing docs, so if someone wants to collaborate, ping me |
Closes #10760