[7.3.0] Deduplicate locally executed path mapped spawns (#23069)
When path mapping is enabled, different `Spawn`s in the same build can
have identical `RemoteAction.ActionKey`s and can thus provide remote
cache hits for each other. However, cache hits are only possible after
the first local execution has concluded and uploaded its result to the
cache.
To avoid unnecessary duplication of local work, the first `Spawn` for
each `RemoteAction.ActionKey` is tracked until its results have been
uploaded and all other concurrently scheduled `Spawn`s wait for it and
then copy over its local outputs.
Fixes #21043
Closes #22556.
PiperOrigin-RevId: 655097996
Change-Id: I4368f9210c67a306775164d252aae122d8b46f9b
Closes #23060
diff --git a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
index 60c14b8..95633d1 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/SpawnResult.java
@@ -488,6 +488,149 @@
}
}
+ /**
+ * A helper class for wrapping an existing {@link SpawnResult} and modifying a subset of its
+ * methods.
+ */
+ class DelegateSpawnResult implements SpawnResult {
+ private final SpawnResult delegate;
+
+ public DelegateSpawnResult(SpawnResult delegate) {
+ this.delegate = delegate;
+ }
+
+ @Override
+ public boolean setupSuccess() {
+ return delegate.setupSuccess();
+ }
+
+ @Override
+ public boolean isCatastrophe() {
+ return delegate.isCatastrophe();
+ }
+
+ @Override
+ public Status status() {
+ return delegate.status();
+ }
+
+ @Override
+ public int exitCode() {
+ return delegate.exitCode();
+ }
+
+ @Override
+ @Nullable
+ public FailureDetail failureDetail() {
+ return delegate.failureDetail();
+ }
+
+ @Override
+ @Nullable
+ public String getExecutorHostName() {
+ return delegate.getExecutorHostName();
+ }
+
+ @Override
+ public String getRunnerName() {
+ return delegate.getRunnerName();
+ }
+
+ @Override
+ public String getRunnerSubtype() {
+ return delegate.getRunnerSubtype();
+ }
+
+ @Override
+ @Nullable
+ public Instant getStartTime() {
+ return delegate.getStartTime();
+ }
+
+ @Override
+ public int getWallTimeInMs() {
+ return delegate.getWallTimeInMs();
+ }
+
+ @Override
+ public int getUserTimeInMs() {
+ return delegate.getUserTimeInMs();
+ }
+
+ @Override
+ public int getSystemTimeInMs() {
+ return delegate.getSystemTimeInMs();
+ }
+
+ @Override
+ @Nullable
+ public Long getNumBlockOutputOperations() {
+ return delegate.getNumBlockOutputOperations();
+ }
+
+ @Override
+ @Nullable
+ public Long getNumBlockInputOperations() {
+ return delegate.getNumBlockInputOperations();
+ }
+
+ @Override
+ @Nullable
+ public Long getNumInvoluntaryContextSwitches() {
+ return delegate.getNumInvoluntaryContextSwitches();
+ }
+
+ @Override
+ @Nullable
+ public Long getMemoryInKb() {
+ return delegate.getMemoryInKb();
+ }
+
+ @Override
+ public SpawnMetrics getMetrics() {
+ return delegate.getMetrics();
+ }
+
+ @Override
+ public boolean isCacheHit() {
+ return delegate.isCacheHit();
+ }
+
+ @Override
+ public String getFailureMessage() {
+ return delegate.getFailureMessage();
+ }
+
+ @Override
+ @Nullable
+ public InputStream getInMemoryOutput(ActionInput output) {
+ return delegate.getInMemoryOutput(output);
+ }
+
+ @Override
+ public String getDetailMessage(
+ String message, boolean catastrophe, boolean forciblyRunRemotely) {
+ return delegate.getDetailMessage(message, catastrophe, forciblyRunRemotely);
+ }
+
+ @Override
+ @Nullable
+ public MetadataLog getActionMetadataLog() {
+ return delegate.getActionMetadataLog();
+ }
+
+ @Override
+ public boolean wasRemote() {
+ return delegate.wasRemote();
+ }
+
+ @Override
+ @Nullable
+ public Digest getDigest() {
+ return delegate.getDigest();
+ }
+ }
+
/** Builder class for {@link SpawnResult}. */
final class Builder {
private int exitCode;
diff --git a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
index 05bf139..8e1a1aa 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/AbstractSpawnStrategy.java
@@ -221,7 +221,7 @@
? resultMessage
: CommandFailureUtils.describeCommandFailure(
executionOptions.verboseFailures, cwd, spawn);
- throw new SpawnExecException(message, spawnResult, /*forciblyRunRemotely=*/ false);
+ throw new SpawnExecException(message, spawnResult, /* forciblyRunRemotely= */ false);
}
return ImmutableList.of(spawnResult);
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/BUILD b/src/main/java/com/google/devtools/build/lib/exec/BUILD
index 37870bd..ef276bd 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/BUILD
+++ b/src/main/java/com/google/devtools/build/lib/exec/BUILD
@@ -225,6 +225,7 @@
deps = [
":spawn_runner",
"//src/main/java/com/google/devtools/build/lib/actions",
+ "//src/main/java/com/google/devtools/build/lib/profiler",
],
)
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
index 9b46802..694e079 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnCache.java
@@ -18,6 +18,7 @@
import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
+import com.google.devtools.build.lib.actions.Spawns;
import com.google.devtools.build.lib.exec.SpawnRunner.SpawnExecutionContext;
import java.io.Closeable;
import java.io.IOException;
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
index 31d7897..893132e 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteExecutionService.java
@@ -17,6 +17,7 @@
import static com.google.common.base.Preconditions.checkNotNull;
import static com.google.common.base.Preconditions.checkState;
import static com.google.common.base.Strings.isNullOrEmpty;
+import static com.google.common.collect.ImmutableMap.toImmutableMap;
import static com.google.common.util.concurrent.Futures.immediateFailedFuture;
import static com.google.common.util.concurrent.Futures.immediateFuture;
import static com.google.common.util.concurrent.Futures.transform;
@@ -62,6 +63,7 @@
import com.google.common.eventbus.Subscribe;
import com.google.common.util.concurrent.Futures;
import com.google.common.util.concurrent.ListenableFuture;
+import com.google.common.util.concurrent.SettableFuture;
import com.google.devtools.build.lib.actions.ActionInput;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.ArtifactPathResolver;
@@ -128,6 +130,7 @@
import io.reactivex.rxjava3.core.SingleObserver;
import io.reactivex.rxjava3.disposables.Disposable;
import io.reactivex.rxjava3.schedulers.Schedulers;
+import java.io.FileNotFoundException;
import java.io.IOException;
import java.time.Instant;
import java.util.ArrayList;
@@ -143,9 +146,11 @@
import java.util.SortedMap;
import java.util.TreeMap;
import java.util.TreeSet;
+import java.util.concurrent.CancellationException;
import java.util.concurrent.CompletableFuture;
import java.util.concurrent.CompletionException;
import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.ExecutionException;
import java.util.concurrent.Executor;
import java.util.concurrent.Phaser;
import java.util.concurrent.Semaphore;
@@ -863,15 +868,12 @@
}
}
- /**
- * Copies moves the downloaded outputs from their download location to their declared location.
- */
+ /** Moves the locally created outputs from their temporary location to their declared location. */
private void moveOutputsToFinalLocation(
- List<FileMetadata> finishedDownloads, Map<Path, Path> realToTmpPath) throws IOException {
+ Iterable<Path> localOutputs, Map<Path, Path> realToTmpPath) throws IOException {
// Move the output files from their temporary name to the actual output file name. Executable
// bit is ignored since the file permission will be changed to 0555 after execution.
- for (FileMetadata outputFile : finishedDownloads) {
- Path realPath = outputFile.path();
+ for (Path realPath : localOutputs) {
Path tmpPath = Preconditions.checkNotNull(realToTmpPath.get(realPath));
realPath.getParentDirectory().createDirectoryAndParents();
FileSystemUtils.moveFile(tmpPath, realPath);
@@ -1270,7 +1272,8 @@
// TODO(chiwang): Stage directories directly
((BazelOutputService) outputService).stageArtifacts(finishedDownloads);
} else {
- moveOutputsToFinalLocation(finishedDownloads, realToTmpPath);
+ moveOutputsToFinalLocation(
+ Iterables.transform(finishedDownloads, FileMetadata::path), realToTmpPath);
}
List<SymlinkMetadata> symlinksInDirectories = new ArrayList<>();
@@ -1327,6 +1330,152 @@
return null;
}
+ /** An ongoing local execution of a spawn. */
+ public static final class LocalExecution {
+ private final RemoteAction action;
+ private final SettableFuture<SpawnResult> spawnResultFuture;
+
+ private LocalExecution(RemoteAction action) {
+ this.action = action;
+ this.spawnResultFuture = SettableFuture.create();
+ }
+
+ /**
+ * Creates a new {@link LocalExecution} instance tracking the potential local execution of the
+ * given {@link RemoteAction} if there is a chance that the same action will be executed by a
+ * different Spawn.
+ *
+ * <p>This is only done for local (as in, non-remote) execution as remote executors are expected
+ * to already have deduplication mechanisms for actions in place, perhaps even across different
+ * builds and clients.
+ */
+ @Nullable
+ public static LocalExecution createIfDeduplicatable(RemoteAction action) {
+ if (action.getSpawn().getPathMapper().isNoop()) {
+ return null;
+ }
+ return new LocalExecution(action);
+ }
+
+ /**
+ * Signals to all potential consumers of the {@link #spawnResultFuture} that this execution has
+ * been cancelled and that the result will not be available.
+ */
+ public void cancel() {
+ spawnResultFuture.cancel(true);
+ }
+ }
+
+ /**
+ * Makes the {@link SpawnResult} available to all parallel {@link Spawn}s for the same {@link
+ * RemoteAction} waiting for it or notifies them that the spawn failed.
+ *
+ * @return Whether the spawn result should be uploaded to the cache.
+ */
+ public boolean commitResultAndDecideWhetherToUpload(
+ SpawnResult result, @Nullable LocalExecution execution) {
+ if (result.status().equals(SpawnResult.Status.SUCCESS) && result.exitCode() == 0) {
+ if (execution != null) {
+ execution.spawnResultFuture.set(result);
+ }
+ return true;
+ } else {
+ if (execution != null) {
+ execution.spawnResultFuture.cancel(true);
+ }
+ return false;
+ }
+ }
+
+ /**
+ * Reuses the outputs of a concurrent local execution of the same RemoteAction in a different
+ * spawn.
+ *
+ * <p>Since each output file is generated by a unique action and actions generally take care to
+ * run a unique spawn for each output file, this method is only useful with path mapping enabled,
+ * which allows different spawns in a single build to have the same RemoteAction.ActionKey.
+ *
+ * @return The {@link SpawnResult} of the previous execution if it was successful, otherwise null.
+ */
+ @Nullable
+ public SpawnResult waitForAndReuseOutputs(RemoteAction action, LocalExecution previousExecution)
+ throws InterruptedException, IOException {
+ checkState(!shutdown.get(), "shutdown");
+
+ SpawnResult previousSpawnResult;
+ try {
+ previousSpawnResult = previousExecution.spawnResultFuture.get();
+ } catch (CancellationException | ExecutionException e) {
+ if (e.getCause() != null) {
+ Throwables.throwIfInstanceOf(e.getCause(), InterruptedException.class);
+ Throwables.throwIfUnchecked(e.getCause());
+ }
+ // The spawn this action was deduplicated against failed due to an exception or
+ // non-zero exit code. Since it isn't possible to transparently replay its failure for the
+ // current spawn, we rerun the action instead.
+ return null;
+ }
+
+ Preconditions.checkArgument(
+ action.getActionKey().equals(previousExecution.action.getActionKey()));
+
+ ImmutableMap<Path, ActionInput> previousOutputs =
+ previousExecution.action.getSpawn().getOutputFiles().stream()
+ .collect(toImmutableMap(output -> execRoot.getRelative(output.getExecPath()), o -> o));
+ Map<Path, Path> realToTmpPath = new HashMap<>();
+ for (String output : action.getCommand().getOutputPathsList()) {
+ Path sourcePath =
+ previousExecution
+ .action
+ .getRemotePathResolver()
+ .outputPathToLocalPath(encodeBytestringUtf8(output));
+ ActionInput outputArtifact = previousOutputs.get(sourcePath);
+ Path tmpPath = tempPathGenerator.generateTempPath();
+ tmpPath.getParentDirectory().createDirectoryAndParents();
+ try {
+ if (outputArtifact.isDirectory()) {
+ tmpPath.createDirectory();
+ FileSystemUtils.copyTreesBelow(sourcePath, tmpPath, Symlinks.NOFOLLOW);
+ } else if (outputArtifact.isSymlink()) {
+ FileSystemUtils.ensureSymbolicLink(tmpPath, sourcePath.readSymbolicLink());
+ } else {
+ FileSystemUtils.copyFile(sourcePath, tmpPath);
+ }
+ } catch (FileNotFoundException e) {
+ // The spawn this action was deduplicated against failed to create an output file. If the
+ // output is mandatory, we cannot reuse the previous execution.
+ if (action.getSpawn().isMandatoryOutput(outputArtifact)) {
+ return null;
+ }
+ }
+
+ Path targetPath =
+ action.getRemotePathResolver().outputPathToLocalPath(encodeBytestringUtf8(output));
+ realToTmpPath.put(targetPath, tmpPath);
+ }
+
+ // TODO: FileOutErr is action-scoped, not spawn-scoped, but this is not a problem for the
+ // current use case of supporting deduplication of path mapped spawns:
+ // 1. Starlark and C++ compilation actions always create a single spawn.
+ // 2. Java compilation actions may run a fallback spawn, but reset the FileOutErr before
+ // running it.
+ // If this changes, we will need to introduce a spawn-scoped OutErr.
+ FileOutErr.dump(
+ previousExecution.action.getSpawnExecutionContext().getFileOutErr(),
+ action.getSpawnExecutionContext().getFileOutErr());
+
+ action
+ .getSpawnExecutionContext()
+ .lockOutputFiles(
+ previousSpawnResult.exitCode(),
+ previousSpawnResult.getFailureMessage(),
+ action.getSpawnExecutionContext().getFileOutErr());
+ // All outputs are created locally.
+ moveOutputsToFinalLocation(realToTmpPath.keySet(), realToTmpPath);
+
+ return previousSpawnResult;
+ }
+
private boolean shouldDownload(RemoteActionResult result, PathFragment execPath) {
if (outputService instanceof BazelOutputService) {
return false;
@@ -1401,7 +1550,7 @@
}
/** Upload outputs of a remote action which was executed locally to remote cache. */
- public void uploadOutputs(RemoteAction action, SpawnResult spawnResult)
+ public void uploadOutputs(RemoteAction action, SpawnResult spawnResult, Runnable onUploadComplete)
throws InterruptedException, ExecException {
checkState(!shutdown.get(), "shutdown");
checkState(
@@ -1437,6 +1586,7 @@
Profiler.instance()
.completeTask(startTime, ProfilerTask.UPLOAD_TIME, "upload outputs");
backgroundTaskPhaser.arriveAndDeregister();
+ onUploadComplete.run();
}
@Override
@@ -1445,6 +1595,7 @@
.completeTask(startTime, ProfilerTask.UPLOAD_TIME, "upload outputs");
backgroundTaskPhaser.arriveAndDeregister();
reportUploadError(e);
+ onUploadComplete.run();
}
});
} else {
@@ -1454,6 +1605,8 @@
manifest.upload(action.getRemoteActionExecutionContext(), remoteCache, reporter);
} catch (IOException e) {
reportUploadError(e);
+ } finally {
+ onUploadComplete.run();
}
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
index ad0c51c..3224f56 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnCache.java
@@ -26,7 +26,6 @@
import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnMetrics;
import com.google.devtools.build.lib.actions.SpawnResult;
-import com.google.devtools.build.lib.actions.SpawnResult.Status;
import com.google.devtools.build.lib.actions.cache.VirtualActionInput;
import com.google.devtools.build.lib.concurrent.ThreadSafety.ThreadSafe;
import com.google.devtools.build.lib.events.Event;
@@ -35,9 +34,11 @@
import com.google.devtools.build.lib.profiler.Profiler;
import com.google.devtools.build.lib.profiler.ProfilerTask;
import com.google.devtools.build.lib.profiler.SilentCloseable;
+import com.google.devtools.build.lib.remote.RemoteExecutionService.LocalExecution;
import com.google.devtools.build.lib.remote.RemoteExecutionService.RemoteActionResult;
import com.google.devtools.build.lib.remote.common.BulkTransferException;
import com.google.devtools.build.lib.remote.common.CacheNotFoundException;
+import com.google.devtools.build.lib.remote.common.RemoteCacheClient;
import com.google.devtools.build.lib.remote.options.RemoteOptions;
import com.google.devtools.build.lib.remote.util.DigestUtil;
import com.google.devtools.build.lib.remote.util.Utils;
@@ -45,6 +46,7 @@
import com.google.devtools.build.lib.vfs.Path;
import java.io.IOException;
import java.util.NoSuchElementException;
+import java.util.concurrent.ConcurrentHashMap;
/** A remote {@link SpawnCache} implementation. */
@ThreadSafe // If the RemoteActionCache implementation is thread-safe.
@@ -55,6 +57,8 @@
private final RemoteExecutionService remoteExecutionService;
private final DigestUtil digestUtil;
private final boolean verboseFailures;
+ private final ConcurrentHashMap<RemoteCacheClient.ActionKey, LocalExecution> inFlightExecutions =
+ new ConcurrentHashMap<>();
RemoteSpawnCache(
Path execRoot,
@@ -96,7 +100,19 @@
context.setDigest(digestUtil.asSpawnLogProto(action.getActionKey()));
Profiler prof = Profiler.instance();
+ LocalExecution thisExecution = null;
if (shouldAcceptCachedResult) {
+ // With path mapping enabled, different Spawns in a single build can have the same ActionKey.
+ // When their result isn't in the cache and two of them are scheduled concurrently, neither
+ // will result in a cache hit before the other finishes and uploads its result, which results
+ // in unnecessary work. To avoid this, we keep track of in-flight executions as long as their
+ // results haven't been uploaded to the cache yet and deduplicate all of them against the
+ // first one.
+ LocalExecution previousExecution = null;
+ thisExecution = LocalExecution.createIfDeduplicatable(action);
+ if (shouldUploadLocalResults && thisExecution != null) {
+ previousExecution = inFlightExecutions.putIfAbsent(action.getActionKey(), thisExecution);
+ }
// Metadata will be available in context.current() until we detach.
// This is done via a thread-local variable.
try {
@@ -146,9 +162,41 @@
remoteExecutionService.report(Event.warn(errorMessage));
}
}
+ if (previousExecution != null) {
+ Stopwatch fetchTime = Stopwatch.createStarted();
+ SpawnResult previousResult;
+ try (SilentCloseable c = prof.profile(REMOTE_DOWNLOAD, "reuse outputs")) {
+ previousResult = remoteExecutionService.waitForAndReuseOutputs(action, previousExecution);
+ }
+ if (previousResult != null) {
+ spawnMetrics
+ .setFetchTimeInMs((int) fetchTime.elapsed().toMillis())
+ .setTotalTimeInMs((int) totalTime.elapsed().toMillis())
+ .setNetworkTimeInMs((int) action.getNetworkTime().getDuration().toMillis());
+ SpawnMetrics buildMetrics = spawnMetrics.build();
+ return SpawnCache.success(
+ new SpawnResult.DelegateSpawnResult(previousResult) {
+ @Override
+ public String getRunnerName() {
+ return "deduplicated";
+ }
+
+ @Override
+ public SpawnMetrics getMetrics() {
+ return buildMetrics;
+ }
+ });
+ }
+ // If we reach here, the previous execution was not successful (it encountered an exception
+ // or the spawn had an exit code != 0). Since it isn't possible to accurately recreate the
+ // failure without rerunning the action, we fall back to running the action locally. This
+ // means that we have introduced an unnecessary wait, but that can only happen in the case
+ // of a failing build with --keep_going.
+ }
}
if (shouldUploadLocalResults) {
+ final LocalExecution thisExecutionFinal = thisExecution;
return new CacheHandle() {
@Override
public boolean hasResult() {
@@ -167,8 +215,8 @@
@Override
public void store(SpawnResult result) throws ExecException, InterruptedException {
- boolean uploadResults = Status.SUCCESS.equals(result.status()) && result.exitCode() == 0;
- if (!uploadResults) {
+ if (!remoteExecutionService.commitResultAndDecideWhetherToUpload(
+ result, thisExecutionFinal)) {
return;
}
@@ -185,12 +233,14 @@
}
}
- remoteExecutionService.uploadOutputs(action, result);
+ // As soon as the result is in the cache, actions can get the result from it instead of
+ // from the first in-flight execution. Not keeping in-flight executions around
+ // indefinitely is important to avoid excessive memory pressure - Spawns can be very
+ // large.
+ remoteExecutionService.uploadOutputs(
+ action, result, () -> inFlightExecutions.remove(action.getActionKey()));
}
- @Override
- public void close() {}
-
private void checkForConcurrentModifications()
throws IOException, ForbiddenActionInputException {
for (ActionInput input : action.getInputMap(true).values()) {
@@ -204,6 +254,13 @@
}
}
}
+
+ @Override
+ public void close() {
+ if (thisExecutionFinal != null) {
+ thisExecutionFinal.cancel();
+ }
+ }
};
} else {
return SpawnCache.NO_RESULT_NO_STORE;
diff --git a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
index 934cdff..f5fa69f 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/RemoteSpawnRunner.java
@@ -695,7 +695,7 @@
}
}
- remoteExecutionService.uploadOutputs(action, result);
+ remoteExecutionService.uploadOutputs(action, result, () -> {});
return result;
}
diff --git a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java
index 3a2c708..5819cc3a 100644
--- a/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/remote/common/RemotePathResolver.java
@@ -17,7 +17,6 @@
import com.google.common.base.Preconditions;
import com.google.devtools.build.lib.actions.ActionInput;
-import com.google.devtools.build.lib.actions.ActionInputHelper;
import com.google.devtools.build.lib.actions.ForbiddenActionInputException;
import com.google.devtools.build.lib.actions.PathMapper;
import com.google.devtools.build.lib.actions.Spawn;
@@ -76,12 +75,6 @@
*/
Path outputPathToLocalPath(String outputPath);
- /** Resolves the local {@link Path} for the {@link ActionInput}. */
- default Path outputPathToLocalPath(ActionInput actionInput) {
- String outputPath = localPathToOutputPath(actionInput.getExecPath());
- return outputPathToLocalPath(outputPath);
- }
-
/** Creates the default {@link RemotePathResolver}. */
static RemotePathResolver createDefault(Path execRoot) {
return new DefaultRemotePathResolver(execRoot);
@@ -139,11 +132,6 @@
public Path outputPathToLocalPath(String outputPath) {
return execRoot.getRelative(outputPath);
}
-
- @Override
- public Path outputPathToLocalPath(ActionInput actionInput) {
- return ActionInputHelper.toInputPath(actionInput, execRoot);
- }
}
/**
@@ -224,10 +212,6 @@
return getBase().getRelative(outputPath);
}
- @Override
- public Path outputPathToLocalPath(ActionInput actionInput) {
- return ActionInputHelper.toInputPath(actionInput, execRoot);
- }
}
/**
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
index 653e207..4a46e6d 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteExecutionServiceTest.java
@@ -1646,7 +1646,7 @@
// act
UploadManifest manifest = service.buildUploadManifest(action, spawnResult);
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
// assert
ActionResult.Builder expectedResult = ActionResult.newBuilder();
@@ -1693,7 +1693,7 @@
// act
UploadManifest manifest = service.buildUploadManifest(action, spawnResult);
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
// assert
ActionResult.Builder expectedResult = ActionResult.newBuilder();
@@ -1767,7 +1767,7 @@
// act
UploadManifest manifest = service.buildUploadManifest(action, spawnResult);
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
// assert
ActionResult.Builder expectedResult = ActionResult.newBuilder();
@@ -1803,7 +1803,7 @@
// act
UploadManifest manifest = service.buildUploadManifest(action, spawnResult);
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
// assert
ActionResult.Builder expectedResult = ActionResult.newBuilder();
@@ -1855,7 +1855,7 @@
.build();
// act
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
// assert
assertThat(
@@ -1881,7 +1881,7 @@
.when(cache)
.uploadActionResult(any(), any(), any());
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
assertThat(eventHandler.getEvents()).hasSize(1);
Event evt = eventHandler.getEvents().get(0);
@@ -1906,7 +1906,7 @@
.setRunnerName("test")
.build();
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
assertThat(eventHandler.getPosts())
.containsAtLeast(
@@ -1933,7 +1933,7 @@
.setRunnerName("test")
.build();
- service.uploadOutputs(action, spawnResult);
+ service.uploadOutputs(action, spawnResult, () -> {});
// assert
assertThat(cache.getNumFindMissingDigests()).isEmpty();
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
index 5f89e19..057ea41 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnCacheTest.java
@@ -18,6 +18,7 @@
import static com.google.common.truth.extensions.proto.ProtoTruth.assertThat;
import static com.google.common.util.concurrent.Futures.immediateVoidFuture;
import static com.google.common.util.concurrent.MoreExecutors.directExecutor;
+import static java.nio.charset.StandardCharsets.UTF_8;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.anyBoolean;
import static org.mockito.ArgumentMatchers.eq;
@@ -51,6 +52,7 @@
import com.google.devtools.build.lib.actions.InputMetadataProvider;
import com.google.devtools.build.lib.actions.ResourceSet;
import com.google.devtools.build.lib.actions.SimpleSpawn;
+import com.google.devtools.build.lib.actions.Spawn;
import com.google.devtools.build.lib.actions.SpawnResult;
import com.google.devtools.build.lib.actions.SpawnResult.Status;
import com.google.devtools.build.lib.clock.JavaClock;
@@ -81,6 +83,7 @@
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.DigestHashFunction;
import com.google.devtools.build.lib.vfs.FileSystem;
+import com.google.devtools.build.lib.vfs.FileSystemUtils;
import com.google.devtools.build.lib.vfs.OutputService;
import com.google.devtools.build.lib.vfs.Path;
import com.google.devtools.build.lib.vfs.PathFragment;
@@ -97,6 +100,7 @@
import org.junit.runners.JUnit4;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
+import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import org.mockito.invocation.InvocationOnMock;
import org.mockito.stubbing.Answer;
@@ -118,7 +122,7 @@
private Path execRoot;
private TempPathGenerator tempPathGenerator;
private SimpleSpawn simpleSpawn;
- private FakeActionInputFileCache fakeFileCache;
+ private SpawnExecutionContext simplePolicy;
@Mock private RemoteCache remoteCache;
private FileOutErr outErr;
@@ -127,101 +131,102 @@
private Reporter reporter;
private RemotePathResolver remotePathResolver;
- private final SpawnExecutionContext simplePolicy =
- new SpawnExecutionContext() {
- @Nullable private com.google.devtools.build.lib.exec.Protos.Digest digest;
+ private static SpawnExecutionContext createSpawnExecutionContext(
+ Spawn spawn, Path execRoot, FakeActionInputFileCache fakeFileCache, FileOutErr outErr) {
+ return new SpawnExecutionContext() {
+ @Nullable private com.google.devtools.build.lib.exec.Protos.Digest digest;
- @Override
- public int getId() {
- return 0;
- }
+ @Override
+ public int getId() {
+ return 0;
+ }
- @Override
- public void setDigest(com.google.devtools.build.lib.exec.Protos.Digest digest) {
- checkState(this.digest == null);
- this.digest = digest;
- }
+ @Override
+ public void setDigest(com.google.devtools.build.lib.exec.Protos.Digest digest) {
+ checkState(this.digest == null);
+ this.digest = digest;
+ }
- @Override
- @Nullable
- public com.google.devtools.build.lib.exec.Protos.Digest getDigest() {
- return digest;
- }
+ @Override
+ @Nullable
+ public com.google.devtools.build.lib.exec.Protos.Digest getDigest() {
+ return digest;
+ }
- @Override
- public ListenableFuture<Void> prefetchInputs() {
- return immediateVoidFuture();
- }
+ @Override
+ public ListenableFuture<Void> prefetchInputs() {
+ return immediateVoidFuture();
+ }
- @Override
- public void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) {}
+ @Override
+ public void lockOutputFiles(int exitCode, String errorMessage, FileOutErr outErr) {}
- @Override
- public boolean speculating() {
- return false;
- }
+ @Override
+ public boolean speculating() {
+ return false;
+ }
- @Override
- public InputMetadataProvider getInputMetadataProvider() {
- return fakeFileCache;
- }
+ @Override
+ public InputMetadataProvider getInputMetadataProvider() {
+ return fakeFileCache;
+ }
- @Override
- public ArtifactPathResolver getPathResolver() {
- return ArtifactPathResolver.forExecRoot(execRoot);
- }
+ @Override
+ public ArtifactPathResolver getPathResolver() {
+ return ArtifactPathResolver.forExecRoot(execRoot);
+ }
- @Override
- public ArtifactExpander getArtifactExpander() {
- throw new UnsupportedOperationException();
- }
+ @Override
+ public ArtifactExpander getArtifactExpander() {
+ throw new UnsupportedOperationException();
+ }
- @Override
- public SpawnInputExpander getSpawnInputExpander() {
- return new SpawnInputExpander(execRoot, /*strict*/ false);
- }
+ @Override
+ public SpawnInputExpander getSpawnInputExpander() {
+ return new SpawnInputExpander(execRoot, /*strict*/ false);
+ }
- @Override
- public Duration getTimeout() {
- return Duration.ZERO;
- }
+ @Override
+ public Duration getTimeout() {
+ return Duration.ZERO;
+ }
- @Override
- public FileOutErr getFileOutErr() {
- return outErr;
- }
+ @Override
+ public FileOutErr getFileOutErr() {
+ return outErr;
+ }
- @Override
- public SortedMap<PathFragment, ActionInput> getInputMapping(
- PathFragment baseDirectory, boolean willAccessRepeatedly)
- throws IOException, ForbiddenActionInputException {
- return getSpawnInputExpander()
- .getInputMapping(simpleSpawn, SIMPLE_ARTIFACT_EXPANDER, baseDirectory, fakeFileCache);
- }
+ @Override
+ public SortedMap<PathFragment, ActionInput> getInputMapping(
+ PathFragment baseDirectory, boolean willAccessRepeatedly)
+ throws IOException, ForbiddenActionInputException {
+ return getSpawnInputExpander()
+ .getInputMapping(spawn, SIMPLE_ARTIFACT_EXPANDER, baseDirectory, fakeFileCache);
+ }
- @Override
- public void report(ProgressStatus progress) {
- }
+ @Override
+ public void report(ProgressStatus progress) {}
- @Override
- public boolean isRewindingEnabled() {
- return false;
- }
+ @Override
+ public boolean isRewindingEnabled() {
+ return false;
+ }
- @Override
- public void checkForLostInputs() {}
+ @Override
+ public void checkForLostInputs() {}
- @Override
- public <T extends ActionContext> T getContext(Class<T> identifyingType) {
- throw new UnsupportedOperationException();
- }
+ @Override
+ public <T extends ActionContext> T getContext(Class<T> identifyingType) {
+ throw new UnsupportedOperationException();
+ }
- @Nullable
- @Override
- public FileSystem getActionFileSystem() {
- return null;
- }
- };
+ @Nullable
+ @Override
+ public FileSystem getActionFileSystem() {
+ return null;
+ }
+ };
+ }
private static SimpleSpawn simpleSpawnWithExecutionInfo(
ImmutableMap<String, String> executionInfo) {
@@ -236,6 +241,27 @@
ResourceSet.ZERO);
}
+ private static SimpleSpawn simplePathMappedSpawn(String configSegment) {
+ String inputPath = "bazel-bin/%s/bin/input";
+ String outputPath = "bazel-bin/%s/bin/output";
+ return new SimpleSpawn(
+ new FakeOwner("Mnemonic", "Progress Message", "//dummy:label"),
+ ImmutableList.of("cp", inputPath.formatted("cfg"), outputPath.formatted("cfg")),
+ ImmutableMap.of("VARIABLE", "value"),
+ ImmutableMap.of(ExecutionRequirements.SUPPORTS_PATH_MAPPING, ""),
+ /* runfilesSupplier= */ null,
+ /* filesetMappings= */ ImmutableMap.of(),
+ /* inputs= */ NestedSetBuilder.create(
+ Order.STABLE_ORDER, ActionInputHelper.fromPath(inputPath.formatted(configSegment))),
+ /* tools= */ NestedSetBuilder.emptySet(Order.STABLE_ORDER),
+ /* outputs= */ ImmutableSet.of(
+ ActionInputHelper.fromPath(outputPath.formatted(configSegment))),
+ /* mandatoryOutputs= */ null,
+ ResourceSet.ZERO,
+ execPath ->
+ execPath.subFragment(0, 1).getRelative("cfg").getRelative(execPath.subFragment(2)));
+ }
+
private RemoteSpawnCache createRemoteSpawnCache() {
return remoteSpawnCacheWithOptions(Options.getDefaults(RemoteOptions.class));
}
@@ -271,7 +297,7 @@
execRoot = fs.getPath("/exec/root");
execRoot.createDirectoryAndParents();
tempPathGenerator = new TempPathGenerator(fs.getPath("/execroot/_tmp/actions/remote"));
- fakeFileCache = new FakeActionInputFileCache(execRoot);
+ FakeActionInputFileCache fakeFileCache = new FakeActionInputFileCache(execRoot);
simpleSpawn = simpleSpawnWithExecutionInfo(ImmutableMap.of());
Path stdout = fs.getPath("/tmp/stdout");
@@ -284,6 +310,7 @@
reporter.addHandler(eventHandler);
remotePathResolver = RemotePathResolver.createDefault(execRoot);
+ simplePolicy = createSpawnExecutionContext(simpleSpawn, execRoot, fakeFileCache, outErr);
fakeFileCache.createScratchInput(simpleSpawn.getInputFiles().getSingleton(), "xyz");
}
@@ -336,7 +363,7 @@
verify(service)
.downloadOutputs(
any(), eq(RemoteActionResult.createFromCache(CachedActionResult.remote(actionResult))));
- verify(service, never()).uploadOutputs(any(), any());
+ verify(service, never()).uploadOutputs(any(), any(), any());
assertThat(result.getDigest())
.isEqualTo(digestUtil.asSpawnLogProto(actionKeyCaptor.getValue()));
assertThat(result.setupSuccess()).isTrue();
@@ -367,9 +394,9 @@
.setStatus(Status.SUCCESS)
.setRunnerName("test")
.build();
- doNothing().when(service).uploadOutputs(any(), any());
+ doNothing().when(service).uploadOutputs(any(), any(), any());
entry.store(result);
- verify(service).uploadOutputs(any(), any());
+ verify(service).uploadOutputs(any(), any(), any());
}
@Test
@@ -533,7 +560,7 @@
.setRunnerName("test")
.build();
entry.store(result);
- verify(service, never()).uploadOutputs(any(), any());
+ verify(service, never()).uploadOutputs(any(), any(), any());
}
@Test
@@ -556,9 +583,9 @@
.setRunnerName("test")
.build();
- doNothing().when(service).uploadOutputs(any(), any());
+ doNothing().when(service).uploadOutputs(any(), any(), any());
entry.store(result);
- verify(service).uploadOutputs(any(), eq(result));
+ verify(service).uploadOutputs(any(), eq(result), any());
assertThat(eventHandler.getEvents()).hasSize(1);
Event evt = eventHandler.getEvents().get(0);
@@ -604,9 +631,9 @@
.setRunnerName("test")
.build();
- doNothing().when(service).uploadOutputs(any(), any());
+ doNothing().when(service).uploadOutputs(any(), any(), any());
entry.store(result);
- verify(service).uploadOutputs(any(), eq(result));
+ verify(service).uploadOutputs(any(), eq(result), any());
assertThat(eventHandler.getEvents()).isEmpty(); // no warning is printed.
}
@@ -680,4 +707,141 @@
assertThat(evt.getKind()).isEqualTo(EventKind.WARNING);
assertThat(evt.getMessage()).contains(downloadFailure.getMessage());
}
+
+ @Test
+ public void pathMappedActionIsDeduplicated() throws Exception {
+ // arrange
+ RemoteSpawnCache cache = createRemoteSpawnCache();
+
+ SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild");
+ FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot);
+ firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz");
+ SpawnExecutionContext firstPolicy =
+ createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr);
+
+ SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt");
+ FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot);
+ secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz");
+ SpawnExecutionContext secondPolicy =
+ createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr);
+
+ RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService();
+ Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any());
+ // Simulate a very slow upload to the remote cache to ensure that the second spawn is
+ // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing
+ // concurrency to this test.
+ Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any());
+
+ // act
+ try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) {
+ FileSystemUtils.writeContent(
+ fs.getPath("/exec/root/bazel-bin/k8-fastbuild/bin/output"), UTF_8, "hello");
+ firstCacheHandle.store(
+ new SpawnResult.Builder()
+ .setExitCode(0)
+ .setStatus(Status.SUCCESS)
+ .setRunnerName("test")
+ .build());
+ }
+ CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy);
+
+ // assert
+ assertThat(secondCacheHandle.hasResult()).isTrue();
+ assertThat(secondCacheHandle.getResult().getRunnerName()).isEqualTo("deduplicated");
+ assertThat(
+ FileSystemUtils.readContent(
+ fs.getPath("/exec/root/bazel-bin/k8-opt/bin/output"), UTF_8))
+ .isEqualTo("hello");
+ assertThat(secondCacheHandle.willStore()).isFalse();
+ }
+
+ @Test
+ public void deduplicatedActionWithNonZeroExitCodeIsACacheMiss() throws Exception {
+ // arrange
+ RemoteSpawnCache cache = createRemoteSpawnCache();
+
+ SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild");
+ FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot);
+ firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz");
+ SpawnExecutionContext firstPolicy =
+ createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr);
+
+ SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt");
+ FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot);
+ secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz");
+ SpawnExecutionContext secondPolicy =
+ createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr);
+
+ RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService();
+ Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any());
+ // Simulate a very slow upload to the remote cache to ensure that the second spawn is
+ // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing
+ // concurrency to this test.
+ Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any());
+
+ // act
+ try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) {
+ FileSystemUtils.writeContent(
+ fs.getPath("/exec/root/bazel-bin/k8-fastbuild/bin/output"), UTF_8, "hello");
+ firstCacheHandle.store(
+ new SpawnResult.Builder()
+ .setExitCode(1)
+ .setStatus(Status.NON_ZERO_EXIT)
+ .setFailureDetail(
+ FailureDetail.newBuilder()
+ .setMessage("test spawn failed")
+ .setSpawn(
+ FailureDetails.Spawn.newBuilder()
+ .setCode(FailureDetails.Spawn.Code.NON_ZERO_EXIT))
+ .build())
+ .setRunnerName("test")
+ .build());
+ }
+ CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy);
+
+ // assert
+ assertThat(secondCacheHandle.hasResult()).isFalse();
+ assertThat(secondCacheHandle.willStore()).isTrue();
+ }
+
+ @Test
+ public void deduplicatedActionWithMissingOutputIsACacheMiss() throws Exception {
+ // arrange
+ RemoteSpawnCache cache = createRemoteSpawnCache();
+
+ SimpleSpawn firstSpawn = simplePathMappedSpawn("k8-fastbuild");
+ FakeActionInputFileCache firstFakeFileCache = new FakeActionInputFileCache(execRoot);
+ firstFakeFileCache.createScratchInput(firstSpawn.getInputFiles().getSingleton(), "xyz");
+ SpawnExecutionContext firstPolicy =
+ createSpawnExecutionContext(firstSpawn, execRoot, firstFakeFileCache, outErr);
+
+ SimpleSpawn secondSpawn = simplePathMappedSpawn("k8-opt");
+ FakeActionInputFileCache secondFakeFileCache = new FakeActionInputFileCache(execRoot);
+ secondFakeFileCache.createScratchInput(secondSpawn.getInputFiles().getSingleton(), "xyz");
+ SpawnExecutionContext secondPolicy =
+ createSpawnExecutionContext(secondSpawn, execRoot, secondFakeFileCache, outErr);
+
+ RemoteExecutionService remoteExecutionService = cache.getRemoteExecutionService();
+ Mockito.doCallRealMethod().when(remoteExecutionService).waitForAndReuseOutputs(any(), any());
+ // Simulate a very slow upload to the remote cache to ensure that the second spawn is
+ // deduplicated rather than a cache hit. This is a slight hack, but also avoid introducing
+ // concurrency to this test.
+ Mockito.doNothing().when(remoteExecutionService).uploadOutputs(any(), any(), any());
+
+ // act
+ try (CacheHandle firstCacheHandle = cache.lookup(firstSpawn, firstPolicy)) {
+ // Do not create the output.
+ firstCacheHandle.store(
+ new SpawnResult.Builder()
+ .setExitCode(0)
+ .setStatus(Status.SUCCESS)
+ .setRunnerName("test")
+ .build());
+ }
+ CacheHandle secondCacheHandle = cache.lookup(secondSpawn, secondPolicy);
+
+ // assert
+ assertThat(secondCacheHandle.hasResult()).isFalse();
+ assertThat(secondCacheHandle.willStore()).isTrue();
+ }
}
diff --git a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
index ce9c71a..616e404 100644
--- a/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/remote/RemoteSpawnRunnerTest.java
@@ -242,7 +242,7 @@
// TODO(olaola): verify that the uploaded action has the doNotCache set.
verify(service, never()).lookupCache(any());
- verify(service, never()).uploadOutputs(any(), any());
+ verify(service, never()).uploadOutputs(any(), any(), any());
verifyNoMoreInteractions(localRunner);
}
@@ -317,7 +317,7 @@
RemoteSpawnRunner runner = spy(newSpawnRunner());
RemoteExecutionService service = runner.getRemoteExecutionService();
- doNothing().when(service).uploadOutputs(any(), any());
+ doNothing().when(service).uploadOutputs(any(), any(), any());
// Throw an IOException to trigger the local fallback.
when(executor.executeRemotely(
@@ -343,7 +343,7 @@
verify(localRunner).exec(eq(spawn), eq(policy));
verify(runner)
.execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true));
- verify(service).uploadOutputs(any(), eq(res));
+ verify(service).uploadOutputs(any(), eq(res), any());
}
@Test
@@ -376,7 +376,7 @@
verify(localRunner).exec(eq(spawn), eq(policy));
verify(runner)
.execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true));
- verify(service, never()).uploadOutputs(any(), any());
+ verify(service, never()).uploadOutputs(any(), any(), any());
}
@Test
@@ -403,7 +403,7 @@
any(ExecuteRequest.class),
any(OperationObserver.class)))
.thenThrow(IOException.class);
- doNothing().when(service).uploadOutputs(any(), any());
+ doNothing().when(service).uploadOutputs(any(), any(), any());
Spawn spawn = newSimpleSpawn();
SpawnExecutionContext policy = getSpawnContext(spawn);
@@ -421,7 +421,7 @@
verify(localRunner).exec(eq(spawn), eq(policy));
verify(runner)
.execLocallyAndUpload(any(), eq(spawn), eq(policy), /* uploadLocalResults= */ eq(true));
- verify(service).uploadOutputs(any(), eq(result));
+ verify(service).uploadOutputs(any(), eq(result), any());
verify(service, never()).downloadOutputs(any(), any());
}
diff --git a/src/test/shell/bazel/path_mapping_test.sh b/src/test/shell/bazel/path_mapping_test.sh
index 560625b..e2ed5dc 100755
--- a/src/test/shell/bazel/path_mapping_test.sh
+++ b/src/test/shell/bazel/path_mapping_test.sh
@@ -46,6 +46,15 @@
source "$(rlocation "io_bazel/src/test/shell/bazel/remote/remote_utils.sh")" \
|| { echo "remote_utils.sh not found!" >&2; exit 1; }
+case "$(uname -s | tr [:upper:] [:lower:])" in
+msys*|mingw*|cygwin*)
+ function is_windows() { true; }
+ ;;
+*)
+ function is_windows() { false; }
+ ;;
+esac
+
function set_up() {
start_worker
@@ -546,4 +555,269 @@
expect_log ' 3 remote cache hit'
}
+function test_path_stripping_deduplicated_action() {
+ if is_windows; then
+ echo "Skipping test_path_stripping_deduplicated_action on Windows as it requires sandboxing"
+ return
+ fi
+
+ mkdir rules
+ touch rules/BUILD
+ cat > rules/defs.bzl <<'EOF'
+def _slow_rule_impl(ctx):
+ out_file = ctx.actions.declare_file(ctx.attr.name + "_file")
+ out_dir = ctx.actions.declare_directory(ctx.attr.name + "_dir")
+ out_symlink = ctx.actions.declare_symlink(ctx.attr.name + "_symlink")
+ outs = [out_file, out_dir, out_symlink]
+ args = ctx.actions.args().add_all(outs, expand_directories = False)
+ ctx.actions.run_shell(
+ outputs = outs,
+ command = """
+ # Sleep to ensure that two actions are scheduled in parallel.
+ sleep 3
+
+ echo "Hello, stdout!"
+ >&2 echo "Hello, stderr!"
+
+ echo 'echo "Hello, file!"' > $1
+ chmod +x $1
+ echo 'Hello, dir!' > $2/file
+ ln -s $(basename $2)/file $3
+ """,
+ arguments = [args],
+ execution_requirements = {"supports-path-mapping": ""},
+ )
+ return [
+ DefaultInfo(files = depset(outs)),
+ ]
+
+slow_rule = rule(_slow_rule_impl)
+EOF
+
+ mkdir -p pkg
+ cat > pkg/BUILD <<'EOF'
+load("//rules:defs.bzl", "slow_rule")
+
+slow_rule(name = "my_rule")
+
+COMMAND = """
+function validate() {
+ # Sorted by file name.
+ local -r dir=$$1
+ local -r file=$$2
+ local -r symlink=$$3
+
+ [[ $$($$file) == "Hello, file!" ]] || exit 1
+
+ [[ -d $$dir ]] || exit 1
+ [[ $$(cat $$dir/file) == "Hello, dir!" ]] || exit 1
+
+ [[ -L $$symlink ]] || exit 1
+ [[ $$(cat $$symlink) == "Hello, dir!" ]] || exit 1
+}
+validate $(execpaths :my_rule)
+touch $@
+"""
+
+genrule(
+ name = "gen_exec",
+ outs = ["out_exec"],
+ cmd = COMMAND,
+ tools = [":my_rule"],
+)
+
+genrule(
+ name = "gen_target",
+ outs = ["out_target"],
+ cmd = COMMAND,
+ srcs = [":my_rule"],
+)
+EOF
+
+ bazel build \
+ --experimental_output_paths=strip \
+ --remote_cache=grpc://localhost:${worker_port} \
+ //pkg:all &> $TEST_log || fail "build failed unexpectedly"
+ # The first slow_write action plus two genrules.
+ expect_log '3 \(linux\|darwin\|processwrapper\)-sandbox'
+ expect_log '1 deduplicated'
+
+ # Even though the spawn is only executed once, its stdout/stderr should be
+ # printed as if it wasn't deduplicated.
+ expect_log_once 'INFO: From Action pkg/my_rule_file:'
+ expect_log_once 'INFO: From Action pkg/my_rule_file \[for tool\]:'
+ expect_log_n 'Hello, stderr!' 2
+ expect_log_n 'Hello, stdout!' 2
+
+ bazel clean || fail "clean failed unexpectedly"
+ bazel build \
+ --experimental_output_paths=strip \
+ --remote_cache=grpc://localhost:${worker_port} \
+ //pkg:all &> $TEST_log || fail "build failed unexpectedly"
+ # The cache is checked before deduplication.
+ expect_log '4 remote cache hit'
+}
+
+function test_path_stripping_deduplicated_action_output_not_created() {
+ if is_windows; then
+ echo "Skipping test_path_stripping_deduplicated_action_output_not_created on Windows as it requires sandboxing"
+ return
+ fi
+
+ mkdir rules
+ touch rules/BUILD
+ cat > rules/defs.bzl <<'EOF'
+def _slow_rule_impl(ctx):
+ out_file = ctx.actions.declare_file(ctx.attr.name + "_file")
+ outs = [out_file]
+ args = ctx.actions.args().add_all(outs)
+ ctx.actions.run_shell(
+ outputs = outs,
+ command = """
+ # Sleep to ensure that two actions are scheduled in parallel.
+ sleep 3
+
+ echo "Hello, stdout!"
+ >&2 echo "Hello, stderr!"
+
+ # Do not create the output file
+ """,
+ arguments = [args],
+ execution_requirements = {"supports-path-mapping": ""},
+ )
+ return [
+ DefaultInfo(files = depset(outs)),
+ ]
+
+slow_rule = rule(_slow_rule_impl)
+EOF
+
+ mkdir -p pkg
+ cat > pkg/BUILD <<'EOF'
+load("//rules:defs.bzl", "slow_rule")
+
+slow_rule(name = "my_rule")
+
+COMMAND = """
+[[ $$($(execpath :my_rule)) == "Hello, file!" ]] || exit 1
+touch $@
+"""
+
+genrule(
+ name = "gen_exec",
+ outs = ["out_exec"],
+ cmd = COMMAND,
+ tools = [":my_rule"],
+)
+
+genrule(
+ name = "gen_target",
+ outs = ["out_target"],
+ cmd = COMMAND,
+ srcs = [":my_rule"],
+)
+EOF
+
+ bazel build \
+ --experimental_output_paths=strip \
+ --remote_cache=grpc://localhost:${worker_port} \
+ --keep_going \
+ //pkg:all &> $TEST_log && fail "build succeeded unexpectedly"
+ # The second action runs normally after discovering that the first one failed
+ # to create the output file.
+ expect_log '2 \(linux\|darwin\|processwrapper\)-sandbox'
+ expect_not_log '[0-9] deduplicated'
+
+ expect_log 'Action pkg/my_rule_file failed:'
+ expect_log 'Action pkg/my_rule_file \[for tool\] failed:'
+ # Remote cache warning.
+ expect_log 'Expected output pkg/my_rule_file was not created locally.'
+
+ expect_log_once 'INFO: From Action pkg/my_rule_file:'
+ expect_log_once 'INFO: From Action pkg/my_rule_file \[for tool\]:'
+ expect_log_n 'Hello, stderr!' 2
+ expect_log_n 'Hello, stdout!' 2
+}
+
+function test_path_stripping_deduplicated_action_non_zero_exit_code() {
+ if is_windows; then
+ echo "Skipping test_path_stripping_deduplicated_action_non_zero_exit_code on Windows as it requires sandboxing"
+ return
+ fi
+
+ mkdir rules
+ touch rules/BUILD
+ cat > rules/defs.bzl <<'EOF'
+def _slow_rule_impl(ctx):
+ out_file = ctx.actions.declare_file(ctx.attr.name + "_file")
+ outs = [out_file]
+ args = ctx.actions.args().add_all(outs)
+ ctx.actions.run_shell(
+ outputs = outs,
+ command = """
+ # Sleep to ensure that two actions are scheduled in parallel.
+ sleep 3
+
+ echo "Hello, stdout!"
+ >&2 echo "Hello, stderr!"
+
+ # Create the output file, but with a non-zero exit code.
+ echo 'echo "Hello, file!"' > $1
+ exit 1
+ """,
+ arguments = [args],
+ execution_requirements = {"supports-path-mapping": ""},
+ )
+ return [
+ DefaultInfo(files = depset(outs)),
+ ]
+
+slow_rule = rule(_slow_rule_impl)
+EOF
+
+ mkdir -p pkg
+ cat > pkg/BUILD <<'EOF'
+load("//rules:defs.bzl", "slow_rule")
+
+slow_rule(name = "my_rule")
+
+COMMAND = """
+[[ $$($(execpath :my_rule)) == "Hello, file!" ]] || exit 1
+touch $@
+"""
+
+genrule(
+ name = "gen_exec",
+ outs = ["out_exec"],
+ cmd = COMMAND,
+ tools = [":my_rule"],
+)
+
+genrule(
+ name = "gen_target",
+ outs = ["out_target"],
+ cmd = COMMAND,
+ srcs = [":my_rule"],
+)
+EOF
+
+ bazel build \
+ --experimental_output_paths=strip \
+ --remote_cache=grpc://localhost:${worker_port} \
+ --keep_going \
+ //pkg:all &> $TEST_log && fail "build succeeded unexpectedly"
+ # Failing actions are not deduplicated.
+ expect_not_log '[0-9] deduplicated'
+
+ expect_log 'Action pkg/my_rule_file failed:'
+ expect_log 'Action pkg/my_rule_file \[for tool\] failed:'
+
+ # The first execution emits stdout/stderr, the second doesn't.
+ # stdout/stderr are emitted as part of the failing action error, not as an
+ # info.
+ expect_not_log 'INFO: From Action pkg/my_rule_file'
+ expect_log_n 'Hello, stderr!' 2
+ expect_log_n 'Hello, stdout!' 2
+}
+
run_suite "path mapping tests"
