Delete top-level rule transition logic from ConfiguredTarget test code.
This is no longer needed now that rule transitions are always applied
by ConfiguredTargetFunction.
* Cleans up references to no longer existing PrepareAnalysisPhaseFunction.
PiperOrigin-RevId: 535905468
Change-Id: I2733a2148fa9f070a0ce5c1f85b80368dde89ffa
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
index cec2f00..c3891df 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/AnalysisUtils.java
@@ -180,7 +180,6 @@
*
* <p>Preserves the original input ordering.
*/
- // Keep this in sync with PrepareAnalysisPhaseFunction.
public static TopLevelTargetsAndConfigsResult getTargetsWithConfigs(
BuildConfigurationValue targetConfiguration,
Collection<Target> targets,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
index fbe68d4..a7b99a5 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/ConfigurationResolver.java
@@ -437,7 +437,6 @@
// should never make it through analysis (and especially not seed ConfiguredTargetValues)
// TODO(gregce): merge this more with resolveConfigurations? One crucial difference is
// resolveConfigurations can null-return on missing deps since it executes inside Skyfunctions.
- // Keep this in sync with {@link PrepareAnalysisPhaseFunction#resolveConfigurations}.
public static TopLevelTargetsAndConfigsResult getConfigurationsFromExecutor(
Iterable<TargetAndConfiguration> defaultContext,
Multimap<BuildConfigurationValue, DependencyKey> targetsToEvaluate,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java b/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java
index ae4338d..c83cff9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/producers/TargetAndConfigurationProducer.java
@@ -164,30 +164,47 @@
// this. It won't be possible afterwards because null configuration keys will only be used for
// visibility dependencies.
BuildConfigurationKey configurationKey = preRuleTransitionKey.getConfigurationKey();
- if ((configurationKey != null) != target.isConfigurable()) {
- // We somehow ended up in a target that requires a non-null configuration as a dependency of
- // one that requires a null configuration or the other way round. This is always an error, but
- // we need to analyze the dependencies of the latter target to realize that. Short-circuit the
- // evaluation to avoid doing useless work and running code with a null configuration that's
- // not prepared for it.
- sink.acceptTargetAndConfigurationError(
- TargetAndConfigurationError.of(new InconsistentNullConfigException()));
- return DONE;
- }
-
- // TODO(b/261521010): after removing the rule transition from dependency resolution, the logic
- // here changes.
- //
- // A null configuration key will only be used for visibility dependencies so when that's true, a
- // check that the target is a PackageGroup will be performed, throwing
- // InvalidVisibilityDependencyException on failure.
if (configurationKey == null) {
+ if (target.isConfigurable()) {
+ // We somehow ended up in a target that requires a non-null configuration but with a key
+ // that doesn't have a configuration. This is always an error, but we need to analyze the
+ // dependencies of the latter target to realize that. Short-circuit the evaluation to avoid
+ // doing useless work and running code with a null configuration that's not prepared for it.
+ sink.acceptTargetAndConfigurationError(
+ TargetAndConfigurationError.of(new InconsistentNullConfigException()));
+ return DONE;
+ }
+ // TODO(b/261521010): after removing the rule transition from dependency resolution, the logic
+ // here changes.
+ //
+ // A null configuration key will only be used for visibility dependencies so when that's
+ // true, a check that the target is a PackageGroup will be performed, throwing
+ // InvalidVisibilityDependencyException on failure.
+ //
// The ConfiguredTargetKey cannot fan-in in this case.
sink.acceptTargetAndConfiguration(
new TargetAndConfiguration(target, /* configuration= */ null), preRuleTransitionKey);
return DONE;
}
+ // This may happen for top-level ConfiguredTargets.
+ //
+ // TODO(b/261521010): this may also happen for targets that are not top-level after removing
+ // rule transitions from dependency resolution. Update this comment.
+ if (!target.isConfigurable()) {
+ var nullConfiguredTargetKey =
+ ConfiguredTargetKey.builder().setDelegate(preRuleTransitionKey).build();
+ ActionLookupKey delegate = nullConfiguredTargetKey.toKey();
+ if (!delegate.equals(preRuleTransitionKey)) {
+ // Delegates to the key that already owns the null configuration.
+ delegateTo(tasks, delegate);
+ return DONE;
+ }
+ sink.acceptTargetAndConfiguration(
+ new TargetAndConfiguration(target, /* configuration= */ null), nullConfiguredTargetKey);
+ return DONE;
+ }
+
return new RuleTransitionConfigurationProducer();
}
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 b4eaf60..2e055bf 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
@@ -1747,7 +1747,7 @@
ExtendedEventHandler eventHandler,
BuildConfigurationValue originalConfig,
Iterable<DependencyKey> keys)
- throws TransitionException, InvalidConfigurationException, InterruptedException {
+ throws InvalidConfigurationException, InterruptedException {
return getConfiguredTargetMapForTesting(eventHandler, originalConfig, keys).values().asList();
}
@@ -1996,7 +1996,6 @@
*
* <p>Skips targets with loading phase errors.
*/
- // Keep this in sync with {@link PrepareAnalysisPhaseFunction#getConfigurations}.
// TODO(ulfjack): Remove this legacy method after switching to the Skyframe-based implementation.
@Override
public ConfigurationsResult getConfigurations(
@@ -2205,22 +2204,12 @@
@VisibleForTesting
@Nullable
public ConfiguredTarget getConfiguredTargetForTesting(
- ExtendedEventHandler eventHandler, Label label, BuildConfigurationValue configuration)
- throws TransitionException, InvalidConfigurationException, InterruptedException {
- return getConfiguredTargetForTesting(eventHandler, label, configuration, NoTransition.INSTANCE);
- }
-
- /** Returns a particular configured target after applying the given transition. */
- @VisibleForTesting
- @Nullable
- public ConfiguredTarget getConfiguredTargetForTesting(
ExtendedEventHandler eventHandler,
Label label,
- BuildConfigurationValue configuration,
- ConfigurationTransition transition)
- throws TransitionException, InvalidConfigurationException, InterruptedException {
+ @Nullable BuildConfigurationValue configuration)
+ throws InvalidConfigurationException, InterruptedException {
ConfiguredTargetAndData configuredTargetAndData =
- getConfiguredTargetAndDataForTesting(eventHandler, label, configuration, transition);
+ getConfiguredTargetAndDataForTesting(eventHandler, label, configuration);
return configuredTargetAndData == null ? null : configuredTargetAndData.getConfiguredTarget();
}
@@ -2229,29 +2218,19 @@
public ConfiguredTargetAndData getConfiguredTargetAndDataForTesting(
ExtendedEventHandler eventHandler,
Label label,
- BuildConfigurationValue configuration,
- ConfigurationTransition transition)
- throws TransitionException, InvalidConfigurationException, InterruptedException {
-
- ConfigurationTransition transition1 =
- configuration == null ? NullTransition.INSTANCE : transition;
+ @Nullable BuildConfigurationValue configuration)
+ throws InvalidConfigurationException, InterruptedException {
DependencyKey dependencyKey =
- DependencyKey.builder().setLabel(label).setTransition(transition1).build();
+ DependencyKey.builder()
+ .setLabel(label)
+ .setTransition(configuration == null ? NullTransition.INSTANCE : NoTransition.INSTANCE)
+ .build();
return Iterables.getFirst(
getConfiguredTargetsForTesting(
eventHandler, configuration, ImmutableList.of(dependencyKey)),
null);
}
- @VisibleForTesting
- @Nullable
- public ConfiguredTargetAndData getConfiguredTargetAndDataForTesting(
- ExtendedEventHandler eventHandler, Label label, BuildConfigurationValue configuration)
- throws TransitionException, InvalidConfigurationException, InterruptedException {
- return getConfiguredTargetAndDataForTesting(
- eventHandler, label, configuration, NoTransition.INSTANCE);
- }
-
/**
* Invalidates Skyframe values corresponding to the given set of modified files under the given
* path entry.
@@ -2823,6 +2802,11 @@
return statusReporterRef.get();
}
+ @VisibleForTesting
+ public void clearEmittedEventStateForTesting() {
+ emittedEventState.clear();
+ }
+
/**
* Initializes and syncs the graph with the given options, readying it for the next evaluation.
*
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
index 643573d..ae534b6 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/AnalysisTestCase.java
@@ -43,11 +43,7 @@
import com.google.devtools.build.lib.analysis.config.BuildConfigurationValue;
import com.google.devtools.build.lib.analysis.config.BuildOptions;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
-import com.google.devtools.build.lib.analysis.config.TransitionResolver;
-import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
-import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.configuredtargets.InputFileConfiguredTarget;
-import com.google.devtools.build.lib.analysis.starlark.StarlarkTransition;
import com.google.devtools.build.lib.bazel.bzlmod.BazelLockFileFunction;
import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleResolutionFunction;
import com.google.devtools.build.lib.bazel.bzlmod.FakeRegistry;
@@ -61,8 +57,6 @@
import com.google.devtools.build.lib.cmdline.LabelSyntaxException;
import com.google.devtools.build.lib.exec.ExecutionOptions;
import com.google.devtools.build.lib.packages.NativeAspectClass;
-import com.google.devtools.build.lib.packages.NoSuchPackageException;
-import com.google.devtools.build.lib.packages.NoSuchTargetException;
import com.google.devtools.build.lib.packages.PackageFactory;
import com.google.devtools.build.lib.packages.Target;
import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions;
@@ -553,9 +547,7 @@
}
try {
return skyframeExecutor.getConfiguredTargetAndDataForTesting(reporter, parsedLabel, config);
- } catch (StarlarkTransition.TransitionException
- | InvalidConfigurationException
- | InterruptedException e) {
+ } catch (InvalidConfigurationException | InterruptedException e) {
throw new AssertionError(e);
}
}
@@ -603,22 +595,9 @@
throw new AssertionError(e);
}
try {
- // Need to emulate the activities of the top-level trimming transition since not going
- // through sufficiently normal evaluation channels.
- Target target = skyframeExecutor.getPackageManager().getTarget(reporter, parsedLabel);
- ConfigurationTransition transition =
- TransitionResolver.evaluateTransition(
- configuration,
- NoTransition.INSTANCE,
- target,
- ruleClassProvider.getTrimmingTransitionFactory());
return skyframeExecutor.getConfiguredTargetAndDataForTesting(
- reporter, parsedLabel, configuration, transition);
- } catch (StarlarkTransition.TransitionException
- | InvalidConfigurationException
- | InterruptedException
- | NoSuchPackageException
- | NoSuchTargetException e) {
+ reporter, parsedLabel, configuration);
+ } catch (InvalidConfigurationException | InterruptedException e) {
throw new AssertionError(e);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
index aa2c41c..64c494c 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewForTesting.java
@@ -71,9 +71,6 @@
import com.google.devtools.build.lib.analysis.config.ConfigurationResolver;
import com.google.devtools.build.lib.analysis.config.DependencyEvaluationException;
import com.google.devtools.build.lib.analysis.config.InvalidConfigurationException;
-import com.google.devtools.build.lib.analysis.config.TransitionResolver;
-import com.google.devtools.build.lib.analysis.config.transitions.ConfigurationTransition;
-import com.google.devtools.build.lib.analysis.config.transitions.NoTransition;
import com.google.devtools.build.lib.analysis.configuredtargets.MergedConfiguredTarget;
import com.google.devtools.build.lib.analysis.constraints.IncompatibleTargetChecker.IncompatibleTargetException;
import com.google.devtools.build.lib.analysis.platform.ConstraintValueInfo;
@@ -518,30 +515,6 @@
return result;
}
- private ConfigurationTransition getTopLevelTransitionForTarget(
- Label label, BuildConfigurationValue config, ExtendedEventHandler handler) {
- Target target;
- try {
- target = skyframeExecutor.getPackageManager().getTarget(handler, label);
- } catch (NoSuchPackageException | NoSuchTargetException e) {
- // TODO(bazel-team): refactor this method so we actually throw an exception here (likely
- // {@link TransitionException}. Every version of getConfiguredTarget runs through this
- // method and many test cases rely on not erroring out here so be able to reach an error
- // later on.
- return NoTransition.INSTANCE;
- } catch (InterruptedException e) {
- Thread.currentThread().interrupt();
- throw new AssertionError("Configuration of " + label + " interrupted");
- }
- // Return early if a rule target is in error. We don't want whatever caused the rule error to
- // also cause problems in computing the transition (e.g. an unchecked exception).
- if (target instanceof Rule && ((Rule) target).containsErrors()) {
- return null;
- }
- return TransitionResolver.evaluateTransition(
- config, NoTransition.INSTANCE, target, ruleClassProvider.getTrimmingTransitionFactory());
- }
-
/**
* Returns a configured target for the specified target and configuration. If the target in
* question has a top-level rule class transition, that transition is applied in the returned
@@ -551,27 +524,14 @@
*/
public ConfiguredTarget getConfiguredTargetForTesting(
ExtendedEventHandler eventHandler, Label label, BuildConfigurationValue config)
- throws StarlarkTransition.TransitionException, InvalidConfigurationException,
- InterruptedException {
- ConfigurationTransition transition =
- getTopLevelTransitionForTarget(label, config, eventHandler);
- if (transition == null) {
- return null;
- }
- return skyframeExecutor.getConfiguredTargetForTesting(eventHandler, label, config, transition);
+ throws InvalidConfigurationException, InterruptedException {
+ return skyframeExecutor.getConfiguredTargetForTesting(eventHandler, label, config);
}
ConfiguredTargetAndData getConfiguredTargetAndDataForTesting(
ExtendedEventHandler eventHandler, Label label, BuildConfigurationValue config)
- throws StarlarkTransition.TransitionException, InvalidConfigurationException,
- InterruptedException {
- ConfigurationTransition transition =
- getTopLevelTransitionForTarget(label, config, eventHandler);
- if (transition == null) {
- return null;
- }
- return skyframeExecutor.getConfiguredTargetAndDataForTesting(
- eventHandler, label, config, transition);
+ throws InvalidConfigurationException, InterruptedException {
+ return skyframeExecutor.getConfiguredTargetAndDataForTesting(eventHandler, label, config);
}
/**
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 a2aab1e..0c69cdc 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
@@ -1056,9 +1056,7 @@
protected ConfiguredTarget getConfiguredTarget(Label label, BuildConfigurationValue config) {
try {
return view.getConfiguredTargetForTesting(reporter, label, config);
- } catch (InvalidConfigurationException
- | StarlarkTransition.TransitionException
- | InterruptedException e) {
+ } catch (InvalidConfigurationException | InterruptedException e) {
throw new AssertionError(e);
}
}
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
index e9e208f..25c2499 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkIntegrationTest.java
@@ -2987,7 +2987,10 @@
assertContainsEvent("bad transition");
// Try #2: make sure the cache doesn't suppress the error message.
+ invalidatePackages();
+ skyframeExecutor.clearEmittedEventStateForTesting();
eventCollector.clear();
+
getConfiguredTarget("//test:mytarget");
assertContainsEvent("bad transition");
}
diff --git a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
index 71dbb7c..6c84ba2 100644
--- a/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
+++ b/src/test/java/com/google/devtools/build/lib/starlark/StarlarkRuleImplementationFunctionsTest.java
@@ -27,7 +27,7 @@
import com.google.common.collect.Lists;
import com.google.devtools.build.lib.actions.Action;
import com.google.devtools.build.lib.actions.ActionAnalysisMetadata;
-import com.google.devtools.build.lib.actions.ActionLookupKey;
+import com.google.devtools.build.lib.actions.ActionLookupKeyOrProxy;
import com.google.devtools.build.lib.actions.Artifact;
import com.google.devtools.build.lib.actions.Artifact.ArtifactExpander;
import com.google.devtools.build.lib.actions.Artifact.DerivedArtifact;
@@ -3167,7 +3167,7 @@
DerivedArtifact.create(
artifact.getRoot(),
artifact.getExecPath().getRelative(file),
- (ActionLookupKey) artifact.getArtifactOwner()));
+ (ActionLookupKeyOrProxy) artifact.getArtifactOwner()));
}
};
}
