Skip to content

Commit

Permalink
Automated rollback of commit 50c16b3.
Browse files Browse the repository at this point in the history 
*** Reason for rollback ***

Breaks remote server which only support REPI v2.0.

*** Original change description ***

Conditionally set output_paths based on Remote Executor capabilities

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: 537883626
Change-Id: I13c03cb3f4d64f106dc90767a6e62dfbae4027e2
  • Loading branch information
@coeuvre @copybara-github
coeuvre authored and copybara-github committed Jun 5, 2023
1 parent 36b2f8c commit 471f048
Show file tree
Hide file tree
Showing 5 changed files with 40 additions and 163 deletions.
12 changes: 4 additions & 8 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,7 +17,9 @@
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 @@ -26,12 +28,7 @@ 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).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 static final ApiVersion current = new ApiVersion(SemVer.newBuilder().setMajor(2).build());

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

private final String message;
private final State state;

Expand Down
48 changes: 16 additions & 32 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,37 +221,30 @@ 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();
if (useOutputPaths) {
var outputPaths = new ArrayList<String>();
for (ActionInput output : outputs) {
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
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);
}
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);
}
Collections.sort(outputFiles);
Collections.sort(outputDirectories);
command.addAllOutputFiles(outputFiles);
command.addAllOutputDirectories(outputDirectories);
outputPaths.add(pathString);
}
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 @@ -549,17 +542,8 @@ 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: 2 additions & 111 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,7 +39,6 @@
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 @@ -48,11 +47,9 @@
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 @@ -141,20 +138,6 @@ 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 @@ -194,7 +177,6 @@ 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 @@ -213,27 +195,9 @@ public void buildRemoteAction_withRegularFileAsOutput() throws Exception {

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

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().getOutputPathsList()).containsExactly(execPath.toString());
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
}

@Test
Expand All @@ -249,28 +213,8 @@ 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 @@ -286,28 +230,9 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() 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/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();
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link");
}

@Test
Expand All @@ -321,42 +246,8 @@ 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: 1 addition & 2 deletions ...ava/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -302,8 +302,6 @@ 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 @@ -355,6 +353,7 @@ 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: 17 additions & 10 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,21 +271,28 @@ private ActionResult execute(
Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory());
workingDirectory.createDirectoryAndParents();

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

for (String output : command.getOutputPathsList()) {
for (String output : command.getOutputFilesList()) {
Path file = workingDirectory.getRelative(output);
// 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()) {
if (file.exists()) {
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 @@ -412,8 +419,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 471f048

Please sign in to comment.