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

[build] Add a new flag to disable cross-compiling the compiler #38441

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

finagolfin
Copy link
Member

@finagolfin finagolfin commented Jul 16, 2021

This is useful when building cross-compilation toolchains where you want the stdlib and corelibs cross-compiled but don't want the Swift compiler cross-compiled too with --cross-compile-hosts. This pull demonstrates using the new flag for the Android build presets.

I'm submitting this after discussing cross-compiling the corelibs on the Android CI with @drodriguez, as the easiest way to do so after #33724 is by using --cross-compile-hosts but we don't want to cross-compile the Swift compiler too.

To build this, I first had to unpack three Termux packages for Android, libandroid-spawn, libcurl, and libxml2 (I also unpacked libicu but the Android CI does that differently) as detailed here, because those libraries are dependencies of swift-corelibs-foundation.

I then passed that swift-android-aarch64-24-sdk path into this build-script invocation with the July 9 trunk source snapshot to build an Android cross-compilation toolchain that includes the corelibs:

./swift/utils/build-script -RA --android --android-ndk /home/butta/src/android-ndk-r21e/
--android-arch aarch64 --android-api-level 24
--android-icu-uc /home/butta/swift-android-aarch64-24-sdk/usr/lib/libicuuc.so
--android-icu-uc-include /home/butta/swift-android-aarch64-24-sdk/usr/include/
--android-icu-i18n /home/butta/swift-android-aarch64-24-sdk/usr/lib/libicui18n.so
--android-icu-i18n-include /home/butta/swift-android-aarch64-24-sdk/usr/include/
--android-icu-data /home/butta/swift-android-aarch64-24-sdk/usr/lib/libicudata.so
--xctest --cross-compile-hosts=android-aarch64 --install-swift --install-libdispatch
--install-foundation --install-xctest
--cross-compile-deps-path=/home/butta/swift-android-aarch64-24-sdk
--cross-compile-build-swift-tools=0 --llvm-ninja-targets-for-cross-compile-hosts=help

swift-corelibs-foundation recently added a dependency on some posix_spawn functions on Android, which as discussed on the forum isn't officially supported before Android API 28, so I linked against Termux's backported library with these additional patches:

diff --git a/swift/stdlib/public/Platform/CMakeLists.txt b/swift/stdlib/public/Platform/CMakeLists.txt
index a7a60a063d9..8894800509a 100644
--- a/swift/stdlib/public/Platform/CMakeLists.txt
+++ b/swift/stdlib/public/Platform/CMakeLists.txt
@@ -99,6 +99,7 @@ foreach(sdk ${SWIFT_SDKS})
         OUTPUT "${glibc_modulemap_out}"
         FLAGS
             "-DCMAKE_SDK=${sdk}"
+            "-DSDK_INCLUDE_PATH=${SWIFT_${sdk}_${arch}_ICU_UC_INCLUDE}"
             "-DGLIBC_INCLUDE_PATH=${SWIFT_SDK_${sdk}_ARCH_${arch}_LIBC_INCLUDE_DIRECTORY}"
             "-DGLIBC_ARCH_INCLUDE_PATH=${SWIFT_SDK_${sdk}_ARCH_${arch}_LIBC_ARCHITECTURE_INCLUDE_DIRECTORY}")
 
@@ -142,6 +143,7 @@ foreach(sdk ${SWIFT_SDKS})
         SOURCE "${glibc_modulemap_source}"
         OUTPUT "${glibc_sysroot_relative_modulemap_out}"
         FLAGS "-DCMAKE_SDK=${sdk}"
+              "-DSDK_INCLUDE_PATH=${SWIFT_${sdk}_${arch}_ICU_UC_INCLUDE}"
               "-DGLIBC_INCLUDE_PATH=${absolute_libc_include_path}"
               "-DGLIBC_ARCH_INCLUDE_PATH=${absolute_libc_arch_include_path}")
 
diff --git a/swift/stdlib/public/Platform/bionic.modulemap.gyb b/swift/stdlib/public/Platform/bionic.modulemap.gyb
index e44f9082653..09a92df50bb 100644
--- a/swift/stdlib/public/Platform/bionic.modulemap.gyb
+++ b/swift/stdlib/public/Platform/bionic.modulemap.gyb
@@ -185,7 +185,7 @@ module SwiftGlibc [system] {
       export *
     }
     module spawn {
-      header "${GLIBC_INCLUDE_PATH}/spawn.h"
+      header "${SDK_INCLUDE_PATH}/spawn.h"
       export *
     }
     module syslog {
diff --git a/swift-corelibs-foundation/Sources/Foundation/CMakeLists.txt b/swift-corelibs-foundation/Sources/Foundation/CMakeLists.txt
index 016bf294..1473f4dc 100644
--- a/swift-corelibs-foundation/Sources/Foundation/CMakeLists.txt
+++ b/swift-corelibs-foundation/Sources/Foundation/CMakeLists.txt
@@ -167,6 +167,10 @@ if(CMAKE_SYSTEM_NAME STREQUAL Windows)
     $<TARGET_OBJECTS:CoreFoundationResources>)
 elseif(NOT CMAKE_SYSTEM_NAME STREQUAL Darwin)
   target_link_options(Foundation PRIVATE "SHELL:-no-toolchain-stdlib-rpath")
