[android] A few tweaks for native compilation and to get more tests working #27167

finagolfin · 2019-09-13T10:34:48Z

Now that CMAKE_HOST_SYSTEM_NAME and CMAKE_SYSTEM_NAME are set by default to Android in the Termux app, termux/termux-packages#4222, make the needed tweaks. Several tests were adapted to work natively on Android too and includes the start of native Android platform support in the build-script.

In addition, I had to apply this cut-down version of a previously pasted patch before running the same build-script invocation shown there:

diff --git a/lib/Driver/UnixToolChains.cpp b/lib/Driver/UnixToolChains.cpp
index 307c552048..f226b0f159 100644
--- a/lib/Driver/UnixToolChains.cpp
+++ b/lib/Driver/UnixToolChains.cpp
@@ -380,7 +380,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 69564568bf..3ba95c895a 100644
--- a/stdlib/public/Platform/bionic.modulemap.gyb
+++ b/stdlib/public/Platform/bionic.modulemap.gyb
@@ -216,6 +216,14 @@ module SwiftGlibc [system] {
       header "${GLIBC_INCLUDE_PATH}/fcntl.h"
       export *
     }
+    module bits {
+      export *
+
+    module fcntl {
+      header "${GLIBC_INCLUDE_PATH}/bits/fcntl.h"
+      export *
+    }
+    }
     module fnmatch {
       header "${GLIBC_INCLUDE_PATH}/fnmatch.h"
       export *
@@ -287,11 +295,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/utils/build-script b/utils/build-script
index 338f229e91..ca8e9eb853 100755
--- a/utils/build-script
+++ b/utils/build-script
@@ -187,7 +187,7 @@ class BuildScriptInvocation(object):
         android_tgts = [tgt for tgt in args.stdlib_deployment_targets
                         if StdlibDeploymentTarget.Android.contains(tgt)]
         if not args.android and len(android_tgts) > 0:
-            args.android = True
+            #args.android = True
             args.build_android = False
 
         # Include the Darwin supported architectures in the CMake options.
@@ -551,6 +551,7 @@ class BuildScriptInvocation(object):
                 "--android-icu-i18n-include", args.android_icu_i18n_include,
                 "--android-icu-data", args.android_icu_data,
             ]
+        impl_args += ["--android-api-level", args.android_api_level]
         if args.android_deploy_device_path:
             impl_args += [
                 "--android-deploy-device-path",
diff --git a/utils/build-script-impl b/utils/build-script-impl
index 5758487f1b..7f76c62618 100755
--- a/utils/build-script-impl
+++ b/utils/build-script-impl
@@ -451,6 +451,8 @@ function set_build_options_for_host() {
             SWIFT_HOST_VARIANT_SDK="HAIKU"
             SWIFT_HOST_VARIANT_ARCH="x86_64"
             ;;
+        android-*)
+            ;;
         linux-*)
             SWIFT_HOST_VARIANT="linux"
             SWIFT_HOST_VARIANT_SDK="LINUX"
@@ -718,6 +720,7 @@ function set_build_options_for_host() {
     llvm_cmake_options+=(
         -DLLVM_TOOL_COMPILER_RT_BUILD:BOOL="$(false_true ${SKIP_BUILD_COMPILER_RT})"
         -DLLVM_BUILD_EXTERNAL_COMPILER_RT:BOOL="$(false_true ${SKIP_BUILD_COMPILER_RT})"
+        -DLLVM_DEFAULT_TARGET_TRIPLE=aarch64-unknown-linux-android
     )
 
     # If we are asked to not generate test targets for LLVM and or Swift,
@@ -2373,6 +2376,7 @@ for host in "${ALL_HOSTS[@]}"; do
                     -DSWIFT_TOOLS_ENABLE_LTO:STRING="${SWIFT_TOOLS_ENABLE_LTO}"
                     -DSWIFT_BUILD_RUNTIME_WITH_HOST_COMPILER:BOOL=$(true_false "${BUILD_RUNTIME_WITH_HOST_COMPILER}")
                     -DLIBDISPATCH_CMAKE_BUILD_TYPE:STRING="${LIBDISPATCH_BUILD_TYPE}"
+                    -DSWIFT_ANDROID_API_LEVEL:STRING="${ANDROID_API_LEVEL}"
                     "${swift_cmake_options[@]}"
                 )

I'll fill out these remaining build-script changes and get them working with its tests before submitting that portion in a later pull.

