Change WorkspaceStatusAction incrementality logic. We no longer manually invalidate the BUILD_INFO_KEY node on --workspace_status_command and related flag changes. Instead, the action has a supplier that allows it to retrieve the correct values at execution time.

This does not sacrifice correctness because the action executes unconditionally on every build, so it will never have stale data.

PiperOrigin-RevId: 200265375

7 files changed, 65 insertions, 136 deletions

diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
index 4e1d59e9e3..a78ccea927 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/BuildView.java
@@ -591,7 +591,6 @@ public class BuildView {
               target.getFirst(), target.getSecond(), aspectConfigurations.get(target)));
     }
 
-    skyframeExecutor.maybeInvalidateWorkspaceStatusValue(loadingResult.getWorkspaceName());
     SkyframeAnalysisResult skyframeAnalysisResult;
     try {
       skyframeAnalysisResult =
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/WorkspaceStatusAction.java b/src/main/java/com/google/devtools/build/lib/analysis/WorkspaceStatusAction.java
index bd08e3e561..d650252a4c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/WorkspaceStatusAction.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/WorkspaceStatusAction.java
@@ -15,7 +15,6 @@
 package com.google.devtools.build.lib.analysis;
 
 import com.google.common.base.Splitter;
-import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableMap;
 import com.google.devtools.build.lib.actions.AbstractAction;
 import com.google.devtools.build.lib.actions.ActionContext;
@@ -36,7 +35,6 @@ import java.io.IOException;
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
-import java.util.UUID;
 
 /**
  * An action writing the workspace status files.
@@ -184,13 +182,12 @@ public abstract class WorkspaceStatusAction extends AbstractAction {
     /**
      * Creates the workspace status action.
      *
-     * <p>If the objects returned for two builds are equals, the workspace status action can be
-     * be reused between them. Note that this only applies to the action object itself (the action
-     * will be unconditionally re-executed on every build)
+     * <p>The action will have a supplier inside it allowing it to access data that may change on
+     * every build. Since the action is unconditionally executed on each build, we don't recreate
+     * the action on every build, just re-executing and letting it read the updated data each time.
      */
     WorkspaceStatusAction createWorkspaceStatusAction(
-        ArtifactFactory artifactFactory, ArtifactOwner artifactOwner, Supplier<UUID> buildId,
-        String workspaceName);
+        ArtifactFactory artifactFactory, ArtifactOwner artifactOwner, String workspaceName);
 
     /**
      * Creates a dummy workspace status map. Used in cases where the build failed, so that part of
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
index 2434bff695..b462cb8312 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/BazelWorkspaceStatusModule.java
@@ -62,9 +62,7 @@ import com.google.devtools.common.options.OptionsBase;
 import java.io.IOException;
 import java.nio.charset.StandardCharsets;
 import java.util.Map;
-import java.util.Objects;
 import java.util.TreeMap;
-import java.util.UUID;
 
 /**
  * Provides information about the workspace (e.g. source control context, current machine, current
@@ -79,19 +77,18 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
   static class BazelWorkspaceStatusAction extends WorkspaceStatusAction {
     private final Artifact stableStatus;
     private final Artifact volatileStatus;
-    private final Options options;
+    private final Supplier<Options> options;
     private final String username;
     private final String hostname;
-    private final com.google.devtools.build.lib.shell.Command getWorkspaceStatusCommand;
-    private final ImmutableMap<String, String> clientEnv;
+    private final Supplier<ImmutableMap<String, String>> clientEnv;
 
     @SuppressWarnings("unused") // Read by serialization.
     private final Path workspace;
 
     @AutoCodec.VisibleForSerialization
     BazelWorkspaceStatusAction(
-        WorkspaceStatusAction.Options options,
-        ImmutableMap<String, String> clientEnv,
+        Supplier<Options> options,
+        Supplier<ImmutableMap<String, String>> clientEnv,
         Path workspace,
         Artifact stableStatus,
         Artifact volatileStatus,
@@ -106,37 +103,45 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
       this.username = USER_NAME.value();
       this.hostname = hostname;
       this.clientEnv = clientEnv;
-      this.getWorkspaceStatusCommand =
-          options.workspaceStatusCommand.equals(PathFragment.EMPTY_FRAGMENT)
-              ? null
-              : new CommandBuilder()
-                  .addArgs(options.workspaceStatusCommand.toString())
-                  // Pass client env, because certain SCM client(like
-                  // perforce, git) relies on environment variables to work
-                  // correctly.
-                  .setEnv(clientEnv)
-                  .setWorkingDir(workspace)
-                  .useShell(true)
-                  .build();
       this.workspace = workspace;
     }
 
-    private String getAdditionalWorkspaceStatus(ActionExecutionContext actionExecutionContext)
+    private com.google.devtools.build.lib.shell.Command getGetWorkspaceStatusCommand(
+        Options options, ImmutableMap<String, String> clientEnv) {
+      return options.workspaceStatusCommand.equals(PathFragment.EMPTY_FRAGMENT)
+          ? null
+          : new CommandBuilder()
+              .addArgs(options.workspaceStatusCommand.toString())
+              // Pass client env, because certain SCM client(like
+              // perforce, git) relies on environment variables to work
+              // correctly.
+              .setEnv(clientEnv)
+              .setWorkingDir(workspace)
+              .useShell(true)
+              .build();
+    }
+
+    private String getAdditionalWorkspaceStatus(
+        Options options,
+        ImmutableMap<String, String> clientEnv,
+        ActionExecutionContext actionExecutionContext)
         throws ActionExecutionException {
+      com.google.devtools.build.lib.shell.Command getWorkspaceStatusCommand =
+          getGetWorkspaceStatusCommand(options, clientEnv);
       try {
-        if (this.getWorkspaceStatusCommand != null) {
+        if (getWorkspaceStatusCommand != null) {
           actionExecutionContext
               .getEventHandler()
               .handle(
                   Event.progress(
                       "Getting additional workspace status by running "
                           + options.workspaceStatusCommand));
-          CommandResult result = this.getWorkspaceStatusCommand.execute();
+          CommandResult result = getWorkspaceStatusCommand.execute();
           if (result.getTerminationStatus().success()) {
             return new String(result.getStdout(), UTF_8);
           }
           throw new BadExitStatusException(
-              this.getWorkspaceStatusCommand,
+              getWorkspaceStatusCommand,
               result,
               "workspace status command failed: " + result.getTerminationStatus());
         }
@@ -193,9 +198,12 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
     @Override
     public ActionResult execute(ActionExecutionContext actionExecutionContext)
         throws ActionExecutionException {
+      Options options = this.options.get();
+      ImmutableMap<String, String> clientEnv = this.clientEnv.get();
       try {
-        Map<String, String> statusMap = parseWorkspaceStatus(
-            getAdditionalWorkspaceStatus(actionExecutionContext));
+        Map<String, String> statusMap =
+            parseWorkspaceStatus(
+                getAdditionalWorkspaceStatus(options, clientEnv, actionExecutionContext));
         Map<String, String> volatileMap = new TreeMap<>();
         Map<String, String> stableMap = new TreeMap<>();
 
@@ -210,7 +218,8 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
         stableMap.put(BuildInfo.BUILD_EMBED_LABEL, options.embedLabel);
         stableMap.put(BuildInfo.BUILD_HOST, hostname);
         stableMap.put(BuildInfo.BUILD_USER, username);
-        volatileMap.put(BuildInfo.BUILD_TIMESTAMP, Long.toString(getCurrentTimeMillis() / 1000));
+        volatileMap.put(
+            BuildInfo.BUILD_TIMESTAMP, Long.toString(getCurrentTimeMillis(clientEnv) / 1000));
 
         Map<String, String> overallMap = new TreeMap<>();
         overallMap.putAll(volatileMap);
@@ -242,7 +251,7 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
      * This method returns the current time for stamping, using SOURCE_DATE_EPOCH
      * (https://reproducible-builds.org/specs/source-date-epoch/) if provided.
      */
