Automated rollback of commit f309ad3be36363070e87eef0ee04b12f4956d601.

*** Reason for rollback ***

Fixed duplicate derived inputs bug. Test is in diffbase.

RELNOTES[INC]: If the same artifact is generated by two distinct but identical actions, and a downstream action has both those actions' outputs in its inputs, the artifact will now appear twice in the downstream action's inputs. If this causes problems in Skylark actions, you can use the uniquify=True argument in Args.add_args.

PiperOrigin-RevId: 205863806

1 files changed, 15 insertions, 4 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
index 503e8ce2fe..505d75a3c4 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/HeaderDiscovery.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.vfs.PathFragment;
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.List;
+import java.util.Map;
 
 /**
  * HeaderDiscovery checks whether all header files that a compile action uses are actually declared
@@ -242,13 +243,23 @@ public class HeaderDiscovery {
 
     /** Creates a CppHeaderDiscovery instance. */
     public HeaderDiscovery build() {
-      ImmutableMap.Builder<PathFragment, Artifact> allowedDerivedInputsMap = ImmutableMap.builder();
+      Map<PathFragment, Artifact> allowedDerivedInputsMap = new HashMap<>();
       ImmutableSet.Builder<PathFragment> treeArtifactPrefixes = ImmutableSet.builder();
       for (Artifact a : allowedDerivedInputs) {
+        PathFragment execPath = a.getExecPath();
         if (a.isTreeArtifact()) {
-          treeArtifactPrefixes.add(a.getExecPath());
+          treeArtifactPrefixes.add(execPath);
         }
-        allowedDerivedInputsMap.put(a.getExecPath(), a);
+        // We may encounter duplicate keys in the derived inputs if two artifacts have different
+        // owners. Just use the first one. The two artifacts must be generated by equivalent
+        // (shareable) actions in order to have not generated a conflict in Bazel. If on an
+        // incremental build one changes without the other one changing, then if their paths remain
+        // the same, that will trigger an action conflict and fail the build. If one path changes,
+        // then this action will be re-analyzed, and will execute in Skyframe. It can legitimately
+        // get an action cache hit in that case, since even if it previously depended on the
+        // artifact whose path changed, that is not taken into account by the action cache, and it
+        // will get an action cache hit using the remaining un-renamed artifact.
+        allowedDerivedInputsMap.putIfAbsent(execPath, a);
       }
 
       return new HeaderDiscovery(
@@ -257,7 +268,7 @@ public class HeaderDiscovery {
           shouldValidateInclusions,
           dependencies,
           permittedSystemIncludePrefixes,
-          allowedDerivedInputsMap.build(),
+          ImmutableMap.copyOf(allowedDerivedInputsMap),
           treeArtifactPrefixes.build());
     }
   }