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

2 files changed, 20 insertions, 1 deletions

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 bc8eccb86b..2c2adf7e8c 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 @@ public class BuildViewTest extends BuildViewTestBase {
 
   @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 @@ public class BuildViewTest extends BuildViewTestBase {
         ruleClassProvider.getUniversalFragment());
   }
 
-
   /** Runs the same test with the reduced loading phase. */
   @TestSpec(size = Suite.SMALL_TESTS)
   @RunWith(JUnit4.class)
@@ -1240,4 +1245,14 @@ public class BuildViewTest extends BuildViewTestBase {
       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 d4a8791f16..5f0bdec314 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 abstract class AnalysisTestCase extends FoundationTestCase {
   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 @@ public abstract class AnalysisTestCase extends FoundationTestCase {
         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);