-    private long getCurrentTimeMillis() {
+    private static long getCurrentTimeMillis(ImmutableMap<String, String> clientEnv) {
       if (clientEnv.containsKey("SOURCE_DATE_EPOCH")) {
         String value = clientEnv.get("SOURCE_DATE_EPOCH").trim();
         if (!value.isEmpty()) {
@@ -257,27 +266,6 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
     }
 
     @Override
-    public boolean equals(Object o) {
-      if (!(o instanceof BazelWorkspaceStatusAction)) {
-        return false;
-      }
-
-      // We consider clientEnv in equality because we pass it when executing the workspace status
-      // command
-
-      BazelWorkspaceStatusAction that = (BazelWorkspaceStatusAction) o;
-      return this.clientEnv.equals(that.clientEnv)
-          && this.stableStatus.equals(that.stableStatus)
-          && this.volatileStatus.equals(that.volatileStatus)
-          && this.options.equals(that.options);
-    }
-
-    @Override
-    public int hashCode() {
-      return Objects.hash(clientEnv, stableStatus, volatileStatus, options);
-    }
-
-    @Override
     public String getMnemonic() {
       return "BazelWorkspaceStatusAction";
     }
@@ -315,8 +303,7 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
 
     @Override
     public WorkspaceStatusAction createWorkspaceStatusAction(
-        ArtifactFactory factory, ArtifactOwner artifactOwner, Supplier<UUID> buildId,
-        String workspaceName) {
+        ArtifactFactory factory, ArtifactOwner artifactOwner, String workspaceName) {
       ArtifactRoot root = env.getDirectories().getBuildDataDirectory(workspaceName);
 
       Artifact stableArtifact = factory.getDerivedArtifact(
@@ -325,24 +312,24 @@ public class BazelWorkspaceStatusModule extends BlazeModule {
           PathFragment.create("volatile-status.txt"), root, artifactOwner);
 
       return new BazelWorkspaceStatusAction(
-          options,
-          ImmutableMap.copyOf(env.getClientEnv()),
+          () -> options,
+          () -> ImmutableMap.copyOf(env.getClientEnv()),
           env.getDirectories().getWorkspace(),
           stableArtifact,
           volatileArtifact,
           getHostname());
     }
+  }
 
-    /**
-     * Returns cached short hostname.
-     *
-     * <p>Hostname lookup performs reverse DNS lookup which in bad cases can take seconds. To
-     * speedup builds we only lookup hostname once and cache the result. Therefore if hostname
-     * changes during bazel server lifetime, bazel will not see the change.
-     */
-    private String getHostname() {
-      return NetUtil.getCachedShortHostName();
-    }
+  /**
+   * Returns cached short hostname.
+   *
+   * <p>Hostname lookup performs reverse DNS lookup which in bad cases can take seconds. To speed up
+   * builds we only lookup hostname once and cache the result. Therefore if the hostname changes
+   * during bazel server lifetime, bazel will not see the change.
+   */
+  private static String getHostname() {
+    return NetUtil.getCachedShortHostName();
   }
 
   @ExecutionStrategy(contextType = WorkspaceStatusAction.Context.class)
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
index 1e0a4f0677..b7c9386bc3 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java
@@ -66,7 +66,6 @@ import com.google.devtools.build.lib.analysis.Dependency;
 import com.google.devtools.build.lib.analysis.ToolchainContext;
 import com.google.devtools.build.lib.analysis.TopLevelArtifactContext;
 import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
-import com.google.devtools.build.lib.analysis.WorkspaceStatusAction.Factory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory;
 import com.google.devtools.build.lib.analysis.buildinfo.BuildInfoFactory.BuildInfoKey;
 import com.google.devtools.build.lib.analysis.config.BuildConfiguration;
@@ -260,7 +259,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
   // would be preferable, but we have no way to have the Action depend on that value directly.
   // Having the BuildInfoFunction own the supplier is currently not possible either, because then
   // it would be invalidated on every build, since it would depend on the build id value.
-  private MutableSupplier<UUID> buildId = new MutableSupplier<>();
+  private final MutableSupplier<UUID> buildId = new MutableSupplier<>();
   private final ActionKeyContext actionKeyContext;
 
   protected boolean active = true;
@@ -332,7 +331,7 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
       FileSystem fileSystem,
       BlazeDirectories directories,
       ActionKeyContext actionKeyContext,
-      Factory workspaceStatusActionFactory,
+      WorkspaceStatusAction.Factory workspaceStatusActionFactory,
       ImmutableList<BuildInfoFactory> buildInfoFactories,
       ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions,
       ExternalFileAction externalFileAction,
@@ -809,22 +808,14 @@ public abstract class SkyframeExecutor implements WalkableGraphFactory {
     PrecomputedValue.DEFAULTS_PACKAGE_CONTENTS.set(injectable(), defaultsPackageContents);
   }
 
-  public void maybeInvalidateWorkspaceStatusValue(String workspaceName)
-      throws InterruptedException {
-    WorkspaceStatusAction newWorkspaceStatusAction = makeWorkspaceStatusAction(workspaceName);
-    WorkspaceStatusAction oldWorkspaceStatusAction = getLastWorkspaceStatusAction();
-    if (oldWorkspaceStatusAction != null
-        && !newWorkspaceStatusAction.equals(oldWorkspaceStatusAction)) {
-      // TODO(janakr): don't invalidate here, just use different keys for different configs. Can't
-      // be done right now because of lack of configuration trimming and fact that everything
-      // depends on workspace status action.
-      invalidate(WorkspaceStatusValue.BUILD_INFO_KEY::equals);
-    }
-  }
-
   private WorkspaceStatusAction makeWorkspaceStatusAction(String workspaceName) {
     return workspaceStatusActionFactory.createWorkspaceStatusAction(
-        artifactFactory.get(), WorkspaceStatusValue.BUILD_INFO_KEY, buildId, workspaceName);
+        artifactFactory.get(), WorkspaceStatusValue.BUILD_INFO_KEY, workspaceName);
+  }
+
+  @VisibleForTesting
+  public WorkspaceStatusAction.Factory getWorkspaceStatusActionFactoryForTesting() {
+    return workspaceStatusActionFactory;
   }
 
   @VisibleForTesting
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
index 41968ff240..9d14119cd7 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/AnalysisCachingTest.java
@@ -451,22 +451,6 @@ public class AnalysisCachingTest extends AnalysisCachingTestBase {
     update();
     assertContainsEvent("Heuristic sharding is intended as a one-off experimentation tool");
   }
-  
-  @Test
-  public void testWorkspaceStatusCommandIsNotCachedForNullBuild() throws Exception {
-    if (getInternalTestExecutionMode() != InternalTestExecutionMode.NORMAL) {
-      // TODO(b/66477180): maybe just ignore.
-      return;
-    }
-    update();
-    WorkspaceStatusAction actionA = getView().getLastWorkspaceBuildInfoActionForTesting();
-    assertThat(actionA.getMnemonic()).isEqualTo("DummyBuildInfoAction");
-
-    workspaceStatusActionFactory.setKey("Second");
-    update();
-    WorkspaceStatusAction actionB = getView().getLastWorkspaceBuildInfoActionForTesting();
-    assertThat(actionB.getMnemonic()).isEqualTo("DummyBuildInfoActionSecond");
-  }
 
   @Test
   public void testSkyframeCacheInvalidationBuildFileChange() throws Exception {
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 51c0fcf6e3..1464e08091 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
@@ -13,7 +13,6 @@
 // limitations under the License.
 package com.google.devtools.build.lib.analysis.util;
 
-import com.google.common.base.Supplier;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
 import com.google.common.collect.ImmutableSet;
@@ -63,7 +62,6 @@ import java.util.LinkedHashSet;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
-import java.util.UUID;
 import java.util.regex.Pattern;
 
 /**
@@ -207,17 +205,14 @@ public final class AnalysisTestUtil {
   /** A dummy WorkspaceStatusAction. */
   @Immutable
   public static final class DummyWorkspaceStatusAction extends WorkspaceStatusAction {
-    private final String key;
     private final Artifact stableStatus;
     private final Artifact volatileStatus;
 
-    public DummyWorkspaceStatusAction(String key,
-        Artifact stableStatus, Artifact volatileStatus) {
+    public DummyWorkspaceStatusAction(Artifact stableStatus, Artifact volatileStatus) {
       super(
           ActionOwner.SYSTEM_ACTION_OWNER,
           ImmutableList.<Artifact>of(),
           ImmutableList.of(stableStatus, volatileStatus));
-      this.key = key;
       this.stableStatus = stableStatus;
       this.volatileStatus = volatileStatus;
     }
@@ -236,7 +231,7 @@ public final class AnalysisTestUtil {
 
     @Override
     public String getMnemonic() {
-      return "DummyBuildInfoAction" + key;
+      return "DummyBuildInfoAction";
     }
 
     @Override
@@ -251,21 +246,6 @@ public final class AnalysisTestUtil {
     public Artifact getStableStatus() {
       return stableStatus;
     }
-
-    @Override
-    public boolean equals(Object o) {
-      if (!(o instanceof DummyWorkspaceStatusAction)) {
-        return false;
-      }
-
-      DummyWorkspaceStatusAction that = (DummyWorkspaceStatusAction) o;
-      return that.key.equals(this.key);
-    }
-
-    @Override
-    public int hashCode() {
-      return key.hashCode();
-    }
   }
 
   /** A WorkspaceStatusAction.Context that has no stable keys and no volatile keys. */
@@ -287,28 +267,21 @@ public final class AnalysisTestUtil {
    */
   public static class DummyWorkspaceStatusActionFactory implements WorkspaceStatusAction.Factory {
     private final BlazeDirectories directories;
-    private String key;
 
     public DummyWorkspaceStatusActionFactory(BlazeDirectories directories) {
       this.directories = directories;
-      this.key = "";
-    }
-
-    public void setKey(String key) {
-      this.key = key;
     }
 
     @Override
     public WorkspaceStatusAction createWorkspaceStatusAction(
-        ArtifactFactory artifactFactory, ArtifactOwner artifactOwner, Supplier<UUID> buildId,
-        String workspaceName) {
+        ArtifactFactory artifactFactory, ArtifactOwner artifactOwner, String workspaceName) {
       Artifact stableStatus = artifactFactory.getDerivedArtifact(
           PathFragment.create("build-info.txt"),
           directories.getBuildDataDirectory(workspaceName), artifactOwner);
       Artifact volatileStatus = artifactFactory.getConstantMetadataArtifact(
           PathFragment.create("build-changelist.txt"),
           directories.getBuildDataDirectory(workspaceName), artifactOwner);
-      return new DummyWorkspaceStatusAction(key, stableStatus, volatileStatus);
+      return new DummyWorkspaceStatusAction(stableStatus, volatileStatus);
     }
 
     @Override
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 45e39349d6..a243790ca3 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
@@ -900,8 +900,6 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
             .build());
 
     invalidatePackages();
-    // Need to re-initialize the workspace status.
-    getSkyframeExecutor().maybeInvalidateWorkspaceStatusValue("test");
   }
 
   /**