Skip to content

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

Merged
compnerd merged 1 commit intoswiftlang:mainfrom
finagolfin:android-vendor
Dec 9, 2020
Merged

[android] Switch armv7 vendor to 'unknown' in target triple to match other arches#34919
compnerd merged 1 commit intoswiftlang:mainfrom
finagolfin:android-vendor

Conversation

@finagolfin
Copy link
Copy Markdown
Member

@finagolfin finagolfin commented Dec 2, 2020

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?

@finagolfin
[android] Switch armv7 vendor to 'unknown' in target triple to match …
1a28a91 
…other arches

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

drodriguez commented Dec 4, 2020 •
edited
Loading

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
Copy Markdown
Contributor

swift-ci commented Dec 4, 2020

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

@finagolfin
Copy link
Copy Markdown
Member Author

finagolfin commented Dec 4, 2020

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

@drodriguez
Copy link
Copy Markdown
Contributor

drodriguez commented Dec 4, 2020

@swift-ci please test macOS platform

@drodriguez
Copy link
Copy Markdown
Contributor

drodriguez commented Dec 4, 2020

@swift-ci please test Windows platform

compnerd
compnerd approved these changes Dec 4, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor

drodriguez commented Dec 5, 2020

@swift-ci please test Windows platform

@drodriguez
Copy link
Copy Markdown
Contributor

drodriguez commented Dec 5, 2020

@swift-ci please test macOS platform

@swift-ci
Copy link
Copy Markdown
Contributor

swift-ci commented Dec 5, 2020

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

@finagolfin
Copy link
Copy Markdown
Member Author

finagolfin commented Dec 5, 2020

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
Copy Markdown
Member Author

finagolfin commented Dec 7, 2020

Ping, we should get this in.

@xwu
Copy link
Copy Markdown
Collaborator

xwu commented Dec 7, 2020

@swift-ci please test macOS platform

@swift-ci
Copy link
Copy Markdown
Contributor

swift-ci commented Dec 7, 2020

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

@finagolfin
Copy link
Copy Markdown
Member Author

finagolfin commented Dec 8, 2020

Yet another spurious failure on the Mac CI.

@drodriguez
Copy link
Copy Markdown
Contributor

drodriguez commented Dec 8, 2020

@swift-ci please test macOS platform

@compnerd
Copy link
Copy Markdown
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
@vgorloff
[#113] Renamed target to use new name "armv7-unknown-linux-android"
b1728f5
@finagolfin finagolfin mentioned this pull request Aug 31, 2021
Closed
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@compnerd compnerd compnerd approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@finagolfin @drodriguez @swift-ci @xwu @compnerd