Apple: Refactor deployment target version parsing by madsmtm · Pull Request #129341 · rust-lang/rust

madsmtm · 2024-08-21T02:13:24Z

Refactor deployment target parsing to make it easier to do #129342 (I wanted to make sure of all the places that std::env::var is called).

Specifically, my goal was to minimize the amount of target-specific configuration, so to that end I renamed the opts function that generates the TargetOptions to base, and made it return the LLVM target and target_arch too. In the future, I would like to move even more out of the target files and into spec::apple, as it makes it easier for me to maintain.

For example, this fixed a bug in aarch64-apple-watchos, which wasn't passing the deployment target as part of the LLVM triple. This (probably) fixes#123582 and fixes#107630.

We also now parse the patch version of deployment targets, allowing the user to specify e.g. MACOSX_DEPLOYMENT_TARGET=10.12.6.

Finally, this fixes the LLVM target name for visionOS, it should be *-apple-xros and not *-apple-visionos.

Since I have changed all the Apple targets here, I smoke-tested my changes by running the following:

# Build each target./x build library --target="aarch64-apple-darwin,aarch64-apple-ios,aarch64-apple-ios-macabi,aarch64-apple-ios-sim,aarch64-apple-tvos,aarch64-apple-tvos-sim,aarch64-apple-visionos,aarch64-apple-visionos-sim,aarch64-apple-watchos,aarch64-apple-watchos-sim,arm64_32-apple-watchos,arm64e-apple-ios,armv7k-apple-watchos,armv7s-apple-ios,i386-apple-ios,x86_64-apple-darwin,x86_64-apple-ios,x86_64-apple-ios-macabi,x86_64-apple-tvos,x86_64-apple-watchos-sim,x86_64h-apple-darwin"
# Test that we can still at least link basic projectscargo new foobar && cd foobar && cargo +stage1 build --target=aarch64-apple-darwin --target=aarch64-apple-ios --target=aarch64-apple-ios-macabi --target=aarch64-apple-ios-sim --target=aarch64-apple-tvos --target=aarch64-apple-tvos-sim --target=aarch64-apple-visionos --target=aarch64-apple-visionos-sim --target=aarch64-apple-watchos --target=aarch64-apple-watchos-sim --target=arm64_32-apple-watchos --target=armv7s-apple-ios --target=i386-apple-ios --target=x86_64-apple-darwin --target=x86_64-apple-ios --target=x86_64-apple-ios-macabi --target=x86_64-apple-tvos --target=x86_64-apple-watchos-sim --target=x86_64h-apple-darwin

I couldn't build for the arm64e-apple-darwin target, the armv7k-apple-watchos and arm64e-apple-ios targets failed to link, and I know that the i686-apple-darwin target requires a bit of setup, but all of this is as it was before this PR.

r? thomcc

CC @BlackHoleFox

I would recommend using rollup=never when merging this, in case we need to bisect this later.

rustbot · 2024-08-21T02:13:34Z

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

madsmtm · 2024-08-22T15:18:22Z

@bors rollup=never

There are quite a few changes here, and we might need to bisect this later.

EDIT: Oh well, I don't have permission to configure that, but I'd still recommend it when merging.

bors · 2024-08-22T15:18:24Z

@madsmtm: 🔑 Insufficient privileges: not in try users

thomcc · 2024-08-23T19:50:20Z

@bors rollup=never

thomcc

This looks fine to me. Was a hassle to verify all the targets match what they used to.

thomcc · 2024-08-23T20:31:40Z

I'm not a compiler-team or compiler-contributor so I'll roll to someone who is, but I'd have R+ed this if I were.
r? compiler

madsmtm · 2024-09-03T09:29:28Z

r? compiler

madsmtm · 2024-09-04T19:57:43Z

Thanks for the review!

@rustbot ready

BlackHoleFox

Since this has been looked at someone on the compiler team, here's my lookover from the Apple perspective since I was CCed.

- Merge minimum OS version list into one function (makes it easier to see the logic in it). - Parse patch deployment target versions. - Consistently specify deployment target in LLVM target (previously omitted on `aarch64-apple-watchos`).

petrochenkov · 2024-09-05T12:59:40Z

@bors r+

bors · 2024-09-05T12:59:43Z

📌 Commit bd56857 has been approved by petrochenkov

It is now in the queue for this repository.

bors · 2024-09-07T01:37:55Z

⌛ Testing commit bd56857 with merge 9afe713...

bors · 2024-09-07T04:03:30Z

☀️ Test successful - checks-actions
Approved by: petrochenkov
Pushing 9afe713 to master...

rust-timer · 2024-09-07T05:18:53Z

