From e230024d9929548f710814568bad6edf3f33a0c4 Mon Sep 17 00:00:00 2001 From: juliexxia Date: Wed, 23 Sep 2020 13:58:18 -0700 Subject: [PATCH] Fix toolchain deps for --noimplicit_deps cquery. Toolchain deps were not being caught by implicit deps filtering because there's no way to tell them apart from regular deps. Thus, we add (back) logic to do it during ruleContext building. This was caught by a user in unknown commit when they fixed our test for us. The test fix in included in this CL. Fixing the test triggered the bug. Fixes https://github.com/bazelbuild/bazel/issues/11993 PiperOrigin-RevId: 333367049 --- .../lib/analysis/DependencyResolver.java | 15 ++++++++++- .../devtools/build/lib/analysis/Util.java | 27 +++++++++++++++++++ .../testutil/PostAnalysisQueryTest.java | 7 ++--- 3 files changed, 43 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java index 7547dc461cba1f..5d80db2ecbc6e2 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/DependencyResolver.java @@ -80,6 +80,17 @@ public abstract class DependencyResolver { // TODO(#10523): Remove this when the migration period for toolchain transitions has ended. public static boolean shouldUseToolchainTransition( @Nullable BuildConfiguration configuration, Target target) { + return shouldUseToolchainTransition( + configuration, target instanceof Rule ? (Rule) target : null); + } + + /** + * Returns whether or not to use the new toolchain transition. Checks the global incompatible + * change flag and the rule's toolchain transition readiness attribute. + */ + // TODO(#10523): Remove this when the migration period for toolchain transitions has ended. + public static boolean shouldUseToolchainTransition( + @Nullable BuildConfiguration configuration, @Nullable Rule rule) { // Check whether the global incompatible change flag is set. if (configuration != null) { PlatformOptions platformOptions = configuration.getOptions().get(PlatformOptions.class); @@ -89,7 +100,7 @@ public static boolean shouldUseToolchainTransition( } // Check the rule definition to see if it is ready. - if (target instanceof Rule && ((Rule) target).getRuleClassObject().useToolchainTransition()) { + if (rule != null && rule.getRuleClassObject().useToolchainTransition()) { return true; } @@ -313,6 +324,8 @@ public final OrderedSetMultimap dependentNodeMap( // a missing toolchain a bit better. // TODO(lberki): This special-casing is weird. Find a better way to depend on toolchains. // TODO(#10523): Remove check when this is fully released. + // This logic needs to stay in sync with the dep finding logic in + // //third_party/bazel/src/main/java/com/google/devtools/build/lib/analysis/Util.java#findImplicitDeps. if (useToolchainTransition) { ToolchainDependencyKind tdk = (ToolchainDependencyKind) entry.getKey(); ToolchainContext toolchainContext = diff --git a/src/main/java/com/google/devtools/build/lib/analysis/Util.java b/src/main/java/com/google/devtools/build/lib/analysis/Util.java index 3618cf95e77740..5f9c8e52c2c3cb 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/Util.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/Util.java @@ -17,6 +17,7 @@ import com.google.common.collect.ImmutableSet; import com.google.common.collect.ListMultimap; import com.google.common.collect.Sets; +import com.google.devtools.build.lib.analysis.config.BuildConfiguration; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.collect.compacthashset.CompactHashSet; import com.google.devtools.build.lib.packages.AttributeMap; @@ -80,6 +81,7 @@ public static boolean containsHyphen(PathFragment path) { public static ImmutableSet findImplicitDeps(RuleContext ruleContext) { Set maybeImplicitDeps = CompactHashSet.create(); Set explicitDeps = CompactHashSet.create(); + // Consider rule attribute dependencies. AttributeMap attributes = ruleContext.attributes(); ListMultimap targetMap = ruleContext.getConfiguredTargetAndDataMap(); @@ -93,6 +95,31 @@ public static ImmutableSet findImplicitDeps(RuleContext rul } } } + // Consider toolchain dependencies. + ToolchainContext toolchainContext = ruleContext.getToolchainContext(); + if (toolchainContext != null) { + // This logic should stay up to date with the dep creation logic in + // DependencyResolver#partiallyResolveDependencies. + BuildConfiguration targetConfiguration = ruleContext.getConfiguration(); + BuildConfiguration hostConfiguration = ruleContext.getHostConfiguration(); + for (Label toolchain : toolchainContext.resolvedToolchainLabels()) { + if (DependencyResolver.shouldUseToolchainTransition( + targetConfiguration, ruleContext.getRule())) { + maybeImplicitDeps.add( + ConfiguredTargetKey.builder() + .setLabel(toolchain) + .setConfiguration(targetConfiguration) + .setToolchainContextKey(toolchainContext.key()) + .build()); + } else { + maybeImplicitDeps.add( + ConfiguredTargetKey.builder() + .setLabel(toolchain) + .setConfiguration(hostConfiguration) + .build()); + } + } + } return ImmutableSet.copyOf(Sets.difference(maybeImplicitDeps, explicitDeps)); } diff --git a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java index dc67d4a0269fd3..2798a4ee8ef1f1 100644 --- a/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java +++ b/src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java @@ -215,11 +215,9 @@ public void testNoImplicitDeps() throws Exception { assertThat(evalToListOfStrings("deps(//test:my_rule)")) .containsAtLeastElementsIn(evalToListOfStrings(explicits)); assertThat(evalToListOfStrings("deps(//test:my_rule)")) - .doesNotContain( - /* expected: String, actual: ImmutableList */ evalToListOfStrings(implicits)); + .containsNoneIn(evalToListOfStrings(implicits)); } - @SuppressWarnings("TruthIncompatibleType") @Test public void testNoImplicitDeps_toolchains() throws Exception { MockRule ruleWithImplicitDeps = @@ -266,8 +264,7 @@ public void testNoImplicitDeps_toolchains() throws Exception { assertThat(evalToListOfStrings("deps(//test:my_rule)")) .containsAtLeastElementsIn(evalToListOfStrings(explicits)); assertThat(evalToListOfStrings("deps(//test:my_rule)")) - .doesNotContain( - /* expected: String, actual: ImmutableList */ evalToListOfStrings(implicits)); + .containsNoneIn(evalToListOfStrings(implicits)); } // Regression test for b/148550864