Fix xctoolchain AppleClang to corrrectly use -isysroot #703
Conversation
youknowone
force-pushed
the
apple-clang
branch
7 times, most recently
from
f55cce3
to
6498d9c
Compare
youknowone
changed the title
youknowone
force-pushed
the
apple-clang
branch
2 times, most recently
from
80aa69d
to
f066b3a
Compare
youknowone
changed the title
youknowone
force-pushed
the
apple-clang
branch
2 times, most recently
from
ce58a7c
to
221cf31
Compare
There was a problem hiding this comment.
This looks like a good idea to me, but I'm not sure this is the way to go about it. I've left some comments but will need to do further investigation of what's expected here (and review some of the work by @indygreg in apple-sdk).
src/lib.rs
Outdated
| "macosx", | ||
| "", | ||
| std::env::var("MACOSX_DEPLOYMENT_TARGET") | ||
| .unwrap_or_else(|_| (if self.cpp { "10.9" } else { "10.4" }).into()), |
There was a problem hiding this comment.
As I mentioned in the other PR, I'd prefer this match the logic in https://github.com/rust-lang/rust/blob/6365e5ad9fa9e2ec867a67aeeae414e7c62d8354/compiler/rustc_target/src/spec/apple_base.rs#L105-L114
| cmd.args.push(sdk_path); | ||
| } | ||
|
|
||
| if !is_mac { |
There was a problem hiding this comment.
Can you explain why this is conditional now?
There was a problem hiding this comment.
The whole function apple_flags - which was previously ios_watchos_flags was not called from macos target before. I'd like to keep the previous behavior for macos target in this PR. If you'd like to add this to macos too, I will remove it.
src/lib.rs
Outdated
| @@ -2103,12 +2111,23 @@ impl Build { | |||
| None => false, | |||
| }; | |||
|
|
|||
| let is_sim = match target.split('-').nth(3) { | |||
| let is_arm_sim = match target.split('-').nth(3) { | |||
There was a problem hiding this comment.
This should still be named is_sim -- As it is, further down we have x86-specific logic in an if is_arm_sim branch.
youknowone
force-pushed
the
apple-clang
branch
from
221cf31
to
ba0a420
Compare
|
The first commit is #708 |
youknowone
force-pushed
the
apple-clang
branch
from
ba0a420
to
ca1ccd9
Compare
|
@thomcc rebase done, could you review it again? |
src/lib.rs
Outdated
| /// Whether the tool is AppleClang under .xctoolchain | ||
| #[cfg(target_vendor = "apple")] | ||
| fn is_xctoolchain_clang(&self) -> bool { | ||
| let path = self.path.to_str().unwrap(); |
There was a problem hiding this comment.
This has bitten us before, it should probably be to_string_lossy.
There was a problem hiding this comment.
Thank you, I fixed it.
youknowone
force-pushed
the
apple-clang
branch
from
ca1ccd9
to
8fb6eb9
Compare
The first commit is #708 but unseparatable.
Fix AppleClang build for -apple-darwin to add -isysroot for MacOSX.sdk correctly when cc is under xctoolchain.
Becasue
-isysrootpriors to$SDKROOTenv variable, the original ad-hoc fix is not required anymore.