Fix cross-compiling ios targets with cmake 3.14 #88

kylefleming · 2019-10-01T19:28:36Z

Summary

This PR fixes iOS cross-compilation support when using Cmake 3.14+. The issue is described in more detail here: #87

Implementation

This PR checks the target triplet (either the target member variable if it's set, otherwise the TARGET environment variable) and looks for an ending of -apple-ios to determine if we're cross-compiling for iOS or not.

If we are cross-compiling for iOS, it disables the default compiler flags for the cc-rs build config, and proceeds to set the required flags to allow cmake itself to set them:

If CMAKE_SYSTEM_NAME is not set, set it to iOS.
If CMAKE_OSX_ARCHITECTURES is not set, set it to the cmake equivalent of the target architecture pulled from the target variable. (cmake equivalents are: arm64, armv7, armv7s, i386, or x86_64)
If CMAKE_OSX_DEPLOYMENT_TARGET is not set, we look for the IPHONEOS_DEPLOYMENT_TARGET environment variable (which is what cc-rs, llvm, rustc, etc use). If that doesn't exist, we default to 7.0, which is what cc-rs defaults to (note that cmake defaults to 9.0, but in my opinion, it's better to have consistency across the whole project than to have vertical consistency within a particular tool).
If CMAKE_OSX_SYSROOT isn't set, set it to either iphoneos or iphonesimulator to match the (canonical, version-less) sdk name xcode uses (xcodebuild -showsdks). This could also be set to a path to an SDK, but by setting it to the name of an SDK, cmake automatically selects the path to the most recent version it can find for that sdk.

I set the variables up so that it only sets them if they weren't previously defined, which should allow users to override them however they want.

-fPIC and -fembed-bitcode are added, as those were previously added by cc-rs (which we disabled, as noted above) but are not currently added by cmake. (I couldn't find documentation stating whether -fPIC is necessary, but I've left it in for continuity)

(I only have minimal rust experience at the moment, so if there are any conventions you'd like me to follow that I might not know about, feel free to suggest.)

Questions

Cmake <3.14

One caveat is that I've only tested this with cmake 3.15. I'm assuming it works with cmake 3.14 since the code in question within cmake hasn't changed. However, I don't know how it handles cmake <3.14.

Prior to cmake 3.14, I believe the procedure was to set CMAKE_OSX_SYSROOT (same as above) and CMAKE_OSX_SYSROOT (same as above). CMAKE_SYSTEM_NAME will be set to Darwin automatically within cmake.

I'm not sure 100% how cmake <3.14 handled CMAKE_OSX_DEPLOYMENT_TARGET/IPHONEOS_DEPLOYMENT_TARGET so I can't speak to that.

Do you want cmake-rs to support versions <3.14? If so we will need a way to determine which cmake version is installed. An easier middle option would be to have the above variables set only for 3.14+ and do nothing for versions <3.14, which in theory might facilitate backwards compatibility with existing projects using cmake-rs.

I don't currently have a good understanding of the needs of cmake-rs and the community using it, so I leave that up to your discretion. How do you want to handle introducing this feature?

Config variables

Do you want the deploymentTarget to be a Config variable? Practically speaking, it can currently be set by defining the cmake variable CMAKE_OSX_DEPLOYMENT_TARGET. (Although, I imagine deployment target might be a configuration option that other targets might have, which is why I mention deploymentTarget instead of osxDeploymentTarget or iosDeploymentTarget.)

This gets a bit hairy though because CMAKE_OSX_DEPLOYMENT_TARGET is also the variable for setting the macOS deployment target when building for a macOS target. Cmake automatically sets CMAKE_OSX_DEPLOYMENT_TARGET from MACOSX_DEPLOYMENT_TARGET when CMAKE_SYSTEM_NAME is set to Darwin but not when it's set to iOS.

My point is mainly just that a) if environment variables for the deployment target are namespaced per platform, it could suggest that if a cmake-rs Config parameter were to be added, it might want to do the same. And b) I could see a future where cmake adds support for CMAKE_OSX_DEPLOYMENT_TARGET defaulting to IPHONEOS_DEPLOYMENT_TARGET, in which case using environment variables as the canonical way to set the deployment target might be the way to go anyways, rather than a Config variable (with the added benefit that it takes cmake-rs out of the loop).

alexcrichton · 2019-10-03T14:59:09Z

src/lib.rs

