Make it possible to override the path of runfiles trees.
This is needed for a quirk in the remote execution implementation at Google. Previously, its implementation was in Google-internal code but removing RunfilesSupplier requires that we move this code here.
It's quite a bit of of boilerplate, but in exchange we can delete RunfilesSupplier.getRunfilesDirOverride() and remove the "possibly incorrect" path from getPossiblyIncorrectExecPath(). This also encapsulates knowledge about the runfiles trees in InputMetadataProvider, where it belongs.
RELNOTES: None.
PiperOrigin-RevId: 606613420
Change-Id: I4d9220ba02758a100f7115b2f3a5995c4c3e4ac4
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
index 78585cf..cee4c93 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionContext.java
@@ -19,11 +19,15 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
+import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
+import com.google.devtools.build.lib.actions.RunfilesSupplier.RunfilesTree;
import com.google.devtools.build.lib.actions.cache.OutputMetadataStore;
+import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue.RunfileSymlinksMode;
import com.google.devtools.build.lib.bugreport.BugReporter;
import com.google.devtools.build.lib.clock.Clock;
import com.google.devtools.build.lib.cmdline.Label;
+import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.events.Event;
import com.google.devtools.build.lib.events.EventKind;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -32,6 +36,7 @@
import com.google.devtools.build.lib.util.io.FileOutErr;
import com.google.devtools.build.lib.vfs.FileSystem;
import com.google.devtools.build.lib.vfs.Path;
+import com.google.devtools.build.lib.vfs.PathFragment;
import com.google.devtools.build.lib.vfs.Root;
import com.google.devtools.build.lib.vfs.SyscallCache;
import com.google.devtools.build.skyframe.SkyFunction.Environment;
@@ -44,6 +49,106 @@
/** A class that groups services in the scope of the action. Like the FileOutErr object. */
public class ActionExecutionContext implements Closeable, ActionContext.ActionContextRegistry {
+ /**
+ * A {@link RunfilesTree} implementation that wraps another one while overriding the path it
+ * should be materialized at.
+ */
+ private static class OverriddenPathRunfilesTree implements RunfilesTree {
+ private final PathFragment execPath;
+ private final RunfilesTree wrapped;
+
+ private OverriddenPathRunfilesTree(RunfilesTree wrapped, PathFragment execPath) {
+ this.wrapped = wrapped;
+ this.execPath = execPath;
+ }
+
+ @Override
+ public PathFragment getExecPath() {
+ return execPath;
+ }
+
+ @Override
+ public Map<PathFragment, Artifact> getMapping() {
+ return wrapped.getMapping();
+ }
+
+ @Override
+ public NestedSet<Artifact> getArtifacts() {
+ return wrapped.getArtifacts();
+ }
+
+ @Override
+ public RunfileSymlinksMode getSymlinksMode() {
+ return wrapped.getSymlinksMode();
+ }
+
+ @Override
+ public boolean isBuildRunfileLinks() {
+ return wrapped.isBuildRunfileLinks();
+ }
+
+ @Override
+ public String getWorkspaceName() {
+ return wrapped.getWorkspaceName();
+ }
+ }
+
+ /**
+ * An {@link InputMetadataProvider} wrapping another while overriding the materialization path of
+ * the single runfiles tree it contains.
+ *
+ * <p>The wrapped input metadata provider must contain exactly one runfiles tree.
+ */
+ private static class OverriddenRunfilesPathInputMetadataProvider
+ implements InputMetadataProvider {
+ private final InputMetadataProvider wrapped;
+ private final OverriddenPathRunfilesTree overriddenTree;
+
+ private OverriddenRunfilesPathInputMetadataProvider(
+ InputMetadataProvider wrapped, PathFragment execPath) {
+ this.wrapped = wrapped;
+ this.overriddenTree =
+ new OverriddenPathRunfilesTree(
+ Iterables.getOnlyElement(wrapped.getRunfilesTrees()), execPath);
+ }
+
+ @Nullable
+ @Override
+ public FileArtifactValue getInputMetadata(ActionInput input) throws IOException {
+ return wrapped.getInputMetadata(input);
+ }
+
+ @Nullable
+ @Override
+ public ActionInput getInput(String execPath) {
+ return wrapped.getInput(execPath);
+ }
+
+ @Nullable
+ @Override
+ public RunfilesArtifactValue getRunfilesMetadata(ActionInput input) {
+ RunfilesArtifactValue original = wrapped.getRunfilesMetadata(input);
+ // There is only one runfiles tree in the wrapped metadata provider so we can be sure that
+ // we need to override this.
+ return original == null ? null : original.withOverriddenRunfilesTree(overriddenTree);
+ }
+
+ @Override
+ public ImmutableList<RunfilesTree> getRunfilesTrees() {
+ return ImmutableList.of(overriddenTree);
+ }
+
+ @Override
+ public FileSystem getFileSystemForInputResolution() {
+ return wrapped.getFileSystemForInputResolution();
+ }
+
+ @Override
+ public boolean mayGetGeneratingActionsFromSkyframe() {
+ return wrapped.mayGetGeneratingActionsFromSkyframe();
+ }
+ }
+
/** Enum for --subcommands flag */
public enum ShowSubcommands {
TRUE(true, false), PRETTY_PRINT(true, true), FALSE(false, false);
@@ -406,27 +511,11 @@
}
}
- /**
- * Creates a new {@link ActionExecutionContext} whose {@link InputMetadataProvider} has the given
- * {@link ActionInput}s as inputs.
- *
- * <p>Each {@link ActionInput} must be an output of the current {@link ActionExecutionContext} and
- * it must already have been built.
- */
- public ActionExecutionContext withOutputsAsInputs(
- Iterable<? extends ActionInput> additionalInputs) throws IOException, InterruptedException {
- ImmutableMap.Builder<ActionInput, FileArtifactValue> additionalInputMap =
- ImmutableMap.builder();
-
- for (ActionInput input : additionalInputs) {
- additionalInputMap.put(input, getOutputMetadataStore().getOutputMetadata(input));
- }
- StaticInputMetadataProvider additionalInputMetadata =
- new StaticInputMetadataProvider(additionalInputMap.buildOrThrow());
-
+ private ActionExecutionContext withInputMetadataProvider(
+ InputMetadataProvider newInputMetadataProvider) {
return new ActionExecutionContext(
executor,
- new DelegatingPairInputMetadataProvider(additionalInputMetadata, inputMetadataProvider),
+ newInputMetadataProvider,
actionInputPrefetcher,
actionKeyContext,
outputMetadataStore,
@@ -444,6 +533,36 @@
syscallCache,
threadStateReceiverForMetrics);
}
+
+ /**
+ * Creates a new {@link ActionExecutionContext} whose {@link InputMetadataProvider} has the given
+ * {@link ActionInput}s as inputs.
+ *
+ * <p>Each {@link ActionInput} must be an output of the current {@link ActionExecutionContext} and
+ * it must already have been built.
+ */
+ public ActionExecutionContext withOutputsAsInputs(
+ Iterable<? extends ActionInput> additionalInputs) throws IOException, InterruptedException {
+ ImmutableMap.Builder<ActionInput, FileArtifactValue> additionalInputMap =
+ ImmutableMap.builder();
+
+ for (ActionInput input : additionalInputs) {
+ additionalInputMap.put(input, getOutputMetadataStore().getOutputMetadata(input));
+ }
+
+ StaticInputMetadataProvider additionalInputMetadata =
+ new StaticInputMetadataProvider(additionalInputMap.buildOrThrow());
+
+ return withInputMetadataProvider(
+ new DelegatingPairInputMetadataProvider(additionalInputMetadata, inputMetadataProvider));
+ }
+
+ public ActionExecutionContext withOverriddenRunfilesPath(PathFragment overrideRunfilesPath) {
+ return withInputMetadataProvider(
+ new OverriddenRunfilesPathInputMetadataProvider(
+ inputMetadataProvider, overrideRunfilesPath));
+ }
+
/**
* Allows us to create a new context that overrides the FileOutErr with another one. This is
* useful for muting the output for example.
diff --git a/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java
index fd85f9f..ec0c4fa 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/CompositeRunfilesSupplier.java
@@ -66,7 +66,7 @@
for (RunfilesSupplier supplier : nonEmptySuppliers) {
for (RunfilesTree tree : supplier.getRunfilesTrees()) {
- if (execPaths.add(tree.getPossiblyIncorrectExecPath())) {
+ if (execPaths.add(tree.getExecPath())) {
trees.add(tree);
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java b/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java
index 6f2219a..c5b3da8 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/RunfilesArtifactValue.java
@@ -62,6 +62,10 @@
this);
}
+ public RunfilesArtifactValue withOverriddenRunfilesTree(RunfilesTree overrideTree) {
+ return new RunfilesArtifactValue(metadata, overrideTree, files, fileValues, trees, treeValues);
+ }
+
/** Returns the data of the artifact for this value, as computed by the action cache checker. */
public FileArtifactValue getMetadata() {
return metadata;
diff --git a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
index bd86d34..688a15e2 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/RunfilesSupplier.java
@@ -19,7 +19,6 @@
import com.google.devtools.build.lib.collect.nestedset.NestedSet;
import com.google.devtools.build.lib.vfs.PathFragment;
import java.util.Map;
-import javax.annotation.Nullable;
/** Convenience wrapper around runfiles allowing lazy expansion. */
public interface RunfilesSupplier {
@@ -27,15 +26,9 @@
// TODO(bazel-team): Ideally we could refer to Runfiles objects directly here, but current package
// structure makes this difficult. Consider moving things around to make this possible.
interface RunfilesTree {
- /**
- * Returns the exec path of the root directory of the runfiles tree.
- *
- * <p>Should only ever be called indirectly through {@link
- * RunfilesSupplier#getExecPathForTree(RunfilesSupplier, RunfilesTree)} because {@link
- * RunfilesSupplier} may override it.
- */
+ /** Returns the exec path of the root directory of the runfiles tree. */
// TODO(lberki): Remove this method once RunfilesSupplier.getRunfilesTrees() is gone.
- PathFragment getPossiblyIncorrectExecPath();
+ PathFragment getExecPath();
/** Returns the mapping from the location in the runfiles tree to the artifact that's there. */
Map<PathFragment, Artifact> getMapping();
@@ -60,26 +53,4 @@
/** Returns the runfiles trees to be materialized on the inputs of the action. */
ImmutableList<RunfilesTree> getRunfilesTrees();
-
- /**
- * If not null, the runfile tree in this runfiles supplier should be assumed to be rooted at the
- * path this method returns, regardless of the return value of {@link
- * RunfilesTree#getPossiblyIncorrectExecPath()}
- */
- @Nullable
- default PathFragment getRunfilesDirOverride() {
- return null;
- }
-
- /**
- * Returns the effective root for the given runfiles tree, taking the potential override directory
- * of the {@link RunfilesSupplier} into account.
- */
- static PathFragment getExecPathForTree(
- RunfilesSupplier runfilesSupplier, RunfilesTree runfilesTree) {
- if (runfilesSupplier.getRunfilesDirOverride() != null) {
- return runfilesSupplier.getRunfilesDirOverride();
- }
- return runfilesTree.getPossiblyIncorrectExecPath();
- }
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
index 5ebd101..00d5a4d 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/RunfilesSupport.java
@@ -126,7 +126,7 @@
}
@Override
- public PathFragment getPossiblyIncorrectExecPath() {
+ public PathFragment getExecPath() {
return execPath;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java b/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java
index fa75202..27f606b 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/SingleRunfilesSupplier.java
@@ -72,7 +72,7 @@
}
@Override
- public PathFragment getPossiblyIncorrectExecPath() {
+ public PathFragment getExecPath() {
return runfilesDir;
}
diff --git a/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java b/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
index 2a2e068..c0afd5f 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/CompactSpawnLogContext.java
@@ -203,10 +203,7 @@
// The runfiles symlink tree might not have been materialized on disk, so use the mapping.
additionalDirectoryIds.add(
logRunfilesDirectory(
- RunfilesSupplier.getExecPathForTree(runfilesSupplier, tree),
- tree.getMapping(),
- inputMetadataProvider,
- fileSystem));
+ tree.getExecPath(), tree.getMapping(), inputMetadataProvider, fileSystem));
}
for (Artifact fileset : spawn.getFilesetMappings().keySet()) {
diff --git a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
index fff51d3..33b73f4 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/RunfilesTreeUpdater.java
@@ -78,7 +78,7 @@
RunfilesSupplier runfilesSupplier, ImmutableMap<String, String> env, OutErr outErr)
throws ExecException, IOException, InterruptedException {
for (RunfilesTree tree : runfilesSupplier.getRunfilesTrees()) {
- PathFragment runfilesDir = RunfilesSupplier.getExecPathForTree(runfilesSupplier, tree);
+ PathFragment runfilesDir = tree.getExecPath();
if (tree.isBuildRunfileLinks()) {
continue;
}
@@ -89,7 +89,7 @@
if (priorFuture == null) {
// We are the first attempt; update the runfiles tree and mark the future complete.
try {
- updateRunfilesTree(runfilesSupplier, tree, env, outErr);
+ updateRunfilesTree(tree, env, outErr);
freshFuture.complete(null);
} catch (Exception e) {
freshFuture.completeExceptionally(e);
@@ -114,13 +114,11 @@
}
private void updateRunfilesTree(
- RunfilesSupplier runfilesSupplier,
RunfilesTree tree,
ImmutableMap<String, String> env,
OutErr outErr)
throws IOException, ExecException, InterruptedException {
- Path runfilesDirPath =
- execRoot.getRelative(RunfilesSupplier.getExecPathForTree(runfilesSupplier, tree));
+ Path runfilesDirPath = execRoot.getRelative(tree.getExecPath());
Path inputManifest = RunfilesSupport.inputManifestPath(runfilesDirPath);
if (!inputManifest.exists()) {
return;
diff --git a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
index cec139a..bef262b 100644
--- a/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
+++ b/src/main/java/com/google/devtools/build/lib/exec/SpawnInputExpander.java
@@ -108,7 +108,7 @@
for (RunfilesTree runfilesTree : runfilesSupplier.getRunfilesTrees()) {
addSingleRunfilesTreeToInputs(
inputSink,
- RunfilesSupplier.getExecPathForTree(runfilesSupplier, runfilesTree),
+ runfilesTree.getExecPath(),
runfilesTree.getMapping(),
artifactExpander,
pathMapper,
@@ -385,7 +385,7 @@
InputVisitor visitor)
throws IOException, ForbiddenActionInputException {
for (RunfilesTree runfilesTree : runfilesSupplier.getRunfilesTrees()) {
- PathFragment root = RunfilesSupplier.getExecPathForTree(runfilesSupplier, runfilesTree);
+ PathFragment root = runfilesTree.getExecPath();
Map<PathFragment, Artifact> mappings = runfilesTree.getMapping();
visitor.visit(
// Cache key for the sub-mapping containing this runfiles tree.
diff --git a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
index 1f84ad6..f7b1578 100644
--- a/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
+++ b/src/main/java/com/google/devtools/build/lib/runtime/commands/RunCommand.java
@@ -975,7 +975,7 @@
BuildConfigurationValue configuration,
RunfilesTreeUpdater runfilesTreeUpdater)
throws RunfilesException, InterruptedException {
- PathFragment runfilesDir = runfilesSupport.getRunfilesTree().getPossiblyIncorrectExecPath();
+ PathFragment runfilesDir = runfilesSupport.getRunfilesTree().getExecPath();
Path workingDir = env.getExecRoot().getRelative(runfilesDir);
// On Windows, runfiles tree is disabled.
// Workspace name directory doesn't exist, so don't add it.
diff --git a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
index b052657..d469959 100644
--- a/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
+++ b/src/main/java/com/google/devtools/build/lib/worker/WorkerFilesHash.java
@@ -79,7 +79,7 @@
RunfilesSupplier runfilesSupplier = spawn.getRunfilesSupplier();
for (RunfilesTree runfilesTree : runfilesSupplier.getRunfilesTrees()) {
- PathFragment root = RunfilesSupplier.getExecPathForTree(runfilesSupplier, runfilesTree);
+ PathFragment root = runfilesTree.getExecPath();
Preconditions.checkState(!root.isAbsolute(), root);
for (Map.Entry<PathFragment, Artifact> mapping : runfilesTree.getMapping().entrySet()) {
Artifact localArtifact = mapping.getValue();
diff --git a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
index ae5e7a7..81ea277 100644
--- a/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
+++ b/src/test/java/com/google/devtools/build/lib/exec/SpawnLogContextTestBase.java
@@ -880,11 +880,10 @@
mappingByPath.put(PathFragment.create(entry.getKey()), entry.getValue());
}
RunfilesTree runfilesTree = mock(RunfilesTree.class);
- when(runfilesTree.getPossiblyIncorrectExecPath()).thenReturn(root);
+ when(runfilesTree.getExecPath()).thenReturn(root);
when(runfilesTree.getMapping()).thenReturn(mappingByPath);
RunfilesSupplier runfilesSupplier = mock(RunfilesSupplier.class);
when(runfilesSupplier.getRunfilesTrees()).thenReturn(ImmutableList.of(runfilesTree));
- when(runfilesSupplier.getRunfilesDirOverride()).thenReturn(null);
return runfilesSupplier;
}
@@ -920,7 +919,7 @@
ImmutableSortedMap.naturalOrder();
for (RunfilesTree tree : runfilesSupplier.getRunfilesTrees()) {
// Emulate SpawnInputExpander: expand runfiles, replacing nulls with empty inputs.
- PathFragment root = tree.getPossiblyIncorrectExecPath();
+ PathFragment root = tree.getExecPath();
for (Map.Entry<PathFragment, Artifact> entry : tree.getMapping().entrySet()) {
PathFragment execPath = root.getRelative(entry.getKey());
Artifact artifact = entry.getValue();
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 5589ac3..8a4d43c 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
@@ -2477,7 +2477,7 @@
RunfilesTree tree =
new RunfilesTree() {
@Override
- public PathFragment getPossiblyIncorrectExecPath() {
+ public PathFragment getExecPath() {
return PathFragment.create(root);
}
