Fix `IPHONEOS_DEPLOYMENT_TARGET` on Mac Catalyst #121296

madsmtm · 2024-02-19T12:38:20Z

Some of the target code invalidly assumed that the deployment target variable on Mac Catalyst is MACOSX_DEPLOYMENT_TARGET, which is wrong, Mac Catalyst uses the same environment variable as iOS.

Additionally, the deployment target was hardcoded to 14.0, I've lowered this to 13.1 (same default as Clang), and made it properly load from the environment.

This shouldn't require any changes to the cc crate, as that uses rustc --print=deployment-target to get this information automatically.

CC @BlackHoleFox
r? @rust-lang/macos

Mac Catalyst uses IPHONEOS_DEPLOYMENT_TARGET to specify the deployment target, so it makes no sense to remove that variable.

Same default as Clang: https://github.com/llvm/llvm-project/blob/d022f32c73c57b59a9121eba909f5034e89c628e/clang/lib/Driver/ToolChains/Darwin.cpp#L2038

rustbot · 2024-02-19T12:38:26Z

Failed to set assignee to nvzqz: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

rustbot · 2024-02-19T12:38:28Z

These commits modify compiler targets.
(See the Target Tier Policy.)

madsmtm · 2024-02-19T12:42:29Z

r? shepmaster

