From 634277322800e0eda0bd935132cbddd3e3f46a2f Mon Sep 17 00:00:00 2001 From: Olivier Notteghem Date: Sun, 12 Apr 2026 21:21:52 -0700 Subject: [PATCH] Fix non-deterministic placeholder resource ID assignment in PlaceholderIdFieldInitializerBuilder MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Summary: Intent: - Fix non-deterministic placeholder resource ID assignment in R.java generation that caused remote cache misses and non-reproducible outputs across incremental builds. Changes: - Replace LinkedHashMap with TreeMap for styleableAttrs and per-styleable attr maps, ensuring deterministic sorted iteration regardless of resource merge order. - Simplify assignAttrIds() by removing the inline/non-inline partition, which was itself non-deterministic (a side effect of first-come-first-served ReferenceResolver.shouldInline/markInlined semantics). All attrs are now assigned IDs in sorted order. Test Plan: Verified repeated builds of //apps/presidio/uberlite:bin_debug with remote cache disabled produce identical resources.jar outputs across multiple clean builds with Claude. USE_LOCAL_BAZEL=1 ./bazelw build uberlite builds successfully end-to-end. Revert Plan: Revert this PR via `git revert `. Jira Issues: --- Generated by the 🪄 [pr-create](https://sg.uberinternal.com/code.uber.internal/uber-code/devexp-agent-marketplace/-/blob/claude-code/plugins/dev/uber-dev/skills/pr-create/SKILL.md) skill in devexp-agent-marketplace --- .../PlaceholderIdFieldInitializerBuilder.java | 43 ++++++------------- 1 file changed, 14 insertions(+), 29 deletions(-) diff --git a/src/tools/java/com/google/devtools/build/android/PlaceholderIdFieldInitializerBuilder.java b/src/tools/java/com/google/devtools/build/android/PlaceholderIdFieldInitializerBuilder.java index 36373752d..e726606e7 100644 --- a/src/tools/java/com/google/devtools/build/android/PlaceholderIdFieldInitializerBuilder.java +++ b/src/tools/java/com/google/devtools/build/android/PlaceholderIdFieldInitializerBuilder.java @@ -143,8 +143,10 @@ public static PlaceholderIdFieldInitializerBuilder from(Path androidJar) { private final Map> innerClasses = new EnumMap<>(ResourceType.class); - private final Map> styleableAttrs = - new LinkedHashMap<>(); + // Use TreeMap for deterministic iteration order. LinkedHashMap depends on + // insertion order from addStyleableResource() calls, which varies across builds + // due to non-deterministic resource merge ordering. + private final Map> styleableAttrs = new TreeMap<>(); private PlaceholderIdFieldInitializerBuilder(AndroidFrameworkAttrIdProvider androidIdProvider) { this.androidIdProvider = androidIdProvider; @@ -170,8 +172,8 @@ public void addStyleableResource( // However, we don't combine across configurations, so there can be a pre-existing definition. Map normalizedAttrs = styleableAttrs.get(normalizedStyleableName); if (normalizedAttrs == null) { - // We need to maintain the original order of the attrs. - normalizedAttrs = new LinkedHashMap<>(); + // Use TreeMap for deterministic iteration order regardless of resource merge ordering. + normalizedAttrs = new TreeMap<>(); styleableAttrs.put(normalizedStyleableName, normalizedAttrs); } for (Map.Entry attrEntry : attrs.entrySet()) { @@ -181,8 +183,12 @@ public void addStyleableResource( } private Map assignAttrIds(int attrTypeId) { - // Attrs are special, since they can be defined within a declare-styleable. Those are sorted - // after top-level definitions. + // Attrs are special, since they can be defined within a declare-styleable. Those were + // historically sorted after top-level definitions to match aapt1 ordering. However, the + // inline vs non-inline classification depends on resource merge order (via + // ReferenceResolver.shouldInline/markInlined first-come-first-served semantics), making + // the partition non-deterministic. Since these are placeholder IDs that get reassigned + // during android_binary linking, we assign all attrs in sorted order for determinism. if (!innerClasses.containsKey(ResourceType.ATTR)) { return ImmutableMap.of(); } @@ -191,36 +197,15 @@ private Map assignAttrIds(int attrTypeId) { // After assigning public IDs, we count up monotonically, so we don't need to track additional // assignedIds to avoid collisions (use an ImmutableSet to ensure we don't add more). Set assignedIds = ImmutableSet.of(); - Set inlineAttrs = new LinkedHashSet<>(); - Set styleablesWithInlineAttrs = new TreeSet<>(); - for (Map.Entry> styleableAttrEntry : styleableAttrs.entrySet()) { - Map attrs = styleableAttrEntry.getValue(); - for (Map.Entry attrEntry : attrs.entrySet()) { - if (attrEntry.getValue()) { - inlineAttrs.add(attrEntry.getKey()); - styleablesWithInlineAttrs.add(styleableAttrEntry.getKey()); - } - } - } int nextId = nextFreeId(getInitialIdForTypeId(attrTypeId), assignedIds); - // Technically, aapt assigns based on declaration order, but the merge should have sorted - // the non-inline attributes, so assigning by sorted order is the same. + // Assign all attrs in sorted order regardless of inline status for determinism. SortedMap sortedAttrs = innerClasses.get(ResourceType.ATTR); for (String attr : sortedAttrs.keySet()) { - if (!inlineAttrs.contains(attr) && !attrToId.containsKey(attr)) { + if (!attrToId.containsKey(attr)) { attrToId.put(attr, nextId); nextId = nextFreeId(nextId + 1, assignedIds); } } - for (String styleable : styleablesWithInlineAttrs) { - Map attrs = styleableAttrs.get(styleable); - for (Map.Entry attrEntry : attrs.entrySet()) { - if (attrEntry.getValue() && !attrToId.containsKey(attrEntry.getKey())) { - attrToId.put(attrEntry.getKey(), nextId); - nextId = nextFreeId(nextId + 1, assignedIds); - } - } - } return ImmutableMap.copyOf(attrToId); }