Catch action conflicts in the same target during configured target analysis, and fail hard in other cases.

--
MOS_MIGRATED_REVID=89720528

4 files changed, 62 insertions, 21 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValue.java
index 1dfa7225ec..4d51cfe9d5 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ActionLookupValue.java
@@ -19,12 +19,12 @@ import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.Action;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.skyframe.SkyFunctionName;
 import com.google.devtools.build.skyframe.SkyKey;
 import com.google.devtools.build.skyframe.SkyValue;
 
-import java.util.HashMap;
 import java.util.Map;
 
 /**
@@ -35,23 +35,28 @@ import java.util.Map;
 public class ActionLookupValue implements SkyValue {
   protected final ImmutableMap<Artifact, Action> generatingActionMap;
 
-  ActionLookupValue(Iterable<Action> actions) {
-    // Duplicate/shared actions get passed in all the time. Blaze is weird. We can't double-register
-    // the generated artifacts in an immutable map builder, so we double-register them in a more
-    // forgiving map, and then use that map to create the immutable one.
-    Map<Artifact, Action> generatingActions = new HashMap<>();
-    for (Action action : actions) {
-      for (Artifact artifact : action.getOutputs()) {
-        generatingActions.put(artifact, action);
-      }
+  private static Map<Artifact, Action> filterSharedActionsAndThrowRuntimeIfConflict(
+      Iterable<Action> actions) {
+    try {
+      return ConfiguredTargetFunction.filterSharedActionsAndThrowIfConflict(actions);
+    } catch (ActionConflictException e) {
+      // Programming bug.
+      throw new IllegalStateException(e);
     }
-    generatingActionMap = ImmutableMap.copyOf(generatingActions);
+  }
+
+  ActionLookupValue(Iterable<Action> actions) {
+    this(filterSharedActionsAndThrowRuntimeIfConflict(actions));
   }
 
   ActionLookupValue(Action action) {
     this(ImmutableList.of(action));
   }
 
+  ActionLookupValue(Map<Artifact, Action> generatingActionMap) {
+    this.generatingActionMap = ImmutableMap.copyOf(generatingActionMap);
+  }
+
   Action getGeneratingAction(Artifact artifact) {
     return generatingActionMap.get(artifact);
   }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 8b1b1bc194..563174d79f 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -16,10 +16,13 @@ package com.google.devtools.build.lib.skyframe;
 import com.google.common.base.Function;
 import com.google.common.base.Preconditions;
 import com.google.common.collect.ArrayListMultimap;
-import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableSet;
 import com.google.common.collect.Iterables;
 import com.google.common.collect.ListMultimap;
+import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.actions.Actions;
+import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.Aspect;
 import com.google.devtools.build.lib.analysis.CachingAnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.ConfiguredAspectFactory;
@@ -494,8 +497,27 @@ final class ConfiguredTargetFunction implements SkyFunction {
     analysisEnvironment.disable(target);
     Preconditions.checkNotNull(configuredTarget, target);
 
-    return new ConfiguredTargetValue(configuredTarget,
-        ImmutableList.copyOf(analysisEnvironment.getRegisteredActions()));
+    try {
+      return new ConfiguredTargetValue(configuredTarget,
+          filterSharedActionsAndThrowIfConflict(analysisEnvironment.getRegisteredActions()));
+    } catch (ActionConflictException e) {
+      throw new ConfiguredTargetFunctionException(e);
+    }
+  }
+
+  static Map<Artifact, Action> filterSharedActionsAndThrowIfConflict(Iterable<Action> actions)
+      throws ActionConflictException {
+    Map<Artifact, Action> generatingActions = new HashMap<>();
+    for (Action action : actions) {
+      for (Artifact artifact : action.getOutputs()) {
+        Action previousAction = generatingActions.put(artifact, action);
+        if (previousAction != null && previousAction != action
+            && !Actions.canBeShared(previousAction, action)) {
+          throw new ActionConflictException(artifact, previousAction, action);
+        }
+      }
+    }
+    return generatingActions;
   }
 
   /**
@@ -522,6 +544,10 @@ final class ConfiguredTargetFunction implements SkyFunction {
       super(error, Transience.PERSISTENT);
     };
 
+    private ConfiguredTargetFunctionException(ActionConflictException e) {
+      super(e, Transience.PERSISTENT);
+    }
+
     private ConfiguredTargetFunctionException(
         @Nullable SkyKey childKey, Exception transitiveError) {
       super(transitiveError, childKey);
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java
index 200e05ff3f..98aa992ae8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetValue.java
@@ -18,6 +18,7 @@ import com.google.common.base.Preconditions;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.Iterables;
 import com.google.devtools.build.lib.actions.Action;
+import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.analysis.ConfiguredTarget;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
@@ -25,6 +26,8 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
 import com.google.devtools.build.lib.syntax.Label;
 import com.google.devtools.build.skyframe.SkyKey;
 
+import java.util.Map;
+
 import javax.annotation.Nullable;
 
 /**
@@ -43,10 +46,11 @@ public final class ConfiguredTargetValue extends ActionLookupValue {
   // separate variable in order to save memory.
   @Nullable private volatile Iterable<Action> actions;
 
-  ConfiguredTargetValue(ConfiguredTarget configuredTarget, Iterable<Action> actions) {
-    super(actions);
+  ConfiguredTargetValue(ConfiguredTarget configuredTarget,
+      Map<Artifact, Action> generatingActionMap) {
+    super(generatingActionMap);
     this.configuredTarget = configuredTarget;
-    this.actions = actions;
+    this.actions = generatingActionMap.values();
   }
 
   @VisibleForTesting
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 1221f0bd92..b859f7b3a3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -27,6 +27,7 @@ import com.google.devtools.build.lib.actions.ArtifactFactory;
 import com.google.devtools.build.lib.actions.ArtifactOwner;
 import com.google.devtools.build.lib.actions.ArtifactPrefixConflictException;
 import com.google.devtools.build.lib.actions.MutableActionGraph;
+import com.google.devtools.build.lib.actions.MutableActionGraph.ActionConflictException;
 import com.google.devtools.build.lib.analysis.AnalysisEnvironment;
 import com.google.devtools.build.lib.analysis.AnalysisFailureEvent;
 import com.google.devtools.build.lib.analysis.Aspect;
@@ -178,8 +179,6 @@ public final class SkyframeBuildView {
     } finally {
       enableAnalysis(false);
     }
-    // For Skyframe m1, note that we already reported action conflicts during action registration
-    // in the legacy action graph.
     ImmutableMap<Action, ConflictException> badActions = skyframeExecutor.findArtifactConflicts();
 
     // Filter out all CTs that have a bad action and convert to a list of configured targets. This
@@ -229,6 +228,9 @@ public final class SkyframeBuildView {
           errorInfo);
       String errorMsg = "Analysis of target '" + ConfiguredTargetValue.extractLabel(topLevel)
           + "' failed; build aborted";
+      if (cause instanceof ActionConflictException) {
+        ((ActionConflictException) cause).reportTo(skyframeExecutor.getReporter());
+      }
       throw new ViewCreationFailedException(errorMsg);
     }
 
@@ -254,6 +256,10 @@ public final class SkyframeBuildView {
           root = maybeGetConfiguredTargetCycleCulprit(errorInfo.getCycleInfo());
         }
         if (warningListener != null) {
+          Exception cause = errorInfo.getException();
+          if (cause instanceof ActionConflictException) {
+            ((ActionConflictException) cause).reportTo(warningListener);
+          }
           warningListener.handle(Event.warn("errors encountered while analyzing target '"
               + label + "': it will not be built"));
         }
@@ -318,8 +324,8 @@ public final class SkyframeBuildView {
     if (cause != null) {
       // We should only be trying to configure targets when the loading phase succeeds, meaning
       // that the only errors should be analysis errors.
-      Preconditions.checkState(cause instanceof ConfiguredValueCreationException,
-          "%s -> %s", key, errorInfo);
+      Preconditions.checkState(cause instanceof ConfiguredValueCreationException
+          || cause instanceof ActionConflictException, "%s -> %s", key, errorInfo);
     }
   }