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

Support for Android NDK r22, r23, r24, r25 #12889

Closed
cpsauer opened this issue Jan 23, 2021 · 55 comments
Closed

Support for Android NDK r22, r23, r24, r25 #12889

cpsauer opened this issue Jan 23, 2021 · 55 comments
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Android Issues for Android team type: feature request

Comments

@cpsauer
Copy link
Contributor

cpsauer commented Jan 23, 2021

Happened to see that the Android NDK r22 had been released. Creating an issue so folks can track bazel support for the latest NDK versions!

Update: r23, r24, r25 now also released. Since it looks like Google's Bazel<->Android support is falling behind a bit, maybe just adding support for the latest would be easier and good enough?

For reference: Things were last updated for r21. Issue. Resolving PR.

@steeve
Copy link
Contributor

steeve commented Feb 23, 2021

Just tried building with NDK 22, it failed with this:

In file included from /private/var/tmp/_bazel_steeve/42f5ad0f9796dcb2aba57da250d2fe89/sandbox/darwin-sandbox/1366/execroot/xxx/external/androidndk/ndk/sources/cxx-stl/llvm-libc++/include/stdlib.h:97:
/private/var/tmp/_bazel_steeve/42f5ad0f9796dcb2aba57da250d2fe89/sandbox/darwin-sandbox/1366/execroot/xxx/external/androidndk/ndk/sources/android/support/include/stdlib.h:31:15: fatal error: 'stdlib.h' file not found

At ndk/sources/android/support/include/stdlib.h:31:15 we find:

#include_next <stdlib.h>

@oakad
Copy link

oakad commented Mar 25, 2021

Also, #13191
Layout change appears to break bazel outright

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 29, 2021

One thing I ran across that might raise the priority of this--and supporting newer API levels and NDKs more generally: Google Play is requiring API level 30, starting in August.

@steeve
Copy link
Contributor

steeve commented Mar 30, 2021

For the record, here is the Build System Maintainers Guide for r22.

@oakad
Copy link

oakad commented Mar 31, 2021

Considering how big and important Android platform is, Bazel should assign some more priority to it.

The people are suffering with cmake here, for god's sake. :-)

@steeve
Copy link
Contributor

steeve commented Mar 31, 2021

Considering how big and important Android platform is, Bazel should assign some more priority to it.

The bazel team is like you, me and any other team out there: flooded with tasks and constrained by time. If not support, let's at least show empathy. If you want to help, you could try taking a jab at this.

The people are suffering with cmake here, for god's sake. :-)

This one is bad though.

@oakad
Copy link

oakad commented Apr 1, 2021

The rules in question are in the older java part of Bazel. Similar c++ stuff was ported to starlark. Not something to undertake on one's own initiative (because effort is big and chances of merge are nil).

On the other hand, Bazel is a Google product and Android is also a Google product. Don't think it's somehow unreasonable to expect a bit more synergy between them.

@steeve
Copy link
Contributor

steeve commented Apr 1, 2021

My second ever PR in the bazel core was adding support for NDK r21 (albeit probably a lot easier), so I tend to disagree. Not to say it'll be easy, but if somebody were to do it, I'm sure they would be glad and would gladly merge it. As they did with mine.

@EwoutH
Copy link

EwoutH commented Apr 20, 2021

Would adding support for NDK r22 be more complex than for r21? If not, it would be a fairly simple PR right? Or are there changes affecting Bazel?

@steeve
Copy link
Contributor

steeve commented Apr 20, 2021

Would adding support for NDK r22 be more complex than for r21? If not, it would be a fairly simple PR right? Or are there changes affecting Bazel?

I'm not 100% sure, but it feels like the changes are more substantial than to r21.

@EwoutH
Copy link

EwoutH commented Apr 28, 2021

@ahumesky, since you reviewed the PR for the NDK r21, would you be able to help out with this issue?

@ahumesky ahumesky added P2 We'll consider working on this in future. (Assignee optional) and removed untriaged labels Jun 18, 2021
@ahumesky
Copy link
Contributor

@cpsauer re: "Google Play is requiring API level 30, starting in August", I believe that ndk r21 supports API level 30, and it's an LTS release, so it should be supported for some time. I'll look at #13421

