[android] Enable several C++ Interop and other tests #74883

finagolfin · 2024-07-02T02:58:05Z

Also, update NDK and Ubuntu versions to the latest in the Android doc, enable inline bridging when building the compiler for Android, and update the default arch set by build-script to AArch64, as that is by far the most commonly used now.

These C++ Interop tests were all disabled over the years because of build errors with the Android NDK, but @hyp's recent pull #72161 adding a modularized Bionic module map fixed them and the inline bridging issue in the compiler itself. Alex, let me know if these tests all pass for you too.

@drodriguez, perhaps you'd like to take a look at this pull too.

finagolfin · 2024-07-02T03:14:24Z

test/Interop/SwiftToCxx/core/unsigned-return-type-no-zext.cpp

@@ -4,7 +4,6 @@
 // RUN: %FileCheck %s < %t/ir.ll

 // UNSUPPORTED: OS=windows-msvc
-// XFAIL: OS=linux-android, OS=linux-androideabi


I disabled this last year, 12f20ec, because it was failing with the clang 14 in NDK 25 at that time, but it builds fine with clang 17 in NDK 26 now.

finagolfin · 2024-07-02T03:19:58Z

test/Interpreter/llvm_link_time_opt.swift

@@ -1,8 +1,6 @@
 // UNSUPPORTED: OS=windows-msvc
 // static library is not well supported yet on Windows

-// XFAIL: OS=linux-android, OS=linux-androideabi


Similarly, I disabled this last year because it didn't work with clang 14 in NDK 25, but it works with clang 17 in NDK 26 now.

finagolfin · 2024-07-02T03:37:54Z

test/lit.cfg

@@ -615,7 +615,7 @@ else:
            config.swift_driver_test_options,
        )
    )
-    toolchain_lib_dir = make_path(config.swift_lib_dir, 'swift', host_os)
+    toolchain_lib_dir = make_path(config.swift_lib_dir, 'swift', 'android' if kIsAndroid else host_os)


This path was just added late last year and broke building several macro tests natively on Android that have started using it.

Hmm, the host_os should be android if we are building for android right?

host_os is parsed from the host triple, which on Android is linux. That heuristic works on most platforms, but not for an Android host.

Ah, that is annoying. Thanks for the explanation!

compnerd · 2024-07-02T15:06:22Z

test/lit.cfg

@@ -2428,8 +2428,12 @@ elif not kIsWindows and not run_os == 'wasi':
        lit_config.note('Testing with the just-built libraries')

    lit_config.note('Library load path: {0}'.format(os.path.pathsep.join(target_stdlib_path)))
+    env_path = "/usr/bin/env "
+    # Android doesn't have /usr/bin/, so just grab whatever `env` is in the PATH.


It does have /bin/env which is pretty standard, why not use that?

I'll try running the tests with that instead.

compnerd · 2024-07-02T15:07:00Z

utils/build_swift/build_swift/driver_arguments.py

@@ -1403,7 +1403,7 @@ def create_argument_parser():

    option('--android-arch', store,
           choices=['armv7', 'aarch64', 'x86_64'],
-           default='armv7',
+           default='aarch64',


This seems unrelated? It's a welcome change, just seems not related to the test enablement.

Unrelated to the title, yes, but these are all Android changes, and I specifically mention these non-test changes in the commit message.

Sure, but it still helps to keep changes focused :)

Right, this should probably be a separate patch, since it isn't just a test-only change.

Right, this should probably be a separate patch, since it isn't just a test-only change.

It never says it's a test-only change, the label used is [android] and the commit message lists the non-test changes.

If you would like, I can split this into separate commits, but I see no reason to go farther.

The PR title doesn't mention any non-test changes, which makes it confusing. I initially assumed that this is a test-only change.
I think the arch change should be its own commit.

OK, I will split this commit up.

compnerd · 2024-07-03T15:27:56Z

@swift-ci please smoke test

finagolfin · 2024-07-03T18:01:42Z

Sigh, File "/home/build-user/swift/test/lit.cfg", line 618, in <module> toolchain_lib_dir = make_path(config.swift_lib_dir, 'swift', 'android' if kIsAndroid else host_os) NameError: name 'kIsAndroid' is not defined.

I had simply hacked in make_path(config.swift_lib_dir, 'swift', 'android') in my local testing and then cleaned it up with the kIsAndroid check before submitting, without checking if that variable had been defined yet.

I'll clean that up and run the validation suite natively on Android on the entirety of this pull before asking for another CI run.

finagolfin · 2024-07-10T12:53:44Z

Alright, fixed the lit.cfg error and ran this entire pull through the compiler validation suite natively on Android, before splitting it up into narrower commits and rebasing it here.

This is ready for review and another CI run.

test/SILOptimizer/stack-promotion-crash.swift

finagolfin · 2024-07-10T13:01:31Z

test/lit.cfg

+# Manually set the host OS on Android, because the host OS from the triple says
+# `linux`.
+if kIsAndroid:
+    host_os = 'android'


Rather than overriding this just for the new toolchain_lib_dir below, I overrode host_os itself on Android, as it is only used for that and target-same-as-host just below, which is only checked by a single macOS-only test, so changing this for Android hosts should be fine.

finagolfin · 2024-07-10T13:03:27Z

utils/build_swift/tests/expected_options.py

