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

macOS: Always pass SDK root when linking with cc, and pass it via SDKROOT env var #131477

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

madsmtm
Copy link
Contributor

@madsmtm madsmtm commented Oct 10, 2024

About trampoline binaries

The developer binaries at /usr/bin/* are actually just trampolines (similar in spirit to the Rust binaries in ~/.cargo/bin) which invoke xcrun to get the current developer directory, and then launch the actual binary under /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/*.

The actual binary is then launched with the following environment variables set (but none of them are set if SDKROOT is set explicitly):

  • SDKROOT=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk
  • LIBRARY_PATH=/usr/local/lib (appended)
  • CPATH=/usr/local/include (appended)
  • MANPATH=/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/share/man:/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/usr/share/man:/Applications/Xcode.app/Contents/Developer/usr/share/man:/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/share/man: (prepended)

This allows the user to type e.g. cc --version in their terminal on macOS, and have it automatically pick up a suitable Clang binary from either an installed Xcode.app or the Xcode Command Line Tools (it acts roughly as-if you typed xcrun cc --version).

Always setting SDK root

So, the logic for finding the macOS SDK root is usually handled by the /usr/bin/cc trampoline, which is why we currently just remove the SDKROOT env var when set for the wrong platform, and don't do anything else.

This doesn't work when building inside Xcode though, since Xcode prepends /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin to PATH, which means we end up calling the actual Clang binary, and end up without an SDK root specified at all.

Instead, we now always set the SDK root, even when linking with cc on macOS. This fixes #80817, fixes #96943, and generally makes linking with cc and ld closer in behaviour (ld doesn't understand SDKROOT, so it's not affected by xcrun).

Setting SDKROOT instead of -isysroot

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 #100286), but I suspect it's because we want to support ccs which do not support this option.
To make sure that such use-cases continue to work, we pass the SDK root via the SDKROOT environment variable. 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.

One danger here would be if there's some compiler driver out there which works with the -isysroot flag, but not with the SDKROOT environment variable. I am not aware of any?

Note also that this overrides the behaviour discussed above (/usr/bin/cc sets some extra environment variables), I will argue that is fine:

  • MANPATH and CPATH is useless when linking
  • /usr/local/lib is empty on a default system at least since macOS 10.14, but might be filled by extra libraries installed by the user, but I'd argue that if we want it to be part of the library search path, we should set it explicitly so that it's also set when linking with ld directly.

Alternatives

  • Invoke /usr/bin/cc instead of cc.
    • This breaks many other use-cases though where overriding cc in the PATH is desired.
  • Look up which cc, and do special logic if in Xcode toolchain.
    • Seems brittle, and besides, it's not the cc in the Xcode toolchain that's wrong, it's the /usr/bin/cc behaviour that is a bit too magical.
  • Invoke xcrun --sdk macosx cc.
    • This completely ignores SDKROOT, so we'd still have to parse that first to figure out if it's suitable or not, but would probably be workable.
  • Maybe somehow configure the linker with extra flags such that it'll be able to link regardless of linking for macOS or e.g. iOS? Though I doubt this is possible.

Testing

Tested that this works with:

  • Default /usr/bin/cc
  • /usr/bin/ld
  • Actual Clang from Xcode (/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang)
  • /usr/bin/clang (invoked via clang instead of cc)
  • Homebrew's llvm package (ignores SDKROOT, uses their own SDK)
  • Homebrew's gcc package (SDKROOT is preferred over their own SDK)
  • Macports clang Couldn't get it to build
  • Macports gcc (SDKROOT is preferred over their own SDK)
  • Zig CC installed via. homebrew (ignores both -isysroot and SDKROOT, uses their own SDK)
  • Nixpkgs clang (ignores SDKROOT, uses their own SDK)
  • Nixpkgs gcc (ignores SDKROOT, uses their own SDK)
  • cosmocc? Doesn't accept common flags (like -arch)

Meta

Part of #129432.

CC @BlackHoleFox @thomcc

Builds upon #131433, see the last commit for the actual change.
@rustbot blocked
@rustbot label relnotes

@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

r? @nnethercote

rustbot has assigned @nnethercote.
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 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. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Oct 10, 2024
@madsmtm madsmtm marked this pull request as ready for review October 10, 2024 01:20
@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

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

@rust-log-analyzer

This comment has been minimized.

@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from 6aaa795 to 97e382d Compare October 10, 2024 01:23
@nnethercote
Copy link
Contributor

This looks reasonable, but I don't really have the first idea about any of this stuff and that means I'm not a great reviewer for it. @thomcc, are you able to take a look? I can rubber-stamp it if you are happy.

One question: there are no tests in this PR or in #131433. Should there be?

@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from 97e382d to eeea5f0 Compare October 10, 2024 05:44
@rustbot rustbot added the A-run-make Area: port run-make Makefiles to rmake.rs label Oct 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Oct 10, 2024

This PR modifies tests/run-make/. If this PR is trying to port a Makefile
run-make test to use rmake.rs, please update the
run-make port tracking issue
so we can track our progress. You can either modify the tracking issue
directly, or you can comment on the tracking issue and link this PR.

cc @jieyouxu

@madsmtm
Copy link
Contributor Author

madsmtm commented Oct 10, 2024

One question: there are no tests in this PR or in #131433. Should there be?

There are a few unit tests under compiler/rustc_codegen_ssa/src/back/apple/tests.rs, but your point stands, there are no integration tests for this.

The main reason for that is that this is really hard to write a robust test for, since this is so heavily environment specific - but I guess a test that adds the Xcode paths to PATH as a regression test for #80817 would be doable, I've done that and rebased, thanks for the nudge to do it ;).

@rust-log-analyzer

This comment has been minimized.

@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from eeea5f0 to 8ca8d4f Compare October 10, 2024 05:52
@rust-log-analyzer

This comment has been minimized.

@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from 25e7f0f to 5d010db Compare October 31, 2024 20:39
@rustbot
Copy link
Collaborator

rustbot commented Oct 31, 2024

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

cc @Noratrieb

@rustbot rustbot added the relnotes Marks issues that should be documented in the release notes of the next release. label Oct 31, 2024
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I approve of the macOS specific parts of this. I'm not a compiler reviewer so I haven't done any review for style/etc. That said that part does look fine to me. It's a great PR overall. Great PR description too, clarified some things I had always wondered about.

@nnethercote
Copy link
Contributor

Thanks, @thomcc. Once #131433 is merged I'm happy for this to merge.

@bors delegate=madsmtm

@bors
Copy link
Contributor

bors commented Oct 31, 2024

✌️ @madsmtm, you can now approve this pull request!

If @nnethercote told you to "r=me" after making some further change, please make that change, then do @bors r=@nnethercote

@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 sdkroot-via-env-var branch from 5d010db to 91c8460 Compare November 3, 2024 22:10
@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from 91c8460 to c3f0919 Compare November 3, 2024 22:15
@rust-log-analyzer

This comment has been minimized.

@madsmtm
Copy link
Contributor Author

madsmtm commented Nov 10, 2024

I have become aware via rust-lang/cc-rs#1278 that Homebrew's Clang uses configuration files that passes --sysroot, will have to investigate whether Rust using the SDKROOT env var is still desirable there?

@madsmtm madsmtm marked this pull request as draft November 10, 2024 18:01
@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from f928d11 to 425ca73 Compare November 10, 2024 18:47
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).
@madsmtm madsmtm force-pushed the sdkroot-via-env-var branch from 425ca73 to 98cde29 Compare November 10, 2024 19:00
@nnethercote nnethercote 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 Nov 18, 2024
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) relnotes Marks issues that should be documented in the release notes of the next release. S-blocked Status: Blocked on something else such as an RFC or other implementation work. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
7 participants