As a result of this pull, I'm down to only 19 failing tests from the Swift validation and test suite when run on an Android host, most having to do with compiler-rt sanitizers unsupported on Android or the RUNPATH issues with shared libraries mentioned previously. A few more are related to libdispatch issues, ~~will see if one of those can be fixed and then~~ (never mind, looks like stdlib/DispatchData isn't run on the linux CI anyway, because libdispatch.so was moved around in the libdispatch build directory and that will likely need adjusting also, so I'll punt that libdispatch test for later) this pull ~~should be~~ is ready to go.

finagolfin · 2019-09-13T10:46:27Z

cmake/modules/AddSwift.cmake

+    # flags manually in add_swift_target_library instead, see there with
+    # variable `swiftlib_link_flags_all`.
+    if(SWIFTLIB_SINGLE_TARGET_LIBRARY)
+      set_target_properties("${target}" PROPERTIES NO_SONAME TRUE)


Turns out this caused the Syntax/Parser issues because swift-syntax-parser-test no longer linked properly, as lld would try to place a dependency on lib/lib_InternalSwiftSyntaxParser.so if that host library's soname wasn't set. This fixes that by only removing the soname for target libraries on Android.

@vgorloff, you added this, look good?

Not 100% sure. I see that difference in junction if(SWIFTLIB_SINGLE_TARGET_LIBRARY) .... The fix I made in past also was addressed link issues when building single targets, like libswiftCore.so, libswiftSwiftOnoneSupport.so, etc. Seems should be OK.

finagolfin · 2019-09-13T10:48:17Z

cmake/modules/SwiftConfigureSDK.cmake

@@ -262,6 +262,8 @@ macro(configure_sdk_unix name architectures)
        set(_swift_android_prebuilt_build linux-x86_64)
      elseif(CMAKE_HOST_SYSTEM_NAME STREQUAL Windows)
        set(_swift_android_prebuilt_build Windows-x86_64)
+      elseif(CMAKE_HOST_SYSTEM_NAME STREQUAL Android)
+        # When building natively on an Android host, there's no NDK or prebuilt suffix.


Didn't hit this before because it was set to Linux, before I patched CMake to set it to Android on an Android host.

finagolfin · 2019-09-13T10:54:07Z

test/lit.cfg

@@ -1219,7 +1222,7 @@ ENV_VAR_PREFIXES = {
    'appletvsimulator': SIMULATOR_ENV_PREFIX,
    'android': 'ANDROID_CHILD_'
 }
-TARGET_ENV_PREFIX = ENV_VAR_PREFIXES.get(config.target_sdk_name, "")
+TARGET_ENV_PREFIX = ENV_VAR_PREFIXES.get(config.target_sdk_name, "") if not kIsAndroid else ""


This prefix was why two hashing tests were failing before, as this prefix is removed by the adb script for a remote Android test run.

finagolfin · 2019-09-13T11:00:02Z

test/lit.cfg

@@ -1331,7 +1334,7 @@ def source_compiler_rt_libs(path):
                                        and config.compiler_rt_platform in lib])

 compiler_rt_dir = make_path(test_resource_dir, 'clang', 'lib',
-                            platform.system().lower())
+                            platform.system().lower() if not kIsAndroid else 'android')


Several sanitizer tests' shared libraries in the compiler-rt build couldn't be found if this wasn't properly set.

finagolfin · 2019-09-13T11:00:36Z

utils/swift_build_sdk_interfaces.py

+      shared_output_lock = threading.Lock()
+      from multiprocessing.pool import ThreadPool
+      pool = ThreadPool(args.jobs, set_up_child,
+                        (args, shared_output_lock))


I'm not that familiar with Python, so if there's a better way to do this, I'm all ears.

finagolfin · 2019-10-23T07:48:53Z

@drodriguez, I believe you are best qualified to review this pull, would be good to get it in before the 5.2 branch. Also, my CMake patch to build natively on Android, which this pull adapts to, was merged into CMake master yesterday.

finagolfin · 2019-11-10T11:03:35Z

@ddunbar, you reviewed some other Android pulls, mind reviewing this one?

ddunbar · 2019-11-10T16:46:10Z

@buttaface I not really qualified to review for Swift

finagolfin · 2019-11-11T06:43:15Z

Alright, @jrose-apple, you wrote the Python script I modified above, mind reviewing this pull?

jrose-apple · 2019-11-14T18:58:01Z

