-
Notifications
You must be signed in to change notification settings - Fork 442
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
Use rustc's Apple deployment target defaults when available #848
Conversation
// the ordering of env -> rustc -> old defaults is intentional for performance when using | ||
// an explicit target | ||
match os { | ||
AppleOs::MacOs => env::var("MACOSX_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.
Hm... should we emit rerun-if-changed flags for these env vars in any situation? (Perhaps it would be needed in complex situations with cross-compiling?)
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.
Originally I opted to avoid that (a prior revision of the branch actually ran it through the caching env paths) for consistency reasons. Only cc
would rerun, so an arbitrary number of objects not created by cc
in the build tree would have a different deployment target and that seems worse then doing nothing and making people run cargo clean
to switch.
I think that if this is desired behavior, rustc
or cargo
should handle it instead and invalidate the entire build tree at once.
Fix the now-unified default watchOS deployment target
430a770
to
ba90200
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.
This is a really good idea.
Prior to this change, the cc crate just didn't emit any |
To follow on rust-lang/rust#105354 being released in 1.71, and soon rust-lang/rust#104385, the changeset here aims to make Apple deployment targets more consistent and support future minimum bumps better. To do so, the root change is to try asking
rustc
for what it thinks the default deployment target version is before using a hardcoded one aftercc
checks the environment directly for an overridden version. For any users who aren't on 1.71+, everything will work exactly the same.While in the area, duplication of checking the iOS and watchOS default deployment target elsewhere in the library was removed and a bad watchOS version (2.0 -> 5.0) was corrected.
Also, this branch forks from #708 and has a rebased version of its one commit. If only GitHub supported stacked PRs... Due to that though, its better to review this per commit.