Allow cross-compile Android while compiling libicu in Linux. #20083
Conversation
drodriguez
force-pushed
the
android-and-linux-libicu
branch
from
b0fbfaa to
646e030
Compare
| else() | ||
| list(APPEND swift_core_private_link_libraries -licui18n -licuuc -licudata) | ||
| endif() | ||
| list(APPEND swift_core_private_link_libraries ICU_UC ICU_I18N) | ||
| endif() |
There was a problem hiding this comment.
I don't think ICU_UC and ICU_I18N are actually the correct variables I think it should be ICU_UC_LIBRARIES and ICU_I18N_LIBRARIES
There was a problem hiding this comment.
Similar to my explanation above, this is just following the previous code. The code uses those values (they are not variables) as sigils for linking the ICU libraries. When those sigils are found in AddSwift.cmake, the right thing is done: for most SDKs ${SWIFT_${sdk}_${arch}_ICU_UC} and similar are used, but if one provided ${SWIFT_PATH_TO_LIBICU_BUILD}, the normal -licuuc flags are used (since the path will be in the library search path).
cmake/modules/AddSwift.cmake
Outdated
| list(APPEND swiftlib_private_link_libraries_targets | ||
| "${SWIFT_${sdk}_${arch}_ICU_UC}") | ||
| else() | ||
| list(APPEND swiftlib_private_link_libraries_targets -licuuc -licudata) |
There was a problem hiding this comment.
Avoid hardcoding the library names here, -licuuc and -licudata. When the libicu is compiled as part of the swift build, the names are libicuucswift and libicudataswift. I think we should be able to use some ICU_ variable names instead
There was a problem hiding this comment.
Probably this will have to change if libicu gets more integrated into the build system, but for the moment, this change only moves code that was in stdlib/public/core/CMakeLists.txt into here. The code has been moved as similar as possible, and that’s why I kept using -licuuc and -licudata here.
|
Just to note to say that I think they each use a different subset of the variables presented. So a followup PR would be required to fix up |
|
@spevans we can include that in the cmake distribution for swift. Then Foundation can just import it itself from swift. |
|
I’m aware that the way ICU is linked in Anyway, this change was a minimal change to allow compiling both Linux and Android, Linux with a self-compiled ICU, Android with a pre-compiled ICU (which should be the typical setup for many Linux distributions). I didn’t noticed that you finally merged #19860, but this one will probably have to change now. From the recommended scripts to build ICU for Android (SwiftAndroid/libiconv-libicu-android), I don’t think it would be easy to self-compile ICU for Android, but I will see if there’s a way of modifying the scripts to build something that works with |
drodriguez
force-pushed
the
android-and-linux-libicu
branch
from
646e030 to
f08ebf9
Compare
Before this commit, if one used the
--libicuparameter of the build script to compile ICU in Linux, the variableSWIFT_PATH_TO_LIBICU_BUILDis set. While evaluating which link flags to use instdlib/public/core/CMakeLists.txt, the presence of that variable will set all the SDKs to use-licuucstyle link libraries, which will not work while cross-compiling Android (since Android libraries have aswiftsuffix, and they are not in the library search path).This commit delays the selection of
-licuucstyle or full paths to later in the process, where the current SDK is already know, and the script can distinguish between the Android SDK case and other SDKs.This change will become more important if #19860 is merged. It might need to be modified slightly to always use the
swiftsuffixes, though./cc @spevans