-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[android] Remove ICU build flags since that requirement was dropped in #40340 #40625
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
Conversation
4. Change directories into `libiconv-libicu-android`: `cd libiconv-libicu-android` | ||
5. Run the Swift build script: `./build-swift.sh` | ||
6. Confirm that the various `libicuXYZswift.so` libraries are located in the | ||
`armeabi-v7a` directory. |
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.
We may need this back if we ever add instructions for cross-compiling swift-corelibs-foundation for Android as in #38441, since that repo still requires libicu, but in the meantime this libicu cross-compilation is no longer needed and we can always resurrect this doc from git if we want it later.
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 only thing that I’m remotely concerned about is the foundation test. That still requires icu, but assuming that there’s a different script for that/you’ve tested foundation with this change, this looks great!
--android-icu-uc "${ANDROID_ICU_PATH}/armeabi-v7a" \ | ||
--android-icu-uc-include "${ANDROID_ICU_PATH}/armeabi-v7a/icu/source/common" \ | ||
--android-icu-i18n "${ANDROID_ICU_PATH}/armeabi-v7a" \ | ||
--android-icu-i18n-include "${ANDROID_ICU_PATH}/armeabi-v7a/icu/source/i18n" \ |
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 just went ahead and got rid of this whole file because it hasn't worked in years. The expanded build preset from #38441 should replace this.
@@ -945,8 +939,6 @@ installable-package=%(installable_package)s | |||
|
|||
host-test | |||
|
|||
extra-cmake-options=-DSWIFT_ENABLE_LLD_LINKER:BOOL=OFF |
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 hasn't been used since @davezarzycki removed it in #32042.
Cross-compilation of Foundation uses this CMake config I added this summer to get the path to a cross-compiled libicu in a different way. I have Thanks for the quick review. |
30b1884
to
a3ea28f
Compare
|
||
``` | ||
$ ARM_DIR=path/to/libiconv-libicu-android/armeabi-v7a |
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.
Removed this line that I had missed and rebased.
@MaxDesiatov, would you run the CI? |
@swift-ci please test |
@swift-ci please smoke test |
Build failed |
macOS CI failure appears spurious, as this pull doesn't affect macOS and the only failure I see is |
@davezarzycki, I see you're up: would you merge if that CI failure isn't required to pass? It is spurious. |
I lost my window to work today so I don't have time to review this before merging. Maybe @gottesmm can help. |
@davezarzycki, this pull was already approved and passed most of the CI, just need someone to merge. |
Thanks for getting this in quickly, @compnerd. |
Since @Azoy removed libicu as a stdlib dependency, this greatly simplifies the
build-script
command needed to cross-compile the toolchain for Android. I only removed the Android ICU flags in this pull, but there are other uses for Windows and other platforms that can also be removed. I tested this pull with #35707 by applying the following command that is taken from the Android build preset run on the CI, but removing the flags to build libicu and install the toolchain:I ran this for both aarch64 and armv7 and only 13 C++ Interop tests failed, which were already broken on the Android CI because of #39605 a couple weeks ago. The good news is those tests progressed further with #35707, but still failed because of some missing headers and other problems with the NDK.
So this pull passes all the same tests as the current Android CI and should be fine. @drodriguez and @compnerd, you two know this code best, please review.