@EwoutH @steeve I don't know off-hand what would be involved, but it sounds like it would be closer to 00e29b7 than #11872

@cpsauer cpsauer changed the title Support for Android NDK r22 Support for Android NDK r22, r23 Sep 1, 2021
@cpsauer
Copy link
Contributor Author

cpsauer commented Sep 1, 2021

[Updated to reflect the release of r23, which is LTS.]

@freedomtan
Copy link

freedomtan commented Sep 12, 2021

I started working on making NDK r23 work couple days ago, it seems there are at least 2 major issues

  1. deprecation of ${NDK_ROOT}/{platforms,sysroot}
  2. removal of GNU toolchain

I managed to make building a minimal hello world work on macOS and ubuntu with https://github.com/freedomtan/bazel/tree/ndk_r23_hacks

With --copt=-nostdinc++, I could be TFLite //tensorflow/lite/tools/benchmark:benchmark_model and //tensorflow/lite/examples/label_image:label_image for android_arm64.

bazel build --config android_arm64 //tensorflow/lite/tools/benchmark:benchmark_model   --copt=-nostdinc++

Dunno how to get rid of the need of -nostdinc++ yet, but basically it works. I'll clean up and send a PR later.

@ahumesky
Copy link
Contributor

ahumesky commented Oct 4, 2021

Thanks @freedomtan, I'm happy to review a pull request for this if you're able to send one out.

@ahumesky
Copy link
Contributor

ahumesky commented Nov 3, 2021

I have an update to share: I figured out a way to remove the need for --copt=-nostdinc++ when compiling Tensorflow. Basically there are a number of -isystem flags that are added to the crosstool, e.g.:

toolchainBuilder.addUnfilteredCxxFlag(
"-isystem%ndk%/usr/include".replace("%ndk%", ndkPaths.createBuiltinSysroot()));

Through the internal discussion I mentioned on #14077, I learned that with more recent versions of clang, this actually appears to interfere with clang's automatic detection of includes. So at first glance, it would seem like a simple thing to just remove these for R23. The problem though is that these same fields are used elsewhere to create filegroups for picking up all these includes for sandboxing, and this code is shared across NDK versions:

for (String cxxFlag : toolchain.getUnfilteredCxxFlagList()) {
if (!cxxFlag.startsWith("-")) { // Skip flag names
toolchainFileGlobPatterns.add(NdkPaths.stripRepositoryPrefix(cxxFlag) + "/**/*");
}
}

It's a bit of a confusing mess to try to untangle build file / filegroup generation from crosstool generation, so instead I just generated an NDK build file and cc toolchain configuration file with what we have now, and then took out all the file generation logic for R23 and bundled static versions of the NDK build file and cc toolchain configuration file. Then in AndroidNdkRepositoryFunction, it just copies these bundled files into the symlinked output directory for the NDK. Now it's a simpler matter to edit the cc toolchain configs and filegroups. We've been wanting to move in this direction anyway for a while, especially since the NDK has gotten simpler (now clang-only, fewer STLs, etc).

I can share an in-progress change, which incorporates @freedomtan's #14077. The next steps are to set up bazel's CI with NDK 23, and add and test mac and windows support.

@ahumesky
Copy link
Contributor

ahumesky commented Nov 3, 2021

Here are my changes so far: ahumesky@3de69da

@iHuahua
Copy link

iHuahua commented Jan 24, 2022

真TMD无语,拖了一整年也不改,不知道支持一下NDK新版到底是要花多少时间,一个号称跨平台的工具链在跨平台方面做得跟逗你玩一样,这到底还是不是Google

@jiawen
Copy link
Contributor

jiawen commented Feb 27, 2022

Is there any motion on this? It looks like PR#14077 has stalled.

My team, which uses Bazel, is eagerly waiting NDK r23 so we can use Android 12 APIs.

I'm surprised at how difficult it is to integrate a new Android NDK LTS release to Bazel, which was released, wow, back in August. Maybe ahumesky/bazel@3de69da will make upgrades easier by moving the majority of the work into BUILD files instead of Java inside Bazel.

@ahumesky are there any long-term plans to migrate Android repository rules to Starlark? It's a big chunk of work but may be the best solution for bazelbuild/rules_android_ndk#92.

