-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
WIP: Parse Apple SDK versions #131478
Draft
madsmtm
wants to merge
10
commits into
rust-lang:master
Choose a base branch
from
madsmtm:parse-sdkroot
base: master
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Draft
WIP: Parse Apple SDK versions #131478
+2,482
−320
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
The OS version depends on the deployment target environment variables, the access of which we want to move to later in the compilation pipeline that has access to more information, for example `env_depinfo`.
Also make the SDK name be the same casing as used in the file system.
The exact reasoning why we do not always pass the SDK root with `-isysroot` to `cc` when linking on macOS eludes me (the git history dead ends in rust-lang#100286), but I suspect it's because we want to support `cc`s which do not support this option. So instead, we pass the SDK root via the environment variable SDKROOT. This way, compiler drivers that support setting the SDK root (such as Clang and GCC) can use it, while compiler drivers that don't (presumably because they figure out the SDK root in some other way) can just ignore it. This fixes rust-lang#80817 (by always passing the SDK root, even when linking with cc on macOS).
Instead of being a type-alias to a tuple. This allows us to implement Deserialize for OSVersion, which will be useful when parsing SDK settings.
rustbot
added
A-run-make
Area: port run-make Makefiles to rmake.rs
O-apple
Operating system: Apple (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.
S-blocked
Status: Blocked on something else such as an RFC or other implementation work.
labels
Oct 10, 2024
This comment has been minimized.
This comment has been minimized.
Previously, we did a simplistic check against the SDKROOT path to try to figure out whether we should use the environment variable, or try to find a better matching SDK instead. This is brittle since the user might want their SDK to be in a different place in the file system (that's kinda the whole idea of the SDKROOT variable), and also didn't work properly with SDKs in the Xcode Command Line Tools. Instead, we now: 1. Parse the SDKSettings.json file under the SDKROOT, to determine whether it is suitable for the target we're compiling for (which it might not be when using Cargo, and targetting iOS but compiling build scripts on macOS). 2. If it's not, attempt to find another SDK. This allows us to give much more detailed error messages, to better support cross-compile scenarios, and allows using parameters from the SDK (such as the SDK version) elsewhere in the future.
This is an important property of the binary that we were previously setting incorrectly, and is a big step towards making binaries linked with cc and ld64 equal.
madsmtm
force-pushed
the
parse-sdkroot
branch
from
October 10, 2024 01:31
9e37d1b
to
1960f3b
Compare
The job Click to see the possible cause of the failure (guessed by this bot)
|
jieyouxu
removed
the
S-waiting-on-review
Status: Awaiting review from the assignee but also interested parties.
label
Oct 10, 2024
bootstrap can possibly handle that as some kind of target specific config option, depending on the specifics of what you want. |
☔ The latest upstream changes (presumably #132497) made this pull request unmergeable. Please resolve the merge conflicts. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Labels
A-run-make
Area: port run-make Makefiles to rmake.rs
O-apple
Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS)
S-blocked
Status: Blocked on something else such as an RFC or other implementation work.
T-compiler
Relevant to the compiler team, which will review and decide on the PR/issue.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
High-level goals of this PR:
zig cc
(though that maybe only works on macOS? I don't think Zig provides the.tdb
s and headers to build and link iOS?)Previously, we did a simplistic check against the
SDKROOT
path to try to figure out whether we should use the environment variable, or try to find a better matching SDK instead.This is brittle since the user might want their SDK to be in a different place in the file system (that's kinda the whole idea of the
SDKROOT
variable), and also didn't work properly with SDKs in the Xcode Command Line Tools.Instead, we now:
SDKSettings.json
file under theSDKROOT
, to determine whether it is suitable for the target we're compiling for (which it might not be when e.g. using Cargo, and targetting iOS but compiling build scripts on macOS).This allows us to give much more detailed error messages, to better support cross-compile scenarios, and allows using parameters from the SDK, such as the SDK version, which we need to pass to
ld64
for correctness (this fixes the remaining difference between linking withcc
andld64
).Final part of #129432.
Additionally, this fixes #80120 by introducing better error messages, so that it's possible to more clearly see when
/Library/Developer/CommandLineTools
is invalid.Test
Tested with:
Which includes every Apple target except for arm64e.
Note that using the
MacOSX10.13.sdk
now works (and should work even more reliably once this change hits beta).TODO
cargo +stage1
inside Xcode for a project with both build scripts and proc macros (namely Bevy's).zig cc
without any SDKs available.zig cc
from a non macOS host.xcrun --sdk XYZ rustc +stage1
.SDKROOT
by defaultCC @BlackHoleFox @thomcc
Currently an unholy
git
combination of #131037, #131433 and #131477, so:@rustbot blocked