Skip to content

Commit 2c78242

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 e3e666c commit 2c78242

File tree

5 files changed

+149
-40
lines changed

5 files changed

+149
-40
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
@@ -221,30 +221,37 @@ public RemoteExecutionService(
221221
}
222222

223223
static Command buildCommand(
224+
boolean useOutputPaths,
224225
Collection<? extends ActionInput> outputs,
225226
List<String> arguments,
226227
ImmutableMap<String, String> env,
227228
@Nullable Platform platform,
228229
RemotePathResolver remotePathResolver) {
229230
Command.Builder command = Command.newBuilder();
230-
ArrayList<String> outputFiles = new ArrayList<>();
231-
ArrayList<String> outputDirectories = new ArrayList<>();
232-
ArrayList<String> outputPaths = new ArrayList<>();
233-
for (ActionInput output : outputs) {
234-
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
235-
if (output.isDirectory()) {
236-
outputDirectories.add(pathString);
237-
} else {
238-
outputFiles.add(pathString);
231+
if (useOutputPaths) {
232+
var outputPaths = new ArrayList<String>();
233+
for (ActionInput output : outputs) {
234+
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
235+
outputPaths.add(pathString);
239236
}
240-
outputPaths.add(pathString);
237+
Collections.sort(outputPaths);
238+
command.addAllOutputPaths(outputPaths);
239+
} else {
240+
var outputFiles = new ArrayList<String>();
241+
var outputDirectories = new ArrayList<String>();
242+
for (ActionInput output : outputs) {
243+
String pathString = decodeBytestringUtf8(remotePathResolver.localPathToOutputPath(output));
244+
if (output.isDirectory()) {
245+
outputDirectories.add(pathString);
246+
} else {
247+
outputFiles.add(pathString);
248+
}
249+
}
250+
Collections.sort(outputFiles);
251+
Collections.sort(outputDirectories);
252+
command.addAllOutputFiles(outputFiles);
253+
command.addAllOutputDirectories(outputDirectories);
241254
}
242-
Collections.sort(outputFiles);
243-
Collections.sort(outputDirectories);
244-
Collections.sort(outputPaths);
245-
command.addAllOutputFiles(outputFiles);
246-
command.addAllOutputDirectories(outputDirectories);
247-
command.addAllOutputPaths(outputPaths);
248255

249256
if (platform != null) {
250257
command.setPlatform(platform);
@@ -542,8 +549,17 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
542549
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
543550
}
544551

552+
var useOutputPaths = true;
553+
if (mayBeExecutedRemotely(spawn)) {
554+
var capabilities = remoteExecutor.getServerCapabilities();
555+
if (capabilities != null) {
556+
var highApiVersion = new ApiVersion(capabilities.getHighApiVersion());
557+
useOutputPaths = highApiVersion.compareTo(ApiVersion.twoPointOne) >= 0;
558+
}
559+
}
545560
Command command =
546561
buildCommand(
562+
useOutputPaths,
547563
spawn.getOutputFiles(),
548564
spawn.getArguments(),
549565
spawn.getEnvironment(),

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

Lines changed: 97 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;
@@ -138,6 +141,20 @@ public class RemoteExecutionServiceTest {
138141
private final Reporter reporter = new Reporter(new EventBus());
139142
private final StoredEventHandler eventHandler = new StoredEventHandler();
140143

144+
private final ServerCapabilities removeExecutorCapabilities =
145+
ServerCapabilities.newBuilder()
146+
.setLowApiVersion(ApiVersion.current.toSemVer())
147+
.setHighApiVersion(ApiVersion.current.toSemVer())
148+
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
149+
.build();
150+
private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
151+
private final ServerCapabilities legacyRemoveExecutorCapabilities =
152+
ServerCapabilities.newBuilder()
153+
.setLowApiVersion(oldApiVersion.toSemVer())
154+
.setHighApiVersion(oldApiVersion.toSemVer())
155+
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
156+
.build();
157+
141158
RemoteOptions remoteOptions;
142159
private Path execRoot;
143160
private ArtifactRoot artifactRoot;
@@ -177,6 +194,7 @@ public final void setUp() throws Exception {
177194

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

181199
RequestMetadata metadata =
182200
TracingMetadataUtils.buildMetadata("none", "none", "action-id", null);
@@ -195,9 +213,27 @@ public void buildRemoteAction_withRegularFileAsOutput() throws Exception {
195213

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

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

203239
@Test
@@ -213,8 +249,28 @@ public void buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
213249

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

252+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
253+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
254+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
255+
}
256+
257+
@Test
258+
public void legacy_buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
259+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
260+
Spawn spawn =
261+
new SpawnBuilder("dummy")
262+
.withOutput(
263+
ActionsTestUtil.createTreeArtifactWithGeneratingAction(
264+
artifactRoot, PathFragment.create("path/to/dir")))
265+
.build();
266+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
267+
RemoteExecutionService service = newRemoteExecutionService();
268+
269+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
270+
216271
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
217272
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
273+
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
218274
}
219275

220276
@Test
@@ -230,11 +286,30 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
230286

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

233-
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
289+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
234290
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
235291
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link");
236292
}
237293

294+
@Test
295+
public void legacy_buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
296+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
297+
Spawn spawn =
298+
new SpawnBuilder("dummy")
299+
.withOutput(
300+
ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath(
301+
artifactRoot, PathFragment.create("path/to/link")))
302+
.build();
303+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
304+
RemoteExecutionService service = newRemoteExecutionService();
305+
306+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
307+
308+
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
309+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
310+
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
311+
}
312+
238313
@Test
239314
public void buildRemoteAction_withActionInputFileAsOutput() throws Exception {
240315
Spawn spawn =
@@ -246,8 +321,26 @@ public void buildRemoteAction_withActionInputFileAsOutput() throws Exception {
246321

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

324+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
325+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
326+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/file");
327+
}
328+
329+
@Test
330+
public void legacy_buildRemoteAction_withActionInputFileAsOutput() throws Exception {
331+
when(executor.getServerCapabilities()).thenReturn(legacyRemoveExecutorCapabilities);
332+
Spawn spawn =
333+
new SpawnBuilder("dummy")
334+
.withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/file")))
335+
.build();
336+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
337+
RemoteExecutionService service = newRemoteExecutionService();
338+
339+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
340+
249341
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/file");
250342
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
343+
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
251344
}
252345

253346
@Test
@@ -262,7 +355,8 @@ public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exceptio
262355
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
263356

264357
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
265-
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).containsExactly("path/to/dir");
358+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
359+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
266360
}
267361

268362
@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
@@ -302,6 +302,8 @@ public int maxConcurrency() {
302302
});
303303
ServerCapabilities caps =
304304
ServerCapabilities.newBuilder()
305+
.setLowApiVersion(ApiVersion.current.toSemVer())
306+
.setHighApiVersion(ApiVersion.current.toSemVer())
305307
.setExecutionCapabilities(
306308
ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
307309
.build();
@@ -353,7 +355,6 @@ public int maxConcurrency() {
353355
.setName("VARIABLE")
354356
.setValue("value")
355357
.build())
356-
.addAllOutputFiles(ImmutableList.of("bar", "foo"))
357358
.addAllOutputPaths(ImmutableList.of("bar", "foo"))
358359
.build();
359360
cmdDigest = DIGEST_UTIL.compute(command);

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

Lines changed: 10 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -272,27 +272,21 @@ private ActionResult execute(
272272
workingDirectory.createDirectoryAndParents();
273273

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

277-
for (String output : command.getOutputFilesList()) {
277+
for (String output : command.getOutputPathsList()) {
278278
Path file = workingDirectory.getRelative(output);
279-
if (file.exists()) {
279+
// Since https://github.com/bazelbuild/bazel/pull/15818,
280+
// Bazel includes all expected output directories as part of Action's inputs.
281+
//
282+
// Ensure no output file exists before execution happen.
283+
// Ignore if output directories pre-exist.
284+
if (file.exists() && !file.isDirectory()) {
280285
throw new FileAlreadyExistsException("Output file already exists: " + file);
281286
}
282287
file.getParentDirectory().createDirectoryAndParents();
283288
outputs.add(file);
284289
}
285-
for (String output : command.getOutputDirectoriesList()) {
286-
Path file = workingDirectory.getRelative(output);
287-
if (file.exists()) {
288-
if (!file.isDirectory()) {
289-
throw new FileAlreadyExistsException(
290-
"Non-directory exists at output directory path: " + file);
291-
}
292-
}
293-
file.getParentDirectory().createDirectoryAndParents();
294-
outputs.add(file);
295-
}
296290

297291
// TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that
298292
// implementation instead of copying it.
@@ -419,8 +413,8 @@ private static long getUid() throws InterruptedException {
419413
com.google.devtools.build.lib.shell.Command cmd =
420414
new com.google.devtools.build.lib.shell.Command(
421415
new String[] {"id", "-u"},
422-
/*environmentVariables=*/ null,
423-
/*workingDirectory=*/ null,
416+
/* environmentVariables= */ null,
417+
/* workingDirectory= */ null,
424418
uidTimeout);
425419
try {
426420
ByteArrayOutputStream stdout = new ByteArrayOutputStream();

0 commit comments

Comments
 (0)