Updates BuildViewTestCase.invalidatePackages() to also invalidate

configurations.

This is important for dynamic configurations.

invalidatePackages() invalidates every file. This includes CROSSTOOL,
which the CppConfiguration fragment depends on.

For static configurations, this doesn't matter because all configurations
and their fragments are pre-computed at the beginning of the build (or
in the case of tests before the test case starts).

For dynamic configurations, the configuration can get custom-created
each configured target. When that happens after invalidatePackages,
a new CppConfiguration instance gets created. This can impact code
like CcLibraryHelper.addDeps(), which assumes equality (CppConfiguration
has no .equals() method).

Normally that's not a problem because the same CppConfiguration instance
is used for every target in the post-invalidatePackages() graph. But
host configurations break this: we keep a non-Skyframe host config cache
in SkyframeBuildView.hostConfigurationCache. Without this change, it
doesn't get cleared out, so it keeps old pre-invalidation references that
under certain circumstances get applied to post-invalidation targets.

--
MOS_MIGRATED_REVID=125379342

5 files changed, 52 insertions, 19 deletions

diff --git a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
index f0c68a2398..956da5c8b4 100644
--- a/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
+++ b/src/test/java/com/google/devtools/build/lib/analysis/util/BuildViewTestCase.java
@@ -175,6 +175,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   // Note that these configurations are virtual (they use only VFS)
   protected BuildConfigurationCollection masterConfig;
   protected BuildConfiguration targetConfig;  // "target" or "build" config
+  private List<String> configurationArgs;
 
   protected OptionsParser optionsParser;
   private PackageCacheOptions packageCacheOptions;
@@ -276,14 +277,14 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
           ExecutionOptions.class,
           BuildRequest.BuildRequestOptions.class),
           ruleClassProvider.getConfigurationOptions()));
-    List<String> configurationArgs = new ArrayList<>();
+    List<String> allArgs = new ArrayList<>();
     // TODO(dmarting): Add --stamp option only to test that requires it.
-    configurationArgs.add("--stamp");  // Stamp is now defaulted to false.
-    configurationArgs.add("--experimental_extended_sanity_checks");
-    configurationArgs.add("--features=cc_include_scanning");
-    configurationArgs.addAll(getAnalysisMock().getOptionOverrides());
+    allArgs.add("--stamp");  // Stamp is now defaulted to false.
+    allArgs.add("--experimental_extended_sanity_checks");
+    allArgs.add("--features=cc_include_scanning");
+    allArgs.addAll(getAnalysisMock().getOptionOverrides());
 
-    optionsParser.parse(configurationArgs);
+    optionsParser.parse(allArgs);
     optionsParser.parse(args);
 
     InvocationPolicyEnforcer optionsPolicyEnforcer =
@@ -339,13 +340,37 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
     return skyframeExecutor.getPackageManager();
   }
 
+  protected void invalidatePackages() throws InterruptedException {
+    invalidatePackages(true);
+  }
+
   /**
-   * Invalidates all existing packages.
+   * Invalidates all existing packages. Optionally invalidates configurations too.
+   *
+   * <p>Tests should invalidate both unless they have specific reason not to.
+   *
    * @throws InterruptedException
    */
-  protected void invalidatePackages() throws InterruptedException {
+  protected void invalidatePackages(boolean alsoConfigs) throws InterruptedException {
     skyframeExecutor.invalidateFilesUnderPathForTesting(reporter,
         ModifiedFileSet.EVERYTHING_MODIFIED, rootDirectory);
+    if (alsoConfigs) {
+      try {
+        // Also invalidate all configurations. This is important for dynamic configurations: by
+        // invalidating all files we invalidate CROSSTOOL, which invalidates CppConfiguration (and
+        // a few other fragments). So we need to invalidate the
+        // {@link SkyframeBuildView#hostConfigurationCache} as well. Otherwise we end up
+        // with old CppConfiguration instances. Even though they're logically equal to the new ones,
+        // CppConfiguration has no .equals() method and some production code expects equality.
+        useConfiguration(configurationArgs.toArray(new String[0]));
+      } catch (Exception e) {
+        // There are enough dependers on this method that don't handle Exception that just passing
+        // through the Exception would result in a huge refactoring. As it stands this shouldn't
+        // fail anyway because this method only gets called after a successful useConfiguration()
+        // call anyway.
+        throw new RuntimeException(e);
+      }
+    }
   }
 
   /**
@@ -357,6 +382,7 @@ public abstract class BuildViewTestCase extends FoundationTestCase {
   protected final void useConfiguration(String... args) throws Exception {
     masterConfig = createConfigurations(args);
     targetConfig = getTargetConfiguration();
+    configurationArgs = Arrays.asList(args);
     createBuildView();
   }
 
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
index 4ed93634bc..7a2e0b44b3 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/repository/skylark/SkylarkRepositoryIntegrationTest.java
@@ -146,6 +146,13 @@ public class SkylarkRepositoryIntegrationTest extends BuildViewTestCase {
     return ruleProvider;
   }
 
+  @Override
+  protected void invalidatePackages() throws InterruptedException {
+    // Repository shuffling breaks access to config-needed paths like //tools/jdk:toolchain and
+    // these tests don't do anything interesting with configurations anyway. So exempt them.
+    invalidatePackages(/*alsoConfigs=*/false);
+  }
+
   @Test
   public void testSkylarkLocalRepository() throws Exception {
     // A simple test that recreates local_repository with Skylark.
diff --git a/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java b/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
index d431290358..e229cee1b0 100644
--- a/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
+++ b/src/test/java/com/google/devtools/build/lib/packages/ExternalPackageTest.java
@@ -50,7 +50,7 @@ public class ExternalPackageTest extends BuildViewTestCase {
         "    build_file = 'baz',",
         ")");
 
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false);
     // Make sure the second rule "wins."
     assertEquals("new_local_repository rule", getTarget("//external:my_rule").getTargetKind());
   }
