[android] Remove ICU build flags since that requirement was dropped in #40340 #40625

buttaface · 2021-12-19T16:20:45Z

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:

./swift/utils/build-script --release --assertions --no-swift-stdlib-assertions --android
--android-ndk /home/butta/android-ndk-r23b/ --android-arch aarch64 --android-api-level 24
--test --validation-test --long-test --stress-test --test-optimized --lit-args="-v --time-tests"
--build-swift-static-stdlib --build-swift-static-sdk-overlay --build-swift-stdlib-unittest-extra
--host-test --skip-test-linux -j9 --skip-build-compiler-rt

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.

The text was updated successfully, but these errors were encountered:

buttaface · 2021-12-19T16:26:07Z

docs/Android.md

-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.


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.

compnerd

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!

buttaface · 2021-12-19T16:28:56Z

utils/android/build-toolchain

-    --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" \


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.

buttaface · 2021-12-19T16:35:09Z

utils/build-presets.ini

@@ -945,8 +939,6 @@ installable-package=%(installable_package)s

 host-test

-extra-cmake-options=-DSWIFT_ENABLE_LLD_LINKER:BOOL=OFF


This hasn't been used since @davezarzycki removed it in #32042.

buttaface · 2021-12-19T17:08:33Z

assuming that there’s a different script for that/you’ve tested foundation with this change

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~~n't actually~~ now tested this pull with that flag ~~yet but~~and since they're completely separate build paths, ~~I don't see that getting~~the Foundation linking against a cross-compiled libicu wasn't affected.

Thanks for the quick review.

…apple#40340

buttaface · 2021-12-20T06:39:34Z

docs/Android.md


 ```
-$ ARM_DIR=path/to/libiconv-libicu-android/armeabi-v7a


Removed this line that I had missed and rebased.

buttaface · 2021-12-21T15:01:26Z

@MaxDesiatov, would you run the CI?

MaxDesiatov · 2021-12-21T15:02:54Z

@swift-ci please test

MaxDesiatov · 2021-12-21T15:03:08Z

@swift-ci please smoke test

swift-ci · 2021-12-21T20:12:41Z

Build failed
Swift Test OS X Platform
Git Sha - a3ea28f

buttaface · 2021-12-21T22:23:30Z

macOS CI failure appears spurious, as this pull doesn't affect macOS and the only failure I see is FAIL: Testing swift-docc failed but none of the swift-docc tests failed.

buttaface · 2021-12-22T15:09:30Z

@davezarzycki, I see you're up: would you merge if that CI failure isn't required to pass? It is spurious.

davezarzycki · 2021-12-22T16:47:07Z

I lost my window to work today so I don't have time to review this before merging. Maybe @gottesmm can help.

buttaface · 2021-12-22T22:00:26Z

@davezarzycki, this pull was already approved and passed most of the CI, just need someone to merge.

buttaface · 2021-12-24T06:09:19Z

Thanks for getting this in quickly, @compnerd.

buttaface reviewed Dec 19, 2021

View changes

compnerd approved these changes Dec 19, 2021

View changes

buttaface reviewed Dec 19, 2021

View changes

buttaface mentioned this pull request Dec 19, 2021

[android] Switch to new single-header modulemap for Bionic too #35707

Open

[android] Remove ICU build flags since that requirement was dropped in …

a3ea28f

…apple#40340

buttaface force-pushed the droid-remove-icu branch from 30b1884 to a3ea28f Dec 20, 2021

buttaface reviewed Dec 20, 2021

View changes

buttaface mentioned this pull request Dec 21, 2021

[build] Remove last vestiges of ICU for anything other than Foundation #40659

Open

davezarzycki requested a review from gottesmm Dec 22, 2021

compnerd merged commit d1bb98b into apple:main Dec 23, 2021
4 of 5 checks passed

buttaface deleted the droid-remove-icu branch Dec 23, 2021

Nov	DEC	Jan
	30
2020	2021	2022

apple / swift Public

[android] Remove ICU build flags since that requirement was dropped in #40340 #40625

[android] Remove ICU build flags since that requirement was dropped in #40340 #40625

buttaface commented Dec 19, 2021

buttaface Dec 19, 2021

compnerd left a comment

buttaface Dec 19, 2021

buttaface Dec 19, 2021

buttaface commented Dec 19, 2021 •

edited

buttaface Dec 20, 2021

buttaface commented Dec 21, 2021

MaxDesiatov commented Dec 21, 2021

MaxDesiatov commented Dec 21, 2021

swift-ci commented Dec 21, 2021

buttaface commented Dec 21, 2021

buttaface commented Dec 22, 2021

davezarzycki commented Dec 22, 2021

buttaface commented Dec 22, 2021

buttaface commented Dec 24, 2021

		@@ -945,8 +939,6 @@ installable-package=%(installable_package)s

		host-test

		extra-cmake-options=-DSWIFT_ENABLE_LLD_LINKER:BOOL=OFF

apple / swift Public

[android] Remove ICU build flags since that requirement was dropped in #40340 #40625

[android] Remove ICU build flags since that requirement was dropped in #40340 #40625

Conversation

buttaface commented Dec 19, 2021

buttaface Dec 19, 2021

Choose a reason for hiding this comment

compnerd left a comment

buttaface Dec 19, 2021

Choose a reason for hiding this comment

buttaface Dec 19, 2021

Choose a reason for hiding this comment

buttaface commented Dec 19, 2021 • edited

buttaface Dec 20, 2021

Choose a reason for hiding this comment

buttaface commented Dec 21, 2021

MaxDesiatov commented Dec 21, 2021

MaxDesiatov commented Dec 21, 2021

swift-ci commented Dec 21, 2021

buttaface commented Dec 21, 2021

buttaface commented Dec 22, 2021

davezarzycki commented Dec 22, 2021

buttaface commented Dec 22, 2021

buttaface commented Dec 24, 2021

buttaface commented Dec 19, 2021 •

edited