From 10eae89d23a173f6d1601af2a5e3efc4d9c75251 Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 4 Mar 2024 14:11:22 -0800 Subject: [PATCH 1/2] Implicit dependencies should be visible to rule/aspect definitions in `.bzl` files in the same package Fixes: [] PiperOrigin-RevId: 612579829 Change-Id: I429ca24931482cc94543692532ea4c08692020e3 --- .../google/devtools/build/lib/analysis/BUILD | 2 +- .../analysis/CommonPrerequisiteValidator.java | 36 ++++++++++---- .../build/lib/analysis/RuleContext.java | 39 --------------- .../build/lib/analysis/VisibilityTest.java | 48 +++++++++++++++++++ 4 files changed, 75 insertions(+), 50 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index a8c437f0b0eb42..31bced56210ba3 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -655,9 +655,9 @@ java_library( deps = [ ":analysis_cluster", ":rule_error_consumer", + ":visibility_provider", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", - "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//third_party:guava", ], diff --git a/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java index 2544f7b1acebbe..43e968e8824e77 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java @@ -26,6 +26,7 @@ import com.google.devtools.build.lib.packages.FunctionSplitTransitionAllowlist; import com.google.devtools.build.lib.packages.InputFile; import com.google.devtools.build.lib.packages.NonconfigurableAttributeMapper; +import com.google.devtools.build.lib.packages.PackageSpecification.PackageGroupContents; import com.google.devtools.build.lib.packages.RawAttributeMapper; import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; @@ -80,13 +81,6 @@ private void validateDirectPrerequisiteVisibility( checkVisibilityAttributeContents(context, prerequisite, attribute, attrName, rule); - if (isSameLogicalPackage( - rule.getLabel().getPackageIdentifier(), - AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget()) - .getPackageIdentifier())) { - return; - } - // We don't check the visibility of late-bound attributes, because it would break some // features. if (Attribute.isLateBound(attrName)) { @@ -121,7 +115,7 @@ private void validateDirectPrerequisiteVisibility( || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE) || !context.isStarlarkRuleOrAspect()) { // Default check: The attribute must be visible from the target. - if (!context.isVisible(prerequisite.getConfiguredTarget())) { + if (!isVisible(rule.getLabel(), prerequisite)) { handleVisibilityConflict(context, prerequisite, rule.getLabel()); } } else { @@ -142,8 +136,8 @@ private void validateDirectPrerequisiteVisibility( // prerequisite is visible from the target so that adopting this new style of checking // visibility is not a breaking change. if (implicitDefinition != null - && !RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget()) - && !context.isVisible(prerequisite.getConfiguredTarget())) { + && !isVisible(implicitDefinition, prerequisite) + && !isVisible(rule.getLabel(), prerequisite)) { // In the error message, always suggest making the prerequisite visible from the definition, // not the target. handleVisibilityConflict(context, prerequisite, implicitDefinition); @@ -151,6 +145,28 @@ private void validateDirectPrerequisiteVisibility( } } + private boolean isVisible(Label target, ConfiguredTargetAndData prerequisite) { + if (isSameLogicalPackage( + target.getPackageIdentifier(), + AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget()) + .getPackageIdentifier())) { + return true; + } + // Check visibility attribute + for (PackageGroupContents specification : + prerequisite + .getConfiguredTarget() + .getProvider(VisibilityProvider.class) + .getVisibility() + .toList()) { + if (specification.containsPackage(target.getPackageIdentifier())) { + return true; + } + } + + return false; + } + private void checkVisibilityAttributeContents( RuleContext.Builder context, ConfiguredTargetAndData prerequisite, diff --git a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java index ecb5c28e176382..d20c04c48de6fd 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/RuleContext.java @@ -1526,34 +1526,6 @@ public boolean isExecutedOnWindows() { .hasConstraintValue(OS_TO_CONSTRAINTS.get(OS.WINDOWS)); } - /** - * Returns true if {@code label} is visible from {@code prerequisite}. - * - *

This only computes the logic as implemented by the visibility system. The final decision - * whether a dependency is allowed is made by {@link PrerequisiteValidator}. - */ - public static boolean isVisible(Label label, TransitiveInfoCollection prerequisite) { - // Check visibility attribute - for (PackageGroupContents specification : - prerequisite.getProvider(VisibilityProvider.class).getVisibility().toList()) { - if (specification.containsPackage(label.getPackageIdentifier())) { - return true; - } - } - - return false; - } - - /** - * Returns true if {@code rule} is visible from {@code prerequisite}. - * - *

This only computes the logic as implemented by the visibility system. The final decision - * whether a dependency is allowed is made by {@link PrerequisiteValidator}. - */ - public static boolean isVisible(Rule rule, TransitiveInfoCollection prerequisite) { - return isVisible(rule.getLabel(), prerequisite); - } - /** * @return the set of features applicable for the current rule. */ @@ -1977,17 +1949,6 @@ public BuildConfigurationValue getConfiguration() { return configuration; } - /** - * @return true if {@code rule} is visible from {@code prerequisite}. - *

This only computes the logic as implemented by the visibility system. The final - * decision whether a dependency is allowed is made by {@link PrerequisiteValidator}, who is - * supposed to call this method to determine whether a dependency is allowed as per - * visibility rules. - */ - public boolean isVisible(TransitiveInfoCollection prerequisite) { - return RuleContext.isVisible(target.getAssociatedRule(), prerequisite); - } - @Nullable Aspect getMainAspect() { return Streams.findLast(aspects.stream()).orElse(null); diff --git a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java index c725d6bbaa54cf..20f6f3a6fb57eb 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java @@ -204,6 +204,54 @@ public void testConfigSettingVisibilityAlwaysCheckedAtUse() throws Exception { assertThat(hasErrors(getConfiguredTarget("//:my_target"))).isFalse(); } + @Test + public void testImplicitDependency_samePackageAsDefinition_visible() throws Exception { + scratch.file( + "aspect_def/BUILD", + "sh_binary(", + " name = 'aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//visibility:private'])"); + scratch.file( + "aspect_def/lib.bzl", + "def _impl_my_aspect(ctx, target):", + " return []", + "my_aspect = aspect(", + " _impl_my_aspect,", + " attrs = { '_aspect_tool': attr.label(default = '//aspect_def:aspect_tool') },", + ")"); + scratch.file( + "rule_def/BUILD", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//visibility:private'])"); + scratch.file( + "rule_def/lib.bzl", + "load('//aspect_def:lib.bzl', 'my_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [my_aspect]),", + " '_rule_tool': attr.label(default = '//rule_def:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule_def:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'my_target', dep = ':simple_dep')"); + + update("//foo:my_target"); + + assertThat(hasErrors(getConfiguredTarget("//foo:my_target"))).isFalse(); + } + @Test public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exception { scratch.file("inner_aspect/BUILD"); From fe69afdf91b3c775afafc5330d478647349a8384 Mon Sep 17 00:00:00 2001 From: iancha1992 Date: Tue, 5 Mar 2024 11:17:36 -0800 Subject: [PATCH 2/2] added deps --- src/main/java/com/google/devtools/build/lib/analysis/BUILD | 1 + 1 file changed, 1 insertion(+) diff --git a/src/main/java/com/google/devtools/build/lib/analysis/BUILD b/src/main/java/com/google/devtools/build/lib/analysis/BUILD index 31bced56210ba3..4d31fd67fe73de 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -658,6 +658,7 @@ java_library( ":visibility_provider", "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/skyframe:configured_target_and_data", "//third_party:guava", ],