+  if(CMAKE_SYSTEM_NAME STREQUAL Android)
+    target_link_libraries(Foundation PRIVATE android-spawn)
+    target_link_directories(Foundation PUBLIC ${CMAKE_FIND_ROOT_PATH}/usr/lib)
+  endif()
 endif()
 
 

Alternatively, since the build only fails when linking plutil against libFoundation.a and we can't run the tests for the cross-compiled corelibs, we could hack out building a static plutil or revert the small pull adding that dependency, as done in vgorloff/swift-everywhere-toolchain#116.

@drodriguez, let me know what you think.

@@ -899,6 +900,10 @@ android-icu-i18n=%(arm_dir)s/libicui18nswift.so
android-icu-i18n-include=%(arm_dir)s/icu/source/i18n
android-icu-data=%(arm_dir)s/libicudataswift.so

cross-compile-deps-path=%(prebuilt_path)
cross-compile-build-swift-tools=0
llvm-ninja-targets-for-cross-compile-hosts=help
Copy link
Member Author

Choose a reason for hiding this comment

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

@edymtt, I used your new flag since I want LLVM built natively but not cross-compiled. I couldn't use the clean target that --build-llvm=0 uses, because that errors for a cross-compiled LLVM. This help ninja target simply dumps the list of targets and does nothing.

@@ -918,6 +923,9 @@ install-llvm
install-swift
install-libicu
install-libcxx
install-libdispatch
install-foundation
install-xctest
Copy link
Member Author

Choose a reason for hiding this comment

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

This won't actually install the Android corelibs in a directory the compiler knows about, maybe we can fix that later.

Copy link
Contributor

Choose a reason for hiding this comment

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

Technically neither of these libraries should be related to the compiler, so distribution is a different story, but, where do they end up? I hope that at least they don't overwrite anything, or they don't end up intermixed with the build system libraries, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

All non-Darwin cross-compilation hosts are placed in install_destdir + host-name + install_prefix, so if the host toolchain goes in /home/butta/tools/usr, these cross-compiled corelibs are placed in /home/butta/tools/android-aarch64/usr, which the native linux compiler won't find. That hostname is added here. Since it's a separate cross-compilation sysroot, there's no problem with overwriting or mixing.

What I'm saying is that since we're adding this ability to just cross-compile the SDK, it would be nice to install it where the native compiler can find it, but we could add that later.

Btw, now that I'm adding the ability to cross-compile build-script products also in #36917, I add this installation hostname directory to match build-script-impl products there too.

@@ -928,6 +936,7 @@ reconfigure
mixin-preset=buildbot_linux_crosscompile_android,tools=RA,stdlib=RD,build

android-arch=aarch64
cross-compile-hosts=android-aarch64
Copy link
Member Author

Choose a reason for hiding this comment

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

This will need to be added for armv7 above too, but those flags are added to this one. I think I'll need to add another Android preset to keep that flag separate.

Copy link
Contributor

Choose a reason for hiding this comment

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

