Support overriding debuggable in AndroidManifest #13801

[EdbertChan]

Background

Similar to bazelbuild/bazel#13801, the only way to get a non debuggable-apk debuggable="false" is to use --compilation-mode opt which will invalidate the entire dep graph.

Changes

The drawback of this diff is that unlike what exists in Bazel upstream, the source of truth for the "debuggable" flag of the AAPT2 command is not what is in the AndroidManifest.xml itself but whatever is in manifest_values in the BUILD file.

If no value, we fallback to the compilation-mode opt behavior.

Usage

android_binary(
    name = "app",
    dex_shards = 10,
    manifest = "src/bazel/AndroidManifest.xml",
    manifest_values = {
        "minSdkVersion": "19",
        "appName": "...",
        "applicationId": "...",
        "debuggable": "false",
    },
    multidex = "native",
    visibility = ["//visibility:public"],
    deps = ["..."],
)

[EdbertChan]

Per @restingbull (Corbin) offline conversation, the dependency on Compilation.OPT was a matter of convenience from inside of Google. Supposedly, few/nobody who was working on building with the debuggable flag build cared about release so full invalidation was not a problem.

If you weren't building an optimized binary, you should not be building a release anyway since release should be optimized.

Outside of Google, that may not be the case. Some developers very specifically want to work on performance.

[ted-xie]

Hey @EdbertChan, now that starlarkification + tools migration are done, I think we have the bandwidth to try out this CL internally. Could you rebase your fork so that the (now-fixed) BazelCI presubmit can run again?

[EdbertChan]

@ted-xie Looks like they passed

[ted-xie]

@EdbertChan I'm looking a bit more closely at this now. I have two thoughts right now:

1. What happens if someone does bazel build //:app -c dbg with manifest_values = {debuggable: false} or vice versa, bazel build //:app -c opt with manifest_values = {debuggable: true}? In the first scenario, you'd have an un-debuggable debug app, and in the second scenario, you'd potentially be releasing a debuggable app, which opens doors for vulnerabilities. These were previously impossible scenarios that now become possible with this PR. I think we should either fail() in these scenarios, or at least issue a warning.
2. Changing the existing behavior of blaze build -c opt/dbg with manifest_values[debuggable] could potentially be a complicated migration internally, so it would be preferrable to gate this behind a starlark flag.

[ted-xie]

Let's use a more descriptive name here, like manifest_values_debuggable.

[ted-xie]

The two-step boolean logic split between line 738 and 777 is a little hard to follow; the ternary logic and inline if-statement here make it even harder. Can we consolidate this a little bit, like:

# line 738
manifest_values_debuggable = utils.get_bool(manifest_values["debuggable"]) if "debuggable" in manifest_values else None
dbg_compilation_mode = compilation_mode != _compilation_mode.OPT
app_debuggable = dbg_compilation_mode if manifest_values_debuggable == None else manifest_values_debuggable

# line 777-ish
debug = app_debuggable

[ted-xie]

@EdbertChan I had some spare cycles and ended up adding the extra flag along with its plumbing. From a very quick ad hoc test, it seems to work on my end.