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();