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/ConfiguredTargetCycleReporter.java b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
index c7a542f..70b7ed8 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/ConfiguredTargetCycleReporter.java
@@ -13,8 +13,12 @@
// limitations under the License.
package com.google.devtools.build.lib.skyframe;
+import static com.google.devtools.build.lib.skyframe.SkyFunctions.TRANSITIVE_TARGET;
+
+import com.google.common.base.Function;
import com.google.common.base.Predicate;
-import com.google.common.collect.ImmutableList;
+import com.google.common.base.Predicates;
+import com.google.common.base.Verify;
import com.google.common.collect.Iterables;
import com.google.devtools.build.lib.cmdline.Label;
import com.google.devtools.build.lib.events.EventHandler;
@@ -35,29 +39,78 @@
private static final Predicate<SkyKey> IS_CONFIGURED_TARGET_SKY_KEY =
SkyFunctions.isSkyFunction(SkyFunctions.CONFIGURED_TARGET);
+ private static final Predicate<SkyKey> IS_TRANSITIVE_TARGET_SKY_KEY =
+ SkyFunctions.isSkyFunction(TRANSITIVE_TARGET);
+
+ private final TransitiveTargetCycleReporter targetReporter;
+
ConfiguredTargetCycleReporter(PackageProvider packageProvider) {
super(packageProvider);
+ targetReporter = new TransitiveTargetCycleReporter(packageProvider);
}
@Override
protected boolean canReportCycle(SkyKey topLevelKey, CycleInfo cycleInfo) {
- return Iterables.all(Iterables.concat(ImmutableList.of(topLevelKey),
- cycleInfo.getPathToCycle(), cycleInfo.getCycle()), IS_CONFIGURED_TARGET_SKY_KEY);
+ if (!IS_CONFIGURED_TARGET_SKY_KEY.apply(topLevelKey)) {
+ return false;
+ }
+ Iterable<SkyKey> cycleKeys = Iterables.concat(cycleInfo.getPathToCycle(), cycleInfo.getCycle());
+ // Static configurations expect all keys to be ConfiguredTargetValue keys. Dynamic
+ // configurations expect the top-level key to be a ConfiguredTargetValue key, but cycles and
+ // paths to them can travel through TransitiveTargetValue keys because ConfiguredTargetFunction
+ // visits TransitiveTargetFunction as a part of dynamic configuration computation.
+ //
+ // Unfortunately this class can't easily figure out if we're in static or dynamic configuration
+ // mode, so we loosely permit both cases.
+ //
+ // TODO: remove the static-style checking once dynamic configurations fully replace them
+ return Iterables.all(cycleKeys,
+ Predicates.<SkyKey>or(IS_CONFIGURED_TARGET_SKY_KEY, IS_TRANSITIVE_TARGET_SKY_KEY));
}
@Override
protected String getAdditionalMessageAboutCycle(
EventHandler eventHandler, SkyKey topLevelKey, CycleInfo cycleInfo) {
- return "\nThis cycle occurred because of a configuration option";
+ if (Iterables.all(cycleInfo.getCycle(), IS_TRANSITIVE_TARGET_SKY_KEY)) {
+ // The problem happened strictly in loading, so delegate the explanation to
+ // TransitiveTargetCycleReporter.
+ Iterable<SkyKey> pathAsTargetKeys = Iterables.transform(cycleInfo.getPathToCycle(),
+ new Function<SkyKey, SkyKey>() {
+ @Override
+ public SkyKey apply(SkyKey key) {
+ return asTransitiveTargetKey(key);
+ }
+ });
+ return targetReporter.getAdditionalMessageAboutCycle(eventHandler,
+ asTransitiveTargetKey(topLevelKey),
+ new CycleInfo(pathAsTargetKeys, cycleInfo.getCycle()));
+ } else {
+ return "\nThis cycle occurred because of a configuration option";
+ }
+ }
+
+ private SkyKey asTransitiveTargetKey(SkyKey key) {
+ return IS_TRANSITIVE_TARGET_SKY_KEY.apply(key)
+ ? key
+ : SkyKey.create(TRANSITIVE_TARGET, ((ConfiguredTargetKey) key.argument()).getLabel());
}
@Override
public String prettyPrint(SkyKey key) {
- return ((ConfiguredTargetKey) key.argument()).prettyPrint();
+ if (IS_CONFIGURED_TARGET_SKY_KEY.apply(key)) {
+ return ((ConfiguredTargetKey) key.argument()).prettyPrint();
+ } else {
+ return getLabel(key).toString();
+ }
}
@Override
public Label getLabel(SkyKey key) {
- return ((ConfiguredTargetKey) key.argument()).getLabel();
+ if (IS_CONFIGURED_TARGET_SKY_KEY.apply(key)) {
+ return ((ConfiguredTargetKey) key.argument()).getLabel();
+ } else {
+ Verify.verify(IS_TRANSITIVE_TARGET_SKY_KEY.apply(key));
+ return (Label) key.argument();
+ }
}
-}
\ No newline at end of file
+}
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(
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
index 4231583..556d781 100644
--- a/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
+++ b/src/main/java/com/google/devtools/build/lib/skyframe/SkyframeBuildView.java
@@ -391,6 +391,8 @@
}
if (culprit.functionName().equals(SkyFunctions.CONFIGURED_TARGET)) {
return ((ConfiguredTargetKey) culprit.argument()).getLabel();
+ } else if (culprit.functionName().equals(SkyFunctions.TRANSITIVE_TARGET)) {
+ return (Label) culprit.argument();
} else {
return labelToLoad;
}
diff --git a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
index bc8eccb..2c2adf7 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/BuildViewTest.java
@@ -861,6 +861,12 @@
@Test
public void testCycleDueToJavaLauncherConfiguration() throws Exception {
+ if (defaultFlags().contains(Flag.DYNAMIC_CONFIGURATIONS)) {
+ // Dynamic configurations don't yet support late-bound attributes. Development testing already
+ // runs all tests with dynamic configurations enabled, so this will still fail for developers
+ // and won't get lost in the fog.
+ return;
+ }
scratch.file("foo/BUILD",
"java_binary(name = 'java', srcs = ['DoesntMatter.java'])",
"cc_binary(name = 'cpp', data = [':java'])");
@@ -1230,7 +1236,6 @@
ruleClassProvider.getUniversalFragment());
}
-
/** Runs the same test with the reduced loading phase. */
@TestSpec(size = Suite.SMALL_TESTS)
@RunWith(JUnit4.class)
@@ -1240,4 +1245,14 @@
return super.defaultFlags().with(Flag.SKYFRAME_LOADING_PHASE);
}
}
+
+ /** Runs the same test with dynamic configurations. */
+ @TestSpec(size = Suite.SMALL_TESTS)
+ @RunWith(JUnit4.class)
+ public static class WithDynamicConfigurations extends BuildViewTest {
+ @Override
+ protected FlagBuilder defaultFlags() {
+ return super.defaultFlags().with(Flag.DYNAMIC_CONFIGURATIONS);
+ }
+ }
}
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 d4a8791..5f0bdec 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
@@ -93,6 +93,7 @@
public enum Flag {
KEEP_GOING,
SKYFRAME_LOADING_PHASE,
+ DYNAMIC_CONFIGURATIONS,
}
/** Helper class to make it easy to enable and disable flags. */
@@ -214,6 +215,9 @@
ruleClassProvider.getConfigurationOptions()));
optionsParser.parse(new String[] {"--default_visibility=public" });
optionsParser.parse(args);
+ if (defaultFlags().contains(Flag.DYNAMIC_CONFIGURATIONS)) {
+ optionsParser.parse("--experimental_dynamic_configs");
+ }
InvocationPolicyEnforcer optionsPolicyEnforcer =
new InvocationPolicyEnforcer(TestConstants.TEST_INVOCATION_POLICY);
