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/CommonPrerequisiteValidator.java b/src/main/java/com/google/devtools/build/lib/analysis/CommonPrerequisiteValidator.java index 855ed55e83d24e..d8e1780db43d2b 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 @@ -86,10 +86,10 @@ private void validateDirectPrerequisiteVisibility( // 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() @@ -119,8 +119,14 @@ private void validateDirectPrerequisiteVisibility( 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())) { + && !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/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/VisibilityTest.java b/src/test/java/com/google/devtools/build/lib/analysis/VisibilityTest.java index 30b3e96b3a3862..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 @@ -93,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 @@ -274,7 +275,7 @@ public void testAspectImplicitDependencyCheckedAtDefinition_visible() throws Exc } @Ignore( - "TODO(b/206127051): The aspects path implicit dependendencies with same name are incorrectly" + "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() @@ -402,7 +403,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_outerAspectToolNotVi " visibility = [", " '//inner_aspect:__pkg__',", " '//rule:__pkg__',", - " '//foo:__pkg__',", " ],", ")", "sh_binary(", @@ -485,7 +485,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_innerAspectToolNotVi " visibility = [", " '//outer_aspect:__pkg__',", " '//rule:__pkg__',", - " '//foo:__pkg__',", " ],", ")", "sh_binary(", @@ -568,7 +567,6 @@ public void testAspectImplicitDependencyCheckedAtDefinition_ruleToolNotVisible() " visibility = [", " '//outer_aspect:__pkg__',", " '//inner_aspect:__pkg__',", - " '//foo:__pkg__',", " ],", ")"); useConfiguration("--incompatible_visibility_private_attributes_at_definition");