[android] Modify test scripts for aarch64 and modern NDKs. #19503

drodriguez · 2018-09-24T17:23:43Z

Many places on the testing scripts for Android have the ARMv7
architecture hardcoded. Modify all those instances to support both ARMv7
and AArch64.

Besides the paths being modified depending on the architecture, the
script is also modified to adapt to NDK beyond r14, where the headers
are not under the SDK root, but under a unified sysroot. Two new include
paths are passed to the compiler invocations (one the general one, one
the architecture specific one).

In order to link correctly, the -tools-directory and a modern linker
(-use-ld=gold) are also passed in.

There's a new CMake variable named SWIFT_ANDROID_${ARCH}_ICU_DATA that
should point to libicudataswift.so for each architecture. This part of
ICU is necessary while running the test in the host, so it needs to be
uploaded. Since it is normally side by side with other ICU products, the
linker was finding it for free.

NOTE: This doesn’t fix the multiple test that fails in AArch64. I will try to fix those in future commits.

compnerd · 2018-10-05T23:24:15Z

Hmm, what if the user actually wants to use lld by means of the USE_LLD_LINKER option? Does this continue to work with the older NDK releases?

drodriguez · 2018-10-08T18:09:30Z

@compnerd: The “recommended” Android NDK r16 doesn’t include LLD (r18 does). I say “recommend” with quotes, because it is the only one currently referenced in https://github.com/apple/swift/blob/master/docs/Android.md.

In any case, ld.bfd (the default) was failing to compile the tests, and that’s why I changed it. In some way, that same documentation linked above is directing you to compile with gold: it creates a symbolic link with gold and that’s the only linker for ARM that you are suppose to have in the path. But also notice that the step is not really necessary and -use-ld=gold works as well.

Since r18 is now the latest available version, I might need to change this slightly. To allow people use LLD if they prefer it. I’m not sure, but lit.site.cfg.in might be the way.

PD: The stdlib should default to gold, if I’m not mistaken, since both SWIFT_ENABLE_GOLD_LINKER and SWIFT_ENABLE_LLD_LINKER are enabled by default, but lld is nowhere to be found for Android.

compnerd · 2018-10-08T20:47:31Z

Right, lld is not provided by the NDK. However, the binutils linker (BFD) is known to not generate valid binaries and you must use either gold or lld.

drodriguez · 2018-10-10T00:09:39Z

Hopefully this will do the trick. Since both USE_LLD and USE_GOLD can be TRUE at the same time (and that’s the default), I gave priority to lld, but also I’m falling into gold because that’s the only one that worked for me for Android.

drodriguez · 2018-10-13T01:02:31Z

Pushed a new version which fixes a test in Android (Migrator/missing_script) which was missing an explicit -swift-version value that comes from the config.swift_test_options value which wasn’t interpolated into the swiftc command.

test/lit.cfg

drodriguez · 2018-10-16T17:17:37Z

I must have overwrite the changes to use LLD somehow. I will recover them and apply your suggestions here.

drodriguez · 2018-10-24T20:13:49Z

The last change adds android_include_paths_opt to target_clang, because otherwise the host headers will have been used.

test/lit.cfg

test/lit.site.cfg.in

validation-test/lit.site.cfg.in

Support for Android aarch64 in many parts of the build-script. Most of the changes are reuse variables/parameters that already existed for Android ARMv7. There is also a new parameter to specify the ICU data library, which is used by swiftlang#19503. With this one can build either armv7 or aarch64, since building both at the same time requires more changes like swiftlang#19432 (and probably more work to support two set of paths).

drodriguez · 2018-12-13T19:01:11Z

@compnerd: can you have another look at this one? I would love to have this available when I have to test. Thanks!

drodriguez · 2019-02-06T00:41:05Z

@compnerd: I just updated this branch. Care to have another look at this? Thanks!

Many places on the testing scripts for Android have the ARMv7 architecture hardcoded. Modify all those instances to support both ARMv7 and AArch64. Besides the paths being modified depending on the architecture, the script is also modified to adapt to NDK beyond r14, where the headers are not under the SDK root, but under a unified sysroot. Two new include paths are passed to the compiler invocations (one the general one, one the architecture specific one). In order to link correctly, the -tools-directory is passed to the Swift compiler invocation. In order to use a modern linker, the selected linker in the CMake script is written in the lit.site.cfg.in files. The system will prefer lld, but will fallback to gold. Plain ld will not be used, since it cannot link correctly the binaries. There's a new CMake variable named SWIFT_ANDROID_${ARCH}_ICU_DATA that should point to libicudataswift.so for each architecture. This part of ICU is necessary while running the test in the host, so it needs to be uploaded. Since it is normally side by side with other ICU products, the linker was finding it for free.

compnerd · 2019-03-21T21:32:46Z

test/lit.cfg

+        result = kwargs[run_cpu]
+        if result is None:
+          if run_cpu == "armv7s" or run_cpu == "armv7k":
+            result = kwargs["armv7"]


I would rewrite this as: if run_cpu.startswith('armv7'): return kwargs["armv7"]

compnerd · 2019-03-21T21:34:31Z

test/lit.cfg

