diff --git a/site/en/concepts/visibility.md b/site/en/concepts/visibility.md index 8bddd2b2b3b7ae..4214989e3834cf 100644 --- a/site/en/concepts/visibility.md +++ b/site/en/concepts/visibility.md @@ -232,18 +232,22 @@ every instance of that rule. For example, a `cc_library` rule might create an implicit dependency from each of its rule targets to an executable target representing a C++ compiler. -Currently, for visibility purposes these implicit dependencies are treated like -any other dependency. This means that the target being depended on (such as our -C++ compiler) must be visible to every instance of the rule. In practice this -usually means the target must have public visibility. - -You can change this behavior by setting -[`--incompatible_visibility_private_attributes_at_definition`](https://github.com/bazelbuild/proposals/blob/master/designs/2019-10-15-tool-visibility.md){: .external}. When enabled, the -target in question need only be visible to the rule declaring it an implicit -dependency. That is, it must be visible to the package containing the `.bzl` -file in which the rule is defined. In our example, the C++ compiler could be -private so long as it lives in the same package as the definition of the -`cc_library` rule. +The visibility of such an implicit dependency is checked with respect to the +package containing the `.bzl` file in which the rule (or aspect) is defined. In +our example, the C++ compiler could be private so long as it lives in the same +package as the definition of the `cc_library` rule. As a fallback, if the +implicit dependency is not visible from the definition, it is checked with +respect to the `cc_library` target. + +You can change this behavior by disabling +[`--incompatible_visibility_private_attributes_at_definition`](https://github.com/bazelbuild/proposals/blob/master/designs/2019-10-15-tool-visibility.md){: .external}. +When disabled, implicit dependencies are treated like any other dependency. +This means that the target being depended on (such as our C++ compiler) must be +visible to every instance of the rule. In practice this usually means the target +must have public visibility. + +If you want to restrict the usage of a rule to certain packages, use +[load visibility](#load-visibility) instead. ## Load visibility {:#load-visibility} 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 af51abba76a983..0adf8dcf735fec 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/BUILD +++ b/src/main/java/com/google/devtools/build/lib/analysis/BUILD @@ -663,6 +663,7 @@ java_library( "//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 b5721702e1c3b5..7a40a6b03a6ed3 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 @@ -13,10 +13,12 @@ // limitations under the License. package com.google.devtools.build.lib.analysis; +import static com.google.common.base.Preconditions.checkNotNull; import com.google.devtools.build.lib.analysis.AliasProvider.TargetMode; import com.google.devtools.build.lib.analysis.RuleContext.PrerequisiteValidator; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.cmdline.PackageIdentifier; +import com.google.devtools.build.lib.packages.Aspect; import com.google.devtools.build.lib.packages.Attribute; import com.google.devtools.build.lib.packages.FunctionSplitTransitionAllowlist; import com.google.devtools.build.lib.packages.InputFile; @@ -27,6 +29,7 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.RuleClass; +import com.google.devtools.build.lib.packages.StarlarkAspectClass; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.skyframe.ConfiguredTargetAndData; @@ -80,27 +83,54 @@ private void validateDirectPrerequisiteVisibility( AliasProvider.getDependencyLabel(prerequisite.getConfiguredTarget()) .getPackageIdentifier()) && !Attribute.isLateBound(attrName)) { + // Only verify visibility of implicit dependencies of the current aspect. + // Dependencies of other aspects as well as the rule itself are checked when they are + // evaluated. + Aspect mainAspect = context.getMainAspect(); + if (mainAspect != null) { + if (!attribute.isImplicit() + || !mainAspect.getDefinition().getAttributes().containsKey(attrName)) { + return; + } + } // Determine if we should use the new visibility rules for tools. boolean toolCheckAtDefinition = - context - .getStarlarkSemantics() - .getBool( - BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION); + context + .getStarlarkSemantics() + .getBool( + BuildLanguageOptions.INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION); if (!toolCheckAtDefinition - || !attribute.isImplicit() - || attribute.getName().equals(RuleClass.CONFIG_SETTING_DEPS_ATTRIBUTE) - || rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel() == null) { + || !attribute.isImplicit() + || 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())) { handleVisibilityConflict(context, prerequisite, rule.getLabel()); } } else { - // For implicit attributes, check if the prerequisite is visible from the location of the - // rule definition - Label implicitDefinition = rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel(); - if (!RuleContext.isVisible(implicitDefinition, prerequisite.getConfiguredTarget())) { + // For implicit attributes of Starlark rules or aspects, check if the prerequisite is visible + // from the location of the definition that declares the attribute. Only perform this check + // for the current aspect. + Label implicitDefinition = null; + if (mainAspect != null) { + StarlarkAspectClass aspectClass = (StarlarkAspectClass) mainAspect.getAspectClass(); + // Never null since we already checked that the aspect is Starlark-defined. + implicitDefinition = checkNotNull(aspectClass.getExtensionLabel()); + } else { + // Never null since we already checked that the rule is a Starlark rule. + implicitDefinition = + checkNotNull(rule.getRuleClassObject().getRuleDefinitionEnvironmentLabel()); + } + // Check that the prerequisite is visible from the definition. As a fallback, check if the + // 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())) { + // In the error message, always suggest making the prerequisite visible from the definition, + // not the target. handleVisibilityConflict(context, prerequisite, implicitDefinition); } } 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 6c532fd3c664e0..ac42b1f791221f 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 @@ -34,6 +34,7 @@ import com.google.common.collect.Lists; import com.google.common.collect.Maps; import com.google.common.collect.Multimaps; +import com.google.common.collect.Streams; import com.google.common.collect.Sets; import com.google.devtools.build.lib.actions.ActionAnalysisMetadata; import com.google.devtools.build.lib.actions.ActionLookupKey; @@ -84,6 +85,7 @@ import com.google.devtools.build.lib.packages.Rule; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClass.ConfiguredTargetFactory.RuleErrorException; +import com.google.devtools.build.lib.packages.StarlarkAspectClass; import com.google.devtools.build.lib.packages.SymbolGenerator; import com.google.devtools.build.lib.packages.Target; import com.google.devtools.build.lib.packages.TargetUtils; @@ -2113,6 +2115,20 @@ public boolean isVisible(TransitiveInfoCollection prerequisite) { return RuleContext.isVisible(target.getAssociatedRule(), prerequisite); } + @Nullable + Aspect getMainAspect() { + return Streams.findLast(aspects.stream()).orElse(null); + } + + boolean isStarlarkRuleOrAspect() { + Aspect mainAspect = getMainAspect(); + if (mainAspect != null) { + return mainAspect.getAspectClass() instanceof StarlarkAspectClass; + } else { + return getRule().getRuleClassObject().getRuleDefinitionEnvironmentLabel() != null; + } + } + private void validateDirectPrerequisiteFileTypes( ConfiguredTargetAndData prerequisite, Attribute attribute) { if (attribute.isSkipAnalysisTimeFileTypeCheck()) { diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java index 469427b6f32856..248a2c5b6f05c8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java +++ b/src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java @@ -472,13 +472,13 @@ public final class BuildLanguageOptions extends OptionsBase { @Option( name = "incompatible_visibility_private_attributes_at_definition", - defaultValue = "false", + defaultValue = "true", documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS}, metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, help = "If set to true, the visibility of private rule attributes is checked with respect " - + "to the rule definition, rather than the rule usage.") + + "to the rule definition, falling back to rule usage if not visible.") public boolean incompatibleVisibilityPrivateAttributesAtDefinition; @Option( @@ -865,7 +865,7 @@ public StarlarkSemantics toStarlarkSemantics() { public static final String INCOMPATIBLE_UNAMBIGUOUS_LABEL_STRINGIFICATION = "+incompatible_unambiguous_label_stringification"; public static final String INCOMPATIBLE_VISIBILITY_PRIVATE_ATTRIBUTES_AT_DEFINITION = - "-incompatible_visibility_private_attributes_at_definition"; + "+incompatible_visibility_private_attributes_at_definition"; public static final String INCOMPATIBLE_TOP_LEVEL_ASPECTS_REQUIRE_PROVIDERS = "-incompatible_top_level_aspects_require_providers"; public static final String INCOMPATIBLE_DISABLE_STARLARK_HOST_TRANSITIONS = diff --git a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java index fbba584ab58360..c93a4ceffd7f03 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/AspectTest.java @@ -1348,6 +1348,71 @@ public void aspectSeesAspectHintsAttributeOnStarlarkRule() throws Exception { assertThat(info.truncateToInt()).isEqualTo(2); } + @Test + public void ruleDepsVisibilityNotAffectNativeAspect() throws Exception { + setRulesAndAspectsAvailableInTests( + ImmutableList.of(TestAspects.ALL_ATTRIBUTES_ASPECT), ImmutableList.of()); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + scratch.file("defs/BUILD"); + scratch.file( + "defs/build_defs.bzl", + "def _rule_impl(ctx):", + " pass", + "", + "implicit_dep_rule = rule(", + " implementation = _rule_impl,", + " attrs = {", + " '_tool': attr.label(default = '//tool:tool'),", + " 'deps': attr.label_list()", + " },", + ")"); + scratch.file("tool/BUILD", "sh_library(name='tool', visibility = ['//defs:__pkg__'])"); + scratch.file( + "pkg/BUILD", + "load('//defs:build_defs.bzl', 'implicit_dep_rule')", + "implicit_dep_rule(name='y')", + "implicit_dep_rule(name='x', deps = [':y'])"); + + AnalysisResult result = + update( + new EventBus(), + defaultFlags(), + ImmutableList.of(TestAspects.ALL_ATTRIBUTES_ASPECT.getName()), + "//pkg:x"); + + assertThat(result.hasError()).isFalse(); + } + + @Test + public void nativeAspectFailIfDepsNotVisible() throws Exception { + scratch.file("tool/BUILD", "sh_library(name='tool', visibility = ['//visibility:private'])"); + ExtraAttributeAspect extraAttributeAspect = new ExtraAttributeAspect("//tool:tool", false); + setRulesAndAspectsAvailableInTests(ImmutableList.of(extraAttributeAspect), ImmutableList.of()); + scratch.file( + "pkg/build_defs.bzl", + "def _rule_impl(ctx):", + " pass", + "", + "simple_rule = rule(", + " implementation = _rule_impl,", + ")"); + scratch.file( + "pkg/BUILD", "load('//pkg:build_defs.bzl', 'simple_rule')", "simple_rule(name='x')"); + reporter.removeHandler(failFastHandler); + + assertThrows( + ViewCreationFailedException.class, + () -> + update( + new EventBus(), + defaultFlags(), + ImmutableList.of(extraAttributeAspect.getName()), + "//pkg:x")); + assertContainsEvent( + "ExtraAttributeAspect_//tool:tool_false aspect on simple_rule rule //pkg:x: target" + + " '//tool:tool' is not visible from target '//pkg:x'."); + } + private void setupAspectHints() throws Exception { scratch.file( "aspect_hints/hints.bzl", 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 3a3e377974de0d..8fcafca3813605 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 @@ -18,6 +18,7 @@ import static org.junit.Assert.assertThrows; import com.google.devtools.build.lib.analysis.util.AnalysisTestCase; +import org.junit.Ignore; import org.junit.Test; import org.junit.runner.RunWith; import org.junit.runners.JUnit4; @@ -92,14 +93,15 @@ public void testToolVisibilityUseCheckAtUse() throws Exception { } @Test - public void testToolVisibilityUseCheckAtRule() throws Exception { + public void testToolVisibilityUseCheckAtRule_fallbackToUse() throws Exception { setupArgsScenario(); scratch.file("data/BUILD", "exports_files(['data.txt'], visibility=['//visibility:public'])"); scratch.file("tool/BUILD", "exports_files(['tool.sh'], visibility=['//use:__pkg__'])"); useConfiguration("--incompatible_visibility_private_attributes_at_definition"); - reporter.removeHandler(failFastHandler); - assertThrows(ViewCreationFailedException.class, () -> update("//use:world")); + update("//use:world"); + + assertThat(hasErrors(getConfiguredTarget("//use:world"))).isFalse(); } @Test @@ -202,6 +204,380 @@ public void testConfigSettingVisibilityAlwaysCheckedAtUse() throws Exception { assertThat(hasErrors(getConfiguredTarget("//:my_target"))).isFalse(); } + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + + update("//foo:target_with_aspects"); + + assertThat(hasErrors(getConfiguredTarget("//foo:target_with_aspects"))).isFalse(); + } + + @Ignore( + "TODO(b/206127051): The aspects path implicit dependencies with same name are incorrectly" + + " merged, enable this test when this is fixed.") + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_visibleWithNameCollision() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + + update("//foo:target_with_aspects"); + + assertThat(hasErrors(getConfiguredTarget("//foo:target_with_aspects"))).isFalse(); + } + + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_outerAspectToolNotVisible() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = [", + " '//inner_aspect:__pkg__',", + " '//rule:__pkg__',", + " ],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + reporter.removeHandler(failFastHandler); + + assertThrows(ViewCreationFailedException.class, () -> update("//foo:target_with_aspects")); + assertContainsEvent( + "in //inner_aspect:lib.bzl%inner_aspect,//outer_aspect:lib.bzl%outer_aspect " + + "aspect on simple_starlark_rule rule //foo:simple_dep: target " + + "'//tool:outer_aspect_tool' is not visible from target '//outer_aspect:lib.bzl'"); + } + + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_innerAspectToolNotVisible() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = [", + " '//outer_aspect:__pkg__',", + " '//rule:__pkg__',", + " ],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = ['//rule:__pkg__'],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + reporter.removeHandler(failFastHandler); + + assertThrows(ViewCreationFailedException.class, () -> update("//foo:target_with_aspects")); + assertContainsEvent( + "in //inner_aspect:lib.bzl%inner_aspect aspect on simple_starlark_rule " + + "rule //foo:simple_dep: target '//tool:inner_aspect_tool' is not visible from " + + "target '//inner_aspect:lib.bzl'"); + } + + @Test + public void testAspectImplicitDependencyCheckedAtDefinition_ruleToolNotVisible() + throws Exception { + scratch.file("inner_aspect/BUILD"); + scratch.file( + "inner_aspect/lib.bzl", + "InnerAspectInfo = provider()", + "def _impl_inner_aspect(ctx, target):", + " return [InnerAspectInfo()]", + "inner_aspect = aspect(", + " _impl_inner_aspect,", + " attrs = { '_inner_aspect_tool': attr.label(default = '//tool:inner_aspect_tool') },", + " provides = [InnerAspectInfo],", + ")"); + scratch.file("outer_aspect/BUILD"); + scratch.file( + "outer_aspect/lib.bzl", + "load('//inner_aspect:lib.bzl', 'InnerAspectInfo')", + "def _impl_outer_aspect(ctx, target):", + " return []", + "outer_aspect = aspect(", + " _impl_outer_aspect,", + " attrs = { '_outer_aspect_tool': attr.label(default = '//tool:outer_aspect_tool') },", + " required_aspect_providers = [InnerAspectInfo],", + ")"); + scratch.file("rule/BUILD"); + scratch.file( + "rule/lib.bzl", + "load('//inner_aspect:lib.bzl', 'inner_aspect')", + "load('//outer_aspect:lib.bzl', 'outer_aspect')", + "def _impl(ctx):", + " pass", + "my_rule = rule(", + " _impl,", + " attrs = {", + " 'dep': attr.label(aspects = [inner_aspect, outer_aspect]),", + " '_rule_tool': attr.label(default = '//tool:rule_tool'),", + " },", + ")", + "simple_starlark_rule = rule(", + " _impl,", + ")"); + scratch.file( + "foo/BUILD", + "load('//rule:lib.bzl','my_rule', 'simple_starlark_rule')", + "simple_starlark_rule(name = 'simple_dep')", + "my_rule(name = 'target_with_aspects', dep = ':simple_dep')"); + scratch.file( + "tool/BUILD", + "sh_binary(", + " name = 'outer_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//outer_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'inner_aspect_tool',", + " srcs = ['a.sh'],", + " visibility = ['//inner_aspect:__pkg__'],", + ")", + "sh_binary(", + " name = 'rule_tool',", + " srcs = ['a.sh'],", + " visibility = [", + " '//outer_aspect:__pkg__',", + " '//inner_aspect:__pkg__',", + " ],", + ")"); + useConfiguration("--incompatible_visibility_private_attributes_at_definition"); + reporter.removeHandler(failFastHandler); + + assertThrows(ViewCreationFailedException.class, () -> update("//foo:target_with_aspects")); + assertContainsEvent( + "in my_rule rule //foo:target_with_aspects: target '//tool:rule_tool' is " + + "not visible from target '//rule:lib.bzl'"); + } + void setupFilesScenario(String wantRead) throws Exception { scratch.file("src/source.txt", "source"); scratch.file("src/BUILD", "exports_files(['source.txt'], visibility=['//pkg:__pkg__'])");