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.

Sign up for GitHub

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

Add support for using conditionally available NDK APIs #27

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add support for using conditionally available NDK APIs #27

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8a254a0
Add support for using conditionally available NDK APIs
cpsauer Nov 15, 2022
6478869
Make conditionally available NDK API feature opt-in
cpsauer Aug 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions ndk_cc_toolchain_config.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,6 +532,15 @@ def ndk_cc_toolchain_config(
enabled = True,
),

# By default, allow use of APIs above the api_level, so long as the use is
# protected with a call to __builtin_available(android <version>, *).
# For more, see https://android.googlesource.com/platform/ndk/+/master/docs/BuildSystemMaintainers.md#weak-symbols-for-api-definitions
# This takes effect with NDK r26 and greater.
feature(
name = "android_ndk_conditional_api_availabilty",
enabled = True,
Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey, Dan! Thanks again for engaging over here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

@cpsauer cpsauer Nov 17, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link

Choose a reason for hiding this comment

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

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.

Copy link

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cpsauer cpsauer Jan 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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?

),

# User-settable feature controls warning aggressiveness for compilation.
feature(name = "warnings_as_errors"),

Expand Down Expand Up @@ -682,6 +691,14 @@ def ndk_cc_toolchain_config(
],
features = ["android_unwind_tables"],
),
flag_set(
actions = actions.all_compile,
flags = [
"-D__ANDROID_UNAVAILABLE_SYMBOLS_ARE_WEAK__",
"-Werror=unguarded-availability",
],
features = ["android_ndk_conditional_api_availabilty"],
),

## Options for particular compile modes:

Expand Down