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 Jul 22, 2024
1 parent 46c0b8f commit aa31648
Show file tree
Hide file tree
Showing 9 changed files with 276 additions and 60 deletions.
17 changes: 13 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 @@ -23,12 +23,21 @@ public class ApiVersion implements Comparable<ApiVersion> {
public final int patch;
public final String prerelease;

// The version of the Remote Execution API that Bazel supports initially.
public static final ApiVersion twoPointZero =
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).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());
// The latest version of the Remote Execution API that Bazel is compatible with.
public static final ApiVersion twoPointTwo =
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(2).build());

// The current lowest/highest versions (inclusive) of the Remote Execution API that Bazel
// supports. These fields will need to be updated together with all version changes.
public static final ApiVersion low =
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build());
public static final ApiVersion high =
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build());
public static final ApiVersion low = twoPointZero;
public static final ApiVersion high = twoPointTwo;

public ApiVersion(int major, int minor, int patch, String prerelease) {
this.major = major;
Expand Down
51 changes: 35 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 @@ -234,31 +234,38 @@ public RemoteExecutionService(
}

private Command buildCommand(
boolean useOutputPaths,
Collection<? extends ActionInput> outputs,
List<String> arguments,
ImmutableMap<String, String> env,
@Nullable Platform platform,
RemotePathResolver remotePathResolver,
@Nullable SpawnScrubber spawnScrubber) {
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);
}
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);
}
}
outputPaths.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 @@ -585,8 +592,20 @@ 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 supportStatus = ClientApiVersion.current.checkServerSupportedVersions(capabilities);
if (supportStatus.isSupported()) {
useOutputPaths =
supportStatus.getHighestSupportedVersion().compareTo(ApiVersion.twoPointOne) >= 0;
}
}
}
Command command =
buildCommand(
useOutputPaths,
spawn.getOutputFiles(),
spawn.getArguments(),
spawn.getEnvironment(),
Expand Down
122 changes: 116 additions & 6 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 @@ -38,6 +38,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 @@ -46,9 +47,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 @@ -153,6 +156,23 @@ public class RemoteExecutionServiceTest {
private final Reporter reporter = new Reporter(new EventBus());
private final StoredEventHandler eventHandler = new StoredEventHandler();

// In the past, Bazel only supports RemoteApi version 2.0.
// Use this to ensure we are backward compatible with Servers that only support 2.0.
private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
private final ServerCapabilities legacyRemoteExecutorCapabilities =
ServerCapabilities.newBuilder()
.setLowApiVersion(oldApiVersion.toSemVer())
.setHighApiVersion(oldApiVersion.toSemVer())
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
.build();

private final ServerCapabilities remoteExecutorCapabilities =
ServerCapabilities.newBuilder()
.setLowApiVersion(ApiVersion.low.toSemVer())
.setHighApiVersion(ApiVersion.high.toSemVer())
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
.build();

RemoteOptions remoteOptions;
private FileSystem fs;
private Path execRoot;
Expand Down Expand Up @@ -197,6 +217,7 @@ public final void setUp() throws Exception {

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

RequestMetadata metadata =
TracingMetadataUtils.buildMetadata("none", "none", "action-id", null);
Expand All @@ -215,9 +236,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(legacyRemoteExecutorCapabilities);
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 @@ -233,8 +272,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(legacyRemoteExecutorCapabilities);
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 @@ -250,11 +309,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(legacyRemoteExecutorCapabilities);
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 @@ -266,8 +344,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(legacyRemoteExecutorCapabilities);
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 Expand Up @@ -2373,10 +2485,8 @@ public void buildRemoteActionWithPathMapping(@TestParameter boolean remoteMerkle
.containsExactly(
PathFragment.create("outputs/bin/input1"), mappedInput,
PathFragment.create("outputs/bin/input2"), unmappedInput);
assertThat(remoteAction.getCommand().getOutputFilesList())
.containsExactly("outputs/bin/dir/output1", "outputs/bin/other_dir/output2");
assertThat(remoteAction.getCommand().getOutputDirectoriesList())
.containsExactly("outputs/bin/output_dir");
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
assertThat(remoteAction.getCommand().getOutputPathsList())
.containsExactly(
"outputs/bin/dir/output1", "outputs/bin/other_dir/output2", "outputs/bin/output_dir");
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 @@ -311,6 +311,8 @@ public Single<ChannelConnectionWithServerCapabilities> create() {
.build();
ServerCapabilities caps =
ServerCapabilities.newBuilder()
.setLowApiVersion(ApiVersion.low.toSemVer())
.setHighApiVersion(ApiVersion.high.toSemVer())
.setExecutionCapabilities(
ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
.build();
Expand Down Expand Up @@ -371,7 +373,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
40 changes: 40 additions & 0 deletions src/test/shell/bazel/remote/remote_execution_test.sh
View file
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,46 @@ function test_credential_helper_clear_cache() {
expect_credential_helper_calls 10
}

function test_remote_grpc_cache_with_legacy_api() {
# Test if Bazel works with Remote Cache using Remote Api version 2.0.
stop_worker
start_worker --legacy_api

mkdir -p a
cat > a/BUILD <<EOF
genrule(
name = 'foo',
outs = ["foo.txt"],
cmd = "touch \$@",
)
EOF

bazel build \
--remote_cache=grpc://localhost:${worker_port} \
//a:foo \
|| fail "Failed to build //a:foo with legacy api Remote Cache"
}

function test_remote_executor_with_legacy_api() {
# Test if Bazel works with Remote Executor using Remote Api version 2.0.
stop_worker
start_worker --legacy_api

mkdir -p a
cat > a/BUILD <<EOF
genrule(
name = 'foo',
outs = ["foo.txt"],
cmd = "touch \$@",
)
EOF

bazel build \
--remote_executor=grpc://localhost:${worker_port} \
//a:foo \
|| fail "Failed to build //a:foo with legacy api Remote Executor"
}

function test_remote_grpc_cache_with_protocol() {
# Test that if 'grpc' is provided as a scheme for --remote_cache flag, remote cache works.
mkdir -p a
Expand Down
Loading

0 comments on commit aa31648

Please sign in to comment.