@@ -383,7 +385,7 @@ impl Config {
        if let Some(ref generator) = self.generator {
            is_ninja = generator.to_string_lossy().contains("Ninja");
        }
-        if target.contains("windows-gnu") {
+        if target_triplet.contains("windows-gnu") {


If a Target type is being added here, perhaps these methods that all use contains could be moved to methods? Alternatively could the methods on Target just be used inline where they're used here? (it just seems weird to do half for some and half for others)

I think that would be a good direction for this codebase to move towards. I don't have the bandwidth to refactor it though. My thinking is basically that by providing the base structure for organization of targets, it can slowly move in that direction. However, let me know if you'd prefer I get rid of Target and inline everything instead.

alexcrichton · 2019-10-03T14:59:55Z

src/lib.rs

+        if parts.len() < 3 {
+            fail(&format!(
+                "target triplet not in the form arch-vendor-platform: {}",
+                target_triplet
+            ));
+        }


I think this may want to be a bit less strict for platforms like wasm32-wasi which could show up here

I've moved this check into an Apple-specific AppleTarget.

src/lib.rs

alexcrichton · 2019-10-03T15:00:57Z

src/lib.rs

+        &self.rust_target_vendor == "apple" && &self.rust_target_platform == "darwin"
+    }
+
+    fn is_cross_compiling(&self) -> bool {


This seems not quite right perhaps? This is where I think it might be better to inline the definition here above because is_cross_compiling doesn't only take into account iOS

I've removed the is_cross_compiling method.

alexcrichton · 2019-10-03T15:01:24Z

src/lib.rs

+        self.is_ios_target()
+    }
+
+    fn cmake_system_name(&self) -> Option<String> {


similar to above, this sort of seems too general to only handle iOS, so it may be best to inline the definition above where it's called?

kylefleming · 2019-10-04T03:01:54Z

I pushed a new revision. The biggest changes are:

I created an AppleTarget struct to hold the Apple-specific logic
Default compiler flags from cc-rs are no longer disabled. Instead it filters out the specific compiler flags that cmake will be providing (namely -m<platform>-min-version=<deployment_target>, -arch <arch>, and -isysroot <sysroot_path>). This alleviates some duplication logic between cmake-rs and cc-rs that was in my earlier revision.
macOS is now supported by fully specifying the OSX cmake variables when compiling for a macOS target. This is in contrast to letting LLVM choose defaults, which is how it is currently done (which works fine in practice, as long as you don't need to change from the defaults).
Environment variables with the potential to confuse LLVM are filtered out. (This includes IPHONEOS_DEPLOYMENT_TARGET, MACOSX_DEPLOYMENT_TARGET, SDKROOT, etc.)

kylefleming · 2019-10-09T01:06:23Z

It occurred to me that filter_compiler_args should probably only apply to compiler flags supplied by cc-rs and not to ones supplied by the user via the cflag/cxxflag/asmflag methods, so I've pushed a revision to account for that.

alexcrichton · 2019-10-21T14:20:03Z

Thanks for this, and sorry for taking awhile to get back to this!

This is getting to be a pretty significant PR, I would personally prefer to not pick up regex/lazy_static dependencies for example. If using cmake on iOS is so different from all other platforms should there perhaps be a separate crate for that?

simlay · 2020-02-24T06:49:55Z

This is getting to be a pretty significant PR, I would personally prefer to not pick up regex/lazy_static dependencies for example. If using cmake on iOS is so different from all other platforms should there perhaps be a separate crate for that?

I've ran into this issue across a number of different projects I've attempted to compile to iOS. I admit that it's a never ending battle to make things work cross platform but I think that doing it here will save a lot of leg work for other crates using cmake that want to support multiple platforms. That being said, I'm also not big on picking up regex/lazy_static as a dependency for this project. I'd be up to help remove it.

Anyway, it's been a while since this PR was commented on or updated. I just wanted to share my interest and offer to continue the work on it because it'll fix issues with rendy, shaderc and the examples in wgpu. I'm sure there are others but that's just what I was working on this evening.

kylefleming · 2020-02-25T04:12:15Z

@simlay Thanks for offering to continue working on it. Feel free to make whatever changes you see fit. To the best of my knowledge, all the logic in the PR is correct, but it needs refactoring to address @alexcrichton's comments.

aloucks · 2020-03-23T02:52:37Z

The regular expressions seem simple enough that you could just parse it by hand. Adding regex as a dependency doesn't seem like a good idea.

simlay · 2020-03-24T08:06:46Z

So, I'm not sure the best way to take over a PR via github but I've addressed the regex and lazy_static dependencies in simlay@7566053. I've got mixed feelings about it as the logic makes some assumptions on input.

Anyway, I'd like to move forward and hopefully get this PR merged at some point. Is there anything I can do to unblock this?

alexcrichton · 2020-03-24T14:12:58Z

It's fine to send a new PR, and when that one's merged I can close this one.

* Rename target variable to target_triple * Remove a bit of code duplication * Add GenericTarget with several override points * Add support for cross-compiling Apple targets with cmake 3.14 * Removed lazy_static and regex Co-authored-by: Kyle Fleming <kyle@kylefleming.net>

alexcrichton · 2020-03-31T18:17:28Z

Now landed in #93

…#93)" This reverts commit f43568c.

…st-lang#88 states (rust-lang#93)"" This reverts commit 9a53f43.

rust-lang#93) * Rename target variable to target_triple * Remove a bit of code duplication * Add GenericTarget with several override points * Add support for cross-compiling Apple targets with cmake 3.14 * Removed lazy_static and regex Co-authored-by: Kyle Fleming <kyle@kylefleming.net>

rust-lang#93) * Rename target variable to target_triple * Remove a bit of code duplication * Add GenericTarget with several override points * Add support for cross-compiling Apple targets with cmake 3.14 Co-authored-by: simlay <simlay@users.noreply.github.com> Co-authored-by: Kyle Fleming <kyle@kylefleming.net>

kylefleming force-pushed the cmake-ios branch 3 times, most recently from e7b8828 to e15260a Compare October 2, 2019 23:55

alexcrichton reviewed Oct 3, 2019

View reviewed changes

kylefleming added 2 commits October 3, 2019 12:46

Rename target variable to target_triple

22e926d

Remove a bit of code duplication

987f826

kylefleming force-pushed the cmake-ios branch 2 times, most recently from 8609afc to 00a2381 Compare October 4, 2019 02:38

kylefleming requested a review from alexcrichton October 4, 2019 02:51

kylefleming force-pushed the cmake-ios branch from 00a2381 to 6e994c7 Compare October 4, 2019 03:16

kylefleming added 2 commits October 8, 2019 18:02

Add GenericTarget with several override points

7986d2d

Add support for cross-compiling Apple targets with cmake 3.14

93ff3a1

kylefleming force-pushed the cmake-ios branch from 6e994c7 to 93ff3a1 Compare October 9, 2019 01:03

simlay mentioned this pull request Feb 24, 2020

Adding iOS runtime google/shaderc-rs#40

Closed

simlay mentioned this pull request Mar 24, 2020

Removed lazy_static and regex from cmake-ios branch kylefleming/cmake-rs#1

Closed

simlay mentioned this pull request Mar 24, 2020

Fix cross-compiling ios targets with cmake 3.14 as #88 states #93

Merged

alexcrichton closed this Mar 31, 2020

alexcrichton added a commit that referenced this pull request May 18, 2020

Revert "Fix cross-compiling ios targets with cmake 3.14 as #88 states (…

9a53f43

…#93)" This reverts commit f43568c.

Dushistov added a commit to Dushistov/cmake-rs that referenced this pull request Jun 3, 2020

Revert "Revert "Fix cross-compiling ios targets with cmake 3.14 as ru…

bd95a5d

…st-lang#88 states (rust-lang#93)"" This reverts commit 9a53f43.

Dushistov added a commit to Dushistov/cmake-rs that referenced this pull request Jun 9, 2020

Revert "Revert "Fix cross-compiling ios targets with cmake 3.14 as ru…

f931035

…st-lang#88 states (rust-lang#93)"" This reverts commit 9a53f43.

Fix cross-compiling ios targets with cmake 3.14 #88

Fix cross-compiling ios targets with cmake 3.14 #88

Uh oh!

Conversation

kylefleming commented Oct 1, 2019

Summary

Implementation

Questions

Cmake <3.14

Config variables

Uh oh!

alexcrichton Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

kylefleming Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

kylefleming Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

alexcrichton Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

kylefleming Oct 4, 2019

Choose a reason for hiding this comment

Uh oh!

alexcrichton Oct 3, 2019

Choose a reason for hiding this comment

Uh oh!

kylefleming commented Oct 4, 2019

Uh oh!

kylefleming commented Oct 9, 2019

Uh oh!

alexcrichton commented Oct 21, 2019

Uh oh!

simlay commented Feb 24, 2020

Uh oh!

kylefleming commented Feb 25, 2020

Uh oh!

aloucks commented Mar 23, 2020

Uh oh!

simlay commented Mar 24, 2020

Uh oh!

alexcrichton commented Mar 24, 2020

Uh oh!

alexcrichton commented Mar 31, 2020

Uh oh!

Uh oh!