When re-using a ToolchainContextKey, make sure to remove the actual

toolchains to prevent dependency loops. Part of work on toolchain transitions, bazelbuild#10523.
katre · Jun 8, 2020 · d345b5f · d345b5f
1 parent 6094d35
commit d345b5f
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 1 deletion.
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/BUILD b/src/main/java/com/google/devtools/build/lib/skyframe/BUILD
@@ -85,7 +85,6 @@ java_library(
         "ToplevelStarlarkAspectFunction.java",
         "TransitiveTargetFunction.java",
         "TrimmedConfigurationProgressReceiver.java",
-        "UnloadedToolchainContextImpl.java",
         "WorkspaceASTFunction.java",
         "WorkspaceFileFunction.java",
         "actiongraph/ActionGraphDump.java",
@@ -220,6 +219,7 @@ java_library(
         ":transitive_traversal_value",
         ":tree_artifact_value",
         ":unloaded_toolchain_context",
+        ":unloaded_toolchain_context_impl",
         ":workspace_ast_value",
         ":workspace_name_function",
         ":workspace_name_value",
@@ -2648,10 +2648,26 @@ java_library(
     name = "unloaded_toolchain_context",
     srcs = ["UnloadedToolchainContext.java"],
     deps = [
+        ":toolchain_context_key",
+        "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context",
+        "//src/main/java/com/google/devtools/build/lib/analysis/platform",
+        "//src/main/java/com/google/devtools/build/lib/cmdline",
+        "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//third_party:guava",
+    ],
+)
+
+java_library(
+    name = "unloaded_toolchain_context_impl",
+    srcs = ["UnloadedToolchainContextImpl.java"],
+    deps = [
+        ":toolchain_context_key",
+        ":unloaded_toolchain_context",
         "//src/main/java/com/google/devtools/build/lib/analysis:toolchain_context",
         "//src/main/java/com/google/devtools/build/lib/analysis/platform",
         "//src/main/java/com/google/devtools/build/lib/cmdline",
         "//src/main/java/com/google/devtools/build/skyframe:skyframe-objects",
+        "//third_party:auto_value",
         "//third_party:guava",
     ],
 )

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
@@ -538,6 +538,14 @@ static ToolchainCollection<UnloadedToolchainContext> computeUnloadedToolchainCon
           (UnloadedToolchainContext) values.get(unloadedToolchainContextKey.getValue()).get();
       if (!valuesMissing) {
         String execGroup = unloadedToolchainContextKey.getKey();
+        if (parentToolchainContextKey != null) {
+          // Since we inherited the toolchain context from the parent of the dependency, the current
+          // target may also be in the resolved toolchains list. We need to clear it out.
+          // TODO(configurability): When updating this for config_setting, only remove the current
+          // target, not everything, because config_setting might want to check the toolchain
+          // dependencies.
+          unloadedToolchainContext = unloadedToolchainContext.withoutResolvedToolchains();
+        }
         if (execGroup.equals(targetUnloadedToolchainContext)) {
           toolchainContexts.addDefaultContext(unloadedToolchainContext);
         } else {

diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/UnloadedToolchainContext.java b/src/main/java/com/google/devtools/build/lib/skyframe/UnloadedToolchainContext.java
@@ -41,4 +41,7 @@ public interface UnloadedToolchainContext extends ToolchainContext, SkyValue {
 
   @Override
   ImmutableSet<Label> resolvedToolchainLabels();
+
+  /** Returns a copy of this context, without the resolved toolchain data. */
+  UnloadedToolchainContext withoutResolvedToolchains();
 }
diff --git a/src/main/java/com/google/devtools/build/lib/skyframe/UnloadedToolchainContextImpl.java b/src/main/java/com/google/devtools/build/lib/skyframe/UnloadedToolchainContextImpl.java
@@ -72,4 +72,11 @@ Builder setRequestedLabelToToolchainType(
   public ImmutableSet<Label> resolvedToolchainLabels() {
     return toolchainTypeToResolved().values();
   }
+
+  protected abstract Builder toBuilder();
+
+  @Override
+  public UnloadedToolchainContext withoutResolvedToolchains() {
+    return this.toBuilder().setToolchainTypeToResolved(ImmutableBiMap.of()).build();
+  }
 }