From 56d0a049d59b0d71ca9edb413fbc1c0b7a540677 Mon Sep 17 00:00:00 2001 From: Tiago Quelhas Date: Mon, 11 Sep 2023 21:05:29 +0200 Subject: [PATCH] [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 --- .../build/lib/exec/SpawnLogContext.java | 14 ++++- src/main/protobuf/spawn.proto | 4 ++ .../lib/exec/AbstractSpawnStrategyTest.java | 59 +++++++++++++++++++ .../build/lib/exec/util/SpawnBuilder.java | 8 +++ 4 files changed, 84 insertions(+), 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 index 88cfe1b0f5e960..ccb470d373d1d0 100644 --- 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 toolFiles = spawn.getToolFiles().toSet(); + try { for (Map.Entry 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 index 350ff407008a8c..5863f973424431 100644 --- 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 index a1d315267231cc..dd0a4d1954d07c 100644 --- 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 index 9564169e77e2a1..5a396b8d055624 100644 --- 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;