I guess the issue is that neither the Android NDK team nor the Bazel team at Google officially supports the NDK-on-Bazel toolchain? Maybe AOSP on Bazel will speed things along?

@jiawen
Copy link
Contributor

jiawen commented Jul 15, 2022

@radvani13, I second @tndhagedorn's point about C++17. I just ran into Android NDK bazelbuild/bazel#609 again. C++17 is "officially supported" in the NDK as of r21 but is in practice, broken or incomplete in many ways.

The Android NDK has always been at the forefront of Clang/LLVM development, including bleeding edge features but has never been particularly stable. It's important to the ecosystem for developers to be able to use the latest stable versions.

If you need more ammunition, I volunteer to look for a list of security bugs that have been fixed.

@kaliaumem
Copy link

kaliaumem commented Aug 5, 2022

NDK r25 LTS is now out, which includes new APIs for Android 13, improved debugging experience with ASAN turned on, on top of what the previous versions already provided.

If there is something that we can do to help get things moving from our end, please let us know @radvani13. My team keeps having to hack around the aging toolchain, solely because of this issue, so we're really looking forward to something finally moving on this one.

@cpsauer cpsauer changed the title Support for Android NDK r22, r23, r24 Support for Android NDK r22, r23, r24, r25 Aug 5, 2022
@tndhagedorn
Copy link

To add to @kaliaumem's point about r25 - only newer NDKs contain native binaries for Apple's M1.

This effectively forces all M1 Mac users to continue running the older NDK's x64 binaries under Rosetta, which can have a large impact on build performance.

@radvani13
Copy link

Hi Thank you all for your inputs and I am trying to work internally on this issue.

@cpsauer
Copy link
Contributor Author

cpsauer commented Aug 19, 2022

Thanks, Rhadika!

[One other exciting thing expected to come with these releases, continuing into r26: A much improved story around backwards compatibility!]

@iHuahua
Copy link

iHuahua commented Aug 25, 2022

@tndhagedorn this is Radhika and I am the PM at Bazel. i'd love to understand your urgency for NDK support.

Hi @radvani13, thanks for the reply.

NDK r21 is the last NDK supported by Bazel. This means we are forced to use versions of clang, libc++, etc. from that NDK, which are almost three years old by now.

There are still parts of C++17 I cannot use (in 2022), simply because I can not build my code using a newer NDK.

I would hope the Bazel team wants to fix this given that r23c is the last supported version of the NDK, and Bazel's front page claims I can use Bazel for C++ and Android development:

image

In reality this is half true - while the C++ support in Bazel for iOS, MacOS, and Linux has stayed current, NDK support continues to lag, holding back any C++ code base that also targets Android.

At this point we are considering moving away from Bazel since we can not keep current with advances in C++.

I realize everyone is busy and priorities shift, we just need an update. If this is a priority for the Bazel team, that's great. If not, that's fine too - Bazel is a great product for a lot of other uses, with a lot of resource put behind it for something that is completely free.

At the end of the day I can use something else, but it would be nice to at least know when and if this is going to be addressed to help inform my decision here.

IMHO, bazel is an incomplete compilation system with too many issues left unresolved. Our project is a cross-platform project (planning to support Android, iOS, macOS, Windows, Linux), including 8000+ C++ files, a little dart, a little go. I also chose it after seeing the cross-platform in the introduction of bazel. After a year of use, I feel really disappointed, and now I am migrating to CMake.

如何评价 Google 开源的 Bazel ? - iHuahua的回答 - 知乎
https://www.zhihu.com/question/29025960/answer/2273849438

@EwoutH
Copy link

EwoutH commented Aug 30, 2022

Hi Thank you all for your inputs and I am trying to work internally on this issue.

Thanks a lot for looking into this! Is there any update you can share at this moment?

@DanAlbert
Copy link

Hi there, I own the NDK. Let me know if there's anything we can do to help here, or how we can make this easier in the future. The changes we made that are causing the r21 -> r22+ jump to be harder than previous upgrades were designed to make future upgrades easier. These changes landed in r19 and announced the upcoming removal of the legacy support (I'd have loved to just keep the legacy sysroot paths alive and avoid the churn, but my users are always asking me to reduce the size of the NDK), and the legacy support was removed in r22. Once you're over that hump, upgrades should go significantly more smoothly than they having in any release prior.

