-
Notifications
You must be signed in to change notification settings - Fork 10.4k
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
[build] Add the flags to enable cross-compiling the corelibs #33724
Conversation
Just looking through and beginning to understand this patch. I am hesitant in general to add support to build-script-impl. Especially the cross-compiled part of build-script-impl which is the least well tested out. I need to look/think about this in more detail. |
utils/build-script-impl
Outdated
@@ -2217,6 +2282,9 @@ for host in "${ALL_HOSTS[@]}"; do | |||
-DENABLE_TESTING=YES | |||
-DBUILD_SHARED_LIBS=$([[ ${product} == libdispatch_static ]] && echo "NO" || echo "YES") | |||
) | |||
if [[ $(is_cross_tools_host ${host}) ]] ; then | |||
add_cross_cmake_options ${host} cmake_options |
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.
This is the first time that we are using CMake cross compilation under the hood in build-script-impl. I wonder if it would make sense to always cross compile everything in build-script-impl. I don't know how much work that would entail, but it is work that would need to be done. Especially since @shahmishal is going to need to fix the cross compilation loop for build-script products.
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.
What do you mean by "cross-compile everything," explicitly pass in all these compiler and sysroot flags when natively compiling too? That might make sense, but as you said, would be a fair amount of work to track them all down. Of course, the benefit is that every natively supported platform becomes easily cross-compilable from other platforms.
As for fixing "the cross compilation loop for build-script products," I have a pull forthcoming with the tweaks I had to make to build-script when cross-compiling SPM, as that's a build-script product not built here in build-script-impl.
Thinking about this some more, I have a pull in the works to cross-compile SPM too, which is built by build-script, and I could translate these two new bash functions Of course, the remaining parts of this pull would have to stay in this bash script, as that's the only way to configure the corelibs. |
7ee1c4d
to
ddd44d6
Compare
Alright, made the changes discussed above, all split off into a separate commit so it's easy to see what changed. Applying this pull and the related pulls #34491 and #34628, which are not strictly necessary for this one but allow me to use the unified sysroot here and elide one flag, I was able to build a functional Android SDK with the Nov. 26 source snapshot and the official prebuilt trunk snapshot of the linux toolchain, by running this command:
I was then able to cross-compile Swift packages like
by passing it in to the linux SPM like so:
The directory While these commands produce a working SDK, further @drodriguez, once this is in, you could expand the Android CI to build the corelibs and some Swift packages if you like, rather than only cross-compiling the stdlib as it does now. |
CMakeLists.txt
Outdated
@@ -435,7 +435,7 @@ option(SWIFT_BUILD_ONLY_SYNTAXPARSERLIB "Only build the Swift Syntax Parser libr | |||
option(SWIFT_BUILD_SOURCEKIT "Build SourceKit" TRUE) | |||
option(SWIFT_ENABLE_SOURCEKIT_TESTS "Enable running SourceKit tests" ${SWIFT_BUILD_SOURCEKIT}) | |||
|
|||
if(SWIFT_BUILD_SYNTAXPARSERLIB OR SWIFT_BUILD_SOURCEKIT) | |||
if(SWIFT_INCLUDE_TOOLS AND (SWIFT_BUILD_SYNTAXPARSERLIB OR SWIFT_BUILD_SOURCEKIT)) |
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.
This libdispatch build is only needed for these host tools and I got sick of the standalone stdlib unnecessarily building it, so I disabled it in that scenario.
@@ -795,6 +795,7 @@ class BuildScriptInvocation(object): | |||
""" | |||
|
|||
args = self.args | |||
args.build_root = self.workspace.build_root |
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.
I need this later in the new swift_flags()
method.
@@ -222,6 +221,7 @@ KNOWN_SETTINGS=( | |||
cross-compile-hosts "" "space-separated list of targets to cross-compile host Swift tools for" | |||
cross-compile-with-host-tools "" "set to use the clang we build for the host to then build the cross-compile hosts" | |||
cross-compile-install-prefixes "" "semicolon-separated list of install prefixes to use for the cross-compiled hosts. The list expands, so if there are more cross-compile hosts than prefixes, unmatched hosts use the last prefix in the list" | |||
cross-compile-deps-path "" "path for CMake to look for cross-compiled library dependencies, such as libXML2" |
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.
I thought about making this a list like cross-compile-hosts
but realistically only one cross-compilation host is ever passed in, so this will do for now. The issue is that each cross-compilation host will require its own separate C/C++ cross-compilation toolchain, which there's no way to pass in to build-script
yet. We're able to handle a single cross-compilation host with hacked-in flags like --android-ndk
and --native-clang-tools-path
, but for example, we can't even build multiple Android architectures with a single build-script
invocation yet.
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.
@buttaface I think the last sentence is incorrect. I imagine that if one hacked in something similar to what we have on Darwin in the stdlib cmake (which is horrible btw and I am not suggesting this).
That being said, shouldn't this just be found in some notion of an SDK? I think that is how on Darwin this would work.
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.
The reason I said "we can't even build multiple Android architectures with a single build-script invocation yet" in my last sentence is that, for example, we pass in the path to a particular libicu built for one arch when invoking build-script
. Even on Darwin, only compiling the Swift compiler itself for macOS x86_64 was supported until recently, when support for cross-compiling it from macOS x86_64 to arm64 was added but not the other way, swiftlang/swift-package-manager#3500, but again only a single cross-compilation host is supported on Darwin. Of course, one could hack in further changes to support multiple hosts, but it is currently not supported.
As for this just being an SDK, it is more general than that. I use this flag below to find dependencies like libxml2 or libsqlite3, which are additional package dependencies that aren't usually included in cross-compilation SDKs. I found this useful when cross-compiling the toolchain for Android, as I pass in the Termux prefix with those prebuilt library packages installed to this flag and the separate Android NDK, that has the libc sysroot but no libraries like libxml2, to the --android-ndk
flag above.
This gives the most flexibility because if they are all in one SDK, you can always pass the same SDK path in to both flags.
if deployment_target_name != args.host_target and \ | ||
host_target != args.host_target: | ||
self.swift_flags = deployment_target.swift_flags(args) | ||
self.cmake_options = deployment_target.cmake_options(args) |
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.
Note that I didn't name these Python functions with cross
or anything else related to cross-compilation like I had the bash functions in the first commit, so that if you ever want to use them for native compilation too, just remove these two checks above and go.
Added a last two-line commit that provides a build-script-impl flag to pass in Swift flags to the corelibs. Saleem pointed out that adding For example, I mentioned earlier needing to @compnerd, let me know if this addresses your concerns, as |
@CodaFi, could you take a look at this pull? I'd like to get it in before the 5.4 branch, but I think Michael is busy with his ownership work. |
@varungandhi-apple, mind running the CI on this? I just talked to @gottesmm and it looks like we can move forward on this, as I'd like to get it into the 5.5 branch. |
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.
This is almost there. Please respond to my comments. I need to think about a few things with this. But I think it is the right direction and has a very light touch overall.
options = '' | ||
if self.platform.name == 'android': | ||
options = '-DCMAKE_SYSTEM_NAME=Android ' | ||
options += '-DCMAKE_SYSTEM_VERSION=%s ' % (args.android_api_level) |
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.
Can you use the CMakeOptions() thing? The reason why is that I am going to be changing that to autogenerate Toolchain files and then I can just take what you already wrote here and it will just work.
(The plan is to make it so that we are always cross compiling with a toolchain).
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.
Also this is platform specific so I wonder if we can refactor this out onto the Android platform.
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.
I don't think so as CMakeOptions() is used to pass CMake flags in this Python build-script
, whereas this is a string passed to the old bash build-script-impl
. I suppose I could use CMakeOptions() to define these Android flags, then turn that Python list into a string again before passing it to build-script-impl
, but we could always do that later once you move everything to CMake toolchain files. Just let me know what you prefer.
As for refactoring it above, will do.
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.
I was thinking about this... I am fine with leaving it like this for now. But the refactoring I would like.
@@ -152,6 +152,34 @@ def __init__(self, platform, arch): | |||
def name(self): | |||
return "{}-{}".format(self.platform.name, self.arch) | |||
|
|||
def swift_flags(self, args, resource_dir_root=""): | |||
flags = '' | |||
if self.platform.name == 'android': |
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.
If this is specific to the android platform, can you add this as a helper on that class?
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.
Sure.
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.
I changed these to methods on AndroidPlatform
and ran the build again: only after it errored did I remember why I added these methods to this Target
class instead, ie Target
has an arch whereas Platform
just has a list of possible arches. Since these flags are generated for a single architecture, I need to keep them here.
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.
is it possible to pass this in to a method on platform... IDK. My worry here is that this will lead us to a build-script-impl world where we have a large if else depending on what we are building for?
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.
Yes, but I would have to pass in the selected arch as a parameter to swift_flags()
. I have that refactor patch lying around, so I will add the arch part and push it, then you can decide if it's what you want.
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.
Made this change in latest commit.
@@ -222,6 +221,7 @@ KNOWN_SETTINGS=( | |||
cross-compile-hosts "" "space-separated list of targets to cross-compile host Swift tools for" | |||
cross-compile-with-host-tools "" "set to use the clang we build for the host to then build the cross-compile hosts" | |||
cross-compile-install-prefixes "" "semicolon-separated list of install prefixes to use for the cross-compiled hosts. The list expands, so if there are more cross-compile hosts than prefixes, unmatched hosts use the last prefix in the list" | |||
cross-compile-deps-path "" "path for CMake to look for cross-compiled library dependencies, such as libXML2" |
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.
@buttaface I think the last sentence is incorrect. I imagine that if one hacked in something similar to what we have on Darwin in the stdlib cmake (which is horrible btw and I am not suggesting this).
That being said, shouldn't this just be found in some notion of an SDK? I think that is how on Darwin this would work.
utils/build-script-impl
Outdated
) | ||
;; | ||
esac | ||
echo -n "${SWIFT_FLAGS[@]} -module-cache-path \"${module_cache}\" " |
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.
I can't remember if module_cache is expanded out or not. If it must be defined for this to work, please add an assert to make sure it is defined.
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.
It will always be defined, because it is always set in the build loop for all products. I suppose now that I moved this to a function someone may later call it outside that build loop, say in the testing loop, though it is only called in the build loop now. If you're still worried about that, I can pass it in as a parameter to this function or add an assert.
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.
just the assert would be useful. That way if someone messes up we get a nice error msg.
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.
Done.
# --cross-compile-hosts. | ||
if deployment_target_name != args.host_target and \ | ||
host_target != args.host_target: | ||
self.swift_flags = deployment_target.swift_flags(args) |
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.
I really dislike how this mutates global state in self outside of self. I would feel more comfortable if there was an explicit method on self that takes in the deployment_target and sets flags. Something like that.
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.
Not sure what you mean, as self
in this case is HostSpecificConfiguration
for which these lines I'm adding are in the initializer for that class. This is in keeping with how this initializer configures other variables like self.sdks_to_configure
above.
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.
I meant outside of the class. It seems like one could make a separate method that does this on self that way it is self contained and given a nice comment explaining that we are changing the deployment target or updating the deployment target.
Another thing that would be good is if one adds a few build system unit tests for build-script-impl to make sure things keep working as you expect. |
@swift-ci test |
Build failed |
Single Mac test failure is unrelated, and something unrelated appears broken on the Win CI. |
@swift-ci test macos |
@swift-ci test windows |
Build failed |
Win CI passed, same test failing on Mac CI again. Looks like the Mac CI is broken right now, as this completely unrelated pull also shows the same test failing. |
@drexin, Mac CI is fixed, would you run it for this pull again? |
@swift-ci test macos |
Build failed |
Sigh, macOS CI is broken again, latest Mac CI run for this pull fails for the same tests as this unrelated pull. |
@swift-ci test macOS platform |
Gah, this also failed the Python lint that is only run on the Mac CI, I will fix and push again. |
Build failed |
Pass the Swift and CMake flags needed to cross-compile Foundation and so on, with the first example of Android. Add a new flag, --cross-compile-deps-path, which is used to search for cross-compiled libraries, like libcurl, that the corelibs depend on. Also add a new flag, --common-swift-flags, to pass additional Swift flags to the corelibs.
@swift-ci test |
Build failed |
Some unrelated issue with SwiftSyntax on the Mac CI, hit this completely different pull too. |
@drexin, looks like the Mac CI is running again, would you trigger that one again? |
@swift-ci test macos |
Passed CI, ready for merge. |
https://ci.swift.org/job/oss-swift-incremental-RA-macos-apple-silicon/966/console @buttaface We are seeing this failure on Apple Silicon, is it ok to disable this test for macOS platforms? |
@shahmishal, I will submit a pull to disable it for macos-arm64 in 15 min, as I don't know what's going wrong there. |
Pass the Swift and CMake flags needed to cross-compile Foundation and so on, with the first example of Android AArch64. Add a new flag,
--cross-compile-deps-path
, which is used to search for cross-compiled libraries, like libcurl, that the corelibs depend on.This would be useful both for building cross-compilation SDKs and building native SDKs faster for some slow platforms, like the Raspberry Pi.
I've been using an earlier version of this patch for the last couple months to cross-compile a native Android toolchain from linux x86_64, termux/termux-packages#5352. After unpacking the Aug. 18 trunk snapshot of the Swift toolchain for Ubuntu 20.04 and the Termux Android binary packages for libcurl and so on into
/data/data/com.termux/files/usr
, I ran the following command to compile the Swift stdlib and corelibs both natively for linux x86_64 and for Android AArch64 (without this pull applied, the build-script will simply build the corelibs for the linux host twice):The last three redundant clang paths were needed because otherwise two different clangs could be passed to CMake when cross-compiling the corelibs, which confuses and breaks the CMake configuration. This invocation makes sure only one clang is used. The problem is that
--host-cc
is a Pythonbuild-script
flag while--native-clang-tools-path
is a bashbuild-script-impl
flag: I'll look into pulling them all into the Pythonbuild-script
later.I tested the above Android SDK with Foundation and another corelibs example by cross-compiling these small executables like this:
After transferring these executables to an Android AArch64 device and running them with LD_LIBRARY_PATH set to a local copy of the corelibs, everything ran fine. I've also been using these cross-compiled corelibs to cross-compile the Swift package manager in a separate patch and that's been working well, so it works for larger builds too.
@shahmishal, this may be useful to cross-compile for the new Apple Silicon too, at least for those Swift projects that can be built on macOS at all.