Skip to content

Commit 6316491

Browse files
sluongngsluongng
committed
Conditionally set output_paths based on Remote Executor capabilities
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.
1 parent 5d6ac4c commit 6316491

File tree

5 files changed

+138
-35
lines changed

5 files changed

+138
-35
lines changed

‎src/main/java/com/google/devtools/build/lib/remote/ApiVersion.java

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -17,9 +17,7 @@
1717
import build.bazel.remote.execution.v2.ServerCapabilities;
1818
import build.bazel.semver.SemVer;
1919

20-
/**
21-
* Represents a version of the Remote Execution API.
22-
*/
20+
/** Represents a version of the Remote Execution API. */
2321
public class ApiVersion implements Comparable<ApiVersion> {
2422
public final int major;
2523
public final int minor;
@@ -28,7 +26,12 @@ public class ApiVersion implements Comparable<ApiVersion> {
2826

2927
// The current version of the Remote Execution API. This field will need to be updated
3028
// together with all version changes.
31-
public static final ApiVersion current = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
29+
public static final ApiVersion current =
30+
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());
31+
// The version of the Remote Execution API that starts supporting the
32+
// Command.output_paths and ActionResult.output_symlinks fields.
33+
public static final ApiVersion twoPointOne =
34+
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());
3235

3336
public ApiVersion(int major, int minor, int patch, String prerelease) {
3437
this.major = major;
@@ -100,6 +103,7 @@ private enum State {
100103
UNSUPPORTED,
101104
DEPRECATED,
102105
}
106+
103107
private final String message;
104108
private final State state;
105109

‎src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java

Lines changed: 32 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -217,30 +217,37 @@ public RemoteExecutionService(
217217
}
218218

219219
static Command buildCommand(
220+
boolean useOutputPaths,
220221
Collection<? extends ActionInput> outputs,
221222
List<String> arguments,
222223
ImmutableMap<String, String> env,
223224
@Nullable Platform platform,
224225
RemotePathResolver remotePathResolver) {
225226
Command.Builder command = Command.newBuilder();
226-
ArrayList<String> outputFiles = new ArrayList<>();
227-
ArrayList<String> outputDirectories = new ArrayList<>();
228-
ArrayList<String> outputPaths = new ArrayList<>();
229-
for (ActionInput output : outputs) {
230-
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
231-
if (output.isDirectory()) {
232-
outputDirectories.add(pathString);
233-
} else {
234-
outputFiles.add(pathString);
227+
if (useOutputPaths) {
228+
ArrayList<String> outputPaths = new ArrayList<>();
229+
for (ActionInput output : outputs) {
230+
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
231+
outputPaths.add(pathString);
235232
}
236-
outputPaths.add(pathString);
233+
Collections.sort(outputPaths);
234+
command.addAllOutputPaths(outputPaths);
235+
} else {
236+
ArrayList<String> outputFiles = new ArrayList<>();
237+
ArrayList<String> outputDirectories = new ArrayList<>();
238+
for (ActionInput output : outputs) {
239+
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
240+
if (output.isDirectory()) {
241+
outputDirectories.add(pathString);
242+
} else {
243+
outputFiles.add(pathString);
244+
}
245+
}
246+
Collections.sort(outputFiles);
247+
Collections.sort(outputDirectories);
248+
command.addAllOutputFiles(outputFiles);
249+
command.addAllOutputDirectories(outputDirectories);
237250
}
238-
Collections.sort(outputFiles);
239-
Collections.sort(outputDirectories);
240-
Collections.sort(outputPaths);
241-
command.addAllOutputFiles(outputFiles);
242-
command.addAllOutputDirectories(outputDirectories);
243-
command.addAllOutputPaths(outputPaths);
244251

245252
if (platform != null) {
246253
command.setPlatform(platform);
@@ -538,8 +545,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
538545
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
539546
}
540547

548+
var useOutputPaths = false;
549+
if (mayBeExecutedRemotely(spawn)) {
550+
var capabilities = remoteExecutor.getServerCapabilities();
551+
if (capabilities != null) {
552+
var highApiVersion = new ApiVersion(capabilities.getHighApiVersion());
553+
useOutputPaths = highApiVersion.compareTo(ApiVersion.twoPointOne) >= 0;
554+
}
555+
}
541556
Command command =
542557
buildCommand(
558+
useOutputPaths,
543559
spawn.getOutputFiles(),
544560
spawn.getArguments(),
545561
spawn.getEnvironment(),

‎src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java

Lines changed: 93 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -39,6 +39,7 @@
3939
import build.bazel.remote.execution.v2.Digest;
4040
import build.bazel.remote.execution.v2.Directory;
4141
import build.bazel.remote.execution.v2.DirectoryNode;
42+
import build.bazel.remote.execution.v2.ExecutionCapabilities;
4243
import build.bazel.remote.execution.v2.FileNode;
4344
import build.bazel.remote.execution.v2.NodeProperties;
4445
import build.bazel.remote.execution.v2.NodeProperty;
@@ -47,9 +48,11 @@
4748
import build.bazel.remote.execution.v2.OutputSymlink;
4849
import build.bazel.remote.execution.v2.Platform;
4950
import build.bazel.remote.execution.v2.RequestMetadata;
51+
import build.bazel.remote.execution.v2.ServerCapabilities;
5052
import build.bazel.remote.execution.v2.SymlinkAbsolutePathStrategy;
5153
import build.bazel.remote.execution.v2.SymlinkNode;
5254
import build.bazel.remote.execution.v2.Tree;
55+
import build.bazel.semver.SemVer;
5356
import com.google.common.base.Throwables;
5457
import com.google.common.collect.ImmutableClassToInstanceMap;
5558
import com.google.common.collect.ImmutableList;
@@ -135,6 +138,20 @@ public class RemoteExecutionServiceTest {
135138
private final Reporter reporter = new Reporter(new EventBus());
136139
private final StoredEventHandler eventHandler = new StoredEventHandler();
137140

141+
private final ServerCapabilities removeExecutorCapabilities =
142+
ServerCapabilities.newBuilder()
143+
.setLowApiVersion(ApiVersion.current.toSemVer())
144+
.setHighApiVersion(ApiVersion.current.toSemVer())
145+
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
146+
.build();
147+
private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
148+
private final ServerCapabilities legacyRemoveExecutorCapabilities =
149+
ServerCapabilities.newBuilder()
150+
.setLowApiVersion(oldApiVersion.toSemVer())
151+
.setHighApiVersion(oldApiVersion.toSemVer())
152+
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
153+
.build();
154+
138155
RemoteOptions remoteOptions;
139156
private Path execRoot;
140157
private ArtifactRoot artifactRoot;
@@ -174,6 +191,7 @@ public final void setUp() throws Exception {
174191

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

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

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

195-
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString());
213+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
214+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
196215
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly(execPath.toString());
216+
}
217+
218+
@Test
219+
public void legacy_buildRemoteAction_withRegularFileAsOutput() throws Exception {
220+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
221+
PathFragment execPath = execRoot.getRelative("path/to/tree").asFragment();
222+
Spawn spawn =
223+
new SpawnBuilder("dummy")
224+
.withOutput(ActionsTestUtil.createArtifactWithExecPath(artifactRoot, execPath))
225+
.build();
226+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
227+
RemoteExecutionService service = newRemoteExecutionService();
228+
229+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
230+
231+
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly(execPath.toString());
197232
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
198233
}
199234

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

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

248+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
249+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
250+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
251+
}
252+
253+
@Test
254+
public void legacy_buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
255+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
256+
Spawn spawn =
257+
new SpawnBuilder("dummy")
258+
.withOutput(
259+
ActionsTestUtil.createTreeArtifactWithGeneratingAction(
260+
artifactRoot, PathFragment.create("path/to/dir")))
261+
.build();
262+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
263+
RemoteExecutionService service = newRemoteExecutionService();
264+
265+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
266+
213267
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
214268
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
215269
}
@@ -227,11 +281,29 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
227281

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

230-
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
284+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
231285
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
232286
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link");
233287
}
234288

289+
@Test
290+
public void legacy_buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
291+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
292+
Spawn spawn =
293+
new SpawnBuilder("dummy")
294+
.withOutput(
295+
ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath(
296+
artifactRoot, PathFragment.create("path/to/link")))
297+
.build();
298+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
299+
RemoteExecutionService service = newRemoteExecutionService();
300+
301+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
302+
303+
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
304+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
305+
}
306+
235307
@Test
236308
public void buildRemoteAction_withActionInputFileAsOutput() throws Exception {
237309
Spawn spawn =
@@ -243,6 +315,23 @@ public void buildRemoteAction_withActionInputFileAsOutput() throws Exception {
243315

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

318+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
319+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
320+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/file");
321+
}
322+
323+
@Test
324+
public void legacy_buildRemoteAction_withActionInputFileAsOutput() throws Exception {
325+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
326+
Spawn spawn =
327+
new SpawnBuilder("dummy")
328+
.withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/file")))
329+
.build();
330+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
331+
RemoteExecutionService service = newRemoteExecutionService();
332+
333+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
334+
246335
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/file");
247336
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
248337
}
@@ -259,7 +348,8 @@ public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exceptio
259348
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
260349

261350
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
262-
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
351+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
352+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
263353
}
264354

265355
@Test

‎src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerWithGrpcRemoteExecutorTest.java

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -299,6 +299,8 @@ public int maxConcurrency() {
299299
});
300300
ServerCapabilities caps =
301301
ServerCapabilities.newBuilder()
302+
.setLowApiVersion(ApiVersion.current.toSemVer())
303+
.setHighApiVersion(ApiVersion.current.toSemVer())
302304
.setExecutionCapabilities(
303305
ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
304306
.build();
@@ -349,7 +351,6 @@ public int maxConcurrency() {
349351
.setName("VARIABLE")
350352
.setValue("value")
351353
.build())
352-
.addAllOutputFiles(ImmutableList.of("bar", "foo"))
353354
.addAllOutputPaths(ImmutableList.of("bar", "foo"))
354355
.build();
355356
cmdDigest = DIGEST_UTIL.compute(command);

‎src/tools/remote/src/main/java/com/google/devtools/build/remote/worker/ExecutionServer.java

Lines changed: 3 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -274,15 +274,7 @@ private ActionResult execute(
274274
List<Path> outputs =
275275
new ArrayList<>(command.getOutputDirectoriesCount() + command.getOutputFilesCount());
276276

277-
for (String output : command.getOutputFilesList()) {
278-
Path file = workingDirectory.getRelative(output);
279-
if (file.exists()) {
280-
throw new FileAlreadyExistsException("Output file already exists: " + file);
281-
}
282-
file.getParentDirectory().createDirectoryAndParents();
283-
outputs.add(file);
284-
}
285-
for (String output : command.getOutputDirectoriesList()) {
277+
for (String output : command.getOutputPathsList()) {
286278
Path file = workingDirectory.getRelative(output);
287279
if (file.exists()) {
288280
if (!file.isDirectory()) {
@@ -419,8 +411,8 @@ private static long getUid() throws InterruptedException {
419411
com.google.devtools.build.lib.shell.Command cmd =
420412
new com.google.devtools.build.lib.shell.Command(
421413
new String[] {"id", "-u"},
422-
/*environmentVariables=*/ null,
423-
/*workingDirectory=*/ null,
414+
/* environmentVariables= */ null,
415+
/* workingDirectory= */ null,
424416
uidTimeout);
425417
try {
426418
ByteArrayOutputStream stdout = new ByteArrayOutputStream();

0 commit comments

Comments
 (0)