Classify the undeclared test outputs directory as a directory instead of a file.
When constructing a remote execution request, any output other than a tree artifact is classified as a (regular) file. But the undeclared outputs directory is actually an ActionInput, and promoting it to an Artifact seems a rather involved refactor. Instead, add an ActionInput#isDirectory method, similar to the preexisting ActionInput#isSymlink, and use it in RemoteExecutionService#buildCommand.
Also take the opportunity to add tests verifying that all output types are handled correctly.
Fixes #16315.
PiperOrigin-RevId: 482738939
Change-Id: I5d237efd7244cee25ed9702b593c6d0c714e3f8a
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInput.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInput.java
index 573326a..2d26ad5 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInput.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInput.java
@@ -41,6 +41,9 @@
*/
PathFragment getExecPath();
+ /** The input is a directory. */
+ boolean isDirectory();
+
/** The input is a symlink that is supposed to stay un-dereferenced. */
boolean isSymlink();
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
index bc3d15c..6924f04 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionInputHelper.java
@@ -41,6 +41,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public int hashCode() {
return getExecPathString().hashCode();
}
@@ -103,6 +108,32 @@
}
/**
+ * Creates an ActionInput with just the given relative path (which must point to a directory) and
+ * no digest.
+ *
+ * @param path the relative path of the input.
+ * @return a ActionInput.
+ */
+ public static ActionInput fromPathToDirectory(PathFragment path) {
+ return new BasicActionInput() {
+ @Override
+ public String getExecPathString() {
+ return path.getPathString();
+ }
+
+ @Override
+ public PathFragment getExecPath() {
+ return path;
+ }
+
+ @Override
+ public boolean isDirectory() {
+ return true;
+ }
+ };
+ }
+
+ /**
* Expands middleman and tree artifacts in a sequence of {@link ActionInput}s.
*
* <p>The constructed list never contains middleman artifacts. If {@code keepEmptyTreeArtifacts}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
index 81efe6e..543e42f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/CommandLines.java
@@ -305,6 +305,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public boolean isSymlink() {
return false;
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java b/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java
index 8a035a9..8acbcb8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/cache/VirtualActionInput.java
@@ -55,6 +55,11 @@
private EmptyActionInput() {}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public boolean isSymlink() {
return false;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
index 31a6744..eadd829 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/test/TestRunnerAction.java
@@ -350,7 +350,7 @@
if (testConfiguration.getZipUndeclaredTestOutputs()) {
outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsZipPath()));
} else {
- outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsDir()));
+ outputs.add(ActionInputHelper.fromPathToDirectory(getUndeclaredOutputsDir()));
}
outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsManifestPath()));
outputs.add(ActionInputHelper.fromPath(getUndeclaredOutputsAnnotationsPath()));
diff --git a/src/main/java/com/google/devtools/build/lib/exec/BinTools.java b/src/main/java/com/google/devtools/build/lib/exec/BinTools.java
index c91b09c..c7291e7 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/BinTools.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/BinTools.java
@@ -189,6 +189,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public ByteString getBytes() throws IOException {
ByteString.Output out = ByteString.newOutput();
writeTo(out);
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index 1ee3d44..196247f 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -230,7 +230,7 @@
ArrayList<String> outputDirectories = new ArrayList<>();
for (ActionInput output : outputs) {
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
- if (output instanceof Artifact && ((Artifact) output).isTreeArtifact()) {
+ if (output.isDirectory()) {
outputDirectories.add(pathString);
} else {
outputFiles.add(pathString);
diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
index 62b1e2d..b3fb345 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionInputMapTest.java
@@ -506,6 +506,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public boolean isSymlink() {
return false;
}
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 07fe486..118ed7e 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
@@ -347,6 +347,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public boolean isSymlink() {
return false;
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index bb7ac8d..dc69fbe 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -180,6 +180,86 @@
}
@Test
+ public void buildRemoteAction_withRegularFileAsOutput() throws Exception {
+ PathFragment execPath = execRoot.getRelative("path/to/tree").asFragment();
+ Spawn spawn =
+ new SpawnBuilder("dummy")
+ .withOutput(ActionsTestUtil.createArtifactWithExecPath(artifactRoot, execPath))
+ .build();
+ FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+ RemoteExecutionService service = newRemoteExecutionService();
+
+ RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+ assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString());
+ assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+ }
+
+ @Test
+ public void buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
+ Spawn spawn =
+ new SpawnBuilder("dummy")
+ .withOutput(
+ ActionsTestUtil.createTreeArtifactWithGeneratingAction(
+ artifactRoot, PathFragment.create("path/to/dir")))
+ .build();
+ FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+ RemoteExecutionService service = newRemoteExecutionService();
+
+ RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+ assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+ assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
+ }
+
+ @Test
+ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
+ Spawn spawn =
+ new SpawnBuilder("dummy")
+ .withOutput(
+ ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath(
+ artifactRoot, PathFragment.create("path/to/link")))
+ .build();
+ FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+ RemoteExecutionService service = newRemoteExecutionService();
+
+ RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+ assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
+ assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+ }
+
+ @Test
+ public void buildRemoteAction_withActionInputFileAsOutput() throws Exception {
+ Spawn spawn =
+ new SpawnBuilder("dummy")
+ .withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/file")))
+ .build();
+ FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+ RemoteExecutionService service = newRemoteExecutionService();
+
+ RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+ assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/file");
+ assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
+ }
+
+ @Test
+ public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exception {
+ Spawn spawn =
+ new SpawnBuilder("dummy")
+ .withOutput(ActionInputHelper.fromPathToDirectory(PathFragment.create("path/to/dir")))
+ .build();
+ FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
+ RemoteExecutionService service = newRemoteExecutionService();
+
+ RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
+
+ assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
+ assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
+ }
+
+ @Test
public void buildRemoteAction_differentiateWorkspace_generateActionSalt() throws Exception {
Spawn spawn =
new SpawnBuilder("dummy")
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
index ccbfd7a..7be0513 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
@@ -212,6 +212,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public boolean isSymlink() {
return false;
}
@@ -228,6 +233,11 @@
}
@Override
+ public boolean isDirectory() {
+ return false;
+ }
+
+ @Override
public boolean isSymlink() {
return false;
}