I'm sorry, I'm no longer at Apple! @CodaFi, can you find someone else to pick this up?

CodaFi · 2019-12-04T03:47:11Z

@buttaface Could you please rebase this patch?

@compnerd Could you review the CMake changes here?

finagolfin

Rebased and added a couple more fixes for native Android.

finagolfin · 2019-12-04T09:00:38Z

lib/Driver/UnixToolChains.cpp

@@ -42,7 +42,8 @@ std::string
 toolchains::GenericUnix::sanitizerRuntimeLibName(StringRef Sanitizer,
                                                 bool shared) const {
  return (Twine("libclang_rt.") + Sanitizer + "-" +
-          this->getTriple().getArchName() + ".a")
+          this->getTriple().getArchName() +
+          (this->getTriple().isAndroid() ? "-android" : "") + ".a")


Needed similar to clang.

finagolfin · 2019-12-04T09:09:37Z

stdlib/public/Platform/bionic.modulemap.gyb

+      module cdefs {
+        header "${GLIBC_ARCH_INCLUDE_PATH}/sys/cdefs.h"
+        export *
+      }


Starting with Android 6, features.h was emptied out and everything moved into sys/cdefs.h instead. This is why I had the earlier __BEGIN_DECLS and other ClangImporter issues with Foundation, which have since repeated with other modulemaps and are now fixed with this header.

I would not mind accepting this patch as a standalone. If you create a new PR, mention me and I will accept.

finagolfin · 2019-12-06T17:02:59Z

@CodaFi, doesn't look like a review is forthcoming. Also, there's nothing that requires any real Android knowledge here, any Swift contributor aware of the build and test process should be competent to review. And since almost all of this pull is gated for Android, and mostly for building the Swift toolchain on an Android host at that, merging should be very low-risk.

We've notified both the Swift committers who were contributing most to Android support earlier this year and neither has responded, likely either because they've moved on to other things- looks like most of their pulls are Windows-related lately- or because this pull doesn't affect them.

I think any other competent Swift reviewer should be able to get this in.

CodaFi · 2019-12-06T17:14:57Z

CMake is twisty enough that Saleem ought to weigh in. The frontend changes LGTM - though I’m not a fan of the way Clang does this in the first place.

CodaFi · 2019-12-06T17:15:09Z

Regardless

@swift-ci please test

finagolfin · 2019-12-06T17:34:07Z

CMake is twisty enough that Saleem ought to weigh in.

Shouldn't be needed, the four small changes are pretty straightforward. The first is simply a revert of my previous CMake detection of native Android builds, now that CMake detects Android itself.

The second only affects the way host libraries are built on an Android host, to account for a recent patch, 62dd6bd, which its author comments above should be fine. The third is obvious, a NOP for an Android host, and the fourth just a cut-n-paste of how Darwin and linux are handled right above.

No real CMake knowledge required. :)

drodriguez

I marked a couple of places that I think are right and I would not mind accepting as independent changes. It would make this other PR slightly smaller and more focused and easier to review.

drodriguez · 2019-12-06T18:17:27Z

stdlib/public/Platform/bionic.modulemap.gyb

+      module cdefs {
+        header "${GLIBC_ARCH_INCLUDE_PATH}/sys/cdefs.h"
+        export *
+      }


I would not mind accepting this patch as a standalone. If you create a new PR, mention me and I will accept.

drodriguez · 2019-12-06T18:18:28Z

test/Driver/multi-threaded.swift

@@ -5,7 +5,8 @@
 // RUN: %target-swiftc_driver -driver-print-jobs -module-name=ThisModule -wmo -num-threads 4 %S/Inputs/main.swift %s -c | %FileCheck -check-prefix=OBJECT %s
 // RUN: cd %t && %target-swiftc_driver -parseable-output -module-name=ThisModule -wmo -num-threads 4 %S/Inputs/main.swift %s -c 2> %t/parseable-output
 // RUN: cat %t/parseable-output | %FileCheck -check-prefix=PARSEABLE %s
-// RUN: cd %t && env TMPDIR=/tmp %swiftc_driver -driver-print-jobs -module-name=ThisModule -wmo -num-threads 4 %S/Inputs/main.swift %s -o a.out | %FileCheck -check-prefix=EXEC %s
+// RUN: %empty-directory(%t/tmp)
+// RUN: cd %t && env TMPDIR=%t/tmp/ %swiftc_driver -driver-print-jobs -module-name=ThisModule -wmo -num-threads 4 %S/Inputs/main.swift %s -o a.out | %FileCheck -check-prefix=EXEC %s


