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 #18269, toward the discussion in #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.

Closes #18270.

PiperOrigin-RevId: 537817532
Change-Id: Ie299065a7c91abbfc7a4f181410f6a57471e7dc8
  • Loading branch information
@sluongng @copybara-github
sluongng authored and copybara-github committed Jun 5, 2023
1 parent 85dcafe commit 50c16b3
Show file tree
Hide file tree
Showing 5 changed files with 163 additions and 40 deletions.
12 changes: 8 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,12 @@ 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());
// The version of the Remote Execution API that starts supporting the
// Command.output_paths and ActionResult.output_symlinks fields.
public static final ApiVersion twoPointOne =
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 +103,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 @@ -221,30 +221,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) {
var outputPaths = new ArrayList<String>();
for (ActionInput output : outputs) {
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
outputPaths.add(pathString);
}
outputPaths.add(pathString);
Collections.sort(outputPaths);
command.addAllOutputPaths(outputPaths);
} else {
var outputFiles = new ArrayList<String>();
var outputDirectories = new ArrayList<String>();
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 @@ -542,8 +549,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
}

var useOutputPaths = true;
if (mayBeExecutedRemotely(spawn)) {
var capabilities = remoteExecutor.getServerCapabilities();
if (capabilities != null) {
var highApiVersion = new ApiVersion(capabilities.getHighApiVersion());
useOutputPaths = highApiVersion.compareTo(ApiVersion.twoPointOne) >= 0;
}
}
Command command =
buildCommand(
useOutputPaths,
spawn.getOutputFiles(),
spawn.getArguments(),
spawn.getEnvironment(),
Expand Down
113 changes: 111 additions & 2 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 @@ -138,6 +141,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 @@ -177,6 +194,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 @@ -195,9 +213,27 @@ 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();
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
}

@Test
Expand All @@ -213,8 +249,28 @@ 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");
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
}

@Test
Expand All @@ -230,11 +286,30 @@ 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();
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
}

@Test
public void buildRemoteAction_withActionInputAsOutput() throws Exception {
Spawn spawn =
Expand All @@ -246,8 +321,42 @@ public void buildRemoteAction_withActionInputAsOutput() 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();
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
}

@Test
public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exception {
Spawn spawn =
new SpawnBuilder("dummy")
.withOutput(ActionInputHelper.fromPath(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()).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 @@ -302,6 +302,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 @@ -353,7 +355,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
27 changes: 10 additions & 17 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 @@ -271,28 +271,21 @@ private ActionResult execute(
Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory());
workingDirectory.createDirectoryAndParents();

List<Path> outputs =
new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount());
List<Path> outputs = new ArrayList<>(command.getOutputPathsCount());

for (String output : command.getOutputFilesList()) {
for (String output : command.getOutputPathsList()) {
Path file = workingDirectory.getRelative(output);
if (file.exists()) {
// Since https://github.com/bazelbuild/bazel/pull/15818,
// Bazel includes all expected output directories as part of Action's inputs.
//
// Ensure no output file exists before execution happen.
// Ignore if output directories pre-exist.
if (file.exists() && !file.isDirectory()) {
throw new FileAlreadyExistsException("Output file already exists: " + file);
}
file.getParentDirectory().createDirectoryAndParents();
outputs.add(file);
}
for (String output : command.getOutputDirectoriesList()) {
Path file = workingDirectory.getRelative(output);
if (file.exists()) {
if (!file.isDirectory()) {
throw new FileAlreadyExistsException(
"Non-directory exists at output directory path: " + file);
}
}
file.getParentDirectory().createDirectoryAndParents();
outputs.add(file);
}

// TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that
// implementation instead of copying it.
Expand Down Expand Up @@ -419,8 +412,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 50c16b3

Please sign in to comment.