@jin @cgruber Thoughts?

Goes some way to addressing bazelbuild/rules_android#10

cc @donaldchai @timpeut @djwhang

Thanks for reminding me about --experimental_use_rtxt_from_merged_resources. @jin: Do we need to go through the "incompatible change" process to unconditionally switch to that? (I don't expect any downsides to this.)

Jon: overall I like the idea of pruning things from the build, but I think this disables a useful validation step. In particular the "aapt2 link" verifies that resources referenced from XML are defined. So trying to build java/com/foo:foo should fail:

-- java/com/foo/BUILD --
android_library(
  name = "foo",
  manifest = ...
  resource_files = glob(["res/**"]),
  deps = ["//java/com/bar"],
)

-- java/com/foo/res/values/values.xml --
<resources>
  <string name="asdf">@string/missing</string>
</resources>

-- java/com/bar/BUILD --
android_library(
  name = "bar",
  srcs = "Bar.java",
  # no resources
)

Is that still true after this change? I was planning on writing a new validator (potentially emitting a warning if a referenced resource is not in a direct dep) but haven't gotten to it.

Thanks for reminding me about --experimental_use_rtxt_from_merged_resources. @jin: Do we need to go through the "incompatible change" process to unconditionally switch to that? (I don't expect any downsides to this.)

No, I don't think it's required for this.

@donaldchai, since this is behind a flag which defaults to building linking the resources, that seems fine?

@donaldchai, since this is behind a flag which defaults to building linking the resources, that seems fine?

Sure, but this decision is ultimately about how code is merged and how flags interact. I have no authority here. All of my own changes require approval by someone from the Bazel team, or others CCed to this PR.

@jin can you merge this?

@jin any objections?

@donaldchai - with this flag enabled you won't get any validation from linking as its disabled.

Probably this validation should happen in AndroidCompiledResourceMergingAction actually, it already supports a flag --throwOnResourceConflict (it looks like this defaults to true unless the rules is whitelisted which I don't think happens in open source Bazel (i.e: resource conflicts are always allowed) but it seems that this is the action that should validate the resources (does a referenced resource exist? is a resource duplicated? etc) as it does the merge (generates merged R class).

Hi @jin - the PR has been outstanding for a while now, is there any concern with it? I'm happy to discuss and address any issue you may have.

Probably this validation should happen in AndroidCompiledResourceMergingAction actually, it already supports a flag --throwOnResourceConflict

I feel the opposite way, i.e. AndroidCompiledResourceMergingAction shouldn't be doing any of these checks since it's on the critical path of the build, and the checks are not needed to generate R classes.

As as a side note, detection and reporting of "resource conflicts" on android_library should be overhauled so that they can be made errors instead of warnings (i.e. build log spam). The current override semantics leave a lot to be desired.

Time for a decision. Are we going to merge it or not?

@ahumesky could you make a call here about merging the PRs modifying Android native rules?

Any updates on this?

Friendly ping to all.

Given the Starlarkification of the rules, this PR probably should be reopened against https://github.com/bazelbuild/rules_android at one point.

I am going to close the PR, feel free to re-open if you disagree.