Skip to content
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

[android] Switch armv7 vendor to 'unknown' in target triple to match other arches #34919

Merged
merged 1 commit into from
Dec 9, 2020

Conversation

finagolfin
Copy link
Member

Recent changes have made 'none' and 'unknown' not interchangeable anymore, so standardize on 'unknown' for armv7 too.

I don't know exactly when this change was introduced in trunk, maybe with #31170, but both vendors work with the release/5.3 branch. When I just tried to cross-compile the corelibs for armv7 with #33724, where I use 'unknown' in the triple, applied to the Nov. 26 trunk source snapshot, I hit this error when CMake checks that the Swift compiler works when configuring libdispatch, which this pull fixes:

<unknown>:0: error: could not find module 'Swift' for target 'armv7-unknown-linux-android'; found: armv7-none-linux-android

Another way to reproduce is by trying to cross-compile a "hello world" executable by using the Android cross-compilation flags from the docs:

> ./swift-DEVELOPMENT-SNAPSHOT-2020-11-26-a-ubuntu20.04/usr/bin/swiftc
-tools-directory android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/bin/
-L android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/lib/gcc/arm-linux-androideabi/4.9.x/armv7-a/
-sdk android-ndk-r21d/toolchains/llvm/prebuilt/linux-x86_64/sysroot
-resource-dir build/Ninja-Release/swift-android-armv7/lib/swift
swift/test/Interpreter/hello_toplevel.swift -target armv7-unknown-linux-androideabi24
<unknown>:0: error: could not find module 'Swift' for target 'armv7-unknown-linux-android'; found: armv7-none-linux-android

I've modified a couple armv7 tests to use the new vendor, while leaving in others that are meant to check that the vendor and triple are changed before being passed to the linker.

@drodriguez and @compnerd, I'm not sure why these tests on the Android CI didn't catch this, should I add a test that does?

…other arches

Recent changes have made 'none' and 'unknown' not interchangeable anymore, so
standardize on 'unknown' for armv7 too.
@drodriguez
Copy link
Contributor

drodriguez commented Dec 4, 2020

It should have been unknown all along, I think. none doesn't have a special meaning in the vendor field, and it might have been taken as unknown anyway (it can be seen in the test you link and haven't modified, where none and mone are both transformed internally into unknown). none might have been interpreted as an specific vendor, which did not match when unknown is explicitly specified.

@swift-ci please test

@swift-ci
Copy link
Contributor

swift-ci commented Dec 4, 2020

Build failed
Swift Test OS X Platform
Git Sha - 1a28a91

@finagolfin
Copy link
Member Author

Linux passes, mac single test failure is unrelated, and Windows CI fails when checking out git.

@drodriguez
Copy link
Contributor

@swift-ci please test macOS platform

@drodriguez
Copy link
Contributor

@swift-ci please test Windows platform

Copy link
Member

@compnerd compnerd left a comment

Choose a reason for hiding this comment

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

Yes please. I think that as long as the build succeeds with this change, this is something we should get merged right away. The only concern is with the tool prefixing that might happen in some cases (with the NDK toolchain).

@drodriguez
Copy link
Contributor

@swift-ci please test Windows platform

@drodriguez
Copy link
Contributor

@swift-ci please test macOS platform

@swift-ci
Copy link
Contributor

swift-ci commented Dec 5, 2020

Build failed
Swift Test OS X Platform
Git Sha - 1a28a91

@finagolfin
Copy link
Member Author

Mac CI dies when cross-compiling stdlib for Mac-ARM64, running out of memory? Unrelated to this pull of course.

The only concern is with the tool prefixing that might happen in some cases

If you're talking about NDK paths that use these variables, I just checked and they don't. There is a separate SWIFT_SDK_ANDROID_ARCH_${arch}_NDK_TRIPLE variable for that, which never used a vendor.

@finagolfin
Copy link
Member Author

Ping, we should get this in.

@xwu
Copy link
Collaborator

xwu commented Dec 7, 2020

@swift-ci please test macOS platform

@swift-ci
Copy link
Contributor

swift-ci commented Dec 7, 2020

Build failed
Swift Test OS X Platform
Git Sha - 1a28a91

@finagolfin
Copy link
Member Author

Yet another spurious failure on the Mac CI.

@drodriguez
Copy link
Contributor

@swift-ci please test macOS platform

@compnerd
Copy link
Member

compnerd commented Dec 9, 2020

I believe that this is safe and everyone is in agreement here, merging.

@compnerd compnerd merged commit 89c617a into swiftlang:main Dec 9, 2020
@finagolfin finagolfin deleted the android-vendor branch December 9, 2020 02:23
finagolfin referenced this pull request in vgorloff/swift-everywhere-toolchain May 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants