Skip to content

Commit

Permalink
Fix toolchain deps for --noimplicit_deps cquery.
Browse files Browse the repository at this point in the history
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 #11993

PiperOrigin-RevId: 333367049
  • Loading branch information
juliexxia authored and copybara-github committed Sep 23, 2020
1 parent cede76b commit 31b0453
Show file tree
Hide file tree
Showing 3 changed files with 43 additions and 6 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand All @@ -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;
}

Expand Down Expand Up @@ -313,6 +324,8 @@ public final OrderedSetMultimap<DependencyKind, DependencyKey> 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 =
Expand Down
27 changes: 27 additions & 0 deletions src/main/java/com/google/devtools/build/lib/analysis/Util.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -80,6 +81,7 @@ public static boolean containsHyphen(PathFragment path) {
public static ImmutableSet<ConfiguredTargetKey> findImplicitDeps(RuleContext ruleContext) {
Set<ConfiguredTargetKey> maybeImplicitDeps = CompactHashSet.create();
Set<ConfiguredTargetKey> explicitDeps = CompactHashSet.create();
// Consider rule attribute dependencies.
AttributeMap attributes = ruleContext.attributes();
ListMultimap<String, ConfiguredTargetAndData> targetMap =
ruleContext.getConfiguredTargetAndDataMap();
Expand All @@ -93,6 +95,31 @@ public static ImmutableSet<ConfiguredTargetKey> 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));
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<String> */ evalToListOfStrings(implicits));
.containsNoneIn(evalToListOfStrings(implicits));
}

@SuppressWarnings("TruthIncompatibleType")
@Test
public void testNoImplicitDeps_toolchains() throws Exception {
MockRule ruleWithImplicitDeps =
Expand Down Expand Up @@ -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<String> */ evalToListOfStrings(implicits));
.containsNoneIn(evalToListOfStrings(implicits));
}

// Regression test for b/148550864
Expand Down

0 comments on commit 31b0453

Please sign in to comment.