[android] Get build scripts working natively, fix tests and install #29296

finagolfin · 2020-01-18T06:42:08Z

The build scripts assume Android cross-compilation using the NDK, so avoid that configuration if building on an Android host. Fix or disable several tests, and don't install a glibc.modulemap without a sysroot prefix.

Along with this remaining patch to supply an rpath and replace a couple Bionic headers that the Termux app removes, the Swift validation and test suite is down to only 11 failing tests:

diff --git a/lib/Driver/UnixToolChains.cpp b/lib/Driver/UnixToolChains.cpp
index 07a19d9e54..7f79062bd1 100644
--- a/lib/Driver/UnixToolChains.cpp
+++ b/lib/Driver/UnixToolChains.cpp
@@ -381,7 +381,13 @@ std::string toolchains::Android::getTargetForLinker() const {
   }
 }
 
-bool toolchains::Android::shouldProvideRPathToLinker() const { return false; }
+bool toolchains::Android::shouldProvideRPathToLinker() const {
+#if defined(__ANDROID__)
+  return true;
+#else
+  return false;
+#endif
+}
 
 std::string toolchains::Cygwin::getDefaultLinker() const {
   // Cygwin uses the default BFD linker, even on ARM.
diff --git a/stdlib/public/Platform/bionic.modulemap.gyb b/stdlib/public/Platform/bionic.modulemap.gyb
index e44f908265..da77854eb7 100644
--- a/stdlib/public/Platform/bionic.modulemap.gyb
+++ b/stdlib/public/Platform/bionic.modulemap.gyb
@@ -291,11 +291,11 @@ module SwiftGlibc [system] {
         export *
       }
       module sem {
-        header "${GLIBC_ARCH_INCLUDE_PATH}/sys/sem.h"
+        header "${GLIBC_ARCH_INCLUDE_PATH}/linux/sem.h"
         export *
       }
       module shm {
-        header "${GLIBC_ARCH_INCLUDE_PATH}/sys/shm.h"
+        header "${GLIBC_ARCH_INCLUDE_PATH}/linux/shm.h"
         export *
       }
       module inotify {
diff --git a/test/Driver/linker.swift b/test/Driver/linker.swift
index 7b5f0c3425..fd94906760 100644
--- a/test/Driver/linker.swift
+++ b/test/Driver/linker.swift
@@ -29,7 +29,6 @@
 
 // RUN: %swiftc_driver -driver-print-jobs -target armv7-none-linux-androideabi -Ffoo -Fsystem car -F cdr -framework bar -Lbaz -lboo -Xlinker -undefined %s 2>&1 > %t.android.txt
 // RUN: %FileCheck -check-prefix ANDROID-armv7 %s < %t.android.txt
-// RUN: %FileCheck -check-prefix ANDROID-armv7-NEGATIVE %s < %t.android.txt
 
 // RUN: %swiftc_driver -driver-print-jobs -target x86_64-unknown-windows-cygnus -Ffoo -Fsystem car -F cdr -framework bar -Lbaz -lboo -Xlinker -undefined %s 2>&1 > %t.cygwin.txt
 // RUN: %FileCheck -check-prefix CYGWIN-x86_64 %s < %t.cygwin.txt
@@ -228,13 +227,13 @@
 // ANDROID-armv7-DAG: -lswiftCore
 // ANDROID-armv7-DAG: -L [[STDLIB_PATH:[^ ]+(/|\\\\)lib(/|\\\\)swift]]
 // ANDROID-armv7-DAG: -target armv7-unknown-linux-androideabi
+// ANDROID-armv7-DAG: -Xlinker -rpath -Xlinker [[STDLIB_PATH]]
 // ANDROID-armv7-DAG: -F foo -iframework car -F cdr
 // ANDROID-armv7-DAG: -framework bar
 // ANDROID-armv7-DAG: -L baz
 // ANDROID-armv7-DAG: -lboo
 // ANDROID-armv7-DAG: -Xlinker -undefined
 // ANDROID-armv7: -o linker
-// ANDROID-armv7-NEGATIVE-NOT: -Xlinker -rpath
 
 // CYGWIN-x86_64: swift
 // CYGWIN-x86_64: -o [[OBJECTFILE:.*]]

The remaining failures are all related to RUNPATH issues discussed in #23208 and the Termux clang not coming with asan/tsan sanitizer support. I can add a couple tests for the new build-script build path if wanted.

finagolfin · 2020-01-18T06:46:55Z

test/Driver/Dependencies/crash-new.swift

@davidungar, I guess this tee dump and the one below were just for debugging, so I'm removing them since Android doesn't have /tmp.

stdlib/public/Platform/CMakeLists.txt

tools/SourceKit/tools/sourcekitd-test/CMakeLists.txt

finagolfin · 2020-01-18T07:46:07Z

utils/build-script

This invocation simply checks if the build-script is running on an Android host by utilizing host_target()'s if 'ANDROID_DATA' in os.environ, but I can just look for that Android environment variable directly here if that's more clear.

I think it reads better than checking an environment variable. The comment also helps. You might want to add that build_android = False only effect is not providing the NDK variables to CMake.

I hate these build-scripts so much...

Never mind, changed my mind after reading the longer comment I wrote, as this comment really covers them both. android and build_android both simply control the NDK configuration, and this check makes sure both are turned off on an Android host.

finagolfin · 2020-01-24T21:27:40Z

@Rostepher, you seem to deal with the build-script a lot. @drodriguez, any Android input is welcome, should be pretty trivial.

drodriguez

I would love to see different PRs for the different kind of changes that exist in this PR. There are changes unrelated to Android and generally useful that deserve explanation.

In the case of the changes for Android, I would not mind a rethink of the build-script changes, because we are creating patches on a very complicated script, and not thinking holistically. In my opinion the flag names previous usage are not helping you, but the patches make them even worse. Obviously nobody was trying before to support a previously cross-compiled platform with the build-script, so that's part of the problem.

stdlib/public/Platform/CMakeLists.txt

test/ClangImporter/Dispatch_test.swift

test/Driver/Dependencies/crash-new.swift

tools/SourceKit/tools/sourcekitd-test/CMakeLists.txt

drodriguez · 2020-01-25T20:48:32Z

utils/build-script

I think it reads better than checking an environment variable. The comment also helps. You might want to add that build_android = False only effect is not providing the NDK variables to CMake.

I hate these build-scripts so much...

drodriguez · 2020-01-25T21:36:49Z

utils/build-script-impl

I think the intention of this function is for cross-compiled targets, which should not be your case, and it will also apply to the NDK compilation route unintentionally.

I think the right point is probably closer to where the other usage of ANDROID_API_LEVEL happens (around L563 in my copy).

I will simply discard this part, and use something like the following there:

if [[ "${ANDROID_API_LEVEL}"]]; then cmake_options=( "${cmake_options[@]}" -DSWIFT_ANDROID_API_LEVEL:STRING="${ANDROID_API_LEVEL}" ) fi if [[ ! "${SKIP_BUILD_ANDROID}" ]]; then cmake_options=( # the rest without ANDROID_API_LEVEL line

Unless I am misunderstanding something here, the values you provide are the defaults, and the only significant change is the API level (which is not applied to you because confusingly the build-script passes --skip-android-build to the build-script-impl).

Yes, this is really meant for cross-compiling the host toolchain, but I need the host triple and API level when natively compiling the host toolchain on Android too, and since these flags are applied for native compilation of the host toolchain too, I went ahead and filled this out, following the instructions above to specify llvm_target_arch too.

I need the host triple because I've always needed to set LLVM_DEFAULT_TARGET_TRIPLE on Android (scroll down to my build-script-util patch in that long comment to see how I used to hack it in before), because LLVM assumes that it won't run on Android and it can't auto-detect the default triple for Android. I just found that this SWIFT_HOST_TRIPLE is ultimately used to set LLVM_DEFAULT_TARGET_TRIPLE, so I set it this way instead.

As for the API level, I needed it for native compilation so I stuck it here, but your suggestion is better so I will move that one alone, while keeping these other two here.

OK. I didn't know that LLVM didn't autodetect Android triple. As a workaround, we can accept this, but I would not mind a comment explain why it is needed. I would also suggest that this might be something that LLVM should actually do, and upstreaming such patch might be interesting.

No comment needed, as this triple is also definitely needed if cross-compiling the host toolchain, which will be the focus of my next pull. I have not looked into where LLVM auto-detects this, always manually specifying it instead, but you're right that it is worth adding.

drodriguez · 2020-01-25T21:57:59Z

utils/build_swift/build_swift/driver_arguments.py

I think the problem is that a lot of logic is tied to args.android, which at this point means "cross compile Android with the NDK", but having this args.build_android being false for native Android builds is just plain confusing.

In also think that you might want to move the check to the L248, instead of here, because otherwise the test_android_host = False still happens in native compilation, and that means that the executable tests will not happen by default. Something like if not args.build_android or not StdlibDeploymentTarget.Android.contains(StdlibDeploymentTarget.host_target().name)

a lot of logic is tied to args.android, which at this point means "cross compile Android with the NDK", but having this args.build_android being false for native Android builds is just plain confusing.

I found it confusing that there's a pair of seemingly redundant booleans to enable building for each platform, ie android/build_android or ios/build_ios, but I'm guessing it has to do with options parsing so that you can pass both --android and --skip-build-android and not have weird races or ordering issues.

You are right that it's confusing that these variables really mean the Android NDK config, but I tried to clear that up with the comments and I doubt anyone will touch the native host config after this, so probably fine.

If you have some suggestion to clarify it by internal variable renaming or something, e.g. android becomes android_ndk, just let me know.

otherwise the test_android_host = False still happens in native compilation, and that means that the executable tests will not happen by default.

No, that was intentional, I don't want the host tests with adb to run. if android_host_test were set to true, that's when the executable tests are disabled, not the other way around. Maybe you're thinking of some older config in build-script-impl, but that's gone now.

drodriguez · 2020-01-25T22:05:46Z

@swift-ci please test

swift-ci · 2020-01-25T23:09:01Z

Build failed
Swift Test OS X Platform
Git Sha - ea53bd8a7b1f277006679188e67b745c2aa27ee2

finagolfin · 2020-01-27T20:32:00Z

I would love to see different PRs for the different kind of changes that exist in this PR. There are changes unrelated to Android and generally useful that deserve explanation.

I've spun out all the non-Android-specific changes as separate pulls, with the exception of the single Sanitizer test.

I would not mind a rethink of the build-script changes, because we are creating patches on a very complicated script,

While I agree that the distinction that the android/build_android variables are only for the NDK config is not that clear from the variable names alone, it's only three small changes and they're clearly commented to indicate what's going on. Considering that very few people will ever build on Android itself, so this build-script config will likely never change after this, I think that's okay, but as I said above, I'm open to any further variable renaming or other suggestions you have in mind.

I've updated this pull to make the API level move that you recommended in build-script-impl, and to fix the single python linting test failure that caused the mac smoke test to fail.

drodriguez

You probably need to rebase this to resolve the conflicts in the build-script.

test/ClangImporter/Dispatch_test.swift

stdlib/public/Platform/CMakeLists.txt

drodriguez · 2020-01-28T06:16:45Z

utils/build-script-impl

OK. I didn't know that LLVM didn't autodetect Android triple. As a workaround, we can accept this, but I would not mind a comment explain why it is needed. I would also suggest that this might be something that LLVM should actually do, and upstreaming such patch might be interesting.

finagolfin · 2020-01-28T12:13:22Z

Rebased and made the modulemap install change you wanted, along with an explanatory comment.

finagolfin · 2020-02-02T09:13:39Z

Ping, ready to test?

Rostepher · 2020-02-02T09:35:58Z

@swift-ci please test

The build scripts assume Android cross-compilation using the NDK, so avoid that configuration if building on an Android host. Fix or disable some tests, and don't install a glibc.modulemap without a native sysroot prefix.

finagolfin · 2020-02-05T06:11:06Z

Updated this pull with one last fix, setting HOST_CXX_HEADERS_DIR properly in Termux.

@drodriguez, addressed all your comments, let me know if there's anything else.

finagolfin · 2020-02-11T10:01:53Z

I used some of these patches for a Termux pull I just submitted to build the Swift 5.1.4 toolchain on an Android device, termux/termux-packages#4895 (also wrote up some instructions on the forum). It would be good to get this in and then I'll submit a pull for the 5.2 branch, so that the next release can be built for Android as easily as possible.

drodriguez · 2020-02-18T00:49:28Z

I would love a better solution for the patches, but I don't know if it is worth the time.

@swift-ci please test

finagolfin · 2020-02-24T06:32:18Z

Ping, good to go? Also, please take a look at #30020, which fixes broken stdlib and other shared libraries for Android in master for the last month.

I just updated master to the Feb. 20 snapshot and built and tested everything natively on Android, worst run of the Swift validation suite I ever had on Android with 306 failures (prior worst run was 51 six months ago, but was down to just 11 with my last Jan. 13 snapshot build from master). ~~213 are related to sourcekitd-test breaking again with another linker error when run~~ Never mind, this is because I had the Swift 5.1.4 Termux package installed and sourcekitd-test was picking up the older libsourcekitdInProc.so instead, all 213 pass again after uninstalling that older package. Another 36 are ClangImporter issues, maybe because I just changed which of the clang headers and other components are installed, and the remaining 57 across a bunch of areas. I'll start looking into those.

Update: Turns out most of the new failures were related to my switching master over to using the same clang patches and setting C_INCLUDE_DIRS as in the Termux package, rather than the similar patches I used before. With one last tweak to this new approach, down to only 13 failures, pretty much the same as before.

finagolfin commented Jan 18, 2020

View reviewed changes

stdlib/public/Platform/CMakeLists.txt Outdated Show resolved Hide resolved

finagolfin commented Jan 18, 2020

View reviewed changes

tools/SourceKit/tools/sourcekitd-test/CMakeLists.txt Outdated Show resolved Hide resolved

finagolfin commented Jan 18, 2020

View reviewed changes

finagolfin requested review from Rostepher and drodriguez January 22, 2020 07:58

finagolfin force-pushed the droid branch from 1041c28 to ea53bd8 Compare January 24, 2020 21:26

drodriguez requested changes Jan 25, 2020

View reviewed changes

finagolfin mentioned this pull request Jan 26, 2020

[sourcekitd-test] Remove LLVM Core static library that is getting linked twice #29454

Merged

finagolfin force-pushed the droid branch 3 times, most recently from 04b80ad to 3fe7fb5 Compare January 27, 2020 20:21

finagolfin force-pushed the droid branch 2 times, most recently from dadeb61 to 4910593 Compare January 27, 2020 20:36

drodriguez reviewed Jan 28, 2020

View reviewed changes

finagolfin force-pushed the droid branch from 4910593 to 39f4d39 Compare January 28, 2020 12:11

finagolfin force-pushed the droid branch from 39f4d39 to 3f4109b Compare February 5, 2020 06:07

finagolfin mentioned this pull request Feb 14, 2020

[Docs] [android] Update to the latest NDK 21, fix commands, and remove cruft #29439

Merged

drodriguez merged commit 073e44d into swiftlang:master Feb 25, 2020

finagolfin deleted the droid branch February 25, 2020 06:55

finagolfin mentioned this pull request Feb 29, 2020

[SourceKit] don't install libdispatch and libBlocksRuntime twice, outside of Mac/Windows #30096

Merged

drodriguez mentioned this pull request Dec 5, 2020

[android] Support building the host tools and with the static stdlib #34963

Merged

[android] Get build scripts working natively, fix tests and install #29296

[android] Get build scripts working natively, fix tests and install #29296

Uh oh!

Conversation

finagolfin commented Jan 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finagolfin commented Jan 24, 2020

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

drodriguez commented Jan 25, 2020

Uh oh!

swift-ci commented Jan 25, 2020

Uh oh!

finagolfin commented Jan 27, 2020

Uh oh!

drodriguez left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

finagolfin commented Jan 28, 2020

Uh oh!

finagolfin commented Feb 2, 2020

Uh oh!

Rostepher commented Feb 2, 2020

Uh oh!

finagolfin commented Feb 5, 2020

Uh oh!

finagolfin commented Feb 11, 2020

Uh oh!

drodriguez commented Feb 18, 2020

Uh oh!

finagolfin commented Feb 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

finagolfin commented Feb 24, 2020 •

edited

Loading