Refactor cycle detection logic to handle dynamic configurations.
Currently, analysis-time cycle detection expects all cycles to come from ConfiguredTargetFunction.
With dynamic configurations, ConfiguredTargetFunction calls out to TransitiveTargetFunction to figure out which configuration fragments its deps need.
If there's a cycle between the current target and a dep, the dep's TransitiveTargetFunction fails, which the current cycle detection code can't handle.
But even if it could handle it, since the failure occurs in the dep we'd get error messages like:
"in cc_library rule //the:dep: cycle in dependency graph"
instead of the expected:
"in cc_library rule //the:top_level_rule: cycle in dependency graph"
This used to not be a problem because loading-phase cycle detection caught the cycle before all this triggered. But interleaved loading and analysis removes that gate.
Tested: BuildViewTest cycle detection tests with dynamic configurations turned on
--
MOS_MIGRATED_REVID=124391277
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 e8706f2..b0a8137 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
@@ -134,6 +134,10 @@
this.ruleClassProvider = ruleClassProvider;
}
+ private static boolean useDynamicConfigurations(BuildConfiguration config) {
+ return config != null && config.useDynamicConfigurations();
+ }
+
@Override
public SkyValue compute(SkyKey key, Environment env) throws ConfiguredTargetFunctionException,
InterruptedException {
@@ -173,6 +177,17 @@
if (!target.isConfigurable()) {
configuration = null;
}
+
+ // This line is only needed for accurate error messaging. Say this target has a circular
+ // dependency with one of its deps. With this line, loading this target fails so Bazel
+ // associates the corresponding error with this target, as expected. Without this line,
+ // the first TransitiveTargetValue call happens on its dep (in trimConfigurations), so Bazel
+ // associates the error with the dep, which is misleading.
+ if (useDynamicConfigurations(configuration)
+ && env.getValue(TransitiveTargetValue.key(lc.getLabel())) == null) {
+ return null;
+ }
+
TargetAndConfiguration ctgValue = new TargetAndConfiguration(target, configuration);
SkyframeDependencyResolver resolver = view.createDependencyResolver(env);
@@ -283,8 +298,7 @@
// Trim each dep's configuration so it only includes the fragments needed by its transitive
// closure (only dynamic configurations support this).
- if (ctgValue.getConfiguration() != null
- && ctgValue.getConfiguration().useDynamicConfigurations()) {
+ if (useDynamicConfigurations(ctgValue.getConfiguration())) {
depValueNames = trimConfigurations(env, ctgValue, depValueNames, hostConfiguration,
ruleClassProvider);
if (depValueNames == null) {
@@ -725,7 +739,7 @@
// TODO(bazel-team): remove the need for this special transformation. We can probably do this by
// simply passing this through trimConfigurations.
BuildConfiguration targetConfig = ctgValue.getConfiguration();
- if (targetConfig != null && targetConfig.useDynamicConfigurations()) {
+ if (useDynamicConfigurations(targetConfig)) {
ImmutableList.Builder<Dependency> staticConfigs = ImmutableList.builder();
for (Dependency dep : configValueNames) {
staticConfigs.add(