Finished benchmarking commit (9afe713): 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 (primary 2.7%)

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.7%	[2.7%, 2.7%]	1
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	2.7%	[2.7%, 2.7%]	1

Cycles

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

Binary size

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

Bootstrap: 756.217s -> 756.798s (0.08%)
Artifact size: 341.14 MiB -> 341.03 MiB (-0.03%)

rustbot assigned thomcc Aug 21, 2024

rustbot added O-apple Operating system: Apple / Darwin (macOS, iOS, tvOS, visionOS, watchOS) 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 Aug 21, 2024

madsmtm mentioned this pull request Aug 21, 2024
Rebuild on changes to the deployment target when compiling Apple targets #129342
Open

madsmtm force-pushed the refactor-deployment-target branch 2 times, most recently from 009eaea to 1345f40Compare August 21, 2024 18:15

madsmtm mentioned this pull request Aug 21, 2024
[MacCatalyst]: Can't determine the platform when creating an xcframework from rust libraries #107630
Closed

thomcc approved these changes Aug 23, 2024
View reviewed changes

Comment threadcompiler/rustc_target/src/spec/base/apple/mod.rs
Comment threadcompiler/rustc_target/src/spec/base/apple/mod.rs

rustbot assigned BoxyUwU and unassigned thomcc Aug 23, 2024

This comment was marked as resolved.
Sign in to view

rustbot assigned petrochenkov and unassigned BoxyUwU Sep 3, 2024

madsmtm force-pushed the refactor-deployment-target branch from 1345f40 to 03ef705Compare September 3, 2024 09:34

petrochenkov reviewed Sep 4, 2024
View reviewed changes

Comment threadcompiler/rustc_target/src/spec/base/apple/mod.rs Outdated
Comment threadcompiler/rustc_target/src/spec/base/apple/mod.rs Outdated

petrochenkov added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 4, 2024

madsmtm force-pushed the refactor-deployment-target branch from 03ef705 to d69cfd7Compare September 4, 2024 19:56

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Sep 4, 2024

BlackHoleFox approved these changes Sep 4, 2024
View reviewed changes

Comment threadcompiler/rustc_target/src/spec/base/apple/mod.rs Outdated
Comment threadcompiler/rustc_target/src/spec/targets/aarch64_apple_watchos_sim.rs Outdated

This comment has been minimized.
Sign in to view

madsmtm force-pushed the refactor-deployment-target branch from d69cfd7 to b202b0bCompare September 5, 2024 04:04

This comment has been minimized.
Sign in to view

madsmtm force-pushed the refactor-deployment-target branch from b202b0b to 175f887Compare September 5, 2024 04:45

madsmtm added 3 commits September 5, 2024 06:47

Apple: Improve comments for -arch linker argument
23cdb50

Apple: Refactor deployment target version parsing
5b51331
- Merge minimum OS version list into one function (makes it easier to see the logic in it). - Parse patch deployment target versions. - Consistently specify deployment target in LLVM target (previously omitted on `aarch64-apple-watchos`).

Apple: Add comments for -platform_version linker argument
bd56857

madsmtm force-pushed the refactor-deployment-target branch from 175f887 to bd56857Compare September 5, 2024 04:47

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 Sep 5, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 7, 2024

bors merged commit 9afe713 into rust-lang:masterSep 7, 2024

rustbot added this to the 1.83.0 milestone Sep 7, 2024

madsmtm deleted the refactor-deployment-target branch September 7, 2024 09:36

madsmtm mentioned this pull request Sep 7, 2024
Test codegen when setting deployment target #130068
Merged

Uh oh!

Uh oh!

Conversation

madsmtm commented Aug 21, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rustbot commented Aug 21, 2024

Uh oh!

madsmtm commented Aug 22, 2024• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 22, 2024

Uh oh!

thomcc commented Aug 23, 2024

Uh oh!

thomcc left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

thomcc commented Aug 23, 2024

Uh oh!

This comment was marked as resolved.

madsmtm commented Sep 3, 2024

Uh oh!

Uh oh!

Uh oh!

madsmtm commented Sep 4, 2024

Uh oh!

BlackHoleFox left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

petrochenkov commented Sep 5, 2024

Uh oh!

bors commented Sep 5, 2024

Uh oh!

bors commented Sep 7, 2024

Uh oh!

bors commented Sep 7, 2024

Uh oh!

rust-timer commented Sep 7, 2024

Overall result: no relevant changes - no action needed

Instruction count

Max RSS (memory usage)

Cycles

Binary size

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

madsmtm commented Aug 21, 2024•
edited
Loading

madsmtm commented Aug 22, 2024•
edited
Loading