Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[7.4.0] Fix NPE in execlog when runfiles middleman are not top-level #23888

Merged
merged 1 commit into from
Oct 7, 2024
Merged

[7.4.0] Fix NPE in execlog when runfiles middleman are not top-level #23888

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 4 additions & 10 deletions src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@
import com.github.luben.zstd.ZstdOutputStream;
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.common.collect.ImmutableSet;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.AbstractAction;
import com.google.devtools.build.lib.actions.ActionInput;
Expand Down Expand Up @@ -65,7 +64,6 @@
import java.util.Comparator;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.SortedMap;
import java.util.concurrent.ForkJoinPool;
import javax.annotation.Nullable;
Expand Down Expand Up @@ -328,7 +326,6 @@ private int logInputs(
return logInputSet(
spawn.getInputFiles(),
spawn.getRunfilesSupplier().getRunfilesTreesForLogging(),
spawn.getRunfilesSupplier().getRunfilesTreesForLogging().keySet(),
additionalInputIds.build(),
inputMetadataProvider,
fileSystem,
Expand All @@ -348,7 +345,6 @@ private int logTools(
return logInputSet(
spawn.getToolFiles(),
spawn.getRunfilesSupplier().getRunfilesTreesForLogging(),
ImmutableSet.of(),
ImmutableList.of(),
inputMetadataProvider,
fileSystem,
Expand All @@ -371,14 +367,13 @@ private int logTools(
private int logInputSet(
NestedSet<? extends ActionInput> set,
Map<Artifact, RunfilesTree> runfilesTrees,
Set<Artifact> extraMiddleman,
Collection<Integer> additionalDirectoryIds,
InputMetadataProvider inputMetadataProvider,
FileSystem fileSystem,
boolean shared,
boolean isTestRunnerSpawn)
throws IOException, InterruptedException {
if (set.isEmpty() && additionalDirectoryIds.isEmpty() && extraMiddleman.isEmpty()) {
if (set.isEmpty() && additionalDirectoryIds.isEmpty()) {
return 0;
}

Expand All @@ -393,16 +388,15 @@ private int logInputSet(
builder.addTransitiveSetIds(
logInputSet(
transitive,
ImmutableMap.of(),
ImmutableSet.of(),
runfilesTrees,
ImmutableList.of(),
inputMetadataProvider,
fileSystem,
/* shared= */ true,
/* isTestRunnerSpawn= */ false));
}

for (ActionInput input : Iterables.concat(set.getLeaves(), extraMiddleman)) {
for (ActionInput input : set.getLeaves()) {
if (input instanceof Artifact artifact && artifact.isMiddlemanArtifact()) {
RunfilesTree runfilesTree = runfilesTrees.get(artifact);
builder.addInputIds(
Expand Down Expand Up @@ -569,8 +563,8 @@ private int logRunfilesTree(
builder.setInputSetId(
logInputSet(
runfilesTree.getArtifactsAtCanonicalLocationsForLogging(),
// Runfiles trees are never nested.
ImmutableMap.of(),
ImmutableSet.of(),
ImmutableList.of(),
inputMetadataProvider,
fileSystem,
Expand Down
4 changes: 2 additions & 2 deletions src/test/java/com/google/devtools/build/lib/exec/CompactSpawnLogContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,13 +194,13 @@ public void testRunfilesTreeReusedForTool() throws Exception {
Spawn firstSpawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInputs(firstInput)
.withInputs(firstInput, runfilesMiddleman)
.withTool(runfilesMiddleman)
.build();
Spawn secondSpawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInputs(secondInput)
.withInputs(secondInput, runfilesMiddleman)
.withTool(runfilesMiddleman)
.build();

Expand Down
127 changes: 115 additions & 12 deletions src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,8 @@ public void testRunfilesFileInput(@TestParameter InputsMode inputsMode) throws E
RunfilesSupplier runfilesSupplier =
createRunfilesSupplier(runfilesMiddleman, runfilesRoot, runfilesInput);

SpawnBuilder spawnBuilder = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier);
SpawnBuilder spawnBuilder =
defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).withInput(runfilesMiddleman);
if (inputsMode.isTool()) {
spawnBuilder.withTool(runfilesMiddleman);
}
Expand Down Expand Up @@ -453,6 +454,73 @@ public void testRunfilesFileInput(@TestParameter InputsMode inputsMode) throws E
.build());
}

@Test
public void testRunfilesNestedMiddleman() throws Exception {
Artifact runfilesMiddleman = ActionsTestUtil.createArtifact(middlemanDir, "runfiles");
Artifact runfilesInput = ActionsTestUtil.createArtifact(rootDir, "data.txt");
writeFile(runfilesInput, "abc");
Artifact toolFile1 = ActionsTestUtil.createArtifact(rootDir, "tool1");
writeFile(toolFile1, "def");
Artifact toolFile2 = ActionsTestUtil.createArtifact(rootDir, "tool2");
writeFile(toolFile2, "ghi");

PathFragment runfilesRoot = outputDir.getExecPath().getRelative("foo.runfiles");
RunfilesSupplier runfilesSupplier =
createRunfilesSupplier(runfilesMiddleman, runfilesRoot, runfilesInput);

NestedSet<ActionInput> tools =
NestedSetBuilder.<ActionInput>stableOrder()
.add(toolFile1)
.addTransitive(
NestedSetBuilder.<ActionInput>stableOrder()
.add(runfilesMiddleman)
.add(toolFile2)
.build())
.build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInputs(tools)
.withTools(tools)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
createInputMetadataProvider(runfilesSupplier, runfilesInput, toolFile1, toolFile2);

context.logSpawn(
spawn,
inputMetadataProvider,
createInputMap(runfilesSupplier, inputMetadataProvider, toolFile1, toolFile2),
fs,
defaultTimeout(),
defaultSpawnResult());

closeAndAssertLog(
context,
defaultSpawnExecBuilder()
.addInputs(
File.newBuilder()
.setPath(
PRODUCT_NAME
+ "-out/k8-fastbuild/bin/foo.runfiles/"
+ WORKSPACE_NAME
+ "/data.txt")
.setDigest(getDigest("abc"))
.setIsTool(true))
.addInputs(
File.newBuilder()
.setPath("tool1")
.setDigest(getDigest("def"))
.setIsTool(true))
.addInputs(
File.newBuilder()
.setPath("tool2")
.setDigest(getDigest("ghi"))
.setIsTool(true))
.build());
}

@Test
public void testRunfilesDirectoryInput(
@TestParameter DirContents dirContents, @TestParameter InputsMode inputsMode)
Expand All @@ -469,7 +537,8 @@ public void testRunfilesDirectoryInput(
RunfilesSupplier runfilesSupplier =
createRunfilesSupplier(runfilesMiddleman, runfilesRoot, runfilesInput);

SpawnBuilder spawnBuilder = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier);
SpawnBuilder spawnBuilder =
defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).withInput(runfilesMiddleman);
if (inputsMode.isTool()) {
spawnBuilder.withTool(runfilesMiddleman);
}
Expand Down Expand Up @@ -537,7 +606,8 @@ public void testRunfilesEmptyInput(@TestParameter InputsMode inputsMode) throws
externalGenArtifact,
externalSourceArtifact);

SpawnBuilder spawnBuilder = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier);
SpawnBuilder spawnBuilder =
defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).withInput(runfilesMiddleman);
if (inputsMode.isTool()) {
spawnBuilder.withTool(runfilesMiddleman);
}
Expand Down Expand Up @@ -687,7 +757,11 @@ public void testRunfilesMixedRoots(@TestParameter boolean legacyExternalRunfiles
externalSourceArtifact,
externalGenArtifact);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -833,7 +907,11 @@ public void testRunfilesExternalOnly(
externalSourceArtifact,
externalGenArtifact);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -967,7 +1045,11 @@ public void testRunfilesFilesCollide(@TestParameter boolean legacyExternalRunfil
externalSourceArtifact,
externalGenArtifact);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -1074,7 +1156,11 @@ public void testRunfilesFilesAndSymlinksCollide(@TestParameter boolean legacyExt
externalSourceArtifact,
externalGenArtifact);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -1168,7 +1254,11 @@ public void testRunfilesFileAndRootSymlinkCollide() throws Exception {
/* legacyExternalRunfiles= */ false,
sourceArtifact);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -1217,7 +1307,11 @@ public void testRunfilesCrossTypeCollision(@TestParameter boolean symlinkFirst)
/* legacyExternalRunfiles= */ false,
NestedSetBuilder.wrap(Order.STABLE_ORDER, artifacts));

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -1295,7 +1389,11 @@ public void testRunfilesPostOrderCollision(@TestParameter boolean nestBoth) thro
/* legacyExternalRunfiles= */ false,
artifacts);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down Expand Up @@ -1376,7 +1474,8 @@ public void testRunfilesSymlinkTargets(
: ImmutableMap.of(),
/* legacyExternalRunfiles= */ false);

SpawnBuilder spawnBuilder = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier);
SpawnBuilder spawnBuilder =
defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).withInput(runfilesMiddleman);
if (inputsMode.isTool()) {
spawnBuilder.withTool(runfilesMiddleman);
}
Expand Down Expand Up @@ -1455,7 +1554,11 @@ public void testRunfileSymlinkFileWithDirectoryContents(
: ImmutableMap.of(),
/* legacyExternalRunfiles= */ false);

Spawn spawn = defaultSpawnBuilder().withRunfilesSupplier(runfilesSupplier).build();
Spawn spawn =
defaultSpawnBuilder()
.withRunfilesSupplier(runfilesSupplier)
.withInput(runfilesMiddleman)
.build();

SpawnLogContext context = createSpawnLogContext();
InputMetadataProvider inputMetadataProvider =
Expand Down
Loading