[build] rewrite #21707 so that it doesn't break cross-compilation #30170
Conversation
|
Alright, @gottesmm, review requested. |
finagolfin
changed the title
cmake/modules/AddSwift.cmake
Outdated
| # link against the ICU libraries | ||
| list(APPEND link_libraries | ||
| ${SWIFT_ANDROID_${LFLAGS_ARCH}_ICU_I18N} | ||
| ${SWIFT_ANDROID_${LFLAGS_ARCH}_ICU_UC}) |
There was a problem hiding this comment.
I noticed this was adding the full libicu path twice for Android now that it's set directly in swiftCore, so removed this.
finagolfin
force-pushed
the
cross
branch
2 times, most recently
from
de45d93 to
e796245
Compare
|
After thinking about it some more, I realized that the entire previous approach of setting By removing the use of @drodriguez, let me know what you think of this pull. |
|
I think this is going to break, and more than Android. I just tested Android ARMv7 and I have 46 new failures (I am verifying, and I will test Android AArch64 as soon as I can). Many of the failures are a missing symbol swift_addNewDSOImage. Let's not merge this until I figure this out. Starting the tests to see if the Linux tests shows the same problem. @swift-ci please test |
|
Build failed |
|
So I get the following 46 also in AArch64. I guess these are going to fail in the CI machines too. Example of a failing test: Are we forgetting to link something? |
|
I think that's another bug in the way cross-compilation from linux worked. That symbol However, before this pull, the CMake config for swiftCore would also wrongly link that added shared library anytime the static stdlib was being built and the primary variant SDK, ie the bogus alias for the host SDK mentioned above, was So this bug covered up that bug till now, that one will have to be looked at and fixed, ie do we really only want that separate shared library only on linux or not? |
|
I've added changes to the main commit for While this pull is aimed at cross-compiling the Android stdlib, it also makes the stdlib more cross-compilable to other platforms. |
stdlib/public/core/CMakeLists.txt
Outdated
| set(swift_stdlib_compile_flags "${SWIFT_RUNTIME_SWIFT_COMPILE_FLAGS}") | ||
| if(SWIFT_PRIMARY_VARIANT_SDK IN_LIST SWIFT_APPLE_PLATFORMS) | ||
| list(APPEND swift_core_link_flags "-all_load") | ||
| list(APPEND swift_core_private_link_libraries icucore) |
There was a problem hiding this comment.
@vgorloff, this would try to link icucore for all cross-compiled stdlibs from a mac before this pull: would this cause problems for you when cross-compiling from mac to Android or was it simply removed elsewhere?
There was a problem hiding this comment.
Here is how configured SwiftBuilder on my side.
-D SWIFT_ANDROID_${arch.swiftArch}_ICU_UC=${icu.paths.installs}/lib/libicuucswift.so
-D SWIFT_ANDROID_${arch.swiftArch}_ICU_UC_INCLUDE=${icu.paths.sources}/source/common
-D SWIFT_ANDROID_${arch.swiftArch}_ICU_I18N=${icu.paths.installs}/lib/libicui18nswift.so
-D SWIFT_ANDROID_${arch.swiftArch}_ICU_I18N_INCLUDE=${icu.paths.sources}/source/i18n
-D SWIFT_ANDROID_${arch.swiftArch}_ICU_DATA=${icu.paths.installs}/lib/libicudataswift.soAlso I can't find library icucore in build output on my side.
There was a problem hiding this comment.
Well, it should be moot once the ICU dependency is removed, #40340. 🎉
|
Added one last change to replace the last uses of
This pull is ready to go, just needs a review. |
|
Updated this pull to move the generation of the static-stdlib linker file from |
|
Ping, this pull was used in its entirety to cross-compile the stdlib of the pre-built toolchain for Android. It makes it possible to cross-compile the Swift stdlib without compiling the host compiler or stdlib, would be good to get this in before the 5.3 branch. |
|
@swift-ci smoke test |
|
@gottesmm, requesting review again. |
|
The problems I experienced before in Android seems to be gone. Thanks. I am not sure that I have the confidence to merge this. It touches many pieces of the build system. Seems to work, which is great. @swift-ci please test Windows platform |
|
Now that 5.3 has branched, should be okay to get this into master now, if there are worries it might break something and time is needed. All the first commit does is remove libatomic on other platforms, just like it has been on linux. All the second commit does is rewrite the way some linker flags and libraries are added, so that they won't be added to say the Android stdlib incorrectly just because it was cross-compiled from a linux host. It changes nothing for host builds, which is mostly all that has been tried or tested with this codebase, applying all the same flags as before. The only change the second commit makes is switching from |
|
Rebased and this pull is now smaller, since Saleem has been silently merging pieces of it in his own pulls, over the two months this pull sat unreviewed. @CodaFi, I mentioned this pull on the forum last month and got some acknowledgement from @gottesmm, though he says he has bigger plans in this vein this summer. I see no reason why that should preclude merging this small working pull in the meantime, which is already being used when compiling the Swift compiler for Android. |
| ${SWIFT_${SWIFT_PRIMARY_VARIANT_SDK}_${SWIFT_PRIMARY_VARIANT_ARCH}_ICU_I18N}) | ||
| endif() | ||
|
|
||
| function(swift_core_private_libraries sdk arch libraries_var_name) |
There was a problem hiding this comment.
I think that this is the going in the exact opposite direction of where we want to go. We need to start using find_library and imported targets to create the dependencies rather than hard coded names and magic flags.
There was a problem hiding this comment.
It is not clear why you placed this review comment here: this reorganizes a chunk of the following CMake config as a CMake function and is orthogonal to the issue of "imported targets" vs "magic flags." Maybe you meant to place it below, where the flags are?
As for the way those libraries and flags are added below, that predates this pull and has nothing to do with me. I realize you'd like to see it modernized, but that's not what this pull is offering: I simply refactor the way CMake calls the pre-existing "magic flags," so they can be used when cross-compiling too.
finagolfin
force-pushed
the
cross
branch
2 times, most recently
from
1c571ff to
91c6af0
Compare
|
Getting back to this after more than a year, rebased and tweaked it for how things are done now. Let me explain more broadly why this pull is needed: Saleem moved a bunch of target flags from the central To remedy that, I kept the logic adding these link flags in This fixes the command we give people to build the Android SDK, which has been reported as broken on the forum. @compnerd, let me know what you think of the latest iteration of this pull. @edymtt, you know these CMake issues well, would appreciate your input. |
| @@ -148,6 +123,30 @@ add_swift_target_library(swiftImageRegistrationObjectCOFF | |||
| INSTALL_IN_COMPONENT none) | |||
|
|
|||
| foreach(sdk ${SWIFT_SDKS}) | |||
| if(SWIFT_BUILD_STATIC_STDLIB AND "${SWIFT_SDK_${sdk}_OBJECT_FORMAT}" STREQUAL "ELF") | |||
There was a problem hiding this comment.
Rather than only generate this link file if the host SDK is linux, as was done above, create it for all ELF SDKs in the list of wanted SWIFT_SDKS.
finagolfin
changed the title
…ig, so that it doesn't break cross-compilation Passing swift_core_private_link_libraries to PRIVATE_LINK_LIBRARIES only works either when building a single host SDK alone or when needed by all SDKs, so move all that configuration to a swift_core_private_libraries() function that's called from add_swift_target_library() for each SDK/arch instead. Also, generate a static-executable-args.lnk for each ELF SDK, rather than just for linux if it is the host SDK.
Passing swift_core_private_link_libraries to PRIVATE_LINK_LIBRARIES only works either when building a single host SDK alone or when needed by all SDKs, so move all that configuration to a swift_core_private_libraries() function that's called from add_swift_target_library() for each SDK/arch instead. Similarly, call a swift_runtime_static_libraries() function to split off libraries for the static stdlib with linux. Finally, remove the last uses of
SWIFT_CONFIGURED_SDKSfor anything more than checking, usingSWIFT_SDKSinstead, and move the static-stdlib linker file generation to thestdlib/directory.I ran into this when trying out the Android cross-compilation commands in #29439 with Swift 5.1.4 built from source on linux x86_64, which I fixed back then with a much simpler version of this pull. It broke because I wasn't building
SWIFT_PATH_TO_LIBICU_BUILD, instead linking the linux SDK against the system libicu package and the Android SDK against a pre-built libicu for Android.That would make the prior CMake config from #21707 try to link the Android swiftCore against the linux libicu, ie the libicu of the primary variant SDK. The Android CI didn't detect this because it builds libicu from source, not using the system libicu for linux like I did. The rest of these non-Android SDKs aren't cross-compiled on any CI, so the cross-compilation regression wasn't found.
SWIFT_PRIMARY_VARIANT_SDKseems to be a meaningless variable, which AFAICT means "usually the host SDK but not always." It should either be defined much more precisely, or removed altogether and replaced with checking if an sdk is inSWIFT_SDKSor something.This pull was tested by back-porting to that Swift 5.1.4 and now 5.2.1 cross-compilation setup and running the same commands to produce a working executable on Android.