Skip to content
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

Merged
merged 3 commits into from
Sep 14, 2023

Conversation

BlackHoleFox
Copy link
Contributor

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 after cc 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.

tests/test.rs Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
// 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")
Copy link
Member

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?)

Copy link
Contributor Author

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.

Copy link
Contributor

@youknowone youknowone left a 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.

@nox
Copy link

nox commented Nov 14, 2023

Prior to this change, the cc crate just didn't emit any -mmacosx-version-min flag on its own, right? That's a breaking change on its own, and I don't think #901 changes that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants