Skip to content

Commit

Permalink
Fix regression with implicit deps which Automatic Exec Groups caused
Browse files Browse the repository at this point in the history 
Automatic Exec Groups creates one exec group per each toolchain type, which creates additional toolchain contexts.
On the other side, function which marks deps as implicit or explicit was only getting the default toolchain context (not the ones from automatic exec groups) and marking only toolchains from default-exec-group as implicit dependencies. Since deps from AEGs were not marked as implicit, they were not hidden with `--noimplicit_deps` flag.

I've fixed this by marking automatic exec groups' toolchain labels as implicit (looking into all toolchain context, not only the default one).

I've also added 2 tests which check:
  1. Implicit deps from Automatic Exec Groups,
  2. Implicit deps from Custom Exec Groups.

PiperOrigin-RevId: 520921585
Change-Id: I7beb9aeae098021d63e9feffa677329f6dda0453
  • Loading branch information
@kotlaja @copybara-github
kotlaja authored and copybara-github committed Mar 31, 2023
1 parent 24f6fe8 commit b27d1a3
Show file tree
Hide file tree
Showing 2 changed files with 119 additions and 43 deletions.
31 changes: 18 additions & 13 deletions src/main/java/com/google/devtools/build/lib/analysis/Util.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,19 +95,24 @@ 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.
BuildConfigurationValue targetConfiguration = ruleContext.getConfiguration();
for (Label toolchain : toolchainContext.resolvedToolchainLabels()) {
maybeImplicitDeps.add(
ConfiguredTargetKey.builder()
.setLabel(toolchain)
.setConfiguration(targetConfiguration)
.setExecutionPlatformLabel(toolchainContext.executionPlatform().label())
.build());

ToolchainCollection<ResolvedToolchainContext> toolchainContexts =
ruleContext.getToolchainContexts();
if (toolchainContexts != null) {
for (ResolvedToolchainContext toolchainContext : toolchainContexts.getContextMap().values()) {
if (toolchainContext != null) {
// This logic should stay up to date with the dep creation logic in
// DependencyResolver#partiallyResolveDependencies.
BuildConfigurationValue targetConfiguration = ruleContext.getConfiguration();
for (Label toolchain : toolchainContext.resolvedToolchainLabels()) {
maybeImplicitDeps.add(
ConfiguredTargetKey.builder()
.setLabel(toolchain)
.setConfiguration(targetConfiguration)
.setExecutionPlatformLabel(toolchainContext.executionPlatform().label())
.build());
}
}
}
}
return ImmutableSet.copyOf(Sets.difference(maybeImplicitDeps, explicitDeps));
Expand Down
131 changes: 101 additions & 30 deletions src/test/java/com/google/devtools/build/lib/query2/testutil/PostAnalysisQueryTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -259,60 +259,64 @@ public void testNoImplicitDeps_toolchains() throws Exception {

// Check for implicit toolchain dependencies
assertThat(evalToListOfStrings("deps(//test:my_rule)"))
.containsAtLeastElementsIn(
evalToListOfStrings(explicits + " + " + implicits + " + " + PLATFORM_LABEL));
.containsAtLeast(explicits, implicits, PLATFORM_LABEL);

helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS);
ImmutableList<String> filteredDeps = evalToListOfStrings("deps(//test:my_rule)");
assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits));
assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits));
assertThat(filteredDeps).contains(explicits);
assertThat(filteredDeps).doesNotContain(implicits);
}

@Test
public void testNoImplicitDeps_starlark_toolchains() throws Exception {
private void writeSimpleToolchain() throws Exception {
writeFile(
"test/toolchain.bzl",
"test/toolchain_def.bzl",
"def _impl(ctx):",
" toolchain = platform_common.ToolchainInfo()",
" return [toolchain]",
" return [platform_common.ToolchainInfo()]",
"test_toolchain = rule(",
" implementation = _impl,",
")");
writeFile(
"test/rule.bzl",
"test/BUILD",
"load('//test:toolchain_def.bzl', 'test_toolchain')",
"toolchain_type(name = 'toolchain_type')",
"toolchain(",
" name = 'toolchain',",
" toolchain_type = '//test:toolchain_type',",
" toolchain = ':toolchain_impl'",
")",
"test_toolchain(name = 'toolchain_impl')");
}

@Test
public void testNoImplicitDeps_starlark_toolchains() throws Exception {
writeSimpleToolchain();
writeFile(
"test/rule/rule.bzl",
"def _impl(ctx):",
" return []",
"implicit_toolchain_deps_rule = rule(",
" implementation = _impl,",
" toolchains = ['//test:toolchain_type']",
")");
writeFile(
"test/BUILD",
"load(':toolchain.bzl', 'test_toolchain')",
"test/rule/BUILD",
"load(':rule.bzl', 'implicit_toolchain_deps_rule')",
"implicit_toolchain_deps_rule(",
" name = 'my_rule',",
")",
"toolchain_type(name = 'toolchain_type')",
"toolchain(",
" name = 'toolchain',",
" toolchain_type = ':toolchain_type',",
" toolchain = ':toolchain_impl',",
")",
"test_toolchain(name = 'toolchain_impl')");
")");
((PostAnalysisQueryHelper<T>) helper).useConfiguration("--extra_toolchains=//test:toolchain");

String implicits = "//test:toolchain_impl";
String explicits = "//test:my_rule";
String explicits = "//test/rule:my_rule";

// Check for implicit toolchain dependencies
assertThat(evalToListOfStrings("deps(//test:my_rule)"))
.containsAtLeastElementsIn(evalToListOfStrings(explicits + " + " + implicits));
assertThat(evalToListOfStrings("deps(//test/rule:my_rule)"))
.containsAtLeast(explicits, implicits);

helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS);
ImmutableList<String> filteredDeps = evalToListOfStrings("deps(//test:my_rule)");
assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits));
assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits));
ImmutableList<String> filteredDeps = evalToListOfStrings("deps(//test/rule:my_rule)");
assertThat(filteredDeps).contains(explicits);
assertThat(filteredDeps).doesNotContain(implicits);
}

