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.

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: Re-implement SDK discovery instead of using xcrun #131433

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Oct 9, 2024

Shelling out to a global command line tool like this makes it difficult for rustc to properly implement change-tracking of the involved variables and paths (e.g. we could tell Cargo to relink after the user runs xcode-select --switch or installs/updates Xcode) as required for #118204, and makes error handling more difficult. So instead, this PR re-implements the SDK discovery logic that xcrun does, to avoid depending on that.

The logic is now roughly:

let sdkroot = if let Some(sdkroot) = env::var("SDKROOT") && sdk_matches(sdkroot, target_os) {
    sdkroot
} else if let Some(dir) = env::var("DEVELOPER_DIR") {
    search_for_platform_and_sdk(dir, sdk_name)
} else if let Some(path) = fs::read_link("/var/db/xcode_select_link")? { // Special path that `xcode-select --switch` configures, undocumented
    search_for_platform_and_sdk(path, sdk_name)
} else if fs::exists("/Applications/Xcode.app")? { // Hard-coded fall back
    search_for_platform_and_sdk("/Applications/Xcode.app/Contents/Developer", sdk_name)
} else if fs::exists("/Library/Developer/CommandLineTools")? { // Hard-coded fall back
    search_for_sdk("/Library/Developer/CommandLineTools/SDKs", sdk_name)
} else {
    panic!("tell the user that they need to install Xcode")
};

I have tested this in a newly created virtual machine, and verified that:

Part of #129432.

I'm not sure, but I don't think release notes are desirable here? The behaviour should be mostly identical apart from better error messages, and besides, this was very under-documented before.

CC @BlackHoleFox @thomcc.

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2024

r? @TaKO8Ki

rustbot has assigned @TaKO8Ki.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot
Copy link
Collaborator

rustbot commented Oct 9, 2024

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

@rustbot rustbot added 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. labels Oct 9, 2024
@madsmtm madsmtm changed the title Re-implement SDK discovery instead of using xcrun Apple: Re-implement SDK discovery instead of using xcrun Oct 9, 2024
@workingjubilee
Copy link
Member

workingjubilee commented Oct 9, 2024

not saying we shouldn't add this, but how long do you think is left until we get fed up with shoving this kind of code into cg_ssa and split out rustc_linker? ...or would it be rustc_archiver?

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 9, 2024

not saying we shouldn't add this, but how long do you think is left until we get fed up with shoving this kind of code into cg_ssa and split out rustc_linker? ...or would it be rustc_archiver?

No idea honestly, I'm not really familiar enough with the overall crate structure to say.

compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple/tests.rs Outdated Show resolved Hide resolved
compiler/rustc_codegen_ssa/src/apple/tests.rs Outdated Show resolved Hide resolved
@BlackHoleFox
Copy link
Contributor

Short initial review since I was tagged, but not an actual compiler reviewer etc etc.

@madsmtm madsmtm force-pushed the find-sdkroot-manually branch from edf397b to bbb59cd Compare October 31, 2024 20:15
@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

Some changes occurred in src/doc/rustc/src/platform-support

cc @Noratrieb

@workingjubilee
Copy link
Member

r? compiler

@BlackHoleFox
Copy link
Contributor

macOS changes here look fine to me.

@bors
Copy link
Contributor

bors commented Nov 2, 2024

☔ The latest upstream changes (presumably #132497) made this pull request unmergeable. Please resolve the merge conflicts.

Also make the SDK name be the same casing as used in the file system.
@madsmtm madsmtm force-pushed the find-sdkroot-manually branch from bbb59cd to 24e7da1 Compare November 3, 2024 22:10
@madsmtm madsmtm force-pushed the find-sdkroot-manually branch from 24e7da1 to bd1888f Compare November 3, 2024 22:12
@pnkfelix
Copy link
Member

I'm not comfortable approving this without more team discussion. it is adding an extra amount of maintenance labor, for gains that are ... unclear at best to me. (And I say this as someone who develops on a Mac ... though I freely admit that I also bias towards Linux for a lot of development.)

Maybe an MCP is warranted?

@rustbot label I-compiler-nominated

@rustbot rustbot added the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Dec 12, 2024
@pnkfelix
Copy link
Member

@rustbot label: +S-waiting-on-team -S-waiting-on-review

@rustbot rustbot added S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2024
@madsmtm
Copy link
Contributor Author

madsmtm commented Dec 12, 2024

it is adding an extra amount of maintenance labor, for gains that are ... unclear at best to me.

Yeah, that's a fair assessment, I guess an alternative is to parse the error messages from xcrun (they're just very bad, so it's kinda difficult to really glean what's going on), and I think we could still somewhat support change-tracking anyhow.

I'll admit that my main motivation here is that I don't like xcrun, though I do still think the reasons I've noted in a code comment has some merit:

The reason why we implement this logic ourselves is:

  • Reading these directly is faster than spawning a new process.
  • xcrun can be fairly slow to start up after a reboot.
  • In the future, we will be able to integrate this better with the compiler's change tracking mechanisms, allowing rebuilds when the involved env vars and paths here change. See Rebuild if deployment target changes #118204.
  • It's easier for us to emit better error messages.

@oli-obk
Copy link
Contributor

oli-obk commented Dec 13, 2024

r? compiler

@rustbot rustbot assigned estebank and unassigned pnkfelix Dec 13, 2024
@apiraino
Copy link
Contributor

Discussed in T-compiler triage.

We are probably in favor of opening an MCP about this proposal, as Felix suggests. We should be trying to get a wider buyout from T-compiler since there are some concerns about mantainability and bus-factor (how many people are familiar with these changes?).

Thanks!

@rustbot label -I-compiler-nominated

@rustbot rustbot removed the I-compiler-nominated Nominated for discussion during a compiler team meeting. label Dec 19, 2024
@apiraino apiraino removed the S-waiting-on-team Status: Awaiting decision from the relevant subteam (see the T-<team> label). label Dec 19, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-apple Operating system: Apple (macOS, iOS, tvOS, visionOS, watchOS) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
10 participants