Fix wrong SDK being used for the host on macOS

[ghost]

Closes #7283

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @Eh2406 (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.

[Eh2406]

I am sorry I don't use mac things so I will have a hard time testing,
@bors r? @ehuss

[ghost]

I had to set up a Linux virtual machine to figure out why the tests weren't passing, and rewrite this a couple of times in order to wrap my head around it, but I'm confident that it's correct now. Should be good to review!

[alexcrichton]

Thanks for the PR! Unfortunately though Cargo in general we try to keep out of the target-specific build logic like this. Is there perhaps another way to solve this problem that doesn't involve changing Cargo?

[ghost]

It should be noted that SDKROOT is used in many parts of the build process to locate the correct SDK, e.g. by rustc, ld, etc. It appears to be possible to work around this by undoing the changes made by Xcode to the PATH and SDKROOT environment variables with unset SDKROOT and PATH=/usr/local/bin:/usr/bin:/bin:/usr/sbin:/sbin before invoking cargo build, but this doesn't feel like a great solution. It seems to me like this will break later on if a different SDK needs to be used from the one that it happens to default to.

[alexcrichton]

This may be a case for compiler rather than cargo changes. This is similar to how msvc works where if you configure global env vars then it makes cross compilation difficult, so the easiest thing to do is to edit rustc so it doesn't need env vars (and use crates like cc to fix it for C code as well)

[ghost]

If -isysroot or -Wl,-syslibroot are not explicitly specified, they will be set to the SDKROOT environment variable. In order to fix this in the compiler, I believe this would involve specifying the -isysroot and -Wl,-syslibroot pre-link args to the "macosx" SDK on the x86_64-apple-darwin and i686-apple-darwin targets. Then the SDKROOT environment variable would not be used anymore when building for the host. This is equivalent to what this pull request does, but with the functionality moved to the compiler. It looks like cc already specifies -isysroot so no changes are required there.

[ghost]

There may be a case in which the user would want to explicitly specify the SDK path with the SDKROOT environment variable (rust-lang/rust#62903), in which case ignoring the user's preference by always setting -isysroot is not desired. I feel like the correct thing may be to only set -isysroot for custom build scripts, which would enable the user to still override the SDK path, but also allow custom build scripts to be built for the correct platform. This comes back to putting target-specific build logic in cargo.

[alexcrichton]

The cc crate solves this by having target-specific env vars like CC_$target, and perhaps rustc/cc could be updated to support a similar convention? Unfortunately we're just not really ready to encode target-specific build logic into Cargo, it's always been avoided because it's such a rabbit hole and I'm not sure this is the defining moment that we do start adding such logic to Cargo.

[ghost]

I'm not sure what that would look like. I'm not confident it's possible to solve this problem given these parameters. I understand the desire to avoid target-specific logic in cargo, but I can't see an alternative in this case—unless in rustc we always specify -isysroot to match the target platform and ignore the requested SDKROOT, but I'm not totally happy with this solution. I'll think about it a bit more, but I might have to keep working around this for now.

[alexcrichton]

We could add logic to rustc to detect that SDKROOT is set, a cross compilation is happening, and the SDKROOT is clearly not for the host and/or target. The compiler itself could then automatically not use SDKROOT for the host and only use it for the target, and I think that would fix this?

[ghost]

I had the same thought earlier, but I'm usually a bit vary about adding heuristics such as these, since it has the potential to be confusing and unexpected for the user. Perhaps it's not an issue in this case, as building with the wrong SDK is almost certainly a mistake and will cause the build to fail anyway. Maybe we should notify the user when we do this, by outputting a warning or some such?

[bors]

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

[alexcrichton]

Well the heuristic seems like it needs to be somewhere at this point. It's either in Cargo or rustc, and the logic is already in rustc and there's no logic at all in Cargo for this so it seems like this should go in rustc, not Cargo.

[alexcrichton]

This is making progress at rust-lang/rust#64254, so I'm going to close this in favor of that.

[comex]

I've been working on refactoring rustc's Apple OS target code (to support more OSes and generally be more orthogonal), and while rebasing on master I noticed the rustc change corresponding to this. I really don't think it's the right solution. Semantically, it makes a lot more sense to me that when Cargo decides to switch targets from the one the user specified, Cargo should handle correcting target settings for the new target. It shouldn't be rustc's job to guess when the target settings it's been given are wrong.

Practical reasons it would be better to do it in Cargo:

• To properly handle the case where build dependencies try to compile C code, you would need to replicate the same logic in cc-rs. A Cargo-side fix could apply equally to build scripts and rustc invocations.
• Also, ideally there would be a way to specify "SDKROOT/DEPLOYMENT_TARGET when building for host", but "building for host" is a Cargo concept.
• Detecting that the SDK root is for a different platform will not work if you're cross compiling from one version of macOS to a different version of macOS. If the target SDK is an older version than the host, the resulting binaries will probably work on the host anyway (although they might fail to build if a build dependency depends on something only available in a newer version). But it definitely won't work if you're building for a newer version of macOS than the host. That's admittedly a bit of an edge case, but it's actually something I do regularly.

[comex]

Going back to the previous discussion:

Well the heuristic seems like it needs to be somewhere at this point. It's either in Cargo or rustc, and the logic is already in rustc and there's no logic at all in Cargo for this so it seems like this should go in rustc, not Cargo.

If it's in Cargo, there's no need for a heuristic. The logic would be simply "drop the user's target settings when building for host instead of the user-specified target". In fact, that's exactly what this closed PR does.

The rustc solution that was adopted instead is* a heuristic, because rustc doesn't have a concept of "building for host instead of the user-specified target", so it has to guess based on whether the SDK looks like it's compatible with the current build machine (i.e. macOS).

[alexcrichton]

@comex this is following the precedent set by MSVC and selecting visual studio and cl.exe and such there. I didn't want to accidentally fall into a different pattern with respect to macOS just because it was a little easier to implement. If there's good reasons for doing this in Cargo then there's good reasons for doing this in Cargo, and PRs are welcome!