Add support for using conditionally available NDK APIs

[cpsauer]

Coming in r26. For more, see https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#weak-symbols-for-api-definitions and android/ndk#837

[DanAlbert]

Flags LGTM, but someone that actually knows bazel will need to verify that the implementation is right :)

(Should there be a test?)

[DanAlbert]

Is this the default? If you care, this differs from the NDK's build systems. We have no plans to ever make this the default in the NDK

[cpsauer]

Hey, Dan! Thanks again for engaging over here.

[cpsauer]

Yep! Was proposing we enable by default in this toolchain because (from the Build System Maintainers Guide) it seemed sufficiently valuable, safe, and reverse-compatible that you all had concluded that "most developers will likely prefer weak APIs" (i.e. this).

But you're certainly the expert. Could I ask to compare notes? Are there unsafe edges I've missed that should make it off by default? Perhaps more of the problems from earlier on in the issue remain than are listed in the guide?

[cpsauer]

Some finer points for comparing notes, separated into an appendix to save round trips, while keeping the above skimmable for others with less context :)

1. All of the above is conditioned on having kept misuse a build error via -Werror=unguarded-availability and not a runtime error on older devices.
   It seems like the docs conclude it's unsafe without that flag, but good and safe with.
   Hence y'all's decision to keep it off by default in the headers, leaving enabling weak symbols to the build system, which can ensure use of that flag?
2. My understanding is that, under the hood, this is invoking the same clang features that enabled Apple to lean into this on iOS and the rest of their platforms, yes? If so, that increases my confidence, since the conditional compatibility story works so well by default over there and I've used it left right and sideways without getting tripped up. It's so valuable to have conditional compatibility in the NDK, IMO--for the same reason as the it is on the rest of mobile (Android Java, iOS) and beyond. But obviously, you're the one that's been directly engaged in implementing it.
3. It had also seemed like it might become the default elsewhere ("At the time of writing, the NDK's own build systems (ndk-build and CMake) use strong API references by default, but that may change in the future"), but it sounds like that doesn't square with your thoughts.
   I'm trying to understand whether the reluctance elsewhere has to do with strict compatibility inertia and caution around a full, official Android release (which wouldn't apply so much on this new, experimental toolchain that breaks from the previous in a number of ways), or whether there are other problems you know about that make this unsafe to use.

[ahumesky]

If there's a really compelling reason to enable this by default, I'm open to that, but overall my intuition is to match what the NDK does.

(Should there be a test?)

Yes if we decide to move forward, it would be good to add a test. Basically add another example like https://github.com/bazelbuild/rules_android_ndk/tree/main/examples/basic and update https://github.com/bazelbuild/rules_android_ndk/blob/main/.bazelci/presubmit.yml to build it

[DanAlbert]

Are there unsafe edges I've missed that should make it off by default? Perhaps more of the problems from earlier on in the issue remain than are listed in the guide?

The short answer is that we don't know yet. This is not supported in any released NDK, which alone seems like a reason to not enable this by default. Maybe this is a sensible default for bazel in the future, but I think it'd be wise to wait until the feature has had some time in the wild so we can be sure there aren't any surprises lurking before making that change.

If there's a really compelling reason to enable this by default

Whether or not this is useful is going to depend a lot on the exact use case. Some apps won't need it at all, and some apps won't be able to use it because it has limitations. We don't expect it to cause issues for either of those groups, but there's also no benefit. If it turns out that there are no downsides to enabling this it's probably a good default, but we don't really know yet.

[DanAlbert]

IMO we should definitely add this as a bazel feature, whether off by default or on, because it's valuable--and safety-wise you really want to encourage people to toggle the flags on together.

+1

[cpsauer]

Makes sense! Thanks for being great @DanAlbert.

So, proposing a plan: I'll change it to off by default momentarily, we'll land the optional bazel feature so people can try it safely upon release, and then we'll revisit the default status when we've got more usage data?

Dan, could I ask you to ping us back somehow as you learn more about how it's working in the wild, maybe just on that other issue thread, since I presume that's where people are mostly following?

[And if useful as a communications backtrace: I think the miscommunication here was my mistaking the soft launch for an upcoming, higher-conviction hard launch.]

Cheers,
Chris

[ahumesky]

Looks good to make it optional -- could you add a test to ensure this continues to work?
you could add another example that makes use of one of the conditionally available APIs based on the basic example:
https://github.com/bazelbuild/rules_android_ndk/tree/main/examples/basic and update https://github.com/bazelbuild/rules_android_ndk/blob/main/.bazelci/presubmit.yml to build it

[cpsauer]

Always optional! But now off by default for the soft-launch.

Re tests: Happy to in spirit, but for them to pass, I think we'll have to wait until r26 is released and used for these tests, right? We could block on that, but this seems handy early for folks playing around with the feature pre-released and on launch. So I'd propose merge now and add tests on r26 launch, esp since it's now disabled by default and a soft-launch?