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.

Sign up for GitHub

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

Jump to bottom

Improve cross compiling support #158

Merged
merged 3 commits into from
Oct 28, 2022
Merged

Improve cross compiling support #158

merged 3 commits into from
Oct 28, 2022

Conversation

messense
Copy link
Contributor

@messense messense commented Jul 26, 2022 •
edited
Loading

This PR changes cmake-rs to fill out CMAKE_SYSTEM_NAME and CMAKE_SYSTEM_PROCESSOR based CARGO_CFG_TARGET_OS and CARGO_CFG_TARGET_ARCH when cross compiling while CMAKE_TOOLCHAIN_FILE and CMAKE_SYSTEM_NAME isn't set by user.

It's tested on CI using cross.

Closes #75
Closes #80
Closes #151

@messense messense marked this pull request as ready for review July 26, 2022 12:10
@messense messense mentioned this pull request Jul 26, 2022
Closed
messense
messense commented Jul 26, 2022
View reviewed changes
src/lib.rs Outdated
}
} else if target.contains("solaris") {
if !self.defined("CMAKE_SYSTEM_NAME") {
self.define("CMAKE_SYSTEM_NAME", "SunOS");
Copy link
Contributor Author

@messense messense Jul 26, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can also address #151 by removing this if it's desirable.

cc @gco

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, please!

This was referenced Jul 27, 2022
Closed
Closed
@thomwiggers thomwiggers mentioned this pull request Aug 18, 2022
Open
@messense messense mentioned this pull request Aug 30, 2022
Closed
@messense messense mentioned this pull request Sep 14, 2022
Merged
thomcc
thomcc approved these changes Oct 28, 2022
View reviewed changes
Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks great. Thanks for improving the CI too.

@thomcc thomcc merged commit cfe11fc into rust-lang:master Oct 28, 2022
@pnkfelix pnkfelix mentioned this pull request Nov 11, 2022
Closed
@sburton84 sburton84 mentioned this pull request Nov 29, 2022
Open
@messense messense mentioned this pull request Dec 13, 2022
Merged
@luser luser mentioned this pull request Jan 27, 2023
Closed
luser added a commit to luser/cmake-rs that referenced this pull request Jan 27, 2023
@luser
Revert rust-lang#110 to fix iOS cross-compilation after rust-lang#158. …
f658691 
…Fixes rust-lang#173

In rust-lang#158, better support for using CMake's cross-compilation facilities was
added. This made the workaround added in rust-lang#110 for iOS not only unnecessary,
but actively harmful, in that it runs afoul of SDK validation checks in the
CMake iOS codepath.
@luser luser mentioned this pull request Jan 27, 2023
Merged
thomcc added a commit that referenced this pull request Mar 23, 2023
@luser @thomcc @JohnTitor
Revert #110 to fix iOS cross-compilation after #158. Fixes #173 (#176)
9e1614b 
* add an iOS cross-compile test to CI

* Fix zlib-ng compilation for iOS

* Revert #110 to fix iOS cross-compilation after #158. Fixes #173

In #158, better support for using CMake's cross-compilation facilities was
added. This made the workaround added in #110 for iOS not only unnecessary,
but actively harmful, in that it runs afoul of SDK validation checks in the
CMake iOS codepath.

* Use checkout@v3

Co-authored-by: Yuki Okushi <jtitor@2k36.org>

---------

Co-authored-by: Thom Chiovoloni <thom@shift.click>
Co-authored-by: Yuki Okushi <jtitor@2k36.org>
@jrose-signal
Copy link

jrose-signal commented May 11, 2023 •
edited
Loading

I'm afraid this broke no-toolchain cross-compilation using Visual Studio, from an x86_64 Windows host to aarch64-pc-windows-msvc. Things are a bit clunky there in general (the existing support goes through platforms and does not set CMAKE_SYSTEM_PROCESSOR), but it was working in 1.48.0 (and I'm afraid we hadn't been testing with new crate versions, so we didn't discover this sooner).

(Specifically, the auto-search for CMAKE_C_COMPILER and CMAKE_CXX_COMPILER doesn't kick in, even though it works fine for a non-cross-compile MSVS generator build.)

@jrose-signal
Copy link

Workaround: set CMAKE_SYSTEM_NAME to an empty string, which counts as defined for this crate, but unset for CMake itself.

@jrose-signal
Copy link

jrose-signal commented May 31, 2023 •
edited
Loading

Turns out this also broke cross-compiling to Mac Catalyst targets (*-apple-ios-macabi), because CMake doesn't like having CMAKE_SYSTEM_NAME set to "iOS" while compiling against a macOS sysroot. That should probably eventually be fixed in CMake itself, but for now setting CMAKE_SYSTEM_NAME back to "Darwin" does work, at least for simple libraries. I don't know if you want to put that in this crate, though.

EDIT (months later): here's CMake not deciding what to do about Catalyst, which isn't cmake-rs's fault.

@jrose-signal jrose-signal mentioned this pull request Jun 29, 2023
Open
@jrose-signal jrose-signal mentioned this pull request Nov 13, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gco gco gco left review comments

@thomcc thomcc thomcc approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
4 participants
@messense @jrose-signal @gco @thomcc