Skip to content

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

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.

Sign up for GitHub

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

Merged
drodriguez merged 1 commit into from
Feb 25, 2020
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 5 additions & 3 deletions stdlib/public/Platform/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,9 +99,11 @@ foreach(sdk ${SWIFT_SDKS})

list(APPEND glibc_modulemap_target_list ${glibc_modulemap_target})

# If this SDK is a target for a non-native host, create a native modulemap
# without a sysroot prefix. This is the one we'll install instead.
if(NOT "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_ARCH_${arch}_PATH}" STREQUAL "/")
# If this SDK is a target for a non-native host, except if it's for Android
# with its own native sysroot, create a native modulemap without a sysroot
# prefix. This is the one we'll install instead.
if(NOT "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_ARCH_${arch}_PATH}" STREQUAL "/" AND
NOT (${sdk} STREQUAL ANDROID AND NOT "${SWIFT_ANDROID_NATIVE_SYSROOT}" STREQUAL ""))
set(glibc_sysroot_relative_modulemap_out "${module_dir}/sysroot-relative-modulemaps/glibc.modulemap")

string(REPLACE "${SWIFT_SDK_${SWIFT_HOST_VARIANT_SDK}_ARCH_${arch}_PATH}"
Expand Down
1 change: 1 addition & 0 deletions test/ClangImporter/Dispatch_test.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

// REQUIRES: libdispatch
// UNSUPPORTED: OS=linux-gnu
// UNSUPPORTED: OS=linux-android

import Dispatch

Expand Down
2 changes: 1 addition & 1 deletion test/Sanitizers/asan_recover.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

// FIXME: We need this so we can flush stdout but this won't
// work on other Platforms (e.g. Windows).
#if os(Linux)
#if canImport(Glibc)
import Glibc
#else
import Darwin.C
Expand Down
1 change: 1 addition & 0 deletions test/stdlib/Dispatch.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// REQUIRES: executable_test
// REQUIRES: libdispatch
// UNSUPPORTED: OS=linux-gnu
// UNSUPPORTED: OS=linux-android

import Dispatch
import StdlibUnittest
Expand Down
1 change: 1 addition & 0 deletions test/stdlib/DispatchTypes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@

// REQUIRES: libdispatch
// UNSUPPORTED: OS=linux-gnu
// UNSUPPORTED: OS=linux-android

import Dispatch

Expand Down
10 changes: 9 additions & 1 deletion utils/build-script
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -321,7 +321,11 @@ def apply_default_arguments(toolchain, args):
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
# If building natively on an Android host, avoid the NDK
# cross-compilation configuration.
if not StdlibDeploymentTarget.Android.contains(StdlibDeploymentTarget
.host_target().name):
args.android = True
args.build_android = False

# Include the Darwin supported architectures in the CMake options.
Expand Down Expand Up @@ -657,6 +661,10 @@ class BuildScriptInvocation(object):
"--android-icu-i18n-include", args.android_icu_i18n_include,
"--android-icu-data", args.android_icu_data,
]
# If building natively on an Android host, only pass the API level.
if StdlibDeploymentTarget.Android.contains(StdlibDeploymentTarget
.host_target().name):
impl_args += ["--android-api-level", args.android_api_level]
if args.android_deploy_device_path:
impl_args += [
"--android-deploy-device-path",
Expand Down
14 changes: 13 additions & 1 deletion utils/build-script-impl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -460,6 +460,10 @@ function set_build_options_for_host() {
SWIFT_HOST_VARIANT_ARCH=$architecture

case ${host} in
android-aarch64)
SWIFT_HOST_TRIPLE="aarch64-unknown-linux-android"
llvm_target_arch="AArch64"
;;
Copy link
Contributor

@drodriguez drodriguez Jan 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

@finagolfin finagolfin Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@drodriguez drodriguez Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@finagolfin finagolfin Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

linux-armv6)
SWIFT_HOST_TRIPLE="armv6-unknown-linux-gnueabihf"
llvm_target_arch="ARM"
Expand Down Expand Up @@ -1553,12 +1557,18 @@ for host in "${ALL_HOSTS[@]}"; do

swift)

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=(
"${cmake_options[@]}"
-DSWIFT_ANDROID_NDK_PATH:STRING="${ANDROID_NDK}"
-DSWIFT_ANDROID_NDK_GCC_VERSION:STRING="${ANDROID_NDK_GCC_VERSION}"
-DSWIFT_ANDROID_API_LEVEL:STRING="${ANDROID_API_LEVEL}"
-DSWIFT_ANDROID_${ANDROID_ARCH}_ICU_UC:STRING="${ANDROID_ICU_UC}"
-DSWIFT_ANDROID_${ANDROID_ARCH}_ICU_UC_INCLUDE:STRING="${ANDROID_ICU_UC_INCLUDE}"
-DSWIFT_ANDROID_${ANDROID_ARCH}_ICU_I18N:STRING="${ANDROID_ICU_I18N}"
Expand Down Expand Up @@ -2192,6 +2202,8 @@ for host in "${ALL_HOSTS[@]}"; do
HOST_CXX_HEADERS_DIR="$HOST_CXX_DIR/../../usr/include/c++"
elif [[ "$(uname -s)" == "Haiku" ]] ; then
HOST_CXX_HEADERS_DIR="/boot/system/develop/headers/c++"
elif [[ "${ANDROID_DATA}" ]] ; then
HOST_CXX_HEADERS_DIR="$PREFIX/include/c++"
else # Linux
HOST_CXX_HEADERS_DIR="/usr/include/c++"
fi
Expand Down
6 changes: 5 additions & 1 deletion utils/build_swift/build_swift/driver_arguments.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -223,7 +223,11 @@ def _apply_default_arguments(args):
args.test_watchos_simulator = False

if not args.build_android:
args.test_android = False
# If building natively on an Android host, allow running the test suite
# without the NDK config.
if not StdlibDeploymentTarget.Android.contains(StdlibDeploymentTarget
.host_target().name):
args.test_android = False
Copy link
Contributor

@drodriguez drodriguez Jan 25, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Member Author

@finagolfin finagolfin Jan 27, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

args.test_android_host = False

if not args.test_android:
Expand Down