SymlinkTreeAction: require a Runfiles object
This is not particularly useful in itself, but updates the callers to
match the new expectation, and also points out the one place (filesets)
where we may need to do additional work.
I'll make use of the Runfiles reference in a subsequent change to avoid
reading back the local file into Bazel.
PiperOrigin-RevId: 281263527
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 7d5f40a..be87200 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
@@ -374,11 +374,11 @@
.registerAction(
new SymlinkTreeAction(
context.getActionOwner(),
+ config,
inputManifest,
+ runfiles,
outputManifest,
- /*filesetTree=*/ false,
- config.getActionEnvironment(),
- config.runfilesEnabled()));
+ /*filesetTree=*/ false));
return outputManifest;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
index 127289b..db515ae 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeAction.java
@@ -23,9 +23,12 @@
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.ActionResult;
import com.google.devtools.build.lib.actions.Artifact;
+import com.google.devtools.build.lib.analysis.Runfiles;
+import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
import com.google.devtools.build.lib.util.Fingerprint;
+import javax.annotation.Nullable;
/**
* Action responsible for the symlink tree creation.
@@ -38,31 +41,66 @@
private static final String GUID = "63412bda-4026-4c8e-a3ad-7deb397728d4";
private final Artifact inputManifest;
+ private final Runfiles runfiles;
private final Artifact outputManifest;
private final boolean filesetTree;
private final boolean enableRunfiles;
/**
* Creates SymlinkTreeAction instance.
- * @param owner action owner
+ *
+ * @param owner action owner
+ * @param config the action owners build configuration
* @param inputManifest the input runfiles manifest
- * @param outputManifest the generated symlink tree manifest
- * (must have "MANIFEST" base name). Symlink tree root
- * will be set to the artifact's parent directory.
+ * @param runfiles the input runfiles
+ * @param outputManifest the generated symlink tree manifest (must have "MANIFEST" base name).
+ * Symlink tree root will be set to the artifact's parent directory.
+ * @param filesetTree true if this is fileset symlink tree
+ */
+ public SymlinkTreeAction(
+ ActionOwner owner,
+ BuildConfiguration config,
+ Artifact inputManifest,
+ @Nullable Runfiles runfiles,
+ Artifact outputManifest,
+ boolean filesetTree) {
+ this(
+ owner,
+ inputManifest,
+ runfiles,
+ outputManifest,
+ filesetTree,
+ config.getActionEnvironment(),
+ config.runfilesEnabled());
+ }
+
+ /**
+ * Creates SymlinkTreeAction instance. Prefer the constructor that takes a {@link
+ * BuildConfiguration} instance; it is less likely to require changes in the future if we add more
+ * command-line flags that affect this action.
+ *
+ * @param owner action owner
+ * @param inputManifest the input runfiles manifest
+ * @param runfiles the input runfiles
+ * @param outputManifest the generated symlink tree manifest (must have "MANIFEST" base name).
+ * Symlink tree root will be set to the artifact's parent directory.
* @param filesetTree true if this is fileset symlink tree,
- * @param enableRunfiles true is the actual symlink tree needs to be created.
*/
@AutoCodec.Instantiator
public SymlinkTreeAction(
ActionOwner owner,
Artifact inputManifest,
+ @Nullable Runfiles runfiles,
Artifact outputManifest,
boolean filesetTree,
ActionEnvironment env,
boolean enableRunfiles) {
super(owner, ImmutableList.of(inputManifest), ImmutableList.of(outputManifest), env);
Preconditions.checkArgument(outputManifest.getPath().getBaseName().equals("MANIFEST"));
+ Preconditions.checkArgument(
+ (runfiles == null) == filesetTree, "Runfiles must be null iff this is a fileset action");
this.inputManifest = inputManifest;
+ this.runfiles = runfiles;
this.outputManifest = outputManifest;
this.filesetTree = filesetTree;
this.enableRunfiles = enableRunfiles;
@@ -72,6 +110,11 @@
return inputManifest;
}
+ @Nullable
+ public Runfiles getRunfiles() {
+ return runfiles;
+ }
+
public Artifact getOutputManifest() {
return outputManifest;
}
@@ -97,6 +140,18 @@
fp.addBoolean(filesetTree);
fp.addBoolean(enableRunfiles);
env.addTo(fp);
+ // We need to ensure that the fingerprints for two different instances of this action are
+ // different. Consider the hypothetical scenario where we add a second runfiles object to this
+ // class, which could also be null: the sequence
+ // if (r1 != null) r1.fingerprint(fp);
+ // if (r2 != null) r2.fingerprint(fp);
+ // would *not* be safe; we'd get a collision between an action that has only r1 set, and another
+ // that has only r2 set. Prefixing with a boolean indicating the presence of runfiles makes it
+ // safe to add more fields in the future.
+ fp.addBoolean(runfiles != null);
+ if (runfiles != null) {
+ runfiles.fingerprint(fp);
+ }
}
@Override
diff --git a/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java b/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java
index f142033..6bdace7 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/android/NativeLibs.java
@@ -185,11 +185,11 @@
ruleContext.registerAction(
new SymlinkTreeAction(
ruleContext.getActionOwner(),
+ ruleContext.getConfiguration(),
inputManifest,
+ runfiles,
outputManifest,
- false,
- ruleContext.getConfiguration().getActionEnvironment(),
- ruleContext.getConfiguration().runfilesEnabled()));
+ /*filesetTree=*/ false));
return new ManifestAndRunfiles(outputManifest, runfiles);
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java
index 9c61d54..b0b7320 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SymlinkTreeActionTest.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.actions.ActionEnvironment;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.util.ActionsTestUtil;
+import com.google.devtools.build.lib.analysis.Runfiles;
import com.google.devtools.build.lib.analysis.util.ActionTester;
import com.google.devtools.build.lib.analysis.util.ActionTester.ActionCombinationFactory;
import com.google.devtools.build.lib.analysis.util.BuildViewTestCase;
@@ -33,7 +34,7 @@
public class SymlinkTreeActionTest extends BuildViewTestCase {
private enum KeyAttributes {
FILESET,
- RUNFILES,
+ RUNFILES_FLAG,
FIXED_ENVIRONMENT,
VARIABLE_ENVIRONMENT
}
@@ -42,6 +43,7 @@
public void testComputeKey() throws Exception {
final Artifact inputManifest = getBinArtifactWithNoOwner("dir/manifest.in");
final Artifact outputManifest = getBinArtifactWithNoOwner("dir/MANIFEST");
+ final Artifact runfile = getBinArtifactWithNoOwner("dir/runfile");
ActionTester.runTest(
KeyAttributes.class,
@@ -49,17 +51,29 @@
@Override
public Action generate(ImmutableSet<KeyAttributes> attributesToFlip) {
boolean filesetTree = attributesToFlip.contains(KeyAttributes.FILESET);
- boolean enableRunfiles = attributesToFlip.contains(KeyAttributes.RUNFILES);
+ boolean enableRunfiles = attributesToFlip.contains(KeyAttributes.RUNFILES_FLAG);
- ActionEnvironment env = ActionEnvironment.create(
- attributesToFlip.contains(KeyAttributes.FIXED_ENVIRONMENT)
- ? ImmutableMap.of("a", "b") : ImmutableMap.of(),
- attributesToFlip.contains(KeyAttributes.VARIABLE_ENVIRONMENT)
- ? ImmutableSet.of("c") : ImmutableSet.of());
+ ActionEnvironment env =
+ ActionEnvironment.create(
+ attributesToFlip.contains(KeyAttributes.FIXED_ENVIRONMENT)
+ ? ImmutableMap.of("a", "b")
+ : ImmutableMap.of(),
+ attributesToFlip.contains(KeyAttributes.VARIABLE_ENVIRONMENT)
+ ? ImmutableSet.of("c")
+ : ImmutableSet.of());
+
+ // SymlinkTreeAction currently requires that (runfiles == null) == filsetTree, which the
+ // ActionTester doesn't support. We therefore can't check that two actions have
+ // different fingerprints when they have different runfiles objects.
+ Runfiles runfiles =
+ attributesToFlip.contains(KeyAttributes.FILESET)
+ ? null
+ : new Runfiles.Builder("TESTING", false).addArtifact(runfile).build();
return new SymlinkTreeAction(
ActionsTestUtil.NULL_ACTION_OWNER,
inputManifest,
+ runfiles,
outputManifest,
filesetTree,
env,