As you noted, at least part of the work is deleting all the old NDK-specific workarounds, since we shouldn't behave differently than any other Clang cross toolchain these days. If you're not able to reuse your existing LLVM crosstool for modern NDKs, those could be worthy bugreports. If it would help you, please file them so we can fix any lingering NDKisms.

As an aside, this thread was an awesome read for me because it was unintentionally a candid review of the NDK :) It was really cool to hear which of the features we've been working on lately are most important to people (some of which were surprising to me!), and where we've fallen short (which I already had a pretty good hunch about 🙃).

@cpsauer
Copy link
Contributor Author

cpsauer commented Aug 30, 2022

[Smiling] Thanks for being great @DanAlbert!

@ahumesky
Copy link
Contributor

Thanks for the discussion everyone. As alluded to in the earlier updates, the current implementation of NDK integration in Bazel is through a "native" android_ndk_repository rule, which has grown a lot in complexity over the years. Here's a proposal, which I've summarized at the bottom:

We have a Starlark implementation of a cc toolchain definition for NDK r25 that's now used internally (i.e. used with our internal version of Bazel called Blaze), and so we can simplify everything by reusing and open sourcing the internal toolchain definition, and wrapping it in a Starlark version of android_ndk_repository.

The current native android_ndk_repository rule is built into Bazel, which ties NDK integration updates to Bazel's release cycle, even though NDK updates are mostly orthogonal to Bazel changes. So I think it makes sense to create a new repository under the bazelbuild github org, say "rules_android_ndk". This will decouple Bazel NDK integration updates from Bazel, as well as from the rules_android repository.

Having a separate repository, and having a new implementation in Starlark should make it easier to maintain the code and easier to accept contributions. Ongoing development can happen within that repository.

I've started the process to create the rules_android_ndk repo, which will probably be completed by the end of the week or early next week. Then we can start pushing code there. Some of the ground work has already been completed to prepare the toolchain definition for open sourcing, so we should be able to push something soon after the repository is created.

The initial release of code will likely have the follow caveats:

  • The initial version of the Starlark implementation of android_ndk_repository will likely only work on Linux. Enabling it to work on Mac and Windows will take some additional time (hopefully not more than a few days per platform).

  • The internal toolchain definition might only work for NDK r25. The toolchain will need to be checked against earlier versions of the NDK and updated for backwards compatibility, if needing to use an earlier version is a critical blocker.

  • Since this will be based on an internal toolchain definition, there may be some things (like default compilation and linking options) that don't match the defaults in the NDK, and we'll likely need to work through those based on feedback.

  • Likely not all features of the NDK, nor all features of the current native version of android_ndk_repository, will be initially available.

To summarize, the proposal is:

  1. Open source our internal cc toolchain definition for NDK r25
  2. Create a new Starlark version of android_ndk_repository to wrap the above toolchain, and to replace the current native version of android_ndk_repository
  3. Create a new rules_android_ndk repository under the bazelbuild github org for the above code and ongoing development

Let us know what you think of this approach. I think it will improve things, especially for the longer-term.

@cpsauer
Copy link
Contributor Author

cpsauer commented Aug 31, 2022

Sounds pretty great to me, @ahumesky! Fantastic in fact. Thanks for being willing to open up the internal one--and for all the great work you do.

