-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 MACOSX_DEPLOYMENT_TARGET hint for macos platform wheels #11783
base: main
Are you sure you want to change the base?
Conversation
@@ -1657,16 +1657,29 @@ impl std::fmt::Display for PubGrubHint { | |||
tags, | |||
} => { | |||
let s = if tags.len() == 1 { "" } else { "s" }; | |||
let macos_env_hint = tags |
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.
Presumably this will show the hint even if you're not targeting macOS? Is that what we want? e.g., in the changed test invalid_platform
the target is linux
. Showing the hint there seems confusing, right?
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.
Great point, confusing indeed! I have been poking around this afternoon to find a way to filter on platform and passing in ResolverEnvironment
looks promising.
Is this also what you had in mind?
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.
That sounds reasonable — looks like we use that in other hints too.
crates/uv/tests/it/pip_compile.rs
Outdated
@@ -14791,7 +14791,9 @@ fn invalid_platform() -> Result<()> { | |||
hint: You require CPython 3.10 (`cp310`), but we only found wheels for `open3d` (v0.15.2) with the following Python ABI tags: `cp36m`, `cp37m`, `cp38`, `cp39` | |||
|
|||
hint: Wheels are available for `open3d` (v0.18.0) on the following platforms: `manylinux_2_27_aarch64`, `manylinux_2_27_x86_64`, `macosx_11_0_x86_64`, `macosx_13_0_arm64`, `win_amd64` | |||
"###); | |||
|
|||
hint: Use `MACOSX_DEPLOYMENT_TARGET` to set the deployment target on macos platform |
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 we'll also need to explain the current deployment target. e.g., "The current macOS deployment target is <>, set ...
to change the deployment target to a different version"
Do we need to talk about how the deployment target is a minimum or a maximum? Can we use one of the target versions from the available wheels to show an example?
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 is good advice thank you. I have found using a TargetTriple
instance is a good way to grab the current deployment target.
I will add that, talk about minimum and maximum, and utilize one of the PlatformTags
to provide an example.
Cool! Thanks for contributing. I have a few questions. |
- Make macOS deployment target its own hint. - Add support for PubGrubHints. - Add test for this specific scenario.
Alright, in addition to the requested changes I made a few modifications. Most notably:
Hint now looks like this:
|
debug!("Found macOS deployment target: {}.{}", major, minor); | ||
Some((major, minor)) | ||
}) | ||
.unwrap_or((12, 0)) |
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.
Why did you decide to change this call signature instead of unwrapping to something like DEFAULT_MACOS_DEPLOYMENT_TARGET
at callsites?
This might be better, just curious if there was anything in particular that motivated it.
cc @charliermarsh as the owner of this
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.
(Noticed this was related to #11783 (comment))
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 liked centralizing the logic and moving the default version unwrap out of the platform constructor. It had the added benefit that it assisted with #11783 (comment) as you pointed out 😄.
Your suggestion is clean and would work the same. I would just change to DEFAULT_MACOS_DEPLOYMENT_TARGET
in platform()
unwrap.
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 would probably keep this as-is and unwrap at each site. I think each site should be "forced" to consider the value it uses here.
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.
Ack, I will revert these changes in favor of the call site unwrap. Thank you!
@@ -1669,6 +1696,37 @@ impl std::fmt::Display for PubGrubHint { | |||
.join(", "), | |||
) | |||
} | |||
Self::PlatformMacosDeploymentTarget { tag } => { | |||
let current_version = { | |||
let (major, minor) = macos_deployment_target(); |
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.
Here you pull the current target from the environment which I presume motivates the change in https://github.com/astral-sh/uv/pull/11783/files#r1982335893 but shouldn't we be pulling this value from the failing resolution's platform target triple instead?
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'm not entirely sure this matters in practice, because the only way the deployment target can be set is via environment variable right now? But I think it could be an easy cause of a future bug)
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.
As far as I know the environment variable is the only way to control the deployment target, correct.
If you don’t mind, I may need some advice on the best path forward. The major / minor fields in the platform object are not included in the resolution marker env object and the originally constructed target triple is only used to create the marker.
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 see — yeah I wasn't sure if it was necessarily easy to get at. I can take a look. Charlie may also have an idea.
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 what you want to do is... Iterate over self.tags
, and find the minimum macOS major-minor on any platform tag. Then use that as the inferred deployment target.
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 Charlie! I was looking into iterating over self.tags
with one caveat: Should we actually find the max to infer the macosx deployment target?
It looks like we use the major
field of Os as the top end to generate the list of platform_tag versions for anything >= 11. (ex: https://github.com/astral-sh/uv/blob/main/crates/uv-platform-tags/src/tags.rs#L497)
This feels counterintuitive to me because MACOSX_DEPLOYMENT_TARGET specifies the minimum to be supported, but for the purposes of the hint it would make the most sense as feedback to the user.
Btw thanks to you and Zanie for the feedback, I feel like I am making this complicated but I want to be sure I understand what I am changing.
Thank you for following up on my feedback! |
hint: Wheels are available for `mlx` (v0.7.0) on the following platforms: `macosx_13_0_arm64`, `macosx_14_0_arm64` | ||
|
||
hint: The current minimum macOS deployment target is 12.5, set environment variable MACOSX_DEPLOYMENT_TARGET to change the minimum deployment target (e.g. MACOSX_DEPLOYMENT_TARGET=13.0) |
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 still find it a bit surprising these are separate hints, as the latter is only relevant if the first is shown, correct? Can you say a bit more about why you found separate hints more ergonomic?
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.
Yep you are correct, the macOS deployment target hint is dependent on the platform hint. I did mess around with breaking out the logic of the macOS deployment target into a separate function instead of a new PubgrubHint variant, but it didn't feel as clean in the codebase as following the variant pattern in the fmt function. It also feels nice to compose the hints at the tag_hints
level.
Happy to switch it up if necessary!
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 don't know that this hint actually works today... Has anyone tried it? I think we only look at MACOSX_DEPLOYMENT_TARGET
when --python-platform
is specified. I wouldn't expect it to have any affect on uv lock
, for example. We might need to change our tag inference logic to respect it first. (I could be wrong.)
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 haven't tried it, no. Interesting.
@dcarrier don't worry about looking into that (unless you're interested), we can explore it separately from this change.
Summary
Addresses: #10699
Adds a hint that the user may utilize
MACOSX_DEPLOYMENT_TARGET
to change the deployment target for macos platform wheels.Test Plan