-
Notifications
You must be signed in to change notification settings - Fork 562
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
Add --python-platform
to sync
and install
commands
#3154
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.
I was having deja vu here, because I thought I had reviewed a change like this already. I did some digging and:
- Add a
--platform
argument to enable resolving against a target platform #3111 added initial support. It added it as--platform
to onlyuv pip compile
. - Rename
--platform
to--python-platform
#3146 renamed the flag from--platform
to--python-platform
, sincepip
also has a--platform
flag and it doesn't match how ours behaves. - Add
--python-platform
to configuration #3147 adds--python-platform
to the configuration. - This PR adds
--python-platform
to theuv pip sync
anduv pip install
commands.
So with all that context, I think this change makes sense, but:
- Can we add a test covering this flag for the
uv pip sync
anduv pip install
commands? I think we have a test foruv pip compile
, but it would be good to cover the other two as well. - I mentioned this in a review comment, but what is the difference between pip's
--platform
and this--python-platform
flag? And does there need to be a difference? Can we match whatpip
does instead?
Fwiw we've been intentionally not covering flags across every command. No comment on whether or not that's best. |
(I'll answer your other question after goal setting, it's a good one to record in GitHub.) |
a7dd1da
to
f905247
Compare
Okay, so pip supports So our |
/// WARNING: When specified, uv will select wheels that are compatible with the target platform. | ||
/// The resulting environment may not be fully compatible with the current platform. Further, | ||
/// distributions that are built from source may ultimately be incompatible with the target | ||
/// platform. This option is intended for cross-compilation and other advanced use cases. |
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.
This last sentence is confusing given the preceding paragraph, perhaps you meant
/// platform. This option is intended for cross-compilation and other advanced use cases. | |
/// platform. This option is not intended for cross-compilation and other advanced use cases. |
?
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 think "cross-compilation" is the wrong term. It is intended for advanced use-cases, but "cross-compilation" emphasizes building from source in the wrong way.
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.
Ah, okay. Yeah, I was definitely also mislead by the preceding sentence that specifically talks about building from source.
Summary
pip supports providing a
--platform
topip install
, which can be used to seed an environment (e.g., for use in a container or otherwise). This PR adds--python-platform
to our commands to support a similar workflow. It has some caveats, which are documented on the CLI.Closes #2079.