From b27d1a3b66628ec479cdc4dc8629a4bddc0320d1 Mon Sep 17 00:00:00 2001 From: Googler Date: Fri, 31 Mar 2023 07:50:18 -0700 Subject: [PATCH] Fix regression with implicit deps which Automatic Exec Groups caused 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 --- .../devtools/build/lib/analysis/Util.java | 31 +++-- .../testutil/PostAnalysisQueryTest.java | 131 ++++++++++++++---- 2 files changed, 119 insertions(+), 43 deletions(-) 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 193cb9396eeabd..d4519d08645963 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 @@ -95,19 +95,24 @@ 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. - BuildConfigurationValue targetConfiguration = ruleContext.getConfiguration(); - for (Label toolchain : toolchainContext.resolvedToolchainLabels()) { - maybeImplicitDeps.add( - ConfiguredTargetKey.builder() - .setLabel(toolchain) - .setConfiguration(targetConfiguration) - .setExecutionPlatformLabel(toolchainContext.executionPlatform().label()) - .build()); + + ToolchainCollection 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)); 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 21b18bc8bf5f6b..f2f8855c3ad37f 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 @@ -259,27 +259,39 @@ 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 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(", @@ -287,32 +299,24 @@ public void testNoImplicitDeps_starlark_toolchains() throws Exception { " 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) 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 filteredDeps = evalToListOfStrings("deps(//test:my_rule)"); - assertThat(filteredDeps).containsAtLeastElementsIn(evalToListOfStrings(explicits)); - assertThat(filteredDeps).containsNoneIn(evalToListOfStrings(implicits)); + ImmutableList filteredDeps = evalToListOfStrings("deps(//test/rule:my_rule)"); + assertThat(filteredDeps).contains(explicits); + assertThat(filteredDeps).doesNotContain(implicits); } @Test @@ -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 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 @@ -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) 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 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) 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 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 =