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.

Sign up for GitHub

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

Jump to bottom

use MACOSX_DEPLOYMENT_TARGET when set #708

Closed
wants to merge 1 commit into from
Closed

use MACOSX_DEPLOYMENT_TARGET when set #708

wants to merge 1 commit into from

Conversation

youknowone
Copy link
Contributor

@youknowone youknowone commented Jul 30, 2022 •
edited
Loading

@youknowone youknowone mentioned this pull request Jul 30, 2022
Merged
@youknowone
Copy link
Contributor Author

@Mark-Simulacrum Hi, you seem to maintained this repository recently. Could you review this PR?

thomcc
thomcc reviewed Sep 15, 2022
View reviewed changes
src/lib.rs Outdated Show resolved Hide resolved
thomcc
thomcc requested changes Oct 25, 2022
View reviewed changes
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes a lot of other changes. Can you remove those (perhaps put them in a different PR)?

I think it's reasonable to take advantage of MACOSX_DEPLOYMENT_TARGET, but I don't think we should default to setting this to something old. I'd actually probably prefer we don't set it if not provided.

Alternatively, we could mirror the logic used by rustc_target, but I'd probably lean on not passing this if it is not set.

Also sorry for the delay here, this crate should be better maintained going forward.

@thomcc
Copy link
Member

thomcc commented Oct 25, 2022

Oh, I see, the other changes are from #703, which I hadn't seen yet.

@thomcc thomcc added the O-apple Apple targets and toolchains label Oct 26, 2022
@youknowone youknowone force-pushed the macosx-deploy branch 2 times, most recently from 68d73ec to 99f4211 Compare October 26, 2022 12:01
@youknowone
Copy link
Contributor Author

I did it by respecting the policy for other platforms like IPHONEOS. if you want to make different policy for macos, I will change it. Please confirm you want it.

I think it's reasonable to take advantage of MACOSX_DEPLOYMENT_TARGET, but I don't think we should default to setting this to something old. I'd actually probably prefer we don't set it if not provided.

@youknowone
Copy link
Contributor Author

@thomcc could you review it again?

@colincornaby
Copy link

I'm still getting familiar with this project so apologies if I'm misreading something.

On line 2246, Bitcode is enabled if the platform is not Mac.

  • This means Bitcode is also enabled for other places where it should not be (by my reading.) iOS Simulator and Catalyst should also not have Bitcode.
  • Not sure what the stance should be on Bitcode going forward. Bitcode has been discontinued in Xcode 14.0 and many developers are stripping it out of their pipelines. However - developers that need to support Xcode 13.0 may continue to need it.
  • The Bitcode generated will also not be compatible with Xcode anyway in since Rust uses a different version of Bitcode. So enabling this flag may actually break iOS builds.

BlackHoleFox
BlackHoleFox reviewed May 8, 2023
View reviewed changes
src/lib.rs
Comment on lines +2193 to +2479
std::env::var("MACOSX_DEPLOYMENT_TARGET").unwrap_or_else(|_| {
(if arch_str == "aarch64" {
"11.0"
} else {
"10.7"
})
.into()
}),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The rustc PR for --print deployment-target just landed (rust-lang/rust#105354), and should be available starting in 1.71 fyi. Ideally that would be queried for the version and then falling back to this hardcoded logic if the compiler is too old.

@francesca64 francesca64 mentioned this pull request Jun 14, 2023
Closed
@francesca64
Copy link
Contributor

@colincornaby this PR is minimally invasive and doesn't actually change the existing behavior around bitcode; bitcode is already being unconditionally generated for all non-macOS Apple platforms on main:

cc-rs/src/lib.rs

Line 2340 in 57853c4

cmd.args.push("-fembed-bitcode".into());

Deciding how cc should handle bitcode going forward should be left to another issue/PR, since landing this PR would be hugely beneficial to anyone developing macOS apps written in Rust.

@thomcc

I think it's reasonable to take advantage of MACOSX_DEPLOYMENT_TARGET, but I don't think we should default to setting this to something old. I'd actually probably prefer we don't set it if not provided.

clang defaults to targeting the same version of macOS currently being run, which is in contrast to Rust defaulting to a specific old version for maximum support. As it stands, it's impossible for someone developing a macOS app with any dependencies that use cc to target macOS versions earlier than what their development machine is running. This is nearly a non-starter for anyone who wants to make binary distributions of macOS apps, as the only workaround is to fork every such dependency to use this PR.

cc should follow the same conventions Rust itself does, since the current behavior is very surprising and leads to countless difficult-to-trace linker warnings about object files targeting too new of a version.

As mentioned, this PR is minimally invasive and merely extends the existing behavior around Apple platforms to account for macOS as well. I've tested this PR, and it solves all of my build problems flawlessly.

francesca64
francesca64 reviewed Jun 15, 2023
View reviewed changes
src/lib.rs
Comment on lines +2114 to +2396
let is_mac = match os {
Os::MacOs => true,
_ => false,
};
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let is_mac = match os {
Os::MacOs => true,
_ => false,
};
let is_mac = matches!(os, Os::MacOs);

matches! was added in 1.42, which is below the MSRV.

@thomcc
Copy link
Member

thomcc commented Jun 15, 2023

Yeah, I'm fine with this PR now. My complaints about the too old version were from an earlier version. I'd be fine with a rebased version of this.

@youknowone youknowone force-pushed the macosx-deploy branch 2 times, most recently from 416eb10 to 4ca9210 Compare June 16, 2023 10:53
@youknowone
Copy link
Contributor Author

youknowone commented Jun 16, 2023 •
edited
Loading

I wish it can be merged at this time. This is now too old to remember what I thought at the moment. I'm afraid I cannot properly rebase it one day.

@BlackHoleFox BlackHoleFox mentioned this pull request Aug 4, 2023
Merged
@BlackHoleFox
Copy link
Contributor

I opened up #848 to further this PR's changes like said on a previous comment, so it also contains a rebased version of the commit here. Depending on which merges first, the rebase this branch needs (a one line print conflict) might be unnecessary.

@youknowone
Copy link
Contributor Author

@BlackHoleFox Thank you!

@youknowone
Copy link
Contributor Author

I rebased this PR just in case if it need to be merged first.
But I totally agree to #848 and also prefer to merge this patch through it.

@BlackHoleFox
Copy link
Contributor

It looks like my forward PR merged with these changes, so this one could be closed as done @youknowone

@thomcc
Copy link
Member

thomcc commented Sep 15, 2023

Yeah this is done. Sorry for all the delays here.

@thomcc thomcc closed this Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@francesca64 francesca64 francesca64 left review comments

@BlackHoleFox BlackHoleFox BlackHoleFox left review comments

@thomcc thomcc Awaiting requested review from thomcc

Assignees
No one assigned
Labels
O-apple Apple targets and toolchains
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@youknowone @thomcc @colincornaby @francesca64 @BlackHoleFox