Make --experimental_interleave_loading_and_analysis a no-op.

This is now enabled by default, and this change removes the code path where
it's disabled.

Remove a few tests that were testing the removed code, and rewrite some others
that still seem useful. We still drop configured targets on configuration
changes, so we use that to check that things are dropped from Skyframe.

--
MOS_MIGRATED_REVID=132226208

1 files changed, 24 insertions, 53 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 888f9d4c0f..c5960dd05f 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
@@ -46,7 +46,6 @@ import com.google.devtools.build.lib.analysis.util.ExpectedDynamicConfigurationE
 import com.google.devtools.build.lib.cmdline.Label;
 import com.google.devtools.build.lib.packages.Attribute;
 import com.google.devtools.build.lib.packages.Rule;
-import com.google.devtools.build.lib.pkgcache.LoadingFailedException;
 import com.google.devtools.build.lib.skyframe.SkyFunctions;
 import com.google.devtools.build.lib.skyframe.TargetPatternValue.TargetPatternKey;
 import com.google.devtools.build.lib.skyframe.util.SkyframeExecutorTestUtils;
@@ -456,7 +455,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//foo:top");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException e) {
+    } catch (ViewCreationFailedException e) {
       // Expected.
     }
     assertContainsEvent("no such target '//badbuild:isweird': target 'isweird' not declared in "
@@ -912,9 +911,6 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//foo:test");
       fail();
-    } catch (LoadingFailedException expected) {
-      Truth.assertThat(expected.getMessage())
-          .matches("Loading failed; build aborted.*");
     } catch (ViewCreationFailedException expected) {
       Truth.assertThat(expected.getMessage())
           .matches("Analysis of target '//foo:test' failed; build aborted.*");
@@ -934,17 +930,10 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//cycle:foo");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
       assertContainsEvent("in cc_library rule //cycle:foo: cycle in dependency graph:");
-      // In the legacy case, SkyframeLabelVisitor prints the loading error for //cycle:foo. In the
-      // interleaved case, the SkyframeBuildView only throws the exception, but doesn't print the
-      // error - the error is printed by the BuildTool, which isn't used in this test.
-      if (defaultFlags().contains(Flag.SKYFRAME_LOADING_PHASE)) {
-        assertThat(expected.getMessage())
-            .contains("Analysis of target '//cycle:foo' failed; build aborted");
-      } else {
-        assertContainsEvent("Loading of target '//cycle:foo' failed; build aborted");
-      }
+      assertThat(expected.getMessage())
+          .contains("Analysis of target '//cycle:foo' failed; build aborted");
     }
   }
 
@@ -971,31 +960,17 @@ public class BuildViewTest extends BuildViewTestBase {
         "//cycle:foo", "//cycle:bat", "//cycle:baz");
     assertContainsEvent("in cc_library rule //cycle:foo: cycle in dependency graph:");
     assertContainsEvent("in cc_library rule //cycle:bas: cycle in dependency graph:");
-    if (defaultFlags().contains(Flag.SKYFRAME_LOADING_PHASE)) {
-      assertContainsEvent(
-          "errors encountered while analyzing target '//cycle:foo': it will not be built");
-      assertContainsEvent(
-          "errors encountered while analyzing target '//cycle:bat': it will not be built");
-      // With interleaved loading and analysis, we can no longer distinguish loading-phase cycles
-      // and analysis-phase cycles. This was previously reported as a loading-phase cycle, as it
-      // happens with any configuration (cycle is hard-coded in the BUILD files). Also see the
-      // test below.
-      assertThat(Iterables.transform(analysisFailureRecorder.events, ANALYSIS_EVENT_TO_STRING_PAIR))
-          .containsExactly(
-              Pair.of("//cycle:foo", "//cycle:foo"), Pair.of("//cycle:bat", "//cycle:bas"));
-    } else {
-      assertContainsEvent("errors encountered while loading target '//cycle:foo'");
-      assertContainsEvent("errors encountered while loading target '//cycle:bat'");
-
-      assertThat(Iterables.transform(loadingFailureRecorder.events,
-          new Function<Pair<Label, Label>, Pair<String, String>>() {
-            @Override
-            public Pair<String, String> apply(Pair<Label, Label> labelPair) {
-              return Pair.of(labelPair.getFirst().toString(), labelPair.getSecond().toString());
-            }
-          })).containsExactly(
-              Pair.of("//cycle:foo", "//cycle:foo"), Pair.of("//cycle:bat", "//cycle:bas"));
-    }
+    assertContainsEvent(
+        "errors encountered while analyzing target '//cycle:foo': it will not be built");
+    assertContainsEvent(
+        "errors encountered while analyzing target '//cycle:bat': it will not be built");
+    // With interleaved loading and analysis, we can no longer distinguish loading-phase cycles
+    // and analysis-phase cycles. This was previously reported as a loading-phase cycle, as it
+    // happens with any configuration (cycle is hard-coded in the BUILD files). Also see the
+    // test below.
+    assertThat(Iterables.transform(analysisFailureRecorder.events, ANALYSIS_EVENT_TO_STRING_PAIR))
+        .containsExactly(
+            Pair.of("//cycle:foo", "//cycle:foo"), Pair.of("//cycle:bat", "//cycle:bas"));
   }
 
   @Test
@@ -1040,7 +1015,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       assertThat(e.getMessage()).contains("third_party/crosstool/v2");
     }
   }
@@ -1053,7 +1028,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       assertContainsEvent(
           "no such package 'does/not/exist': BUILD file not found on package path");
     }
@@ -1073,7 +1048,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       // Expected
     }
   }
@@ -1089,7 +1064,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update(defaultFlags().with(Flag.KEEP_GOING));
       fail();
-    } catch (LoadingFailedException | InvalidConfigurationException e) {
+    } catch (InvalidConfigurationException e) {
       assertThat(e.getMessage()).contains("//xcode:does_not_exist");
     }
   }
@@ -1105,7 +1080,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//z/b:b");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
       assertContainsEvent("no such package 'nonexistent'");
     }
   }
@@ -1122,7 +1097,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//z/b:b");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
       assertContainsEvent("no such package 'b'");
     }
   }
@@ -1138,7 +1113,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//parent:a");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException expected) {
+    } catch (ViewCreationFailedException expected) {
     }
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
@@ -1175,7 +1150,7 @@ public class BuildViewTest extends BuildViewTestBase {
     try {
       update("//okay");
       fail();
-    } catch (LoadingFailedException | ViewCreationFailedException e) {
+    } catch (ViewCreationFailedException e) {
     }
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
@@ -1194,11 +1169,7 @@ public class BuildViewTest extends BuildViewTestBase {
         "sh_binary(name = 'okay', srcs = ['okay.sh'])");
     useConfiguration("--experimental_action_listener=//parent:a");
     reporter.removeHandler(failFastHandler);
-    try {
-      update(defaultFlags().with(Flag.KEEP_GOING), "//okay");
-    } catch (LoadingFailedException ignored) {
-      // In the legacy case, we get a loading exception even with keep going. Why?
-    }
+    update(defaultFlags().with(Flag.KEEP_GOING), "//okay");
     assertContainsEventWithFrequency("name 'undefined_symbol' is not defined", 1);
     assertContainsEventWithFrequency(
         "Target '//child:b' contains an error and its package is in error and referenced "