[To answer the bullets in more detail: macOS support would be v important to my team. Supporting older NDKs wouldn't--and it doesn't look like anyone's tapped back on that since the original issue, so maybe that's a good thing to skip to save time. Different default linking options might well be a feature rather than a problem. I know it's a switch to LLD, and regardless, the current default set had some problems. If that persists, happy to contribute fixes--plus a dummy pthread fallback workaround to get linux libraries to more seamlessly build for Android. For features (not mentioned) I think the key one would be around C++ stdlib--default static unless a dependency bundles one? And starlarkifying and decoupling for fast collaboration and release sounds like a great way to facilitate fast progress. Thank you, thank you!]

Cheers!
Chris

@oakad
Copy link

oakad commented Aug 31, 2022

NDK on MacOS and support for older toolchains are absolutely imperative. In commercial app development:

  1. The development is focused on iOS version whereupon Android version is essentially a "port". It may not be the nicest thing to say, but this is the current state of affairs in the mobile markets.
  2. A requirement to support a particular, older type of Android "fleet" device comes very often. Yet, we all know that various Android vendors seldom bother to upgrade the OS on the older devices. To which extent legacy device support requires the use of actual old toolchain is, of course, debatable and depends on many things.

Clearly, Android team can always ignore those unfortunate facts, but I'm afraid it will simply make app vendors to more aggressively push their customers toward iOS devices (as if Apple needed any help in this regard :-D).

I do agree that decoupling android rules from the bazel core is a splendid idea.

@cpsauer
Copy link
Contributor Author

cpsauer commented Aug 31, 2022

@oakad, would you actually need older NDK revisions, like r24, or just backwards compatibility with older android OSs?

(An even better story around conditionally using new features while maintaining backwards compatibility seems likely in the next NDK! See links above.)

@DanAlbert
Copy link

Unless you're talking about extremelely old devices (close to a decade), r25 works just fine. r25 was actively tested against KitKat.

@ahumesky
Copy link
Contributor

Hi everyone, the rules_android_ndk repo has been created, and I've pushed the initial body of code:

https://github.com/bazelbuild/rules_android_ndk

See the readme there for instructions, and the example at
https://github.com/bazelbuild/rules_android_ndk/tree/main/examples/basic

@jiawen
Copy link
Contributor

jiawen commented Sep 1, 2022

Unless you're talking about extremelely old devices (close to a decade), r25 works just fine. r25 was actively tested against KitKat.

I agree completely. IMHO, The Android NDK goes to extreme lengths to preserve binary compatibility. @DanAlbert I've been very impressed with your work - especially the benthic depths to which you are willing to dive.

@oakad do you have a particularly old device in mind? Older than KitKat? Perhaps we need a clearer FAQ or table explaining how NDK releases, API levels, OSes, and devices are all related and the various levels of compatibility? Maybe with some example devices?

@cpsauer
Copy link
Contributor Author

cpsauer commented Sep 3, 2022

I've switched my team over to @ahumesky's new toolchain, and am delighted to report that it's pretty sweet! The most painful things about the old NDK setup are fixed by default. And it's also working on macOS, for anyone else who was waiting for that.

For anyone else working through--and because Alex had mentioned feedback above--here're the thoughts and tweaks I had as I adopted. I think it's fairly likely you all will want some of them, too.
bazelbuild/rules_android_ndk#5

Alex, should I close this down, moving things over to the new https://github.com/bazelbuild/rules_android_ndk/issues? Or would you like to keep this open until some more issues are closed, it's in the bazel docs, and you feel like it's ready to be officially deemed a V1?

@oakad
Copy link

oakad commented Sep 5, 2022

We will try the new rules for android as soon as practicable, hopefully this week. However, I'm not opposed to closing this ticket - I can always make new ones in the new repo. :-D

@ahumesky
Copy link
Contributor

ahumesky commented Sep 9, 2022

Yeah let's close this one and consolidate ndk-related issues to rules_android_ndk, unless the issue is specifically with the native implementation of android_ndk_repository that can't be covered by the starlark implementation.

@ahumesky ahumesky closed this as completed Sep 9, 2022
@cpsauer
Copy link
Contributor Author

cpsauer commented Sep 9, 2022

SGTM. Thanks again, @ahumesky!

copybara-service bot pushed a commit that referenced this issue Jul 27, 2023
Hi wonderful Bazelers,

I'd happened to notice that the new NDK rules weren't referenced from the NDK docs, so I thought I'd toss up a quick change to help new users find their way.

Thanks for your consideration! Feel free (of course) to edit or push a different change if you'd prefer something else. (Was trying to strike a balance between the new rule being in their early stages and the old rule no longer being updated and not working for the past few years of NDKs. See #12889 for more context.)

Thanks,
Chris
(ex-Googler, [compile_commands_extractor](https://github.com/hedronvision/bazel-compile-commands-extractor/issues) author, rules_boost collaborator, etc.)

Closes #18523.

PiperOrigin-RevId: 551585249
Change-Id: Ida162f086657673e0c2cd884afc59107bc539f8e
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 We'll consider working on this in future. (Assignee optional) team-Android Issues for Android team type: feature request
Projects
None yet
Development

No branches or pull requests