I would not mind accepting this patch as a standalone. If you create a new PR, mention me and I will accept. It is also better for other platforms than Android.

swift-ci · 2019-12-06T18:22:36Z

Build failed
Swift Test OS X Platform
Git Sha - 79ff645efa7cbe0ce1eb94a6ba84663b91592475

CodaFi · 2019-12-06T18:24:28Z

macOS failure was just XFAIL’d out

@swift-ci please smoke test macOS platform

finagolfin · 2019-12-06T19:23:15Z

It would make this other PR slightly smaller and more focused and easier to review.

While those are two of the only three changes that aren't limited to building on an Android host (the third being the single frontend change, though I don't think anyone has tried cross-compiling and testing those sanitizers on Android, as I have natively), they're sufficiently simple and relevant to this pull that I'd rather just keep it all here. I can split them into separate commits as part of this pull, if that makes any difference.

I think I've fixed the single whitespace linting issue that was causing the macOS smoke test to fail.

…orking Now that CMAKE_HOST_SYSTEM_NAME and CMAKE_SYSTEM_NAME are set by default to Android in the Termux app, make the needed tweaks. Some tests were adapted to work natively on Android too, adds sys/cdefs.h to the Bionic modulemap, and includes the start of native Android platform support in the build-script.

CodaFi · 2019-12-06T19:33:16Z

@swift-ci please python lint

CodaFi · 2019-12-06T19:37:02Z

Yeah, that wasn't a good run.

@swift-ci please python lint

CodaFi · 2019-12-06T19:38:20Z

Hmph, they're all running bad.

@swift-ci please smoke test

shahmishal · 2019-12-06T20:45:47Z

@swift-ci please python lint

shahmishal · 2019-12-06T20:52:21Z

@swift-ci please python lint

CodaFi · 2019-12-06T21:02:57Z

Well, let's not keep this blocked on us then.

⛵️

finagolfin · 2019-12-06T21:12:17Z

Thanks for finally getting this in, now I just need to clean up and submit those last few build-script changes pasted above.

finagolfin commented Sep 13, 2019

View reviewed changes

finagolfin force-pushed the droid branch from f25bbee to ca44823 Compare September 20, 2019 13:14

finagolfin force-pushed the droid branch from ca44823 to 986be3d Compare October 27, 2019 11:35

finagolfin mentioned this pull request Nov 1, 2019

Android: add native platform support swiftlang/swift-package-manager#2396

Merged

finagolfin force-pushed the droid branch from 986be3d to 020c487 Compare November 10, 2019 08:44

CodaFi requested a review from compnerd December 4, 2019 03:47

finagolfin force-pushed the droid branch from 020c487 to 79ff645 Compare December 4, 2019 08:55

finagolfin commented Dec 4, 2019

View reviewed changes

drodriguez reviewed Dec 6, 2019

View reviewed changes

finagolfin force-pushed the droid branch from 79ff645 to 318b872 Compare December 6, 2019 19:19

finagolfin force-pushed the droid branch from 318b872 to 14cc620 Compare December 6, 2019 19:32

CodaFi merged commit 9dab211 into swiftlang:master Dec 6, 2019

[android] A few tweaks for native compilation and to get more tests working #27167

[android] A few tweaks for native compilation and to get more tests working #27167

Uh oh!

Conversation

finagolfin commented Sep 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

finagolfin commented Oct 23, 2019

Uh oh!

finagolfin commented Nov 10, 2019

Uh oh!

ddunbar commented Nov 10, 2019

Uh oh!

finagolfin commented Nov 11, 2019

Uh oh!

jrose-apple commented Nov 14, 2019

Uh oh!

CodaFi commented Dec 4, 2019

Uh oh!

finagolfin left a comment

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 Dec 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

finagolfin commented Dec 6, 2019

Uh oh!

drodriguez left a comment

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!

swift-ci commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

finagolfin commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

shahmishal commented Dec 6, 2019

Uh oh!

shahmishal commented Dec 6, 2019

Uh oh!

CodaFi commented Dec 6, 2019

Uh oh!

finagolfin commented Dec 6, 2019

Uh oh!

Uh oh!

finagolfin commented Sep 13, 2019 •

edited

Loading

finagolfin commented Dec 6, 2019 •

edited

Loading