(Since you're in @rust-lang/macos as stated here)

madsmtm · 2024-02-19T12:53:27Z

See #106925 and #111384 for previous implementation efforts here, including discussion on the choice of defaulting to 14.0; and indeed as said there, 13.0 fails with "invalid version number", but 13.1 works!

shepmaster · 2024-02-19T19:16:55Z

compiler/rustc_target/src/spec/base/apple/mod.rs

 pub fn ios_llvm_target(arch: Arch) -> String {
    // Modern iOS tooling extracts information about deployment target
    // from LC_BUILD_VERSION. This load command will only be emitted when
    // we build with a version specific `llvm_target`, with the version
    // set high enough. Luckily one LC_BUILD_VERSION is enough, for Xcode
    // to pick it up (since std and core are still built with the fallback
    // of version 7.0 and hence emit the old LC_IPHONE_MIN_VERSION).
-    let (major, minor) = ios_deployment_target(arch);
+    let (major, minor) = ios_deployment_target(arch, "");


Feels a bit odd to use an empty string when we have Option available to us. Is this string defined at a higher level and changing it would be as pain?

I mostly just tried to follow the existing conventions for the abi value, but agree that this should change.

Have opened #124762 to fix part of this now

shepmaster · 2024-02-19T19:21:47Z

compiler/rustc_target/src/spec/base/apple/mod.rs

+        add_link_args_iter(
+            &mut args,
+            LinkerFlavor::Darwin(Cc::Yes, Lld::No),
+            ["-target".into(), mac_catalyst_llvm_target(arch).into()].into_iter(),


I know it's not your code, but I wish this function took impl IntoIterator. Maybe a future cleanup.

shepmaster · 2024-02-19T19:23:51Z

Thank you for the well-prepared commit sequence!

@bors r+ rollup=iffy

bors · 2024-02-19T19:23:54Z

📌 Commit a3cf493 has been approved by shepmaster

It is now in the queue for this repository.

BlackHoleFox · 2024-02-19T19:34:40Z

This shouldn't require any changes to the cc crate, as that uses rustc --print=deployment-target to get this information automatically.

This got reverted fwiw because it turns out rust's default versions don't align well with Apple's SDKs and other third-party C libraries. It shouldn't matter through because cc doesnt pass a deployment target for Catalyst targets and is just using the SDK default for the target (which is 17.0 as of XCode 14). As this is much lower there should be no conflicts. 13.1 is actually the lowest catalyst version the Xcode 14 SDK supports too.

That should be corrected eventually but at least this doesn't make that harder.

bors · 2024-02-20T00:04:06Z

⌛ Testing commit a3cf493 with merge 7d61394...

bors · 2024-02-20T02:30:11Z

☀️ Test successful - checks-actions
Approved by: shepmaster
Pushing 7d61394 to master...

rust-timer · 2024-02-20T03:43:55Z

Finished benchmarking commit (7d61394): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	2.8%	[2.8%, 2.8%]	1
Regressions ❌ (secondary)	2.6%	[2.6%, 2.6%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	2.8%	[2.8%, 2.8%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	4.3%	[3.2%, 5.6%]	3
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 639.836s -> 638.5s (-0.21%)
Artifact size: 308.66 MiB -> 308.65 MiB (-0.01%)

madsmtm added 5 commits February 19, 2024 12:34

Remove MACOSX_DEPLOYMENT_TARGET env var when linking Mac Catalyst

94ddbb6

Mac Catalyst uses IPHONEOS_DEPLOYMENT_TARGET to specify the deployment target, so it makes no sense to remove that variable.

Make LLVM target contain correct deployment target info on Mac Catalyst

92d4b31

Fix ld platform_version argument on Mac Catalyst

3cb4e34

Merge deployment target variable loading on iOS and Mac Catalyst

cd530fc

Lower default Mac Catalyst deployment target to 13.1

a3cf493

Same default as Clang: https://github.com/llvm/llvm-project/blob/d022f32c73c57b59a9121eba909f5034e89c628e/clang/lib/Driver/ToolChains/Darwin.cpp#L2038

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 19, 2024

rustbot assigned shepmaster Feb 19, 2024

shepmaster reviewed Feb 19, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 19, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 20, 2024

bors merged commit 7d61394 into rust-lang:master Feb 20, 2024
12 checks passed

rustbot added this to the 1.78.0 milestone Feb 20, 2024

madsmtm deleted the fix-mac-catalyst-deployment-target branch February 20, 2024 09:27

madsmtm mentioned this pull request Feb 22, 2024

Improve Xcode support madsmtm/objc2#459

Open

tgross35 mentioned this pull request Aug 2, 2024

"was built for iOS 16.4" warning after updating from 1.79 to 1.80 #128419

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix `IPHONEOS_DEPLOYMENT_TARGET` on Mac Catalyst #121296

Fix `IPHONEOS_DEPLOYMENT_TARGET` on Mac Catalyst #121296

madsmtm commented Feb 19, 2024 •

edited

Loading

rustbot commented Feb 19, 2024

rustbot commented Feb 19, 2024

madsmtm commented Feb 19, 2024

madsmtm commented Feb 19, 2024 •

edited

Loading

shepmaster Feb 19, 2024

madsmtm Feb 20, 2024

madsmtm May 5, 2024

shepmaster Feb 19, 2024

shepmaster commented Feb 19, 2024

bors commented Feb 19, 2024

BlackHoleFox commented Feb 19, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

rust-timer commented Feb 20, 2024

Fix IPHONEOS_DEPLOYMENT_TARGET on Mac Catalyst #121296

Fix IPHONEOS_DEPLOYMENT_TARGET on Mac Catalyst #121296

Conversation

madsmtm commented Feb 19, 2024 • edited Loading

rustbot commented Feb 19, 2024

rustbot commented Feb 19, 2024

madsmtm commented Feb 19, 2024

madsmtm commented Feb 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shepmaster commented Feb 19, 2024

bors commented Feb 19, 2024

BlackHoleFox commented Feb 19, 2024

bors commented Feb 20, 2024

bors commented Feb 20, 2024

rust-timer commented Feb 20, 2024

Overall result: no relevant changes - no action needed

Fix `IPHONEOS_DEPLOYMENT_TARGET` on Mac Catalyst #121296

Fix `IPHONEOS_DEPLOYMENT_TARGET` on Mac Catalyst #121296

madsmtm commented Feb 19, 2024 •

edited

Loading

madsmtm commented Feb 19, 2024 •

edited

Loading