Refactor the AbstractAction computeKey method to be easier to add invariants

for every type of action.

PiperOrigin-RevId: 187368369

7 files changed, 46 insertions, 28 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
index 15f6bd78b1..87578eb195 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/ActionCacheCheckerTest.java
@@ -35,6 +35,7 @@ import com.google.devtools.build.lib.clock.Clock;
 import com.google.devtools.build.lib.skyframe.FileArtifactValue;
 import com.google.devtools.build.lib.testutil.ManualClock;
 import com.google.devtools.build.lib.testutil.Scratch;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.FileSystem;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -173,16 +174,16 @@ public class ActionCacheCheckerTest {
     Action action =
         new NullAction() {
           @Override
-          protected String computeKey(ActionKeyContext actionKeyContext) {
-            return "key1";
+          protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
+            fp.addString("key1");
           }
         };
     runAction(action);
     action =
         new NullAction() {
           @Override
-          protected String computeKey(ActionKeyContext actionKeyContext) {
-            return "key2";
+          protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
+            fp.addString("key2");
           }
         };
     runAction(action);
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
index e3bd837086..a9cd9f8c67 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/ActionsTestUtil.java
@@ -66,6 +66,7 @@ import com.google.devtools.build.lib.exec.SingleBuildFileCache;
 import com.google.devtools.build.lib.packages.AspectDescriptor;
 import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec;
 import com.google.devtools.build.lib.util.FileType;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.ResourceUsage;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileStatus;
@@ -311,8 +312,8 @@ public final class ActionsTestUtil {
     }
 
     @Override
-    protected String computeKey(ActionKeyContext actionKeyContext) {
-      return "action";
+    protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
+      fp.addString("action");
     }
 
     @Override
diff --git a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
index 82867ba905..b709c6852f 100644
--- a/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
+++ b/src/test/java/com/google/devtools/build/lib/actions/util/TestAction.java
@@ -127,11 +127,9 @@ public class TestAction extends AbstractAction {
   }
 
   @Override
-  protected String computeKey(ActionKeyContext actionKeyContext) {
-    Fingerprint f = new Fingerprint();
-    f.addPaths(Artifact.asSortedPathFragments(getOutputs()));
-    f.addPaths(Artifact.asSortedPathFragments(getMandatoryInputs()));
-    return f.hexDigestAndReset();
+  protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
+    fp.addPaths(Artifact.asSortedPathFragments(getOutputs()));
+    fp.addPaths(Artifact.asSortedPathFragments(getMandatoryInputs()));
   }
 
   @Override
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
index 470909c5b5..b2e3006f30 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/actions/TemplateExpansionActionTest.java
@@ -33,6 +33,7 @@ import com.google.devtools.build.lib.analysis.actions.TemplateExpansionAction.Te
 import com.google.devtools.build.lib.exec.BinTools;
 import com.google.devtools.build.lib.exec.util.TestExecutorBuilder;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.util.io.FileOutErr;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
@@ -125,7 +126,8 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
     TemplateExpansionAction b = new TemplateExpansionAction(NULL_ACTION_OWNER,
          outputArtifact2, Template.forString(TEMPLATE),
          ImmutableList.of(Substitution.of("%key%", "foo")), false);
-    assertThat(b.computeKey(actionKeyContext)).isEqualTo(a.computeKey(actionKeyContext));
+
+    assertThat(computeKey(a)).isEqualTo(computeKey(b));
   }
 
   @Test
@@ -138,7 +140,8 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
     TemplateExpansionAction b = new TemplateExpansionAction(NULL_ACTION_OWNER,
          outputArtifact2, Template.forString(TEMPLATE),
          ImmutableList.of(Substitution.of("%key%", "foo2")), false);
-    assertThat(a.computeKey(actionKeyContext).equals(b.computeKey(actionKeyContext))).isFalse();
+
+    assertThat(computeKey(a)).isNotEqualTo(computeKey(b));
   }
 
   @Test
@@ -151,7 +154,8 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
     TemplateExpansionAction b = new TemplateExpansionAction(NULL_ACTION_OWNER,
          outputArtifact2, Template.forString(TEMPLATE),
          ImmutableList.of(Substitution.of("%key%", "foo")), true);
-    assertThat(a.computeKey(actionKeyContext).equals(b.computeKey(actionKeyContext))).isFalse();
+
+    assertThat(computeKey(a)).isNotEqualTo(computeKey(b));
   }
 
   @Test
@@ -164,7 +168,8 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
     TemplateExpansionAction b = new TemplateExpansionAction(NULL_ACTION_OWNER,
          outputArtifact2, Template.forString(TEMPLATE + " "),
          ImmutableList.of(Substitution.of("%key%", "foo")), false);
-    assertThat(a.computeKey(actionKeyContext).equals(b.computeKey(actionKeyContext))).isFalse();
+
+    assertThat(computeKey(a)).isNotEqualTo(computeKey(b));
   }
 
   private TemplateExpansionAction createWithArtifact() {
@@ -228,4 +233,10 @@ public class TemplateExpansionActionTest extends FoundationTestCase {
 
     executeTemplateExpansion(expected, ImmutableList.of(Substitution.of("%key%", SPECIAL_CHARS)));
   }
+
+  private String computeKey(TemplateExpansionAction action) {
+    Fingerprint fp = new Fingerprint();
+    action.computeKey(actionKeyContext, fp);
+    return fp.hexDigestAndReset();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
index dc15b84842..a86066d1a5 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestUtil.java
@@ -51,6 +51,7 @@ import com.google.devtools.build.lib.concurrent.ThreadSafety.Immutable;
 import com.google.devtools.build.lib.events.ExtendedEventHandler;
 import com.google.devtools.build.lib.syntax.SkylarkSemantics;
 import com.google.devtools.build.lib.testutil.TestConstants;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.skyframe.SkyFunction;
@@ -234,9 +235,7 @@ public final class AnalysisTestUtil {
     }
 
     @Override
-    public String computeKey(ActionKeyContext actionKeyContext) {
-      return "";
-    }
+    public void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {}
 
     @Override
     public Artifact getVolatileStatus() {
diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java
index 6c4a74f289..f24f200509 100644
--- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CreateIncSymlinkActionTest.java
@@ -22,6 +22,7 @@ import com.google.devtools.build.lib.actions.ActionKeyContext;
 import com.google.devtools.build.lib.actions.Artifact;
 import com.google.devtools.build.lib.actions.ArtifactRoot;
 import com.google.devtools.build.lib.testutil.FoundationTestCase;
+import com.google.devtools.build.lib.util.Fingerprint;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
 import com.google.devtools.build.lib.vfs.Symlinks;
@@ -54,8 +55,8 @@ public class CreateIncSymlinkActionTest extends FoundationTestCase {
     d = new Artifact(PathFragment.create("d"), root);
     CreateIncSymlinkAction action2 =
         new CreateIncSymlinkAction(NULL_ACTION_OWNER, ImmutableMap.of(c, d, a, b), includePath);
-    assertThat(action2.computeKey(actionKeyContext))
-        .isEqualTo(action1.computeKey(actionKeyContext));
+
+    assertThat(computeKey(action2)).isEqualTo(computeKey(action1));
   }
 
   @Test
@@ -71,8 +72,8 @@ public class CreateIncSymlinkActionTest extends FoundationTestCase {
     b = new Artifact(PathFragment.create("c"), root);
     CreateIncSymlinkAction action2 =
         new CreateIncSymlinkAction(NULL_ACTION_OWNER, ImmutableMap.of(a, b), includePath);
-    assertThat(action2.computeKey(actionKeyContext))
-        .isNotEqualTo(action1.computeKey(actionKeyContext));
+
+    assertThat(computeKey(action2)).isNotEqualTo(computeKey(action1));
   }
 
   @Test
@@ -88,8 +89,8 @@ public class CreateIncSymlinkActionTest extends FoundationTestCase {
     b = new Artifact(PathFragment.create("b"), root);
     CreateIncSymlinkAction action2 =
         new CreateIncSymlinkAction(NULL_ACTION_OWNER, ImmutableMap.of(a, b), includePath);
-    assertThat(action2.computeKey(actionKeyContext))
-        .isNotEqualTo(action1.computeKey(actionKeyContext));
+
+    assertThat(computeKey(action2)).isNotEqualTo(computeKey(action1));
   }
 
   @Test
@@ -125,4 +126,10 @@ public class CreateIncSymlinkActionTest extends FoundationTestCase {
     action.prepare(fileSystem, rootDirectory);
     assertThat(extra.exists()).isFalse();
   }
+
+  private String computeKey(CreateIncSymlinkAction action) {
+    Fingerprint fp = new Fingerprint();
+    action.computeKey(actionKeyContext, fp);
+    return fp.hexDigestAndReset();
+  }
 }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
index d221a772b3..bc206657e6 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/SkyframeAwareActionTest.java
@@ -209,8 +209,9 @@ public class SkyframeAwareActionTest extends TimestampBuilderTestCase {
     }
 
     @Override
-    protected String computeKey(ActionKeyContext actionKeyContext) {
-      return getPrimaryOutput().getExecPathString() + executionCounter.get();
+    protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
+      fp.addString(getPrimaryOutput().getExecPathString());
+      fp.addInt(executionCounter.get());
     }
   }
 
@@ -655,8 +656,8 @@ public class SkyframeAwareActionTest extends TimestampBuilderTestCase {
     }
 
     @Override
-    protected String computeKey(ActionKeyContext actionKeyContext) {
-      return new Fingerprint().addInt(42).hexDigestAndReset();
+    protected void computeKey(ActionKeyContext actionKeyContext, Fingerprint fp) {
+      fp.addInt(42);
     }
   }