Don't force targets to the null configuration if they shouldn't be in it.
Instead, short circuit their analysis and rely on whoever depended on them to realize that they got an EmptyConfiguredTarget and raise an appropriate error.
RELNOTES: None.
PiperOrigin-RevId: 236793796
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 6d3e5fa..40e82db 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
@@ -23,6 +23,8 @@
import com.google.devtools.build.lib.analysis.config.BuildConfigurationCollection;
import com.google.devtools.build.lib.analysis.config.ConfigurationResolver;
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.skylark.StarlarkTransition.TransitionException;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.ExtendedEventHandler;
@@ -212,13 +214,18 @@
Multimap<BuildConfiguration, Dependency> asDeps =
ArrayListMultimap.<BuildConfiguration, Dependency>create();
for (TargetAndConfiguration targetAndConfig : nodes) {
+ ConfigurationTransition transition =
+ TransitionResolver.evaluateTransition(
+ targetAndConfig.getConfiguration(),
+ NoTransition.INSTANCE,
+ targetAndConfig.getTarget(),
+ ruleClassProvider.getTrimmingTransitionFactory());
if (targetAndConfig.getConfiguration() != null) {
asDeps.put(
targetAndConfig.getConfiguration(),
Dependency.withTransitionAndAspects(
targetAndConfig.getLabel(),
- TransitionResolver.evaluateTopLevelTransition(
- targetAndConfig, ruleClassProvider.getTrimmingTransitionFactory()),
+ transition,
// TODO(bazel-team): support top-level aspects
AspectCollection.EMPTY));
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
index cbf9fc9..be377fc 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java
@@ -378,8 +378,7 @@
Target toTarget = targetMap.get(dep.getLabel());
if (toTarget == null) {
// Dependency pointing to non-existent target. This error was reported in getTargets(), so
- // we can just ignore this dependency. Toolchain dependencies always have toTarget == null
- // since we do not depend on their package.
+ // we can just ignore this dependency.
continue;
}
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
index 01b8781..9b320f9 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/BuildOptions.java
@@ -75,6 +75,8 @@
private static final Comparator<Label> skylarkOptionsComparator = Ordering.natural();
private static final Logger logger = Logger.getLogger(BuildOptions.class.getName());
+ public static final BuildOptions NULL_OPTIONS = builder().build();
+
public static Map<Label, Object> labelizeStarlarkOptions(Map<String, Object> starlarkOptions) {
return starlarkOptions.entrySet().stream()
.collect(
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 26ffee4..a22a75d 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
@@ -448,7 +448,6 @@
ConfigurationTransition transition,
Iterable<Class<? extends BuildConfiguration.Fragment>> requiredFragments,
RuleClassProvider ruleClassProvider, boolean trimResults) {
-
// TODO(bazel-team): safety-check that this never mutates fromOptions.
List<BuildOptions> result = transition.apply(fromOptions);
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
index c057826a..fb1e82c 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/TransitionResolver.java
@@ -15,15 +15,12 @@
package com.google.devtools.build.lib.analysis.config;
import com.google.common.base.Preconditions;
-import com.google.devtools.build.lib.analysis.TargetAndConfiguration;
import com.google.devtools.build.lib.analysis.config.transitions.ComposingTransition;
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.config.transitions.NullTransition;
import com.google.devtools.build.lib.analysis.config.transitions.PatchTransition;
import com.google.devtools.build.lib.analysis.config.transitions.SplitTransition;
-import com.google.devtools.build.lib.packages.InputFile;
-import com.google.devtools.build.lib.packages.PackageGroup;
import com.google.devtools.build.lib.packages.Rule;
import com.google.devtools.build.lib.packages.RuleTransitionFactory;
import com.google.devtools.build.lib.packages.Target;
@@ -42,20 +39,21 @@
*/
public final class TransitionResolver {
/**
- * Given a parent rule and configuration depending on a child through an attribute, determines the
- * configuration the child should take.
+ * Given an original configuration and a base transition, determines the configuration a target
+ * should have.
*
- * @param fromConfig the parent rule's configuration
- * @param attributeTransition the configuration transition of the attribute of this dependency
- * edge
- * @param toTarget the child target (which may or may not be a rule)
+ * @param fromConfig the original configuration
+ * @param baseTransition the base configuration transitions computed by this method should be
+ * composed with (the configuration transition of the attribute through which this dependency
+ * happens or {@code NoTransition.INSTANCE} if this is a top-level target)
+ * @param toTarget the target whose configuration should be computed (may or may not be a rule)
* @param trimmingTransitionFactory the transition factory used to trim rules (note: this is a
- * temporary feature; see the corresponding methods in ConfiguredRuleClassProvider)
- * @return the child's configuration(s), expressed as a diff from the parent's configuration.
+ * temporary feature; see the corresponding methods in {@code ConfiguredRuleClassProvider})
+ * @return the target's configuration(s), expressed as a diff from the original configuration.
*/
public static ConfigurationTransition evaluateTransition(
BuildConfiguration fromConfig,
- ConfigurationTransition attributeTransition,
+ ConfigurationTransition baseTransition,
Target toTarget,
@Nullable RuleTransitionFactory trimmingTransitionFactory) {
@@ -66,7 +64,7 @@
}
// II. Input files and package groups have no configurations. We don't want to duplicate them.
- if (usesNullConfiguration(toTarget)) {
+ if (!toTarget.isConfigurable()) {
return NullTransition.INSTANCE;
}
@@ -97,7 +95,7 @@
// ComposingTransition, which encapsulates them into a single object so calling code
// doesn't need special logic for combinations.
// IV. Apply whatever transition the attribute requires.
- ConfigurationTransition currentTransition = attributeTransition;
+ ConfigurationTransition currentTransition = baseTransition;
// V. Applies any rule transitions associated with the dep target and composes their
// transitions with a passed-in existing transition.
@@ -109,33 +107,6 @@
}
/**
- * Same as evaluateTransition except does not check for transitions coming from parents and
- * enables support for rule-triggered top-level configuration hooks.
- */
- public static ConfigurationTransition evaluateTopLevelTransition(
- TargetAndConfiguration targetAndConfig,
- @Nullable RuleTransitionFactory trimmingTransitionFactory) {
- Target target = targetAndConfig.getTarget();
-
- // Rule class transitions (chosen by rule class definitions):
- if (target.getAssociatedRule() == null) {
- return NoTransition.INSTANCE;
- }
- ConfigurationTransition ruleTransition = applyRuleTransition(NoTransition.INSTANCE, target);
- ConfigurationTransition trimmingTransition =
- applyTransitionFromFactory(ruleTransition, target, trimmingTransitionFactory);
- return trimmingTransition;
- }
-
- /**
- * Returns true if the given target should have a null configuration. This method is the
- * "source of truth" for this determination.
- */
- public static boolean usesNullConfiguration(Target target) {
- return target instanceof InputFile || target instanceof PackageGroup;
- }
-
- /**
* Composes two transitions together efficiently.
*/
public static ConfigurationTransition composeTransitions(ConfigurationTransition transition1,
diff --git a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java
index a26d660..fb233f7 100644
--- a/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java
+++ b/src/main/java/com/google/devtools/build/lib/analysis/config/transitions/NullTransition.java
@@ -26,10 +26,6 @@
@Override
public BuildOptions patch(BuildOptions options) {
- throw new UnsupportedOperationException(
- "This is only referenced in a few places, so it's easier and more efficient to optimize "
- + "Blaze's transition logic in the presence of null transitions vs. actually call this "
- + "method to get results we know ahead of time. If there's ever a need to properly "
- + "implement this method we can always do so.");
+ return BuildOptions.NULL_OPTIONS;
}
}
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
index 508d56b..81c7b6e 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetFunction.java
@@ -44,7 +44,6 @@
import com.google.devtools.build.lib.analysis.config.ConfigMatchingProvider;
import com.google.devtools.build.lib.analysis.config.ConfigurationResolver;
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.configuredtargets.MergedConfiguredTarget.DuplicateException;
import com.google.devtools.build.lib.analysis.configuredtargets.RuleConfiguredTarget;
import com.google.devtools.build.lib.analysis.skylark.StarlarkTransition.TransitionException;
@@ -228,22 +227,14 @@
if (transitivePackagesForPackageRootResolution != null) {
transitivePackagesForPackageRootResolution.add(pkg);
}
- // TODO(bazel-team): This is problematic - we create the right key, but then end up with a value
- // that doesn't match; we can even have the same value multiple times. However, I think it's
- // only triggered in tests (i.e., in normal operation, the configuration passed in is already
- // null).
- if (!target.isConfigurable()) {
- configuration = null;
- }
-
- if (target.isConfigurable() && configuredTargetKey.getConfigurationKey() == null) {
+ if (target.isConfigurable() != (configuredTargetKey.getConfigurationKey() != null)) {
// We somehow ended up in a target that requires a non-null configuration as a dependency of
- // one that requires a null 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.
+ // 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.
return new NonRuleConfiguredTargetValue(
- new EmptyConfiguredTarget(target.getLabel(), null),
+ new EmptyConfiguredTarget(target.getLabel(), configuredTargetKey.getConfigurationKey()),
GeneratingActions.EMPTY,
transitivePackagesForPackageRootResolution == null
? null
@@ -608,31 +599,14 @@
return NO_CONFIG_CONDITIONS;
}
- Map<Label, Target> configurabilityTargets =
- resolver.getTargets(configLabelMap, target, transitiveRootCauses);
- if (configurabilityTargets == null) {
- return null;
- }
-
- // Collect the actual deps, hard-coded to the current configuration (since by definition config
- // conditions evaluate over the current target's configuration).
+ // Collect the actual deps without a configuration transition (since by definition config
+ // conditions evaluate over the current target's configuration). If the dependency is
+ // (erroneously) something that needs the null configuration, its analysis will be
+ // short-circuited. That error will be reported later.
ImmutableList.Builder<Dependency> depsBuilder = ImmutableList.builder();
for (Label configurabilityLabel : configLabelMap.values()) {
- Target configurabilityTarget = configurabilityTargets.get(configurabilityLabel);
- Dependency configurabilityDependency;
- if (TransitionResolver.usesNullConfiguration(configurabilityTarget)) {
- // This is kinda awful: select conditions should be config_setting rules, but we don't know
- // if they are one until after we analyzed them. However, we need to figure out which
- // configuration we analyze them in and input files and package groups are supposed to have
- // the null configuration.
- //
- // We can't check for the Target being a config_setting because configurability targets can
- // also be aliases pointing to config_settig rules.
- configurabilityDependency = Dependency.withNullConfiguration(configurabilityLabel);
- } else {
- configurabilityDependency =
- Dependency.withConfiguration(configurabilityLabel, ctgValue.getConfiguration());
- }
+ Dependency configurabilityDependency =
+ Dependency.withConfiguration(configurabilityLabel, ctgValue.getConfiguration());
depsBuilder.add(configurabilityDependency);
}
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 95685f1..1c2b661 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
@@ -1923,6 +1923,10 @@
if (depFragments != null) {
for (BuildOptions toOptions : ConfigurationResolver.applyTransition(
fromOptions, key.getTransition(), depFragments, ruleClassProvider, true)) {
+ if (toOptions.equals(BuildOptions.NULL_OPTIONS)) {
+ continue;
+ }
+
StarlarkTransition.postProcessStarlarkTransitions(eventHandler, key.getTransition());
configSkyKeys.add(
BuildConfigurationValue.key(
@@ -1947,6 +1951,9 @@
if (depFragments != null) {
for (BuildOptions toOptions : ConfigurationResolver.applyTransition(
fromOptions, key.getTransition(), depFragments, ruleClassProvider, true)) {
+ if (toOptions.equals(BuildOptions.NULL_OPTIONS)) {
+ builder.put(key, null);
+ }
SkyKey configKey =
BuildConfigurationValue.key(
depFragments, BuildOptions.diffForReconstruction(defaultBuildOptions, toOptions));
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
index af8e26f..6a3e402 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/ConfigurableAttributesTest.java
@@ -462,7 +462,7 @@
" values = {'test_arg': 'a'})");
writeHelloRules(/*includeDefaultCondition=*/true);
getConfiguredTarget("//java/hello:hello");
- assertContainsEvent("no such target '//conditions:b': target 'b' not declared in package");
+ assertContainsEvent("errors encountered while analyzing target '//java/hello:hello");
}
/**
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 ae58621..9b1fb50 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
@@ -423,9 +423,8 @@
if (target instanceof Rule && ((Rule) target).containsErrors()) {
return null;
}
- return TransitionResolver.evaluateTopLevelTransition(
- new TargetAndConfiguration(target, config),
- ruleClassProvider.getTrimmingTransitionFactory());
+ return TransitionResolver.evaluateTransition(
+ config, NoTransition.INSTANCE, target, ruleClassProvider.getTrimmingTransitionFactory());
}
/**
