Fix sysroot on macOS when cross-compiling and SDKROOT is set #64254
Conversation
This file contains hidden or 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
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (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
added
the
S-waiting-on-review
Centril
reviewed
Sep 7, 2019
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
alexcrichton
reviewed
Sep 9, 2019
|
Thank you for bearing with me as I rework this. I've gotten rid of the The reason I went with |
| @@ -51,3 +51,19 @@ pub fn macos_llvm_target(arch: &str) -> String { | |||
| let (major, minor) = macos_deployment_target(); | |||
| format!("{}-apple-macosx{}.{}.0", arch, major, minor) | |||
| } | |||
|
|
|||
| pub fn macos_link_env() -> Vec<(String, String)> { | |||
There was a problem hiding this comment.
This seems a bit of an odd interface since it actually only returns env vars which should be cleared out, not env vars which should be set. Do we know what the various tools will behave right when set to blank values which effectively disables them? The alternative would be to extend link_env with the ability to delete env vars as well (e.g. env_remove).
Additionally, it seems like this would also want to encompass the ios targets perhaps? For example if you set SDKROOT for osx or MACOSX_DEPLOYMENT_TARGET, should those values be automatically cleared out for ios compliation in addition to macos compilation?
There was a problem hiding this comment.
This seems a bit of an odd interface since it actually only returns env vars which should be cleared out, not env vars which should be set. Do we know what the various tools will behave right when set to blank values which effectively disables them? The alternative would be to extend
link_envwith the ability to delete env vars as well (e.g.env_remove).
If the variable is set to an empty string, the linker behaves as if the environment variable wasn't set. We can rework this to remove environment variables instead if that feels better.
Additionally, it seems like this would also want to encompass the ios targets perhaps? For example if you set
SDKROOTfor osx orMACOSX_DEPLOYMENT_TARGET, should those values be automatically cleared out for ios compliation in addition to macos compilation?
Yes, this makes sense. We should clear SDKROOT if it's set to the wrong platform when targeting iOS. The value of MACOSX_DEPLOYMENT_TARGET doesn't appear to be meaningful in the same way that IPHONEOS_DEPLOYMENT_TARGET is (i.e. it doesn't appear to be used by the linker to infer the target platform), so perhaps we don't need to clear this one.
There was a problem hiding this comment.
Scratch that last bit actually—MACOSX_DEPLOYMENT_TARGET is used, but it doesn't manifest until you actually try running the executable on the simulator, which will fail with dyld: program was built for a platform that is not supported by this runtime if the environment variable was set.
|
Looks great to me, thanks @aleksijuvani! @bors: r+ |
|
📌 Commit fe6d626 has been approved by |
bors
added
S-waiting-on-bors
|
⌛ Testing commit fe6d626 with merge dad4c3ff1d431facee86453612fe6a4a0d96bfc1... |
|
💥 Test timed out |
bors
added
S-waiting-on-review
|
@bors retry |
bors
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Sep 13, 2019
Fix sysroot on macOS when cross-compiling and SDKROOT is set Fixes rust-lang/cargo#7283 Closes rust-lang/cargo#7284 r? @alexcrichton
|
☀️ Test successful - checks-azure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Fixes rust-lang/cargo#7283
Closes rust-lang/cargo#7284
r? @alexcrichton