BEP: Report configuration for all actions

Whenever we report an action in the build event protocol that has
a configuration associated with it, report the configuration as
well in the protocol (and not only the checksum, if the configuration
is not that of one of the top-level configured targets).

Change-Id: I9b085d5381b3c3509b4f3b99c8a77bc8fba6abfe
PiperOrigin-RevId: 161789745

7 files changed, 90 insertions, 16 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java
index e00555edb0..c8d957224c 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutedEvent.java
@@ -19,7 +19,9 @@ import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.buildeventstream.BuildEventConverters;
 import com.google.devtools.build.lib.buildeventstream.BuildEventId;
 import com.google.devtools.build.lib.buildeventstream.BuildEventStreamProtos;
+import com.google.devtools.build.lib.buildeventstream.BuildEventWithConfiguration;
 import com.google.devtools.build.lib.buildeventstream.GenericBuildEvent;
+import com.google.devtools.build.lib.buildeventstream.NullConfiguration;
 import com.google.devtools.build.lib.buildeventstream.PathConverter;
 import com.google.devtools.build.lib.causes.ActionFailed;
 import com.google.devtools.build.lib.causes.Cause;
@@ -30,7 +32,7 @@ import java.util.Collection;
  * This event is fired during the build, when an action is executed. It contains information about
  * the action: the Action itself, and the output file names its stdout and stderr are recorded in.
  */
