Skip to content

Commit

Permalink
Conditionally set output_paths based on Remote Executor capabilities
Browse files Browse the repository at this point in the history 
This is a follow up to bazelbuild#18269, toward the discussion in bazelbuild#18202.

Bump the Remote API supported version to v2.1.

Based on the Capability of the Remote Executor, either use output_paths
field or the legacy fields output_files and output_directories.
  • Loading branch information
@sluongng
sluongng committed Apr 29, 2023
1 parent 5d6ac4c commit 2d4ce65
Show file tree
Hide file tree
Showing 5 changed files with 134 additions and 35 deletions.
8 changes: 4 additions & 4 deletions src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,7 @@
import build.bazel.remote.execution.v2.ServerCapabilities;
import build.bazel.semver.SemVer;

/**
* Represents a version of the Remote Execution API.
*/
/** Represents a version of the Remote Execution API. */
public class ApiVersion implements Comparable<ApiVersion> {
public final int major;
public final int minor;
Expand All @@ -28,7 +26,8 @@ public class ApiVersion implements Comparable<ApiVersion> {

// The current version of the Remote Execution API. This field will need to be updated
// together with all version changes.
public static final ApiVersion current = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
public static final ApiVersion current =
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());

public ApiVersion(int major, int minor, int patch, String prerelease) {
this.major = major;
Expand Down Expand Up @@ -100,6 +99,7 @@ private enum State {
UNSUPPORTED,
DEPRECATED,
}

private final String message;
private final State state;

Expand Down
48 changes: 32 additions & 16 deletions src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -217,30 +217,37 @@ public RemoteExecutionService(
}

static Command buildCommand(
boolean useOutputPaths,
Collection<? extends ActionInput> outputs,
List<String> arguments,
ImmutableMap<String, String> env,
@Nullable Platform platform,
RemotePathResolver remotePathResolver) {
Command.Builder command = Command.newBuilder();
ArrayList<String> outputFiles = new ArrayList<>();
ArrayList<String> outputDirectories = new ArrayList<>();
ArrayList<String> outputPaths = new ArrayList<>();
for (ActionInput output : outputs) {
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
if (output.isDirectory()) {
outputDirectories.add(pathString);
} else {
outputFiles.add(pathString);
if (useOutputPaths) {
ArrayList<String> outputPaths = new ArrayList<>();
for (ActionInput output : outputs) {
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
outputPaths.add(pathString);
}
outputPaths.add(pathString);
Collections.sort(outputPaths);
command.addAllOutputPaths(outputPaths);
} else {
ArrayList<String> outputFiles = new ArrayList<>();
ArrayList<String> outputDirectories = new ArrayList<>();
for (ActionInput output : outputs) {
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
if (output.isDirectory()) {
outputDirectories.add(pathString);
} else {
outputFiles.add(pathString);
}
}
Collections.sort(outputFiles);
Collections.sort(outputDirectories);
command.addAllOutputFiles(outputFiles);
command.addAllOutputDirectories(outputDirectories);
}
Collections.sort(outputFiles);
Collections.sort(outputDirectories);
Collections.sort(outputPaths);
command.addAllOutputFiles(outputFiles);
command.addAllOutputDirectories(outputDirectories);
command.addAllOutputPaths(outputPaths);

if (platform != null) {
command.setPlatform(platform);
Expand Down Expand Up @@ -538,8 +545,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
}

var useOutputPaths = false;
if (mayBeExecutedRemotely(spawn)) {
var capabilities = remoteExecutor.getServerCapabilities();
if (capabilities != null) {
useOutputPaths =
ApiVersion.current.checkServerSupportedVersions(capabilities).isSupported();
}
}
Command command =
buildCommand(
useOutputPaths,
spawn.getOutputFiles(),
spawn.getArguments(),
spawn.getEnvironment(),
Expand Down
96 changes: 93 additions & 3 deletions src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import build.bazel.remote.execution.v2.Digest;
import build.bazel.remote.execution.v2.Directory;
import build.bazel.remote.execution.v2.DirectoryNode;
import build.bazel.remote.execution.v2.ExecutionCapabilities;
import build.bazel.remote.execution.v2.FileNode;
import build.bazel.remote.execution.v2.NodeProperties;
import build.bazel.remote.execution.v2.NodeProperty;
Expand All @@ -47,9 +48,11 @@
import build.bazel.remote.execution.v2.OutputSymlink;
import build.bazel.remote.execution.v2.Platform;
import build.bazel.remote.execution.v2.RequestMetadata;
import build.bazel.remote.execution.v2.ServerCapabilities;
import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy;
import build.bazel.remote.execution.v2.SymlinkNode;
import build.bazel.remote.execution.v2.Tree;
import build.bazel.semver.SemVer;
import com.google.common.base.Throwables;
import com.google.common.collect.ImmutableClassToInstanceMap;
import com.google.common.collect.ImmutableList;
Expand Down Expand Up @@ -135,6 +138,20 @@ public class RemoteExecutionServiceTest {
private final Reporter reporter = new Reporter(new EventBus());
private final StoredEventHandler eventHandler = new StoredEventHandler();

private final ServerCapabilities removeExecutorCapabilities =
ServerCapabilities.newBuilder()
.setLowApiVersion(ApiVersion.current.toSemVer())
.setHighApiVersion(ApiVersion.current.toSemVer())
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
.build();
private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
private final ServerCapabilities legacyRemoveExecutorCapabilities =
ServerCapabilities.newBuilder()
.setLowApiVersion(oldApiVersion.toSemVer())
.setHighApiVersion(oldApiVersion.toSemVer())
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
.build();

RemoteOptions remoteOptions;
private Path execRoot;
private ArtifactRoot artifactRoot;
Expand Down Expand Up @@ -174,6 +191,7 @@ public final void setUp() throws Exception {

cache = spy(new InMemoryRemoteCache(spy(new InMemoryCacheClient()), remoteOptions, digestUtil));
executor = mock(RemoteExecutionClient.class);
when(executor.getServerCapabilities()).thenReturn(removeExecutorCapabilities);

RequestMetadata metadata =
TracingMetadataUtils.buildMetadata("none", "none", "action-id", null);
Expand All @@ -192,8 +210,25 @@ public void buildRemoteAction_withRegularFileAsOutput() throws Exception {

RemoteAction remoteAction = service.buildRemoteAction(spawn, context);

assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString());
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly(execPath.toString());
}

@Test
public void legacy_buildRemoteAction_withRegularFileAsOutput() throws Exception {
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
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();
}

Expand All @@ -210,6 +245,25 @@ public void buildRemoteAction_withTreeArtifactAsOutput() throws Exception {

RemoteAction remoteAction = service.buildRemoteAction(spawn, context);

assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
}

@Test
public void legacy_buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
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");
}
Expand All @@ -227,11 +281,29 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {

RemoteAction remoteAction = service.buildRemoteAction(spawn, context);

assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link");
}

@Test
public void legacy_buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
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 =
Expand All @@ -243,6 +315,23 @@ public void buildRemoteAction_withActionInputFileAsOutput() throws Exception {

RemoteAction remoteAction = service.buildRemoteAction(spawn, context);

assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/file");
}

@Test
public void legacy_buildRemoteAction_withActionInputFileAsOutput() throws Exception {
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
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();
}
Expand All @@ -259,7 +348,8 @@ public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exceptio
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);

assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
}

@Test
Expand Down
3 changes: 2 additions & 1 deletion ...ava/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -299,6 +299,8 @@ public int maxConcurrency() {
});
ServerCapabilities caps =
ServerCapabilities.newBuilder()
.setLowApiVersion(ApiVersion.current.toSemVer())
.setHighApiVersion(ApiVersion.current.toSemVer())
.setExecutionCapabilities(
ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
.build();
Expand Down Expand Up @@ -349,7 +351,6 @@ public int maxConcurrency() {
.setName("VARIABLE")
.setValue("value")
.build())
.addAllOutputFiles(ImmutableList.of("bar", "foo"))
.addAllOutputPaths(ImmutableList.of("bar", "foo"))
.build();
cmdDigest = DIGEST_UTIL.compute(command);
Expand Down
14 changes: 3 additions & 11 deletions src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -274,15 +274,7 @@ private ActionResult execute(
List<Path> outputs =
new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount());

for (String output : command.getOutputFilesList()) {
Path file = workingDirectory.getRelative(output);
if (file.exists()) {
throw new FileAlreadyExistsException("Output file already exists: " + file);
}
file.getParentDirectory().createDirectoryAndParents();
outputs.add(file);
}
for (String output : command.getOutputDirectoriesList()) {
for (String output : command.getOutputPathsList()) {
Path file = workingDirectory.getRelative(output);
if (file.exists()) {
if (!file.isDirectory()) {
Expand Down Expand Up @@ -419,8 +411,8 @@ private static long getUid() throws InterruptedException {
com.google.devtools.build.lib.shell.Command cmd =
new com.google.devtools.build.lib.shell.Command(
new String[] {"id", "-u"},
/*environmentVariables=*/ null,
/*workingDirectory=*/ null,
/* environmentVariables= */ null,
/* workingDirectory= */ null,
uidTimeout);
try {
ByteArrayOutputStream stdout = new ByteArrayOutputStream();
Expand Down

0 comments on commit 2d4ce65

Please sign in to comment.