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

boring-sys: Don't use CMake cross-compilation for macOS->iOS #187

Merged
merged 1 commit into from
Nov 30, 2023

Conversation

jrose-signal
Copy link
Contributor

(or macOS->macOS)

Fixes the iOS build.

@nox
Copy link
Collaborator

nox commented Nov 17, 2023

Thanks for the PR. Could you explain what it fixes? Maybe show an example of a broken build with the current code?

@jrose-signal
Copy link
Contributor Author

You can see one example in this log after fixing the CI to test cross-compilation more thoroughly (#188). In this case the assembly sources end up getting built for iOS devices rather than iOS simulators, presumably because CMake assumes you'll set all of the relevant cross-compilation properties if you're going to use any of them. But for toolchains that natively support multiple targets, this just makes things worse.

(Honestly, I don't think this sort of policy should be happening here at all; it should be in the cmake crate. But that has similar problems, and since the cmake crate isn't being actively developed I understand doing things here!)

@jrose-signal
Copy link
Contributor Author

Citation from the CMake docs themselves:

Another case to be aware of is that builds targeting Apple platforms other than macOS are handled differently to other cross compiling scenarios. Rather than relying on CMAKE_SYSTEM_NAME to select the target platform, Apple device builds use CMAKE_OSX_SYSROOT to select the appropriate SDK, which indirectly determines the target platform. […] Therefore, the use of CMAKE_CROSSCOMPILING is not recommended for projects targeting Apple devices.

@nox
Copy link
Collaborator

nox commented Nov 22, 2023

Sorry I had other yaks to shave these last few days but I'll review these tomorrow and so far I don't see a reason not to merge those changes.

@jrose-signal
Copy link
Contributor Author

No worries, that was less a "hey, pay attention to me" and more "I found a citation to support this, let me put in the PR for posterity"!

@nox
Copy link
Collaborator

nox commented Nov 23, 2023

I fixed the failing clippy lint in a separate PR so you can just rebase yours for CI to go green.

@nox nox merged commit af0c36a into cloudflare:master Nov 30, 2023
23 checks passed
@jrose-signal jrose-signal deleted the fix-ios-build branch November 30, 2023 17:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants