[CMake] Replace SWIFT_SDKS with SWIFT_CROSS_COMPILE_DEPLOYMENT_TARGETS #2835
Conversation
karwa
force-pushed
the
host-triple
branch
3 times, most recently
from
f0d4474 to
3ab8c22
Compare
| # FreeBSD | ||
| freebsd-x86_64) | ||
| #FIXME: Set a meaningful FreeBSD deployment version | ||
| FREEBSD_DEPLOYMENT_VERSION="10.3" |
There was a problem hiding this comment.
This replaces the FIXME in CMake, about FreeBSD using the local CMAKE_SYSTEM_VERSION to derive a deployment target. It should have its own parameter, like Darwin targets do, and that would solve the cross-compiling issue.
|
Sorry, could you explain where you are hoping to arrive with this? Why do we need to make triples the primary identifier of the platform? You are saying that we don't need to understand them, but even trying to tell apart iOS device from iOS simulator turns into a mess ( |
|
Triples are only the identifier for configuration purposes. The reason is:
You'll notice that those triples only exist once now (in the build script) where before they were calculated twice. That's another benefit. This only affects configuration at the CMake level. There is no change to the outward parameters of the build script (that's still tuples, and it can stay like that. Nobody likes triples). The reason the |
Sorry, I don't think this is entirely true. We pass triples to the compiler from
Sure, but I'd rather translate an SDK/arch representation into triples, than trying to derive something from triples -- even from a fixed set of them; you never know when the set will grow.
Absolutely agreed. But we can do that using SDK/arch pairs just as well. Please feel free to choose your own mangling for this internal interface between build-script and CMake -- I'd prefer one that people don't already have expectations about.
Sorry, this does not follow for me. If using triples requires having regexes in CMake (which I don't see how we would move into testable Python code), then in my opinion it is better to have a little duplication, but keep things more maintainable. |
karwa
changed the title
|
Doesn't configure just yet - I think I've forgotten to quote a CMake variable somewhere. Looking for general feedback/help with the CMake issue (OSX). To explain the rationale: The next step to fully allowing targets to be cross-compiled would be to amend the prefix, so that the places which iterated over |
karwa
force-pushed
the
host-triple
branch
6 times, most recently
from
66bf784 to
9805740
Compare
|
@gribozavr Okay I've tested this on OSX and the configuration seems to work. Can you please trigger a test to check the functionality? EDIT: Oh, just noticed an issue with the buildbot |
karwa
force-pushed
the
host-triple
branch
7 times, most recently
from
15d1f7e to
72784d7
Compare
|
@gribozavr OK, I'm happy with it now. I changed the external |
karwa
changed the title
|
Rebased with the recent build-script changes, which needs to be amended itself. @ddunbar, you might want to check it over. I'd like to get this in as part of a bigger change to use architecture-specific prefixes for CMake SDKs (which would allow, for example, a Linux host to cross-compile a Linux stdlib, something not possible today). It's now also possible to configure and build specific iOS slices from the command line, because they're not hardcoded: Will now configure for the iOS SDK only for |
| "A list of deployment-targets to cross-compile the standard-library for (by default, the standard library is always compiled for the host)") | ||
|
|
||
| set(SWIFT_SKIP_HOST_STDLIB "" CACHE STRING | ||
| "If True, will not build the standard library for the host") |
There was a problem hiding this comment.
This doesn't actually work right now (due to other failures in other parts of the project; assuming SWIFT_HOST_VARIANT_SDK exists or whatnot), but a couple of parts of the old CMakeLists (which was hardcoded so this would never happen) indicated that it was supported at some point, or would at least be desirable in the future.
Specifically, SWIFT_PRIMARY_VARIANT gets set to Android when not building the stdlib for the Linux host, and swift-reflection-dump is gated by checking if we're building for the host's stdlib.
There was a problem hiding this comment.
Is this note still true? There's not much harm in adding a CMake option that doesn't quite work right now, but I think a better approach would be to only add ones that already work as intended.
Would it be possible to either:
- Fix the issues you describe first, before merging this pull request?
- Or add this flag in a later pull request? It seems to be used in five locations, so maybe it's too much trouble.
There was a problem hiding this comment.
Maybe I didn't describe it well, but what I meant to say is that (currently, before merging this PR) we always build for the host even if you don't include it in SWIFT_SDKS. So it doesn't actually work today (SWIFT_PRIMARY_VARIANT will never be Android).
While doing this patch, I removed the part that always builds for the host and found that there were other places which do assume the host is always being built. Maybe there are some other combination of settings which make it work (not building those failing parts)? I don't know; so I added the flag. It's not necessary though.
There was a problem hiding this comment.
Oh, I misunderstood. But yes, I think I encountered failures similar to the ones you mention here when I was working on #3027 -- and those were just the assumptions that the host stdlib was being built.
In any case, sounds good to me! Sorry for the noise.
karwa
force-pushed
the
host-triple
branch
2 times, most recently
from
01fc571 to
0957e47
Compare
| get_sdk_from_stdlib_target("${stdlib_target}" target_sdk) | ||
| get_arch_from_stdlib_target("${stdlib_target}" target_arch) | ||
|
|
||
| if(NOT "${target_arch}" IN_LIST ${target_sdk}_VALID_ARCHITECTURES) |
There was a problem hiding this comment.
Looks like IN_LIST is new in CMake 3.3. (https://cmake.org/cmake/help/v3.3/policy/CMP0057.html)
We support Ubuntu 14.04, which has CMake 2.8.12.
|
@gribozavr Replaced use of |
|
I have part 2 of this done (i.e. cross-compiling linux on linux), going stale somewhere and I'd like to get them merged. |
| else() | ||
| set("${result_var_name}" 1 PARENT_SCOPE) | ||
| endif() | ||
| endfunction() |
There was a problem hiding this comment.
nit-pick: This could use a new line at the end of the file.
There was a problem hiding this comment.
We've raised our minimum-supported CMake since I pushed this, so I think it can be replaced with a built-in function now anyway
There was a problem hiding this comment.
Nifty. For my own education, whats the builtin in newer cmake that does this?
There was a problem hiding this comment.
IN_LIST is new in CMake 3.3. (https://cmake.org/cmake/help/v3.3/policy/CMP0057.html)
|
This looks awesome. @gribozavr, @compnerd, do either of you have any thoughts? @swift-ci Please test |
|
Build failed |
|
Build failed |
|
Yikes. CI must not like me. Those failures don't look normal. @shahmishal, are those interesting to you? |
|
I'll update this patch soon. I'm hoping there's going to be a window after Swift 3 lands that we can make more significant progress on cross-compiling (defining a format for a toolchain "bundle" which includes the sysroot, native tools and build flags, which would be understood by both the build-script so you can build a compiler with it, and by the compiler itself so you can use it for cross-compiling swift applications). This is a prereq for those changes. We need to be able to build for an arbitrary target not just an entire platform. |
| set(triple "${arch}-unknown-linux-gnu") | ||
| elseif("${arch}" MATCHES "(armv6|armv7)") | ||
| set(triple "${arch}-unknown-linux-gnueabihf") | ||
| endif() |
There was a problem hiding this comment.
What if you want to use a compiler from a specific vendor? Lets take a concrete example: armv7-linaro-linux-gnueabihf? I don't see why we couldn't support that.
There was a problem hiding this comment.
Sure - I think we actually have a vendor flag in the build-script, so we can pass that along and substitute it in.
IIRC, it's important that this exactly matches the LLVM_HOST_TRIPLE from the build-script. It's been a long time since I tested it, but I think that even vendor mismatches cause the tools to not want to co-operate.
|
Ping? Let's get this rebased and merge it in if there are no further comments. |
|
I definitely want @erg to pick up where @gribozavr left off, and make sure he understands and approves of the changes here. It's possible this will affect our internal process, and if so we need to know how to update that. |
|
OK, I'll have a go at rebasing this. |
|
It's been a year and this patch has fallen behind and been split and spread across multiple other patches. I'm going to close this since priorities for smaller patches that come out of this seem clear. If that feeling is incorrect, please feel free to rebase this and reopen it. |
What's in this pull request?
Change build-script(-impl) -> CMake configuration arguments to use "deployment targets" instead of SDK names.
Deployment targets are made up of hyphen-separated lists; in the format
{sdk name}-{architecture}, so they're the same things the build-script uses internally, save for the Apple platforms having slightly different SDK names.Since we extract the components, I'm implicitly leaving it open for us to add options to the deployment target specifier (e.g. Soft/hard float ABI) by appending them. So you might see a
linux-arm-hardfloatdeployment target in the future.We extract the SDK name 'blindly', it's validated in the big configuration
if()...else()onSWIFT_HOST_VARIANT_SDK.Architecture is also extracted 'blindly' (sometimes there are small variations, e.g.
ppc64leorpowerpc64le, but the build-script handles this and always gives us valid architectures). Validation happens inget_triple()for all SDKs except the Apple ones, becauseconfigure_sdk_darwintakes a list of architectures and calculates the triples itself.This is part 1/2 when it comes to replacing SDKs with more specific deployment targets (to allow cross-compiling linux on linux, for example). One small win already with this change is that you can configure and build single slices of the standard library for Apple platforms (since the configured architecture list is no longer hardcoded to include them all).
Resolved bug number: (SR-)
Before merging this pull request to apple/swift repository:
Triggering Swift CI
The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:
Smoke Testing
Validation Testing
Lint Testing
Note: Only members of the Apple organization can trigger swift-ci.