-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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
Apple: Refactor deployment target version parsing #129341
Conversation
These commits modify compiler targets. |
009eaea
to
1345f40
Compare
@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. |
@madsmtm: 🔑 Insufficient privileges: not in try users |
@bors rollup=never |
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 looks fine to me. Was a hassle to verify all the targets match what they used to.
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. |
This comment was marked as resolved.
This comment was marked as resolved.
r? compiler |
1345f40
to
03ef705
Compare
03ef705
to
d69cfd7
Compare
Thanks for the review! @rustbot ready |
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.
Since this has been looked at someone on the compiler team, here's my lookover from the Apple perspective since I was CCed.
compiler/rustc_target/src/spec/targets/aarch64_apple_watchos_sim.rs
Outdated
Show resolved
Hide resolved
This comment has been minimized.
This comment has been minimized.
d69cfd7
to
b202b0b
Compare
This comment has been minimized.
This comment has been minimized.
b202b0b
to
175f887
Compare
- 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`).
175f887
to
bd56857
Compare
@bors r+ |
☀️ Test successful - checks-actions |
Finished benchmarking commit (9afe713): comparison URL. Overall result: no relevant changes - no action needed@rustbot label: -perf-regression Instruction countThis 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.
CyclesThis benchmark run did not return any relevant results for this metric. Binary sizeThis benchmark run did not return any relevant results for this metric. Bootstrap: 756.217s -> 756.798s (0.08%) |
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 theTargetOptions
tobase
, and made it return the LLVM target andtarget_arch
too. In the future, I would like to move even more out of the target files and intospec::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:
I couldn't build for the
arm64e-apple-darwin
target, thearmv7k-apple-watchos
andarm64e-apple-ios
targets failed to link, and I know that thei686-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.