@@ -68,7 +68,7 @@ public class ExternalPackageTest extends BuildViewTestCase {
         "    build_file = 'baz',",
         ")");
 
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false);
     // Make sure the second rule "wins."
     assertEquals("new_local_repository rule", getTarget("//external:my_rule").getTargetKind());
   }
diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
index e466bd9f4b..6d2f190cfd 100644
--- a/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skyframe/ASTFileLookupFunctionTest.java
@@ -94,7 +94,7 @@ public class ASTFileLookupFunctionTest extends BuildViewTestCase {
     scratch.file(
         "foo/BUILD", "genrule(name = 'foo',", "  outs = ['out.txt'],", "  cmd = 'echo hello >@')");
     mockFS.statThrowsIoException = true;
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // We don't want to fail early on config creation.
 
     SkyKey skyKey = PackageValue.key(PackageIdentifier.parse("@//foo"));
     EvaluationResult<PackageValue> result =
@@ -121,7 +121,7 @@ public class ASTFileLookupFunctionTest extends BuildViewTestCase {
     scratch.file("/a_remote_repo/remote_pkg/ext.bzl",
         "CONST = 17");
 
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchains.
     SkyKey skyKey =
         ASTFileLookupValue.key(Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:BUILD"));
     EvaluationResult<ASTFileLookupValue> result =
@@ -146,7 +146,7 @@ public class ASTFileLookupFunctionTest extends BuildViewTestCase {
     scratch.file("/a_remote_repo/remote_pkg/ext2.bzl",
         "CONST = 17");
 
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchains.
     SkyKey skyKey =
         ASTFileLookupValue.key(Label.parseAbsoluteUnchecked("@a_remote_repo//remote_pkg:ext1.bzl"));
     EvaluationResult<ASTFileLookupValue> result =
diff --git a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
index c96a65e50b..1a430ed4b3 100644
--- a/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
+++ b/src/test/java/com/google/devtools/build/lib/skylark/SkylarkRuleContextTest.java
@@ -339,7 +339,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
     scratch.file("/r/BUILD", "cc_library(name = 'cclib',", "  srcs = ['sub/my_sub_lib.h'])");
     scratch.file("/r/sub/BUILD", "cc_library(name = 'my_sub_lib', srcs = ['my_sub_lib.h'])");
     scratch.overwriteFile("WORKSPACE", "local_repository(name='r', path='/r')");
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchain labels.
     reporter.removeHandler(failFastHandler);
     getConfiguredTarget("@r//:cclib");
     assertContainsEvent(
@@ -812,7 +812,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
     scratch.overwriteFile("WORKSPACE",
         "local_repository(name='r', path='/r')");
 
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchain labels.
     SkylarkRuleContext context = createRuleContext("@r//a:r");
     Label depLabel = (Label) evalRuleContextCode(context, "ruleContext.attr.internal_dep.label");
     assertThat(depLabel).isEqualTo(Label.parseAbsolute("//:dep"));
@@ -843,7 +843,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
     scratch.overwriteFile("WORKSPACE",
         "local_repository(name='r', path='/r')");
 
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchain labels.
     SkylarkRuleContext context = createRuleContext("@r//a:r");
     Label depLabel = (Label) evalRuleContextCode(context, "ruleContext.attr.internal_dep.label");
     assertThat(depLabel).isEqualTo(Label.parseAbsolute("@r//:dep"));
@@ -879,7 +879,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
         "load('@r2//:other_test.bzl', 'other_macro')",  // We can still refer to r2 in other chunks.
         "macro(NEXT_NAME, '/r2')"  // and we can still use macro outside of its chunk.
     );
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchain labels.
     assertThat(getConfiguredTarget("@r1//:test")).isNotNull();
   }
 
@@ -897,7 +897,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
         "WORKSPACE",
         "local_repository(name = 'foo', path = '/bar')",
         "local_repository(name = 'foo', path = '/baz')");
-    invalidatePackages();
+    invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchain labels.
     assertThat(
             (List<Label>)
                 getConfiguredTarget("@foo//:baz")
@@ -915,7 +915,7 @@ public class SkylarkRuleContextTest extends SkylarkTestCase {
         "load('//:bar.bzl', 'dummy')",
         "local_repository(name = 'foo', path = '/baz')");
     try {
-      invalidatePackages();
+      invalidatePackages(/*alsoConfigs=*/false); // Repository shuffling messes with toolchains.
       createRuleContext("@foo//:baz");
       fail("Should have failed because repository 'foo' is overloading after a load!");
     } catch (Exception ex) {