[6.4.0] Mark tool inputs in the execution log. (#19483)

This is useful to identify actions that don't separate their tool and non-tool inputs properly. PiperOrigin-RevId: 564351781 Change-Id: If93c28e1e10f4ba0b2dd4b802d29d34d095bde12
bazelbuild · Sep 11, 2023 · 56d0a04 · 56d0a04
1 parent 5a0339f
commit 56d0a04
Show file tree

Hide file tree

Showing 4 changed files with 84 additions and 1 deletion.
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnLogContext.java
@@ -15,10 +15,12 @@
 
 import build.bazel.remote.execution.v2.Platform;
 import com.google.common.base.Preconditions;
+import com.google.common.collect.ImmutableSet;
 import com.google.common.flogger.GoogleLogger;
 import com.google.common.hash.HashCode;
 import com.google.devtools.build.lib.actions.ActionContext;
 import com.google.devtools.build.lib.actions.ActionInput;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ExecException;
 import com.google.devtools.build.lib.actions.FileArtifactValue;
 import com.google.devtools.build.lib.actions.MetadataProvider;
@@ -100,6 +102,8 @@ public void logSpawn(
       builder.addEnvironmentVariablesBuilder().setName(var).setValue(env.get(var));
     }
 
+    ImmutableSet<? extends ActionInput> toolFiles = spawn.getToolFiles().toSet();
+
     try {
       for (Map.Entry<PathFragment, ActionInput> e : inputMap.entrySet()) {
         ActionInput input = e.getValue();
@@ -111,7 +115,15 @@ public void logSpawn(
           listDirectoryContents(inputPath, builder::addInputs, metadataProvider);
         } else {
           Digest digest = computeDigest(input, null, metadataProvider, xattrProvider);
-          builder.addInputsBuilder().setPath(input.getExecPathString()).setDigest(digest);
+          boolean isTool =
+              toolFiles.contains(input)
+                  || (input instanceof TreeFileArtifact
+                      && toolFiles.contains(((TreeFileArtifact) input).getParent()));
+          builder
+              .addInputsBuilder()
+              .setPath(input.getExecPathString())
+              .setDigest(digest)
+              .setIsTool(isTool);
         }
       }
     } catch (IOException e) {

diff --git a/src/main/protobuf/spawn.proto b/src/main/protobuf/spawn.proto
@@ -41,6 +41,10 @@ message File {
 
   // Digest of the file's contents.
   Digest digest = 2;
+
+  // Whether the file is a tool.
+  // Only set for inputs, never for outputs.
+  bool is_tool = 3;
 }
 
 // Contents of command environment.

diff --git a/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java b/src/test/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategyTest.java
@@ -29,9 +29,12 @@
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.actions.ActionExecutionContext;
 import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
+import com.google.devtools.build.lib.actions.Artifact.TreeFileArtifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.actions.FutureSpawn;
 import com.google.devtools.build.lib.actions.MetadataProvider;
+import com.google.devtools.build.lib.actions.ArtifactRoot.RootType;
 import com.google.devtools.build.lib.actions.Spawn;
 import com.google.devtools.build.lib.actions.SpawnExecutedEvent;
 import com.google.devtools.build.lib.actions.SpawnResult;
@@ -92,6 +95,7 @@ public TestedSpawnStrategy(Path execRoot, SpawnRunner spawnRunner) {
   private final Path execRoot = fs.getPath("/execroot");
   private Scratch scratch;
   private ArtifactRoot rootDir;
+  private ArtifactRoot outputDir;
   @Mock private SpawnRunner spawnRunner;
   @Mock private ActionExecutionContext actionExecutionContext;
   @Mock private MessageOutputStream messageOutput;
@@ -103,6 +107,7 @@ public final void setUp() throws Exception {
     MockitoAnnotations.initMocks(this);
     scratch = new Scratch(fs);
     rootDir = ArtifactRoot.asSourceRoot(Root.fromPath(scratch.dir("/execroot")));
+    outputDir = ArtifactRoot.asDerivedRoot(scratch.dir("/execroot"), RootType.Output, "out");
     eventHandler = new StoredEventHandler();
     when(actionExecutionContext.getEventHandler()).thenReturn(eventHandler);
     when(actionExecutionContext.getClock()).thenReturn(clock);
@@ -453,6 +458,60 @@ public void testLogSpawn_defaultPlatform_getsLogged() throws Exception {
     verify(messageOutput).write(expected); // output will reflect default properties
   }
 
+  @Test
+  public void testLogSpawn_toolInputs() throws Exception {
+    Artifact toolFile = ActionsTestUtil.createArtifact(rootDir, "tool.file");
+    SpecialArtifact toolDir =
+        ActionsTestUtil.createTreeArtifactWithGeneratingAction(outputDir, "tool.dir");
+
+    scratch.file("/execroot/tool.file", "123");
+    scratch.file("/execroot/out/tool.dir/tool.file", "456");
+
+    setUpExecutionContext(/* executionOptions= */ null, /* remoteOptions= */ null);
+    when(actionExecutionContext.getArtifactExpander())
+        .thenReturn(
+            (artifact, output) -> {
+              if (artifact.equals(toolDir)) {
+                output.add(TreeFileArtifact.createTreeOutput(toolDir, "tool.file"));
+              }
+            });
+    Spawn spawn =
+        new SpawnBuilder("cmd").withInputs(toolFile, toolDir).withTools(toolFile, toolDir).build();
+    assertThrows(
+        SpawnExecException.class,
+        () -> new TestedSpawnStrategy(execRoot, spawnRunner).exec(spawn, actionExecutionContext));
+
+    SpawnExec expected =
+        defaultSpawnExecBuilder("cmd")
+            .addInputs(
+                File.newBuilder()
+                    .setPath("out/tool.dir/tool.file")
+                    .setDigest(
+                        Digest.newBuilder()
+                            .setHash(
+                                "cdfba543ee8ef7fdb3d8b587648cc22dd792bbd6272cc5447307c7c106c2374c")
+                            .setSizeBytes(4)
+                            .setHashFunctionName("SHA-256")
+                            .build())
+                    .setIsTool(true)
+                    .build())
+            .addInputs(
+                File.newBuilder()
+                    .setPath("tool.file")
+                    .setDigest(
+                        Digest.newBuilder()
+                            .setHash(
+                                "181210f8f9c779c26da1d9b2075bde0127302ee0e3fca38c9a83f5b1dd8e5d3b")
+                            .setSizeBytes(4)
+                            .setHashFunctionName("SHA-256")
+                            .build())
+                    .setIsTool(true)
+                    .build())
+            .build();
+
+    verify(messageOutput).write(expected);
+  }
+
   @Test
   public void testLogSpawn_spawnMetrics() throws Exception {
     ExecutionOptions executionOptions = Options.getDefaults(ExecutionOptions.class);

diff --git a/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java b/src/test/java/com/google/devtools/build/lib/exec/util/SpawnBuilder.java
@@ -235,6 +235,14 @@ public SpawnBuilder withTool(ActionInput tool) {
     return this;
   }
 
+  @CanIgnoreReturnValue
+  public SpawnBuilder withTools(ActionInput... tools) {
+    for (ActionInput tool : tools) {
+      this.tools.add(tool);
+    }
+    return this;
+  }
+
   @CanIgnoreReturnValue
   public SpawnBuilder withLocalResources(ResourceSet resourceSet) {
     this.resourceSet = resourceSet;