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
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 9abeb1a..cd641b1 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
@@ -326,7 +326,7 @@
@VisibleForTesting
WorkspaceStatusAction getLastWorkspaceBuildInfoActionForTesting() {
- return skyframeExecutor.getLastWorkspaceStatusActionForTesting();
+ return skyframeExecutor.getLastWorkspaceStatusAction();
}
@Override
@@ -593,7 +593,7 @@
target.getFirst(), target.getSecond(), aspectConfigurations.get(target)));
}
- skyframeExecutor.injectWorkspaceStatusData(loadingResult.getWorkspaceName());
+ skyframeExecutor.maybeInvalidateWorkspaceStatusValue(loadingResult.getWorkspaceName());
SkyframeAnalysisResult skyframeAnalysisResult;
try {
skyframeAnalysisResult =
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
index 5f7c242..1b7ccc9 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/PrecomputedValue.java
@@ -19,7 +19,6 @@
import com.google.common.collect.ImmutableList;
import com.google.common.collect.ImmutableMap;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
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.packages.RuleVisibility;
@@ -95,9 +94,6 @@
static final Precomputed<Map<String, String>> ACTION_ENV =
new Precomputed<>(LegacySkyKey.create(SkyFunctions.PRECOMPUTED, "action_env"));
- static final Precomputed<WorkspaceStatusAction> WORKSPACE_STATUS_KEY =
- new Precomputed<>(LegacySkyKey.create(SkyFunctions.PRECOMPUTED, "workspace_status_action"));
-
static final Precomputed<ImmutableList<ActionAnalysisMetadata>> COVERAGE_REPORT_KEY =
new Precomputed<>(LegacySkyKey.create(SkyFunctions.PRECOMPUTED, "coverage_report_actions"));
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 08eea1f..17bdeb1 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
@@ -441,7 +441,9 @@
SkyFunctions.BUILD_INFO_COLLECTION,
new BuildInfoCollectionFunction(
artifactFactory, buildInfoFactories, removeActionsAfterEvaluation));
- map.put(SkyFunctions.BUILD_INFO, new WorkspaceStatusFunction(removeActionsAfterEvaluation));
+ map.put(
+ SkyFunctions.BUILD_INFO,
+ new WorkspaceStatusFunction(removeActionsAfterEvaluation, this::makeWorkspaceStatusAction));
map.put(SkyFunctions.COVERAGE_REPORT, new CoverageReportFunction(removeActionsAfterEvaluation));
ActionExecutionFunction actionExecutionFunction =
new ActionExecutionFunction(skyframeActionExecutor, tsgm);
@@ -695,10 +697,31 @@
PrecomputedValue.DEFAULTS_PACKAGE_CONTENTS.set(injectable(), defaultsPackageContents);
}
- public void injectWorkspaceStatusData(String workspaceName) {
- PrecomputedValue.WORKSPACE_STATUS_KEY.set(injectable(),
- workspaceStatusActionFactory.createWorkspaceStatusAction(
- artifactFactory.get(), WorkspaceStatusValue.ARTIFACT_OWNER, buildId, workspaceName));
+ public void maybeInvalidateWorkspaceStatusValue(String workspaceName) {
+ 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.SKY_KEY::equals);
+ }
+ }
+
+ private WorkspaceStatusAction makeWorkspaceStatusAction(String workspaceName) {
+ return workspaceStatusActionFactory.createWorkspaceStatusAction(
+ artifactFactory.get(), WorkspaceStatusValue.ARTIFACT_OWNER, buildId, workspaceName);
+ }
+
+ @VisibleForTesting
+ @Nullable
+ public WorkspaceStatusAction getLastWorkspaceStatusAction() {
+ WorkspaceStatusValue workspaceStatusValue =
+ (WorkspaceStatusValue) memoizingEvaluator.getExistingValue(WorkspaceStatusValue.SKY_KEY);
+ return workspaceStatusValue == null
+ ? null
+ : (WorkspaceStatusAction) workspaceStatusValue.getAction(0);
}
public void injectCoverageReportData(ImmutableList<ActionAnalysisMetadata> actions) {
@@ -808,13 +831,6 @@
}
@VisibleForTesting
- public WorkspaceStatusAction getLastWorkspaceStatusActionForTesting() {
- PrecomputedValue value = (PrecomputedValue) buildDriver.getGraphForTesting()
- .getExistingValueForTesting(PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting());
- return (WorkspaceStatusAction) value.get();
- }
-
- @VisibleForTesting
public ToolchainContext getToolchainContextForTesting(
Set<Label> requiredToolchains, BuildConfiguration config, ExtendedEventHandler eventHandler)
throws ToolchainContextException, InterruptedException {
@@ -1547,11 +1563,6 @@
Label label,
BuildConfiguration configuration,
Attribute.Transition transition) {
- if (memoizingEvaluator.getExistingValueForTesting(
- PrecomputedValue.WORKSPACE_STATUS_KEY.getKeyForTesting()) == null) {
- injectWorkspaceStatusData(label.getWorkspaceRoot());
- }
-
return Iterables.getFirst(
getConfiguredTargets(
eventHandler,
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
index d665632..5bed654 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/WorkspaceStatusFunction.java
@@ -13,31 +13,42 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
-import com.google.common.base.Supplier;
import com.google.devtools.build.lib.analysis.WorkspaceStatusAction;
import com.google.devtools.build.lib.util.Preconditions;
import com.google.devtools.build.skyframe.SkyFunction;
import com.google.devtools.build.skyframe.SkyKey;
import com.google.devtools.build.skyframe.SkyValue;
+import java.util.function.Supplier;
/** Creates the workspace status artifacts and action. */
public class WorkspaceStatusFunction implements SkyFunction {
- private final Supplier<Boolean> removeActionAfterEvaluation;
+ interface WorkspaceStatusActionFactory {
+ WorkspaceStatusAction create(String workspaceName);
+ }
- WorkspaceStatusFunction(Supplier<Boolean> removeActionAfterEvaluation) {
+ private final Supplier<Boolean> removeActionAfterEvaluation;
+ private final WorkspaceStatusActionFactory workspaceStatusActionFactory;
+
+ WorkspaceStatusFunction(
+ Supplier<Boolean> removeActionAfterEvaluation,
+ WorkspaceStatusActionFactory workspaceStatusActionFactory) {
this.removeActionAfterEvaluation = Preconditions.checkNotNull(removeActionAfterEvaluation);
+ this.workspaceStatusActionFactory = workspaceStatusActionFactory;
}
@Override
public SkyValue compute(SkyKey skyKey, Environment env) throws InterruptedException {
Preconditions.checkState(
WorkspaceStatusValue.SKY_KEY.equals(skyKey), WorkspaceStatusValue.SKY_KEY);
-
- WorkspaceStatusAction action = PrecomputedValue.WORKSPACE_STATUS_KEY.get(env);
- if (action == null) {
+ WorkspaceNameValue workspaceNameValue =
+ (WorkspaceNameValue) env.getValue(WorkspaceNameValue.key());
+ if (env.valuesMissing()) {
return null;
}
+ WorkspaceStatusAction action =
+ workspaceStatusActionFactory.create(workspaceNameValue.getName());
+
return new WorkspaceStatusValue(
action.getStableStatus(),
action.getVolatileStatus(),
diff --git a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
index f0bc14a5..618c4d1 100644
--- a/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/InMemoryMemoizingEvaluator.java
@@ -281,7 +281,8 @@
}
@Override
- @Nullable public SkyValue getExistingValueForTesting(SkyKey key) {
+ @Nullable
+ public SkyValue getExistingValue(SkyKey key) {
NodeEntry entry = getExistingEntryForTesting(key);
try {
return isDone(entry) ? entry.getValue() : null;
diff --git a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
index 2fe92fa..889c9df 100644
--- a/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
+++ b/src/main/java/com/google/devtools/build/skyframe/MemoizingEvaluator.java
@@ -86,7 +86,7 @@
*
* <p>The returned map may be a live view of the graph.
*/
- // TODO(bazel-team): Replace all usages of getValues, getDoneValues, getExistingValueForTesting,
+ // TODO(bazel-team): Replace all usages of getValues, getDoneValues, getExistingValue,
// and getExistingErrorForTesting with usages of WalkableGraph. Changing the getValues usages
// require some care because getValues gives access to the previous value for changed/dirty nodes.
Map<SkyKey, SkyValue> getValues();
@@ -108,12 +108,12 @@
/**
* Returns a value if and only if an earlier call to {@link #evaluate} created it; null otherwise.
*
- * <p>This method should only be used by tests that need to verify the presence of a value in the
- * graph after an {@link #evaluate} call.
+ * <p>This method should mainly be used by tests that need to verify the presence of a value in
+ * the graph after an {@link #evaluate} call.
*/
@VisibleForTesting
@Nullable
- SkyValue getExistingValueForTesting(SkyKey key);
+ SkyValue getExistingValue(SkyKey key);
/**
* Returns an error if and only if an earlier call to {@link #evaluate} created it; null
diff --git a/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java b/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
index 988412b..82b4ecb 100644
--- a/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
+++ b/src/main/java/com/google/devtools/build/skyframe/SequentialBuildDriver.java
@@ -60,7 +60,7 @@
@Nullable
@Override
public SkyValue getExistingValueForTesting(SkyKey key) {
- return memoizingEvaluator.getExistingValueForTesting(key);
+ return memoizingEvaluator.getExistingValue(key);
}
@Nullable
