-
Notifications
You must be signed in to change notification settings - Fork 131
Fix cross compilation for non-macOS apple targets #244
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
base: master
Are you sure you want to change the base?
Conversation
Define CMAKE_OSX_ARCHITECTURES and CMAKE_OSX_SYSROOT automatically for non-macOS apple targets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments, especially the part with CMAKE_OSX_SYSROOT
may break users that set SDKROOT
themselves.
But I think cross compilation was broken before anyhow, so I've approved the PR in any case, it's probably an improvement regardless of what we do.
} else if target.contains("aarch64") { | ||
cmd.arg("-DCMAKE_OSX_ARCHITECTURES=arm64"); | ||
} else if target.contains("i686") { | ||
cmd.arg("-DCMAKE_OSX_ARCHITECTURES=i686"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ld64
prefers i386
, so we should probably pass that here as well.
cmd.arg("-DCMAKE_OSX_ARCHITECTURES=i686"); | |
cmd.arg("-DCMAKE_OSX_ARCHITECTURES=i386"); |
} else { | ||
panic!("unsupported darwin target: {}", target); | ||
} else if target.contains("apple") { | ||
if !self.defined("CMAKE_OSX_ARCHITECTURES") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Link to the docs. Same for CMAKE_OSX_SYSROOT
.
if !self.defined("CMAKE_OSX_ARCHITECTURES") { | |
// <https://cmake.org/cmake/help/latest/variable/CMAKE_OSX_ARCHITECTURES.html> | |
if !self.defined("CMAKE_OSX_ARCHITECTURES") { |
} | ||
} | ||
|
||
if !self.defined("CMAKE_OSX_SYSROOT") { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading the docs for this, it seems to me that we can (and should) use SDKROOT instead (to allow the user to override with that env var set). And cc-rs
already contains the logic to invoke xcrun
at the appropriate times.
So maybe the solution here is to instead make cc-rs
set the SDKROOT
env var here instead of passing -isysroot
? That would automatically set SDKROOT
for CMake invocations as well.
} else if target.contains("arm64_32") { | ||
cmd.arg("-DCMAKE_OSX_ARCHITECTURES=arm64_32"); | ||
} else { | ||
panic!("unsupported apple target: {target}"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: Maybe we can make this simpler, and remove the panic? See bootstrap's implementation:
https://github.com/rust-lang/rust/blob/718ddf660e6a1802c39b4962cf7eaa4db57025ef/src/bootstrap/src/core/build_steps/llvm.rs#L698-L706
Define CMAKE_OSX_ARCHITECTURES and CMAKE_OSX_SYSROOT automatically for non-macOS apple targets
Picks up where #197 leaves off