Skip to content

Commit 5dddb5c

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 211fd6c commit 5dddb5c

File tree

5 files changed

+167
-37
lines changed

5 files changed

+167
-37
lines changed

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

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -28,7 +28,12 @@ public class ApiVersion implements Comparable<ApiVersion> {
2828
public static final ApiVersion low =
2929
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build());
3030
public static final ApiVersion high =
31-
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(0).build());
31+
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());
32+
33+
// The version of the Remote Execution API that starts supporting the
34+
// Command.output_paths and ActionResult.output_symlinks fields.
35+
public static final ApiVersion twoPointOne =
36+
new ApiVersion(SemVer.newBuilder().setMajor(2).setMinor(1).build());
3237

3338
public ApiVersion(int major, int minor, int patch, String prerelease) {
3439
this.major = major;

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

Lines changed: 35 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -222,30 +222,37 @@ public RemoteExecutionService(
222222
}
223223

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

250257
if (platform != null) {
251258
command.setPlatform(platform);
@@ -543,8 +550,20 @@ public RemoteAction buildRemoteAction(Spawn spawn, SpawnExecutionContext context
543550
platform = PlatformUtils.getPlatformProto(spawn, remoteOptions);
544551
}
545552

553+
var useOutputPaths = true;
554+
if (mayBeExecutedRemotely(spawn)) {
555+
var capabilities = remoteExecutor.getServerCapabilities();
556+
if (capabilities != null) {
557+
var supportStatus = ClientApiVersion.current.checkServerSupportedVersions(capabilities);
558+
if (supportStatus.isSupported()) {
559+
useOutputPaths =
560+
supportStatus.getHighestSupportedVersion().compareTo(ApiVersion.twoPointOne) >= 0;
561+
}
562+
}
563+
}
546564
Command command =
547565
buildCommand(
566+
useOutputPaths,
548567
spawn.getOutputFiles(),
549568
spawn.getArguments(),
550569
spawn.getEnvironment(),

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

Lines changed: 114 additions & 2 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,23 @@ public class RemoteExecutionServiceTest {
138141
private final Reporter reporter = new Reporter(new EventBus());
139142
private final StoredEventHandler eventHandler = new StoredEventHandler();
140143

144+
// In the past, Bazel only supports RemoteApi version 2.0.
145+
// Use this to ensure we are backward compatible with Servers that only support 2.0.
146+
private final ApiVersion oldApiVersion = new ApiVersion(SemVer.newBuilder().setMajor(2).build());
147+
private final ServerCapabilities legacyRemoteExecutorCapabilities =
148+
ServerCapabilities.newBuilder()
149+
.setLowApiVersion(oldApiVersion.toSemVer())
150+
.setHighApiVersion(oldApiVersion.toSemVer())
151+
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
152+
.build();
153+
154+
private final ServerCapabilities remoteExecutorCapabilities =
155+
ServerCapabilities.newBuilder()
156+
.setLowApiVersion(ApiVersion.low.toSemVer())
157+
.setHighApiVersion(ApiVersion.high.toSemVer())
158+
.setExecutionCapabilities(ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
159+
.build();
160+
141161
RemoteOptions remoteOptions;
142162
private Path execRoot;
143163
private ArtifactRoot artifactRoot;
@@ -177,6 +197,7 @@ public final void setUp() throws Exception {
177197

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

181202
RequestMetadata metadata =
182203
TracingMetadataUtils.buildMetadata("none", "none", "action-id", null);
@@ -195,9 +216,27 @@ public void buildRemoteAction_withRegularFileAsOutput() throws Exception {
195216

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

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

203242
@Test
@@ -213,8 +252,28 @@ public void buildRemoteAction_withTreeArtifactAsOutput() throws Exception {
213252

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

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

220279
@Test
@@ -230,11 +289,30 @@ public void buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
230289

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

233-
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
292+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
234293
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
235294
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/link");
236295
}
237296

297+
@Test
298+
public void legacy_buildRemoteAction_withUnresolvedSymlinkAsOutput() throws Exception {
299+
when(executor.getServerCapabilities()).thenReturn(legacyRemoteExecutorCapabilities);
300+
Spawn spawn =
301+
new SpawnBuilder("dummy")
302+
.withOutput(
303+
ActionsTestUtil.createUnresolvedSymlinkArtifactWithExecPath(
304+
artifactRoot, PathFragment.create("path/to/link")))
305+
.build();
306+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
307+
RemoteExecutionService service = newRemoteExecutionService();
308+
309+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
310+
311+
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/link");
312+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
313+
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
314+
}
315+
238316
@Test
239317
public void buildRemoteAction_withActionInputAsOutput() throws Exception {
240318
Spawn spawn =
@@ -246,8 +324,42 @@ public void buildRemoteAction_withActionInputAsOutput() throws Exception {
246324

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

327+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
328+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
329+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/file");
330+
}
331+
332+
@Test
333+
public void legacy_buildRemoteAction_withActionInputFileAsOutput() throws Exception {
334+
when(executor.getServerCapabilities()).thenReturn(legacyRemoteExecutorCapabilities);
335+
Spawn spawn =
336+
new SpawnBuilder("dummy")
337+
.withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/file")))
338+
.build();
339+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
340+
RemoteExecutionService service = newRemoteExecutionService();
341+
342+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
343+
249344
assertThat(remoteAction.getCommand().getOutputFilesList()).containsExactly("path/to/file");
250345
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
346+
assertThat(remoteAction.getCommand().getOutputPathsList()).isEmpty();
347+
}
348+
349+
@Test
350+
public void buildRemoteAction_withActionInputDirectoryAsOutput() throws Exception {
351+
Spawn spawn =
352+
new SpawnBuilder("dummy")
353+
.withOutput(ActionInputHelper.fromPath(PathFragment.create("path/to/dir")))
354+
.build();
355+
FakeSpawnExecutionContext context = newSpawnExecutionContext(spawn);
356+
RemoteExecutionService service = newRemoteExecutionService();
357+
358+
RemoteAction remoteAction = service.buildRemoteAction(spawn, context);
359+
360+
assertThat(remoteAction.getCommand().getOutputFilesList()).isEmpty();
361+
assertThat(remoteAction.getCommand().getOutputDirectoriesList()).isEmpty();
362+
assertThat(remoteAction.getCommand().getOutputPathsList()).containsExactly("path/to/dir");
251363
}
252364

253365
@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
@@ -303,6 +303,8 @@ public int maxConcurrency() {
303303
});
304304
ServerCapabilities caps =
305305
ServerCapabilities.newBuilder()
306+
.setLowApiVersion(ApiVersion.low.toSemVer())
307+
.setHighApiVersion(ApiVersion.high.toSemVer())
306308
.setExecutionCapabilities(
307309
ExecutionCapabilities.newBuilder().setExecEnabled(true).build())
308310
.build();
@@ -354,7 +356,6 @@ public int maxConcurrency() {
354356
.setName("VARIABLE")
355357
.setValue("value")
356358
.build())
357-
.addAllOutputFiles(ImmutableList.of("bar", "foo"))
358359
.addAllOutputPaths(ImmutableList.of("bar", "foo"))
359360
.build();
360361
cmdDigest = DIGEST_UTIL.compute(command);

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

Lines changed: 10 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -271,28 +271,21 @@ private ActionResult execute(
271271
Path workingDirectory = execRoot.getRelative(command.getWorkingDirectory());
272272
workingDirectory.createDirectoryAndParents();
273273

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

277-
for (String output : command.getOutputFilesList()) {
276+
for (String output : command.getOutputPathsList()) {
278277
Path file = workingDirectory.getRelative(output);
279-
if (file.exists()) {
278+
// Since https://github.com/bazelbuild/bazel/pull/15818,
279+
// Bazel includes all expected output directories as part of Action's inputs.
280+
//
281+
// Ensure no output file exists before execution happen.
282+
// Ignore if output directories pre-exist.
283+
if (file.exists() && !file.isDirectory()) {
280284
throw new FileAlreadyExistsException("Output file already exists: " + file);
281285
}
282286
file.getParentDirectory().createDirectoryAndParents();
283287
outputs.add(file);
284288
}
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-
}
296289

297290
// TODO(ulfjack): This is basically a copy of LocalSpawnRunner. Ideally, we'd use that
298291
// implementation instead of copying it.
@@ -419,8 +412,8 @@ private static long getUid() throws InterruptedException {
419412
com.google.devtools.build.lib.shell.Command cmd =
420413
new com.google.devtools.build.lib.shell.Command(
421414
new String[] {"id", "-u"},
422-
/*environmentVariables=*/ null,
423-
/*workingDirectory=*/ null,
415+
/* environmentVariables= */ null,
416+
/* workingDirectory= */ null,
424417
uidTimeout);
425418
try {
426419
ByteArrayOutputStream stdout = new ByteArrayOutputStream();

0 commit comments

Comments
 (0)