-
Notifications
You must be signed in to change notification settings - Fork 10.6k
[Docs] [android] Update to the latest NDK 21, fix commands, and remove cruft #29439
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
docs/Android.md
Outdated
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.
This was made a requirement in #20082, but these commands weren't updated to show that, so the current commands in this doc don't work.
docs/Android.md
Outdated
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.
Unneeded since the -tools-directory flag was added in #8007.
docs/Android.md
Outdated
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.
This is a better architecture-independent path because it will get swift to invoke the NDK's clang, which will simply call the right linker based on the -target flag.
docs/Android.md
Outdated
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 know how the previous attempt to link against the NDK's libc++ ever worked, as the Android port of Swift linked against libc++_shared.so from the beginning, and clang++ will try to link against the file named libc++.so instead.
docs/Android.md
Outdated
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 didn't need any of these three libraries when running the "hello world" example, with the last one not existing anymore.
docs/Android.md
Outdated
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.
This last section was just added with #12530 after two years in limbo, but the referenced build-toolchain script hasn't been updated since then either and most likely won't work, so just take this out.
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.
@amraboelela, let me know if it works.
|
@CodaFi, this doc hasn't been meaningfully updated in years, and you were the last one to merge an old pull into it, so let me know what you think. |
CodaFi
left a comment
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.
One remaining comment. I’m not an authority on this configuration though.
|
@compnerd Does this look alright? |
docs/Android.md
Outdated
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.
This really should be armv7-unknown-linux-androideabi -march=armv7-a.
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.
This is a swiftc flag to build the Swift example and invoke the clang++ linker driver, I don't believe there's a -march flag accepted by swiftc. As for armv7 vs armv7a, Jake Petroules asked that I use the latter before because it's the official triple, and none vs unknown for the vendor are both ignored. I honestly don't care what variation of this target triple is used, so just let me know.
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.
Ping, this is the last remaining issue for this pull, just let me know what you prefer given the details I linked and mentioned.
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.
Finally built the Android-armv7 SDK by using these commands and as expected, the above flags didn't work:
<unknown>:0: error: unknown argument: '-march=armv7-a'
Instead, the flag I used worked, as did -target armv7-unknown-linux-androideabi -Xclang-linker -march=armv7-a, tested by building the hello world executable both ways and running it.
Since both ways work once your suggestion is properly modified, I don't really care what's used, just let me know.
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 experimented with this a bit more and if I cross-compiled the Swift object file first, to avoid any transient strings from each compiler invocation, then linked it into an executable by using this command with either of these flag combos or a couple more I tried, they all produce identical executables with the same hashes, as the final linker commands run are identical.
Since the suggestion makes no difference, the flag used now should be fine.
|
Ping @CodaFi, addressed all issues with this pull, with no response for the last three weeks on the last remaining suggestion from the review. Anyway, I've since run all these commands for Android ARMv7 too, and it worked well as is. I'd like to get this in and then I'll submit a pull for the 5.2 branch with this doc update and #29296 also (I'd appreciate any help on that stalled pull too, parts of which were used to build a full 5.1.4 toolchain natively on Android, termux/termux-packages#4895), so that the upcoming 5.2 release can be used to easily compile for Android, both cross-compiling and natively too. |
|
I’ll merge this here but 5.2 is under lockdown for critical changes. I don’t think these changes need to go there as well. From master you can cut another tag or have CI master a toolchain and it’ll do the trick. |
|
@swift-ci please smoke test and merge |
|
Alright, at least this doc will be fixed for the 5.3 release, thanks for merging. |
The commands from this pull were tested on an Arch Linux x86_64 VPS by building everything with Swift 5.1.3 for Android AArch64. Rather than use adb to test the "hello world" example, I copied the listed files over to the Termux app on an Android AArch64 device and checked that it ran that way. Nor did I actually use adb to test the final command or build the ARMv7 version of these commands, but I checked the NDK ARMv7 paths to make sure they were correct.
I only had to make a couple tweaks to the Swift CMake config to get it to cross-compile for Android, pull forthcoming for that.