@Test
Expand Down Expand Up @@ -366,13 +370,12 @@ public void testNoImplicitDeps_cc_toolchains() throws Exception {

// Check for implicit toolchain dependencies
assertThat(evalToListOfStrings("deps(//test:my_rule)"))
.containsAtLeastElementsIn(
evalToListOfStrings(explicits + " + " + implicits + " + " + PLATFORM_LABEL));
.containsAtLeast(explicits, implicits, PLATFORM_LABEL);

helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS);
ImmutableList<String> filteredDeps = evalToListOfStrings("deps(//test:my_rule)");
assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits));
assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits));
assertThat(filteredDeps).contains(explicits);
assertThat(filteredDeps).doesNotContain(implicits);
}

// Regression test for b/148550864
Expand Down Expand Up @@ -400,6 +403,74 @@ public void testNoImplicitDeps_platformDeps() throws Exception {
assertThat(evalToListOfStrings("deps(//test:my_rule)")).containsExactly("//test:my_rule");
}

// Regression test for b/275502129.
@Test
public void testNoImplicitDepsFromAutoExecGroups_autoExecGroupsEnabled() throws Exception {
writeSimpleToolchain();
writeFile(
"test/aeg/defs.bzl",
"def _impl(ctx):",
" return []",
"custom_rule = rule(",
" implementation = _impl,",
" toolchains = ['//test:toolchain_type'],",
")");
writeFile(
"test/aeg/BUILD",
"load('//test/aeg:defs.bzl', 'custom_rule')",
"custom_rule(name = 'custom_rule_name')");
((PostAnalysisQueryHelper<T>) helper)
.useConfiguration("--incompatible_auto_exec_groups", "--extra_toolchains=//test:all");

String implicits = "//test:toolchain_impl";
String explicits = "//test/aeg:custom_rule_name";

// Check for implicit toolchain dependencies
assertThat(evalToListOfStrings("deps(//test/aeg:custom_rule_name)"))
.containsAtLeast(explicits, implicits);

helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS);
ImmutableList<String> filteredDeps = evalToListOfStrings("deps(//test/aeg:custom_rule_name)");
assertThat(filteredDeps).contains(explicits);
assertThat(filteredDeps).doesNotContain(implicits);
}

// Regression test for b/275502129.
@Test
public void testNoImplicitDepsFromCustomExecGroups_autoExecGroupsEnabled() throws Exception {
writeSimpleToolchain();
writeFile(
"test/aeg/defs.bzl",
"def _impl(ctx):",
" return []",
"custom_rule = rule(",
" implementation = _impl,",
" exec_groups = {",
" 'custom_exec_group': exec_group(",
" toolchains = ['//test:toolchain_type'],",
" ),",
" },",
")");
writeFile(
"test/aeg/BUILD",
"load('//test/aeg:defs.bzl', 'custom_rule')",
"custom_rule(name = 'custom_rule_name')");
((PostAnalysisQueryHelper<T>) helper)
.useConfiguration("--incompatible_auto_exec_groups", "--extra_toolchains=//test:all");

String implicits = "//test:toolchain_impl";
String explicits = "//test/aeg:custom_rule_name";

// Check for implicit toolchain dependencies
assertThat(evalToListOfStrings("deps(//test/aeg:custom_rule_name)"))
.containsAtLeast(explicits, implicits);

helper.setQuerySettings(Setting.NO_IMPLICIT_DEPS);
ImmutableList<String> filteredDeps = evalToListOfStrings("deps(//test/aeg:custom_rule_name)");
assertThat(filteredDeps).contains(explicits);
assertThat(filteredDeps).doesNotContain(implicits);
}

@Test
public void testNoImplicitDeps_computedDefault() throws Exception {
MockRule computedDefaultRule =
Expand Down

0 comments on commit b27d1a3

Please sign in to comment.