+                                                 aarch64="aarch64-linux-android")
+    toolchain_directory_name = "{}-{}".format(ndk_platform_triple, config.android_ndk_gcc_version)
+    tools_directory = make_path(config.android_ndk_path, "toolchains",
+        toolchain_directory_name, "prebuilt", "linux-x86_64",


linux-x86_64 should really be based on the build (e.g. building on Windows, Darwin, or Linux)

compnerd · 2019-03-21T21:36:59Z

test/lit.cfg

+    # Since NDK r14 the headers are unified under $NDK_PATH/sysroot, so the -sdk
+    # switch is not enough. Additionally we have to include both the unified
+    # sysroot, and the architecture sysroot.
+    android_include_paths_opt = "-I {sysroot} -I {sysroot_arch}".format(


I think that we should use -isystem here instead as we do not want to see any warnings from the NDK, its the system.

compnerd · 2019-03-21T21:37:30Z

test/lit.cfg

-        % (config.swiftc, config.variant_triple, config.variant_sdk,
-           android_linker_opt, resource_dir_opt, mcp_opt,
-           config.swift_test_options,
+        '%s -target %s -sdk %r -tools-directory %r %s %s '


Why not the .format version here?

compnerd · 2019-03-21T21:40:37Z

@swift-ci please smoke test

compnerd · 2019-03-21T23:41:41Z

Since this improves the current state of the android testing, lets get this merged; the comments are generally pretty small and can be addressed in a follow up.

@compnerd

Some style fixes that @compnerd proposed in PR swiftlang#19503. - Instead of checking for the two known ARMv7 subarchitectures, check for everything that starts with armv7 and consider it armv7. - Try finding the current platform to select the right prebuilt directory for the NDK tools. - Use -isystem for the system header includes when passing them to clang. - Modify the format strings which started to be very wild into simpler `' '.join([...])` which, IMO, read better. - Hoist some common path parts to not repeat the same long paths in a couple of places. - Add more library search paths (missing for atomic)

Support for Android aarch64 in many parts of the build-script. Most of the changes are reuse variables/parameters that already existed for Android ARMv7. There is also a new parameter to specify the ICU data library, which is used by swiftlang#19503. With this one can build either armv7 or aarch64, since building both at the same time requires more changes like swiftlang#19432 (and probably more work to support two set of paths).

drodriguez force-pushed the android-aarch64-test-script branch from 147aebc to 7959a10 Compare October 10, 2018 00:07

drodriguez force-pushed the android-aarch64-test-script branch 2 times, most recently from cac9d4e to f78f859 Compare October 13, 2018 01:00

compnerd requested changes Oct 16, 2018

View reviewed changes

drodriguez force-pushed the android-aarch64-test-script branch from f78f859 to 3ab3f22 Compare October 16, 2018 17:47

drodriguez force-pushed the android-aarch64-test-script branch from 3ab3f22 to 0aaacce Compare October 24, 2018 20:07

compnerd requested changes Oct 24, 2018

View reviewed changes

test/lit.cfg Outdated Show resolved Hide resolved

test/lit.cfg Outdated Show resolved Hide resolved

test/lit.site.cfg.in Outdated Show resolved Hide resolved

validation-test/lit.site.cfg.in Outdated Show resolved Hide resolved

drodriguez force-pushed the android-aarch64-test-script branch from 0aaacce to 131bb85 Compare October 24, 2018 22:55

drodriguez mentioned this pull request Oct 26, 2018

[build-script] Support for Android aarch64 #20082

Merged

drodriguez force-pushed the android-aarch64-test-script branch from 131bb85 to 1ba0b7c Compare December 13, 2018 18:57

drodriguez force-pushed the android-aarch64-test-script branch from 1ba0b7c to ae0f16c Compare February 6, 2019 00:39

drodriguez mentioned this pull request Mar 11, 2019

[android] Add support for natively building on Android #23208

Merged

drodriguez force-pushed the android-aarch64-test-script branch from ae0f16c to 6d5309a Compare March 20, 2019 18:34

compnerd approved these changes Mar 21, 2019

View reviewed changes

compnerd merged commit 3be7b4e into swiftlang:master Mar 21, 2019

drodriguez mentioned this pull request Mar 23, 2019

[test] Style fixes for "Modify test scripts for aarch64 and modern NDKs" #23506

Merged

drodriguez deleted the android-aarch64-test-script branch July 16, 2019 23:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Modify test scripts for aarch64 and modern NDKs. #19503

[android] Modify test scripts for aarch64 and modern NDKs. #19503

drodriguez commented Sep 24, 2018

compnerd commented Oct 5, 2018

drodriguez commented Oct 8, 2018

compnerd commented Oct 8, 2018

drodriguez commented Oct 10, 2018

drodriguez commented Oct 13, 2018

drodriguez commented Oct 16, 2018

drodriguez commented Oct 24, 2018

drodriguez commented Dec 13, 2018

drodriguez commented Feb 6, 2019

compnerd Mar 21, 2019

compnerd Mar 21, 2019

compnerd Mar 21, 2019

compnerd Mar 21, 2019

compnerd commented Mar 21, 2019

compnerd commented Mar 21, 2019

[android] Modify test scripts for aarch64 and modern NDKs. #19503

[android] Modify test scripts for aarch64 and modern NDKs. #19503

Conversation

drodriguez commented Sep 24, 2018

compnerd commented Oct 5, 2018

drodriguez commented Oct 8, 2018

compnerd commented Oct 8, 2018

drodriguez commented Oct 10, 2018

drodriguez commented Oct 13, 2018

drodriguez commented Oct 16, 2018

drodriguez commented Oct 24, 2018

drodriguez commented Dec 13, 2018

drodriguez commented Feb 6, 2019

compnerd Mar 21, 2019

Choose a reason for hiding this comment

compnerd Mar 21, 2019

Choose a reason for hiding this comment

compnerd Mar 21, 2019

Choose a reason for hiding this comment

compnerd Mar 21, 2019

Choose a reason for hiding this comment

compnerd commented Mar 21, 2019

compnerd commented Mar 21, 2019