The preset buildbot_linux_crosscompile_android,tools=RA,stdlib=RD,build,aarch64 inherits everything from buildbot_linux_crosscompile_android,tools=RA,stdlib=RD,build, but as far as I remember, it also overrides those values (that's how android-arch= in the line above works).

You should be able to add cross-compile-hosts=android-armv7 in the preset above, and override here with this line.

Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't sure if it would override or get confused, I will try it and update this pull.

Copy link
Member Author

Choose a reason for hiding this comment

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

It turns out it appends and builds them both, as the help string points out, and I confirmed by trying it. Therefore, I've split out the presets as I mentioned.

Let me know if you'd like me to rename the armv7 preset above, or you want to keep the same preset name that doesn't mention that it is for armv7.

@@ -1909,6 +1910,12 @@ for host in "${ALL_HOSTS[@]}"; do
cmake_options=(
"${cmake_options[@]}"
-DLLVM_TABLEGEN=$(build_directory "${LOCAL_HOST}" llvm)/bin/llvm-tblgen
-DSWIFT_INCLUDE_TOOLS:BOOL=$(true_false "${CROSS_COMPILE_BUILD_SWIFT_TOOLS}")
Copy link
Member Author

Choose a reason for hiding this comment

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

This changes the way --build-swift-tools works, where before it would control building both native and cross-compiled host tools, but now it only affects the native ones. Since very few people were likely using this flag, I think it's better to split the functionality up like this for cross-compiling the host tools, rather than keeping --build-swift-tools controlling both even after --cross-compile-build-swift-tools is added.

Copy link
Contributor

@drodriguez drodriguez left a comment

Choose a reason for hiding this comment

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

I will have to re-read and play around with the posix_spawn problems to understand them. In general, I would say that things should work with vanilla Android, without special libraries or headers.

For the time being, let's see if this works in macOS and Linux and we are not going to break another platform by mistake.

@@ -918,6 +923,9 @@ install-llvm
install-swift
install-libicu
install-libcxx
install-libdispatch
install-foundation
install-xctest
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically neither of these libraries should be related to the compiler, so distribution is a different story, but, where do they end up? I hope that at least they don't overwrite anything, or they don't end up intermixed with the build system libraries, right?

@@ -928,6 +936,7 @@ reconfigure
mixin-preset=buildbot_linux_crosscompile_android,tools=RA,stdlib=RD,build

android-arch=aarch64
cross-compile-hosts=android-aarch64
Copy link
Contributor

Choose a reason for hiding this comment

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

The preset buildbot_linux_crosscompile_android,tools=RA,stdlib=RD,build,aarch64 inherits everything from buildbot_linux_crosscompile_android,tools=RA,stdlib=RD,build, but as far as I remember, it also overrides those values (that's how android-arch= in the line above works).

You should be able to add cross-compile-hosts=android-armv7 in the preset above, and override here with this line.

@drodriguez
Copy link
Contributor

@swift-ci please test

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 90f4c15bf87f039691be43ee86057637245847b2

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 90f4c15bf87f039691be43ee86057637245847b2

@finagolfin
Copy link
Member Author

I will have to re-read and play around with the posix_spawn problems to understand them. In general, I would say that things should work with vanilla Android, without special libraries or headers.

The problem is that swiftlang/swift-corelibs-foundation#2928 added a dependency on some posix_spawnattr_* functions that were not added to Android till API 28, whereas this preset builds against API 21. There are various ways we could remedy that, such as asking CoreFoundation if it found those APIs and only using them if available?

let's see if this works in macOS and Linux and we are not going to break another platform by mistake

A single failing test because I added a new path variable to the Android preset, didn't know I had to do something else for that, will fix it.

@finagolfin
Copy link
Member Author

I added a line to fix the single failing test on the CI and fixed the preset arch issue we discussed above.

@drodriguez, I think this is ready for you to try on your local version of the Android CI. I think the only remaining issue will be adding library dependencies that the corelibs depend on to the Android CI, which will require the Termux packages I mentioned above for the Android corelibs and perhaps the native Ubuntu packages to build the native linux corelibs, if those Ubuntu packages like libcurl aren't already installed on the Android CI.

@@ -914,20 +919,29 @@ host-test
extra-cmake-options=-DSWIFT_ENABLE_LLD_LINKER:BOOL=OFF

install-prefix=/usr
install-llvm
Copy link
Member Author

@finagolfin finagolfin Jul 25, 2021

Choose a reason for hiding this comment

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

I just tried a full build of the latest July 24 trunk source snapshot with these flags, but with the testing flags excised, and the only problem I had was with this flag. While this works fine for the native linux build, I'm trying to avoid the cross-compiled LLVM build with the llvm-ninja-targets-for-cross-compile-hosts=help flag above, but this install target kicks off the full cross-compile of LLVM for Android later on when installing.

Since this preset won't create a toolchain with the Android corelibs installed in a place where the Swift compiler can detect them anyway, as mentioned below, the easiest way to avoid this LLVM cross-compile is not to install LLVM either.

@drodriguez
Copy link
Contributor

The problem is that apple/swift-corelibs-foundation#2928 added a dependency on some posix_spawnattr_* functions that were not added to Android till API 28, whereas this preset builds against API 21. There are various ways we could remedy that, such as asking CoreFoundation if it found those APIs and only using them if available?

I remember that code 😀. I don't think anyone will object if we switch (again) all the direct usages of posix_spawnattr_* into some _CFPosixSpawn* equivalents. The treatment of the attributes was not done, but as long as it is limited to just POSIX_SPAWN_SETPGROUP, maybe it is doable with setpgid(0, /* the pgroup value */) in _CFPosixSpawnImplPre28 after the comment // This is the child (probably before the actions are processed).

@drodriguez
Copy link
Contributor

@swift-ci please test

@finagolfin
Copy link
Member Author

Alright, I will look into implementing that CF polyfill on top of that posix_spawn wrapper you added a couple years ago and submit a pull for that.

@swift-ci
Copy link
Contributor

Build failed
Swift Test Linux Platform
Git Sha - 26f0200b8d9f5723533cb763ec2392c39e3de6f9

@swift-ci
Copy link
Contributor

Build failed
Swift Test OS X Platform
Git Sha - 26f0200b8d9f5723533cb763ec2392c39e3de6f9

This is useful when building cross-compilation toolchains where you want the
stdlib and corelibs cross-compiled but don't want the Swift compiler
cross-compiled too with '--cross-compile-hosts'. This pull demonstrates using the
new flag for the Android build presets.
@finagolfin
Copy link
Member Author

Gah, playing whack-a-mole with the non-Android CI: I had missed that preset variables need that s at the end, actually ran the single failing test this time to make sure it works now.

@drodriguez
Copy link
Contributor

@swift-ci please test

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.

3 participants