-public class ActionExecutedEvent implements BuildEvent {
+public class ActionExecutedEvent implements BuildEventWithConfiguration {
   private final Action action;
   private final ActionExecutionException exception;
   private final Path stdout;
@@ -84,6 +86,19 @@ public class ActionExecutedEvent implements BuildEvent {
   }
 
   @Override
+  public Collection<BuildEvent> getConfigurations() {
+    if (action.getOwner() != null) {
+      BuildEvent configuration = action.getOwner().getConfiguration();
+      if (configuration == null) {
+        configuration = new NullConfiguration();
+      }
+      return ImmutableList.of(configuration);
+    } else {
+      return ImmutableList.<BuildEvent>of();
+    }
+  }
+
+  @Override
   public BuildEventStreamProtos.BuildEvent asStreamProto(BuildEventConverters converters) {
     PathConverter pathConverter = converters.pathConverter();
     BuildEventStreamProtos.ActionExecuted.Builder actionBuilder =
@@ -108,13 +123,12 @@ public class ActionExecutedEvent implements BuildEvent {
     if (action.getOwner() != null && action.getOwner().getLabel() != null) {
       actionBuilder.setLabel(action.getOwner().getLabel().toString());
     }
-    // TODO(aehlig): ensure the configuration is shown in the stream, even if it is not
-    // one of the configurations of a top-level configured target.
     if (action.getOwner() != null) {
-      actionBuilder.setConfiguration(
-          BuildEventStreamProtos.BuildEventId.ConfigurationId.newBuilder()
-              .setId(action.getOwner().getConfigurationChecksum())
-              .build());
+      BuildEvent configuration = action.getOwner().getConfiguration();
+      if (configuration == null) {
+        configuration = new NullConfiguration();
+      }
+      actionBuilder.setConfiguration(configuration.getEventId().asStreamProto().getConfiguration());
     }
     if (exception == null) {
       actionBuilder.setPrimaryOutput(
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java b/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
index 72ca914811..b8c0dc883f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionOwner.java
@@ -15,6 +15,7 @@ package com.google.devtools.build.lib.actions;
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.buildeventstream.BuildEvent;
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.Location;
@@ -34,8 +35,15 @@ import javax.annotation.Nullable;
 public abstract class ActionOwner {
   /** An action owner for special cases. Usage is strongly discouraged. */
   public static final ActionOwner SYSTEM_ACTION_OWNER =
-      ActionOwner.create(null, ImmutableList.<AspectDescriptor>of(),
-          null, "system", "empty target kind", "system", null);
+      ActionOwner.create(
+          null,
+          ImmutableList.<AspectDescriptor>of(),
+          null,
+          "system",
+          "empty target kind",
+          "system",
+          null,
+          null);
 
   public static ActionOwner create(
       @Nullable Label label,
@@ -44,6 +52,7 @@ public abstract class ActionOwner {
       @Nullable String mnemonic,
       @Nullable String targetKind,
       String configurationChecksum,
+      @Nullable BuildEvent configuration,
       @Nullable String additionalProgressInfo) {
     return new AutoValue_ActionOwner(
         location,
@@ -51,6 +60,7 @@ public abstract class ActionOwner {
         aspectDescriptors,
         mnemonic,
         Preconditions.checkNotNull(configurationChecksum),
+        configuration,
         targetKind,
         additionalProgressInfo);
   }
@@ -77,6 +87,13 @@ public abstract class ActionOwner {
    */
   public abstract String getConfigurationChecksum();
 
+  /**
+   * Return the configuration of the action owner, if any, as it should be reported in the build
+   * event protocol.
+   */
+  @Nullable
+  public abstract BuildEvent getConfiguration();
+
   /** Returns the target kind (rule class name) for this ActionOwner, if any; null otherwise. */
   @Nullable
   public abstract String getTargetKind();
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
index 61daf33027..986a93b308 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java
@@ -423,6 +423,7 @@ public final class RuleContext extends TargetContext
         configuration.getMnemonic(),
         rule.getTargetKind(),
         configuration.checksum(),
+        configuration,
         configuration.isHostConfiguration() ? HOST_CONFIGURATION_PROGRESS_TAG : null);
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
index 6ff3e3bc21..af9d68f79f 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -211,6 +211,7 @@ public final class ActionsTestUtil {
           "dummy-configuration-mnemonic",
           null,
           "dummy-configuration",
+          null,
           null);
 
   public static final ArtifactOwner NULL_ARTIFACT_OWNER =
diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java
index 20ec2b5af9..cf17e08f2e 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/util/FakeOwner.java
@@ -52,6 +52,7 @@ public final class FakeOwner implements ActionExecutionMetadata {
         mnemonic,
         /*targetKind=*/ null,
         "configurationChecksum",
+        /* configuration=*/ null,
         "additionalProgressInfo");
   }
 
@@ -150,4 +151,4 @@ public final class FakeOwner implements ActionExecutionMetadata {
   public boolean shouldReportPathPrefixConflict(ActionAnalysisMetadata action) {
     throw new UnsupportedOperationException();
   }
-}
\ No newline at end of file
+}
diff --git a/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java b/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java
index 6212a7f7e4..09b5dda3fb 100644
--- a/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/runtime/ExperimentalStateTrackerTest.java
@@ -431,8 +431,9 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
         "/home/user/bazel/out/abcdef/some/very/very/long/path/for/some/library/directory/foo.jar");
     Label label =
         Label.parseAbsolute("//some/very/very/long/path/for/some/library/directory:libfoo");
-    ActionOwner owner = ActionOwner.create(
-        label, ImmutableList.<AspectDescriptor>of(), null, null, null, "fedcba", null);
+    ActionOwner owner =
+        ActionOwner.create(
+            label, ImmutableList.<AspectDescriptor>of(), null, null, null, "fedcba", null, null);
     when(action.getOwner()).thenReturn(owner);
 
     clock.advanceMillis(TimeUnit.SECONDS.toMillis(3));
@@ -563,8 +564,15 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
     ConfiguredTarget targetFooTest = Mockito.mock(ConfiguredTarget.class);
     when(targetFooTest.getLabel()).thenReturn(labelFooTest);
     ActionOwner fooOwner =
-        ActionOwner.create(labelFooTest,
-            ImmutableList.<AspectDescriptor>of(), null, null, null, "abcdef", null);
+        ActionOwner.create(
+            labelFooTest,
+            ImmutableList.<AspectDescriptor>of(),
+            null,
+            null,
+            null,
+            "abcdef",
+            null,
+            null);
 
     Label labelBarTest = Label.parseAbsolute("//baz:bartest");
     ConfiguredTarget targetBarTest = Mockito.mock(ConfiguredTarget.class);
@@ -573,8 +581,15 @@ public class ExperimentalStateTrackerTest extends FoundationTestCase {
     when(filteringComplete.getTestTargets())
         .thenReturn(ImmutableSet.of(targetFooTest, targetBarTest));
     ActionOwner barOwner =
-        ActionOwner.create(labelBarTest,
-            ImmutableList.<AspectDescriptor>of(), null, null, null, "fedcba", null);
+        ActionOwner.create(
+            labelBarTest,
+            ImmutableList.<AspectDescriptor>of(),
+            null,
+            null,
+            null,
+            "fedcba",
+            null,
+            null);
 
     stateTracker.testFilteringComplete(filteringComplete);
 
diff --git a/src/test/shell/integration/build_event_stream_test.sh b/src/test/shell/integration/build_event_stream_test.sh
index 6ef198cbb9..348cb3c3be 100755
--- a/src/test/shell/integration/build_event_stream_test.sh
+++ b/src/test/shell/integration/build_event_stream_test.sh
@@ -145,6 +145,20 @@ genrule(
     cmd = "cp \$< \$@",
 )
 EOF
+mkdir -p failingtool
+cat > failingtool/BUILD <<'EOF'
+genrule(
+    name = "tool",
+    outs = ["tool.sh"],
+    cmd = "false",
+)
+genrule(
+    name = "usestool",
+    outs = ["out.txt"],
+    tools = [":tool"],
+    cmd = "$(location :tool) > $@",
+)
+EOF
 }
 
 #### TESTS #############################################################
@@ -449,6 +463,17 @@ function test_root_cause_early() {
       || fail "failed action not before compelted target"
 }
 
+function test_action_conf() {
+  # Verify that the expected configurations for actions are reported.
+  # The example contains a configuration transition (from building for
+  # target to building for host). As the action fails, we expect the
+  # configuration of the action to be reported as well.
+  (bazel build --build_event_text_file=$TEST_log \
+         -k failingtool/... && fail "build failure expected") || true
+  count=`grep '^configuration' "${TEST_log}" | wc -l`
+  [ "${count}" -eq 2 ] || fail "Expected 2 configurations, found $count."
+}
+
 function test_loading_failure() {
   # Verify that if loading fails, this is properly reported as the
   # reason for the target expansion event not resulting in targets