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
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 4e1d59e..a78ccea 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 @@ 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 bd08e3e..d650252 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.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 @@ /** * 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 2434bff..b462cb8 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 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 @@ 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 @@ 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 @@ @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 @@ 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 @@ * 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 @@ } @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 @@ @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 @@ 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 1e0a4f0..b7c9386 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.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 @@ // 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 @@ FileSystem fileSystem, BlazeDirectories directories, ActionKeyContext actionKeyContext, - Factory workspaceStatusActionFactory, + WorkspaceStatusAction.Factory workspaceStatusActionFactory, ImmutableList<BuildInfoFactory> buildInfoFactories, ImmutableMap<SkyFunctionName, SkyFunction> extraSkyFunctions, ExternalFileAction externalFileAction, @@ -809,22 +808,14 @@ 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 41968ff..9d14119 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 @@ 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 51c0fcf..1464e08 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.List; import java.util.Map; import java.util.Set; -import java.util.UUID; import java.util.regex.Pattern; /** @@ -207,17 +205,14 @@ /** 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 @@ @Override public String getMnemonic() { - return "DummyBuildInfoAction" + key; + return "DummyBuildInfoAction"; } @Override @@ -251,21 +246,6 @@ 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 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 45e3934..a243790 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 @@ .build()); invalidatePackages(); - // Need to re-initialize the workspace status. - getSkyframeExecutor().maybeInvalidateWorkspaceStatusValue("test"); } /**