@@ -768,7 +768,7 @@ class BuildScriptImplOption(_BaseOption):
    ChoicesOption('--swift-analyze-code-coverage',
                  choices=['false', 'not-merged', 'merged']),
    ChoicesOption('--android-arch',
-                  choices=['armv7', 'aarch64']),
+                  choices=['armv7', 'aarch64', 'x86_64']),


This never triggered before because this just checks if it's a subset, so updating to all three arches supported now.

finagolfin · 2024-07-12T05:21:45Z

@kateinoigakukun, need a CI run here.

@egorzhdan, maybe you can review.

finagolfin · 2024-07-18T08:44:25Z

@rauhul, need a CI run here.

Also, fix lit.cfg for running the test suite natively in Android and mark one SILOptimizer executable_test as such.

…r Android

…the latest

…s by far the most commonly used now

finagolfin · 2024-07-22T21:08:50Z

@bnbarham, ready for a CI run here.

bnbarham · 2024-07-22T21:34:08Z

@swift-ci please test

finagolfin · 2024-07-23T13:38:11Z

Alright, passed CI, just need @egorzhdan or some one to review.

egorzhdan

C++ interop changes LGTM

finagolfin · 2024-07-23T14:40:30Z

@etcwilde, if you would review the few remaining Android changes, we can get this in.

finagolfin · 2024-07-23T15:35:31Z

@bnbarham, ready for merge.

finagolfin · 2024-07-26T12:37:38Z

@hyp, would you merge?

bnbarham · 2024-07-26T18:18:21Z

Sorry, missed initial ping. Merged!

finagolfin requested review from zoecarver, hyp, egorzhdan and ktoso as code owners July 2, 2024 02:58

finagolfin commented Jul 2, 2024

View reviewed changes

compnerd reviewed Jul 2, 2024

View reviewed changes

finagolfin force-pushed the droid branch from 3cae96f to a6ec383 Compare July 10, 2024 12:47

finagolfin requested a review from eeckstein as a code owner July 10, 2024 12:47

finagolfin commented Jul 10, 2024

View reviewed changes

test/SILOptimizer/stack-promotion-crash.swift Outdated Show resolved Hide resolved

finagolfin commented Jul 10, 2024

View reviewed changes

finagolfin added 4 commits July 20, 2024 17:56

[android][test] Enable several C++ Interop and other tests

15e1c73

Also, fix lit.cfg for running the test suite natively in Android and mark one SILOptimizer executable_test as such.

[android][CMake] Enable inline bridging when building the compiler fo…

3bd6c6b

…r Android

[android][docs] Update NDK/Ubuntu versions and Swift snapshot tag to …

fb7bb19

…the latest

[android][build-script] Update the default arch to AArch64, as that i…

6ede1c2

…s by far the most commonly used now

finagolfin force-pushed the droid branch from a6ec383 to 6ede1c2 Compare July 20, 2024 12:27

egorzhdan approved these changes Jul 23, 2024

View reviewed changes

etcwilde approved these changes Jul 23, 2024

View reviewed changes

bnbarham merged commit d281fb8 into swiftlang:main Jul 26, 2024
5 checks passed

finagolfin deleted the droid branch July 27, 2024 03:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[android] Enable several C++ Interop and other tests #74883

[android] Enable several C++ Interop and other tests #74883

finagolfin commented Jul 2, 2024

finagolfin Jul 2, 2024

finagolfin Jul 2, 2024

finagolfin Jul 2, 2024

compnerd Jul 3, 2024

finagolfin Jul 3, 2024

compnerd Jul 3, 2024

compnerd Jul 2, 2024

finagolfin Jul 3, 2024

finagolfin Jul 10, 2024

compnerd Jul 2, 2024

finagolfin Jul 3, 2024

compnerd Jul 3, 2024

egorzhdan Jul 3, 2024

finagolfin Jul 3, 2024

egorzhdan Jul 3, 2024

finagolfin Jul 3, 2024

finagolfin Jul 10, 2024

compnerd commented Jul 3, 2024

finagolfin commented Jul 3, 2024

finagolfin commented Jul 10, 2024 •

edited

Loading

finagolfin Jul 10, 2024

finagolfin Jul 10, 2024

finagolfin commented Jul 12, 2024

finagolfin commented Jul 18, 2024

finagolfin commented Jul 22, 2024

bnbarham commented Jul 22, 2024

finagolfin commented Jul 23, 2024

egorzhdan left a comment

finagolfin commented Jul 23, 2024

finagolfin commented Jul 23, 2024

finagolfin commented Jul 26, 2024

bnbarham commented Jul 26, 2024

[android] Enable several C++ Interop and other tests #74883

[android] Enable several C++ Interop and other tests #74883

Conversation

finagolfin commented Jul 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

compnerd commented Jul 3, 2024

finagolfin commented Jul 3, 2024

finagolfin commented Jul 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

finagolfin commented Jul 12, 2024

finagolfin commented Jul 18, 2024

finagolfin commented Jul 22, 2024

bnbarham commented Jul 22, 2024

finagolfin commented Jul 23, 2024

egorzhdan left a comment

Choose a reason for hiding this comment

finagolfin commented Jul 23, 2024

finagolfin commented Jul 23, 2024

finagolfin commented Jul 26, 2024

bnbarham commented Jul 26, 2024

finagolfin commented Jul 10, 2024 •

edited

Loading