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

Add RUSTC_XCRUN_PATH var #62551

Closed
wants to merge 1 commit into from
Closed

Conversation

swolchok
Copy link
Contributor

Allows rustc to know about custom xcrun wrappers instead of always
assuming xcrun is on PATH.

We need this because we have a custom xcrun wrapper that gets installed in a different location.

Allows rustc to know about custom xcrun wrappers instead of always
assuming xcrun is on PATH.
@rust-highfive
Copy link
Collaborator

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matthewjasper (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 10, 2019
@matthewjasper
Copy link
Contributor

r? @alexcrichton

@alexcrichton
Copy link
Member

Thanks for the PR! Poking around in clang, I wonder if we could follow their lead here and perhaps support an SDKROOT env var to explicitly specify this instead? That is ideally somewhat more standard rather than adding our own new env var.

@swolchok
Copy link
Contributor Author

I think that SDKROOT is related to what's going on, but does not match what I'm trying to fix.

Here is the flow for SDKROOT and clang:

  1. User or build script invokes xcrun --sdk iphoneos clang
  2. xcrun sets SDKROOT and invokes clang
  3. clang uses SDKROOT to find the iPhoneOS SDK.

Unfortunately, rustc is not part of the iPhone SDK, so it is not invoked through xcrun. Here is the current flow for rustc + iPhoneOS:

  1. User or build script invokes rustc --target aarch64-apple-ios
  2. rustc shells out to xcrun --sdk iphoneos --show-sdk-path ("What would you set SDKROOT to if you were invoking me officially?")
  3. rustc uses the printed path to find the iPhoneOS SDK.

The flow we're trying to enable is a change in step (2). We have a custom xcrun wrapper that may not be ahead of xcrun on PATH, so we want rustc to shell out to $RUSTC_XCRUN_PATH --sdk iphoneos --show-sdk-path.

Just to be concrete, here's what the stock xcrun, which lives at /usr/bin/xcrun, sets SDKROOT to on my Mac:

xcrun -l --sdk iphoneos -- clang++ --help
env SDKROOT=/Applications/Xcode_10.2.1.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS12.2.sdk /Applications/Xcode_10.2.1.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/clang++ --help

As you can see, the path to xcrun and SDKROOT are not necessarily related, as they serve different purposes.

@alexcrichton
Copy link
Member

My worry is that we're just inventing environment variables and configuration for rustc here, adding new stable surface area to the compiler. This env var has no precedent and doesn't really follow any existing convention in rustc itself. Arguably there's already conventions for things like -C linker=... where CLI tools are passed as cli flags instead of environment variables.

For those reasons I wouldn't be super comfortable without adding this in. We would need to consult more teams and go through a longer stabilization process.

If, however, there's existing precedent (like it looks like SDKROOT is) then we can pretty easily follow that from what other compilers do. It means there's something we can reference and the user experience is the same across tools (C/Rust/etc)

@swolchok
Copy link
Contributor Author

I don't particularly care either way on environment variable vs. command-line flag. I went with environment variable to start because 1) there aren't any platform-specific command-line flags yet (IIUC linker is the only one and the concept of a linker is not platform-specific) and 2) there are several rustc-specific environment variables floating around, like RUSTC_BOOTSTRAP, RUSTC_RETRY_LINKER_ON_SEGFAULT, RUSTC_BREAK_ON_ICE, RUST_FORBID_DEP_GRAPH_EDGE, etc.

I think there are two ways forward here:

  1. We support SDKROOT in get_sdk_root, and then we require callers with custom xcrun paths to do something like env SDKROOT=$(path/to/xcrun --sdk iphoneos --show-sdk-path) rustc .... This is less convenient than RUSTC_XCRUN_PATH, but if it's easy to get accepted, I'm all for it.
  2. We add a command-line flag to pass the path to xcrun.

Can you confirm that adding SDKROOT would be accepted? It shouldn't take long to update this PR.

@alexcrichton
Copy link
Member

Oh sure yeah we've got a number of env vars here and there, but they're all intended for development of rustc and aren't intended to be user facing and configured by normal users of the compiler (e.g. we don't document most of them afaik). This, however, is a user-facing annotation which is one that we would want to advertise and recommend for users to use.

I would personally be ok r+'ing a patch using SDKROOT since clang is already using it as well

@swolchok
Copy link
Contributor Author

Would you like SDKROOT in a separate PR or should I repurpose this one?

@alexcrichton
Copy link
Member

I'm personally ok with either strategy, up to you as to which!

@swolchok
Copy link
Contributor Author

Replaced by #62903.

@swolchok swolchok closed this Jul 23, 2019
Centril added a commit to Centril/rust that referenced this pull request Jul 24, 2019
Support SDKROOT env var on iOS

Following what clang does (https://github.com/llvm/llvm-project/blob/296a80102a9b72c3eda80558fb78a3ed8849b341/clang/lib/Driver/ToolChains/Darwin.cpp#L1661-L1678), allow allow SDKROOT to tell us where the Apple SDK lives so we don't have to invoke xcrun.

Replaces rust-lang#62551.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants