Stop injecting WorkspaceStatusAction into the Skyframe graph as a precomputed value. Instead, manually check if the value has changed, and if it has, invalidate its consuming WorkspaceStatusValue node, forcing its re-evaluation, where it will pick up the new value.

This seems more awkward than the original code, but it is more correct in spirit: injecting a precomputed value which can change even while the source state does not is a smell. Long-term, the key for the WorkspaceStatusValue should incorporate a hash of the action, and that hash should be in the configuration, just as other configuration flags are. That isn't possible right now just because we don't have configuration trimming, and we drop all nodes on configuration changes, so putting workspace status options into the configuration would lose change pruning whenever we changed workspace status options.

If/when those problems are fixed, we can extend this change to have WorkspaceStatusFunction continue to request the action out-of-band, but keyed by the hash. Then we can stop invalidating stale nodes.

See also https://github.com/bazelbuild/bazel/issues/3785.

PiperOrigin-RevId: 169947071

5 files changed, 14 insertions, 16 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index faee80dd01..5935aa6351 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -712,15 +712,14 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   }
 
   /**
-   * Returns the ConfiguredTarget for the specified label, using the
-   * given build configuration. If the label corresponds to a target with a top-level configuration
-   * transition, that transition is applied to the given config in the returned ConfiguredTarget.
+   * Returns the ConfiguredTarget for the specified label, using the given build configuration. If
+   * the label corresponds to a target with a top-level configuration transition, that transition is
+   * applied to the given config in the returned ConfiguredTarget.
    *
-   * <p>If the evaluation of the SkyKey corresponding to the configured target fails, this
-   * method may return null.  In that case, use a debugger to inspect the {@link ErrorInfo}
-   * for the evaluation, which is produced by the
-   * {@link MemoizingEvaluator#getExistingValueForTesting} call in
-   * {@link SkyframeExecutor#getConfiguredTargetForTesting}.  See also b/26382502.
+   * <p>If the evaluation of the SkyKey corresponding to the configured target fails, this method
+   * may return null. In that case, use a debugger to inspect the {@link ErrorInfo} for the
+   * evaluation, which is produced by the {@link MemoizingEvaluator#getExistingValue} call in {@link
+   * SkyframeExecutor#getConfiguredTargetForTesting}. See also b/26382502.
    */
   protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfiguration config) {
     return view.getConfiguredTargetForTesting(reporter, BlazeTestUtils.convertLabel(label), config);
@@ -768,7 +767,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
 
     invalidatePackages();
     // Need to re-initialize the workspace status.
-    getSkyframeExecutor().injectWorkspaceStatusData("test");
+    getSkyframeExecutor().maybeInvalidateWorkspaceStatusValue("test");
   }
 
   /**
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
index 5e2dba77c8..673fcfb2b8 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ArtifactFunctionTest.java
@@ -40,7 +40,6 @@ import com.google.devtools.build.lib.actions.util.TestAction.DummyAction;
 import com.google.devtools.build.lib.events.NullEventHandler;
 import com.google.devtools.build.lib.util.Pair;
 import com.google.devtools.build.lib.vfs.FileStatus;
-import com.google.devtools.build.lib.vfs.FileSystem.HashFunction;
 import com.google.devtools.build.lib.vfs.FileSystemUtils;
 import com.google.devtools.build.lib.vfs.Path;
 import com.google.devtools.build.lib.vfs.PathFragment;
@@ -461,7 +460,7 @@ public class ArtifactFunctionTest extends ArtifactFunctionTestCase {
   }
 
   private void setGeneratingActions() {
-    if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
+    if (evaluator.getExistingValue(OWNER_KEY) == null) {
       differencer.inject(
           ImmutableMap.of(
               OWNER_KEY,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
index b243828490..22033a3c89 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TimestampBuilderTestCase.java
@@ -227,7 +227,7 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase {
 
     return new Builder() {
       private void setGeneratingActions() {
-        if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
+        if (evaluator.getExistingValue(OWNER_KEY) == null) {
           differencer.inject(
               ImmutableMap.of(
                   OWNER_KEY, new ActionLookupValue(ImmutableList.copyOf(actions), false)));
@@ -253,8 +253,8 @@ public abstract class TimestampBuilderTestCase extends FoundationTestCase {
         skyframeActionExecutor.prepareForExecution(
             reporter,
             executor,
-            keepGoing, /*explain=*/
-            false,
+            keepGoing,
+            /*explain=*/ false,
             new ActionCacheChecker(actionCache, null, ALWAYS_EXECUTE_FILTER, null),
             null);
         skyframeActionExecutor.setActionExecutionProgressReportingObjects(
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
index b1d8b087ab..6b53a00383 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/TreeArtifactMetadataTest.java
@@ -224,7 +224,7 @@ public class TreeArtifactMetadataTest extends ArtifactFunctionTestCase {
   }
 
   private void setGeneratingActions() {
-    if (evaluator.getExistingValueForTesting(OWNER_KEY) == null) {
+    if (evaluator.getExistingValue(OWNER_KEY) == null) {
       differencer.inject(
           ImmutableMap.of(
               OWNER_KEY,
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
index 21ba998d8f..30b0076010 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/util/SkyframeExecutorTestUtils.java
@@ -50,7 +50,7 @@ public class SkyframeExecutorTestUtils {
    */
   @Nullable
   public static SkyValue getExistingValue(SkyframeExecutor skyframeExecutor, SkyKey key) {
-    return skyframeExecutor.getEvaluatorForTesting().getExistingValueForTesting(key);
+    return skyframeExecutor.getEvaluatorForTesting().getExistingValue(key);
   }
 
   /**