Fix resource merge priority so consumer library overrides dependency resources#464
Open
olbapmar wants to merge 1 commit intobazelbuild:mainfrom
Conversation
olbapmar
force-pushed
the
pablomartin/fix-resource-merge-priority-for-consumer-libraries
branch
from
d2d535f to
d4abac2
Compare
…resources The Android resource busybox uses last-wins semantics when merging duplicate resource names. The current library's ResourcesNodeInfo was placed as the first direct item of the direct_resources_nodes preorder depset, causing it to be processed before its dependencies and lose to them on conflicts. Fix the ordering at the consumption point in busybox.bzl: reverse the direct_resources_nodes list when building --directData (and --directAssets) arguments. This ensures the current library's node (first in preorder) ends up last in the busybox flag, so its resources correctly override those of its dependencies — matching native Bazel and Gradle behavior. A regression test was not added because the fix is only observable in non-legacy manifest_merge_order mode. In legacy mode, _process_starlark swaps each library's dep nodes out of direct_resources_nodes into transitive_resources_nodes before building the provider, so every library exposes only its own node in direct_resources_nodes. Downstream targets therefore see exactly one node per direct dep in --directData, making the reversal a no-op. All CI resource tests run exclusively with --//rules/flags:manifest_merge_order=legacy, so the fix cannot be exercised through the existing analysis-time test framework without adding a non-legacy CI configuration or a configuration transition on the test target.
olbapmar
force-pushed
the
pablomartin/fix-resource-merge-priority-for-consumer-libraries
branch
from
d4abac2 to
012ebad
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The Android resource busybox uses last-wins semantics when merging duplicate resource names. The current library's
ResourcesNodeInfowas placed as the first direct item of thedirect_resources_nodespreorder depset, causing it to be processed before its dependencies and lose to them on conflicts.Fix the ordering at the consumption point in
busybox.bzl: reverse thedirect_resources_nodeslist when building--directData(and--directAssets) arguments. This ensures the current library's node (first in preorder) ends up last in the busybox flag, so its resources correctly override those of its dependencies (matching native Bazel and Gradle behavior)A regression test was not added because the fix is only observable in non-legacy
manifest_merge_ordermode. In legacy mode,_process_starlarkswaps each library's dep nodes out ofdirect_resources_nodesintotransitive_resources_nodesbefore building the provider, so every library exposes only its own node indirect_resources_nodes. Downstream targets therefore see exactly one node per direct dep in--directData, making the reversal a no-op. All CI resource tests run exclusively with--//rules/flags:manifest_merge_order=legacy, so the fix cannot be exercised through the existing analysis-time test framework without adding a non-legacy CI configuration or a configuration transition on the test target.However, this has caused issues in our repos and the this patch fixed them.