-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[android][test] Fix or disable the remaining failing tests on the Android CI #81398
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.
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
Conversation
@swift-ci smoke test |
test/IRGen/lto_autolink.swift
Outdated
// UNSUPPORTED: OS=ios && CPU=arm64e | ||
// UNSUPPORTED: OS=watchos && (CPU=arm64_32 || CPU=armv7k) | ||
// UNSUPPORTED: OS=linux-gnu && CPU=aarch64 | ||
// XFAIL: OS=linux-android && CPU=aarch64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The mac and linux arm64 exclusions were added before @tshortli updated this test last year to build against %target-cpu
, not just x86_64, f6d419c, so probably unneeded now?
The Android AArch64 failure is only because the community Android CI also builds the stdlib for linux x86_64, which confuses the compiler:
+ /usr/bin/python3.12 /home/swiftci/jenkins/workspace/oss-swift-RA-linux-ubuntu-24.04-android-arm64/swift/utils/PathSanitizingFileCheck --allow-unused-prefixes --sanitize BUILD_DIR=/home/swiftci/jenkins/workspace/oss-swift-RA-linux-ubuntu-24.04-android-arm64/buildbot_linux/swift-linux-x86_64 --sanitize SOURCE_DIR=/home/swiftci/jenkins/workspace/oss-swift-RA-linux-ubuntu-24.04-android-arm64/swift --ignore-runtime-warnings --use-filecheck /home/swiftci/jenkins/workspace/oss-swift-RA-linux-ubuntu-24.04-android-arm64/buildbot_linux/llvm-linux-x86_64/bin/FileCheck /home/swiftci/jenkins/workspace/oss-swift-RA-linux-ubuntu-24.04-android-arm64/swift/test/IRGen/lto_autolink.swift --check-prefix CHECK-ELF-MERGE
<unknown>:0: error: could not find module 'Swift' for target 'aarch64-unknown-linux-gnu'; found: x86_64-unknown-linux-gnu, at: /home/swiftci/jenkins/workspace/oss-swift-RA-linux-ubuntu-24.04-android-arm64/buildbot_linux/swift-linux-x86_64/lib/swift/linux/Swift.swiftmodule
FileCheck error: '<stdin>' is empty.
If I disable building the host linux stdlib locally, the test passes for aarch64, as it does on the Android x86_64 CI, because then the host arch happens to match the target arch. The compiler shouldn't be erroring only because an unrelated stdlib is also installed, so disabling this for Android AArch64 alone until that bug is fixed.
@xymus, is the pasted error above the same reason you had to disable iOS and watchOS also?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not just add a requires for the stdlib on the architecture?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean: the problem is that this test creates Swift modules for linux/mac triples with the %target-cpu
, for which no linux/mac stdlib is required. When running the tests for Android AArch64 on linux x86_64 with building that host linux x86_64 stdlib explicitly disabled, this test passed locally when I tried it.
The problem is the preset that the Android AArch64 CI uses also builds the linux x86_64 stdlib, which then causes the error pasted above, even though no linux stdlib seems to be required! This appears to be a clear bug in the compiler, so I'm disabling this for Android AArch64 until we get that compiler bug tracked down.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that this is a bug in the compiler, this sounds like a bug in the test then. If no stdlib is required, then you should do -no-stdimport
when invoking the compiler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I was unaware of that flag and so apparently are the rest of the Swift devs, as it is entirely unused in the compiler validation suite! 😃 It does fix the test in the scenario I mentioned, stopping the autolinking from looking in the stdlib resource directory.
I will experiment a bit and see if it fixes the problem on Apple platforms too. 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried enabling this test on Darwin arm64 platforms again and ran the Apple Silicon tester on it, but that CI appears to be broken when building some embedded Swift files right now, so I couldn't check if this test needs -nostdimport
there too.
@tshortli, since you disabled this test for iOS arm64, can you check if it works now on whatever internal CI it broke on before, particularly if -nostdimport
is added for mac as I just did for linux?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Evan tells me he's not seeing any issue with this test on Darwin, so let's go ahead and get it in.
@compnerd, I rebased and kicked off the tester one last time: if you'd approve, I'll go ahead and merge.
@@ -1,4 +1,4 @@ | |||
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) %s -emit-sil -swift-version 5 -use-clang-function-types -experimental-print-full-convention -o - | %FileCheck %s --check-prefix=CHECK-%target-ptrsize | |||
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk) %s -Xllvm -sil-print-types -emit-sil -swift-version 5 -use-clang-function-types -experimental-print-full-convention -o - | %FileCheck %s --check-prefix=CHECK-%target-ptrsize |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simply bringing over Erik's recent change
if os.path.exists(path): | ||
# We should then have the architecture in the name. | ||
for lib in os.listdir(path): | ||
match = re.match(r'(?:lib)?clang_rt\.(\w+)-' + run_cpu, lib) | ||
match = re.match(r'(?:lib)?clang_rt\.(\w+)-' + run_cpu + env, lib) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If running the Android x86_64 tests on linux x86_64, this was detecting the linux clang_rt libraries and wrongly enabling all these sanitizer tests for Android too, which then fail because the compiler correctly checks for this environment suffix. Checking for the suffix here too makes sure the Android-specific libraries are available, and disables the sanitizer tests on the Android CI when they aren't.
@swift-ci smoke test macos |
@swift-ci smoke test macos |
@swift-ci smoke test |
test/IRGen/lto_autolink.swift
Outdated
// UNSUPPORTED: OS=ios && CPU=arm64e | ||
// UNSUPPORTED: OS=watchos && (CPU=arm64_32 || CPU=armv7k) | ||
// UNSUPPORTED: OS=linux-gnu && CPU=aarch64 | ||
// XFAIL: OS=linux-android && CPU=aarch64 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we not just add a requires for the stdlib on the architecture?
@swift-ci smoke test |
@swift-ci test macos |
@swift-ci please test Apple Silicon |
@swift-ci test apple silicon |
@swift-ci smoke test |
…roid CI Also, fix and enable IRGen/lto_autolink for all non-Wasm targets and IRGen/static_initializer for aarch64.
@swift-ci smoke test |
@@ -12,7 +12,7 @@ | |||
// APPLE-NEXT: (End of search path lists.) | |||
|
|||
// Non-Apple platforms don't have any implicit framework search paths. | |||
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource) -target x86_64-unknown-linux-android -parse %s -Rmodule-loading 2>&1 | %FileCheck -check-prefix=ANDROID %s | |||
// RUN: %target-swift-frontend(mock-sdk: %clang-importer-sdk-nosource) -target x86_64-unknown-linux-android -parse -parse-stdlib %s -Rmodule-loading 2>&1 | %FileCheck -check-prefix=ANDROID %s |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed this test failing on the Android AArch64 CI alone in the last couple days with no output since it was added in #81269, so I ran the same command locally and got this error:
> ~/swift-DEVELOPMENT-SNAPSHOT-2025-04-12-a-fedora39/usr/bin/swift-frontend -target aarch64-unknown-linux-android -sdk /home/finagolfin/android-ndk-r27c/toolchains/llvm/prebuilt/linux-x86_64/sysroot -resource-dir /home/finagolfin/build/Ninja-Release/swift-linux-x86_64/lib/swift -module-cache-path /home/finagolfin/build/Ninja-Release/swift-linux-x86_64/swift-test-results/aarch64-unknown-linux-android/clang-module-cache -swift-version 4 -typo-correction-limit 10 -sdk /home/finagolfin/swift/test/Inputs/clang-importer-sdk -target x86_64-unknown-linux-android -parse foo.swift -Rmodule-loading
<unknown>:0: warning: libc not found for 'x86_64-unknown-linux-android'; C stdlib may be unavailable
Module import search paths:
Framework search paths:
Runtime library import search paths:
[0] /home/finagolfin/build/Ninja-Release/swift-linux-x86_64/lib/swift/android
[1] /home/finagolfin/build/Ninja-Release/swift-linux-x86_64/lib/swift/android/x86_64
[2] /home/finagolfin/swift/test/Inputs/clang-importer-sdk/usr/lib/swift/android
[3] /home/finagolfin/swift/test/Inputs/clang-importer-sdk/usr/lib/swift/android/x86_64
(End of search path lists.)
<unknown>:0: error: could not find module '_Concurrency' for target 'x86_64-unknown-linux-android'; found: aarch64-unknown-linux-android, at: /home/finagolfin/build/Ninja-Release/swift-linux-x86_64/lib/swift/android/_Concurrency.swiftmodule
Since this passes on the Android x86_64 CI, this is another variant of the stdlib search path issue I raised for lto_autolink
last week, which I'm able to avoid locally by passing in this extra -parse-stdlib
flag also, as done in that test already.
@ian-twilightcoder, let me know what you think of this minor tweak to your new test for the Android CI.
Asked @compnerd to review but he's probably busy: since these are all minor tweaks that passed CI and I addressed all his review comments, going ahead and merging to get Android CI green. If any test breaks any internal or external CI later, we can always fix that test then. |
Seems like this CI bot is failing due to this change: https://ci.swift.org/job/oss-swift_tools-R_stdlib-RD_test-simulator/7781/consoleText
|
@finagolfin Can you take a look? |
@aschwaighofer, I only added that line so that it would match for all non-mac/linux/android SYSV OS's that might be added later, while the lines below match those three major OSs. It passed the CI for all those OSs, so is that test-simulator CI you linked doing something different that I need to account for? |
Here's the same test passing on a recent normal macOS x86_64 CI run after this pull:
I'm not familiar with this other CI you're linking. Ultimately, the line is not important: it was only chosen as something that should always be true, and is on all the other CI. The question is why it isn't on this one test-simulator CI, and if we can find some text that always matches there too. |
@aschwaighofer, I'm examining the IR and will submit a fix... |
Submitted in #81643 |
@aschwaighofer, this test now passes on the test-simulator CI because of #81643, let me know if anything else crops up. |
// UNSUPPORTED: OS=macosx && CPU=arm64 | ||
// UNSUPPORTED: OS=ios && CPU=arm64e | ||
// UNSUPPORTED: OS=watchos && (CPU=arm64_32 || CPU=armv7k) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removing these seems to have caused failures in iOS and macOS bots, e.g., https://ci.swift.org/job/oss-swift_tools-RA_stdlib-DA_test-device-non_executable/8901. Is there a specific reason to believe that these are no longer necessary?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I fixed it on linux and Android, so I asked about fixing it on Darwin too. Allan, who originally disabled it, didn't respond, but Evan ran some other CI and said it seemed to work fine.
Looking at the error, it's exactly the same as the linux issue I fixed, so I'll submit the same fix for Darwin and if you can run this same failing CI on the Darwin fix, we can finally get it running on Darwin too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Submitted the fix in #81695
… the Android CI (swiftlang#81398)" This reverts commit 35ee368.
Also, fix and enable
IRGen/lto_autolink
for all non-Wasm targets andIRGen/static_initializer
for aarch64.This should get the community Android CI green again, @marcprux and @weliveindetail, try it out locally if you can.