Add #getKey to SpawnActionTemplate by just creating a dummy SpawnAction and computing its key. That should be sufficient. As a result, we can move #getKey up to ActionAnalysisMetadata.
Can we use this tactic for CppCompileActionTemplate as well?
PiperOrigin-RevId: 244746907
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
index 1e5da6d..e076912 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionAnalysisMetadata.java
@@ -47,6 +47,36 @@
String getMnemonic();
/**
+ * Returns a string encoding all of the significant behaviour of this Action that might affect the
+ * output. The general contract of <code>getKey</code> is this: if the work to be performed by the
+ * execution of this action changes, the key must change.
+ *
+ * <p>As a corollary, the build system is free to omit the execution of an Action <code>a1</code>
+ * if (a) at some time in the past, it has already executed an Action <code>a0</code> with the
+ * same key as <code>a1</code>, (b) the names and contents of the input files listed by <code>
+ * a1.getInputs()</code> are identical to the names and contents of the files listed by <code>
+ * a0.getInputs()</code>, and (c) the names and values in the client environment of the variables
+ * listed by <code>a1.getClientEnvironmentVariables()</code> are identical to those listed by
+ * <code>a0.getClientEnvironmentVariables()</code>.
+ *
+ * <p>Examples of changes that should affect the key are:
+ *
+ * <ul>
+ * <li>Changes to the BUILD file that materially affect the rule which gave rise to this Action.
+ * <li>Changes to the command-line options, environment, or other global configuration resources
+ * which affect the behaviour of this kind of Action (other than changes to the names of the
+ * input/output files, which are handled externally).
+ * <li>An upgrade to the build tools which changes the program logic of this kind of Action
+ * (typically this is achieved by incorporating a UUID into the key, which is changed each
+ * time the program logic of this action changes).
+ * </ul>
+ *
+ * <p>Note the following exception: for actions that discover inputs, the key must change if any
+ * input names change or else action validation may falsely validate.
+ */
+ String getKey(ActionKeyContext actionKeyContext);
+
+ /**
* Returns a pretty string representation of this action, suitable for use in
* progress messages or error messages.
*/
diff --git a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
index c83cea9..255074f 100644
--- a/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
+++ b/src/main/java/com/google/devtools/build/lib/actions/ActionExecutionMetadata.java
@@ -45,36 +45,6 @@
String getProgressMessage();
/**
- * Returns a string encoding all of the significant behaviour of this Action that might affect the
- * output. The general contract of <code>getKey</code> is this: if the work to be performed by the
- * execution of this action changes, the key must change.
- *
- * <p>As a corollary, the build system is free to omit the execution of an Action <code>a1</code>
- * if (a) at some time in the past, it has already executed an Action <code>a0</code> with the
- * same key as <code>a1</code>, (b) the names and contents of the input files listed by <code>
- * a1.getInputs()</code> are identical to the names and contents of the files listed by <code>
- * a0.getInputs()</code>, and (c) the names and values in the client environment of the variables
- * listed by <code>a1.getClientEnvironmentVariables()</code> are identical to those listed by
- * <code>a0.getClientEnvironmentVariables()</code>.
- *
- * <p>Examples of changes that should affect the key are:
- *
- * <ul>
- * <li>Changes to the BUILD file that materially affect the rule which gave rise to this Action.
- * <li>Changes to the command-line options, environment, or other global configuration resources
- * which affect the behaviour of this kind of Action (other than changes to the names of the
- * input/output files, which are handled externally).
- * <li>An upgrade to the build tools which changes the program logic of this kind of Action
- * (typically this is achieved by incorporating a UUID into the key, which is changed each
- * time the program logic of this action changes).
- * </ul>
- *
- * <p>Note the following exception: for actions that discover inputs, the key must change if any
- * input names change or else action validation may falsely validate.
- */
- String getKey(ActionKeyContext actionKeyContext);
-
- /**
* Returns a human-readable description of the inputs to {@link #getKey(ActionKeyContext)}. Used
* in the output from '--explain', and in error messages for '--check_up_to_date' and
* '--check_tests_up_to_date'. May return null, meaning no extra information is available.
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
index ab54b4f..c3716e1 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplate.java
@@ -19,6 +19,7 @@
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
import com.google.devtools.build.lib.actions.ActionExecutionContext;
import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.ActionOwner;
import com.google.devtools.build.lib.actions.ActionTemplate;
import com.google.devtools.build.lib.actions.Artifact;
@@ -46,6 +47,7 @@
private final OutputPathMapper outputPathMapper;
private final SpawnAction.Builder spawnActionBuilder;
private final CustomCommandLine commandLineTemplate;
+ private String cachedKey;
/**
* Interface providing mapping between expanded input files under the input TreeArtifact and
@@ -112,6 +114,21 @@
return expandedActions.build();
}
+ @Override
+ public synchronized String getKey(ActionKeyContext actionKeyContext) {
+ if (cachedKey != null) {
+ return cachedKey;
+ }
+ TreeFileArtifact inputTreeFileArtifact =
+ ActionInputHelper.treeFileArtifact(inputTreeArtifact, "dummy_for_key");
+ TreeFileArtifact outputTreeFileArtifact =
+ ActionInputHelper.treeFileArtifact(
+ outputTreeArtifact, outputPathMapper.parentRelativeOutputPath(inputTreeFileArtifact));
+ SpawnAction dummyAction = createAction(inputTreeFileArtifact, outputTreeFileArtifact);
+ cachedKey = dummyAction.getKey(actionKeyContext);
+ return cachedKey;
+ }
+
/**
* Returns a SpawnAction that takes inputTreeFileArtifact as input and generates
* outputTreeFileArtifact.
diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
index 21310b0..3ce146e 100644
--- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
+++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppCompileActionTemplate.java
@@ -142,7 +142,7 @@
return expandedActions.build();
}
- // TODO(jhorvitz): Move getKey to ActionAnalysisMetadata once all action templates implement it.
+ @Override
public synchronized String getKey(ActionKeyContext actionKeyContext) {
if (cachedKey != null) {
return cachedKey;
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplateTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplateTest.java
index d9cd78d..56ed076 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplateTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/SpawnActionTemplateTest.java
@@ -19,6 +19,7 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionInputHelper;
+import com.google.devtools.build.lib.actions.ActionKeyContext;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifact;
import com.google.devtools.build.lib.actions.Artifact.SpecialArtifactType;
@@ -139,6 +140,62 @@
}
@Test
+ public void getKey_same() {
+ ActionKeyContext keyContext = new ActionKeyContext();
+ SpecialArtifact inputTreeArtifact = createInputTreeArtifact();
+ SpecialArtifact outputTreeArtifact = createOutputTreeArtifact();
+ Artifact executable = createDerivedArtifact("bin/cp");
+
+ // Use two different builders because the same builder would share the underlying
+ // SpawnActionBuilder.
+ SpawnActionTemplate actionTemplate =
+ builder(inputTreeArtifact, outputTreeArtifact)
+ .setExecutable(executable)
+ .setCommandLineTemplate(
+ createSimpleCommandLineTemplate(inputTreeArtifact, outputTreeArtifact))
+ .setOutputPathMapper(IDENTITY_MAPPER)
+ .setMnemonics("ActionTemplate", "ExpandedAction")
+ .build(ActionsTestUtil.NULL_ACTION_OWNER);
+ SpawnActionTemplate actionTemplate2 =
+ builder(inputTreeArtifact, outputTreeArtifact)
+ .setExecutable(executable)
+ .setCommandLineTemplate(
+ createSimpleCommandLineTemplate(inputTreeArtifact, outputTreeArtifact))
+ .setOutputPathMapper(IDENTITY_MAPPER)
+ .setMnemonics("ActionTemplate", "ExpandedAction")
+ .build(ActionsTestUtil.NULL_ACTION_OWNER);
+ assertThat(actionTemplate2.getKey(keyContext)).isEqualTo(actionTemplate.getKey(keyContext));
+ }
+
+ @Test
+ public void getKey_differs() {
+ ActionKeyContext keyContext = new ActionKeyContext();
+ SpecialArtifact inputTreeArtifact = createInputTreeArtifact();
+ SpecialArtifact outputTreeArtifact = createOutputTreeArtifact();
+ Artifact executable = createDerivedArtifact("bin/cp");
+
+ // Use two different builders because the same builder would share the underlying
+ // SpawnActionBuilder.
+ SpawnActionTemplate actionTemplate =
+ builder(inputTreeArtifact, outputTreeArtifact)
+ .setExecutable(executable)
+ .setCommandLineTemplate(
+ createSimpleCommandLineTemplate(inputTreeArtifact, outputTreeArtifact))
+ .setOutputPathMapper(IDENTITY_MAPPER)
+ .setMnemonics("ActionTemplate", "ExpandedAction")
+ .build(ActionsTestUtil.NULL_ACTION_OWNER);
+ SpawnActionTemplate actionTemplate2 =
+ builder(inputTreeArtifact, outputTreeArtifact)
+ .setExecutable(executable)
+ .setCommandLineTemplate(
+ createSimpleCommandLineTemplate(inputTreeArtifact, outputTreeArtifact))
+ .setOutputPathMapper(IDENTITY_MAPPER)
+ .setMnemonics("ActionTemplate", "ExpandedAction2")
+ .build(ActionsTestUtil.NULL_ACTION_OWNER);
+ assertThat(actionTemplate2.getKey(keyContext)).isNotEqualTo(actionTemplate.getKey(keyContext));
+ }
+
+ @Test
public void testExpandedAction_inputAndOutputTreeFileArtifacts() throws Exception {
SpawnActionTemplate actionTemplate = createSimpleSpawnActionTemplate();
SpecialArtifact inputTreeArtifact = createInputTreeArtifact();
