Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature request: visibility-like capability for bzl files (rules, aspects, and macros) #11261

Closed
dgoldstein0 opened this issue Apr 30, 2020 · 11 comments
Assignees
Labels
P1 I'll work on this now. (Assignee required) team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request

Comments

@dgoldstein0
Copy link

dgoldstein0 commented Apr 30, 2020

Description of the problem / feature request:

Today, if you want to restrict the usage of a bazel target, you can use visibility to do so. A simple example:

filegroup(
  name = "foo",
  visibility = ["//visibility:private"],
  srcs = ...
)

In the above, :foo can only be referenced by other targets in the same package. There are of course ways to indicate other packages and subpackages are allowed to use a target.

bzl files, and their contents (rules, macros, aspects, etc) have no such capability. Any .bzl file in your workspace, can be load()ed by any other bzl file or BUILD file. There is one hack, however, you can use to restrict the effective visibility of a rule: you can add a private attr.label attribute that points to a target of limited visibility. This however is going to give a somewhat confusing error message about that target not being visible when the rule is used somewhere it doesn't belong. But in some cases that may be an ok workaround.

Feature requests: what underlying problem are you trying to solve with this feature?

One particular use case which is not served by this, is the ability to make a macro which is usable anywhere, use a rule that you want restricted. E.g. something like

def foo(name, ...):
  foo_internal(name, ...)
  bar(name + "_bar", ...)

In such a case, you may want to expose the inner rules - foo_internal and bar - so that some of your tests can use them; but you may want all other consumers forced to use foo. Currently, none of bazel's restriction mechanisms support this use case.

some thoughts on a possible solutions

The obvious choice for the api is to mirror existing visibility capabilities somehow, though this has a few problems:

  • terminology wise, .bzl files aren't packages, and shouldn't have their own package() declarations. This is easy enough to work around though if we can agree on an alternate name . Maybe bzl_settings() which could also support default_visibility? and if we wanted to go further and have a way to make rules/macros/aspects testonly, we could throw in default_testonly
  • while aspect and rule could be extended to have a visibility parameter, it's less obvious what to do with macros, as macros just look like functions.
    • A possible, extreme answer would be to introduce a macro function, that takes an implementation pointing to the existing functions. The good news is, this would be easy to adopt via codemods, though is more verbose than the status quo. It may even have benefits for stardoc, as it could give a place to add more metadata about macros, like the expected types of different parameters.
    • An alternative idea is to have some sort of psuedo-rule that could declare the visibility of the macro - e,g, a macro_visibility(...) could declare the effective visibility for the foo macro:
def foo(name, ...):
  macro_visibility(["//visibility:public"])
  foo_internal(...)
  bar(...)

foo_internal = rule(
  visibility = ["//tools/foo/tests:__subpackages__"],
  implementation = ...
  attrs = ...
  doc = "foo_internal does <something useful>.  It's is intended to be used everywhere via the foo() macro",
)

For any version of this api to be useful, the "visibility" of macros would have to be enforced in the loading phase, or load into a rule that could error on visibility during analysis; and the visibility of rules within macros would have to be compared to bzl file that contained the macro, not the package that the rule ends up within after the loading phase.

@benjaminp
Copy link
Collaborator

@dgoldstein0
Copy link
Author

dgoldstein0 commented Apr 30, 2020 via email

@laurentlb laurentlb added P3 We're not considering working on this, but happy to review a PR. (No assignee) type: feature request and removed untriaged labels May 5, 2020
@brandjon
Copy link
Member

We are actively interested in (i.e., designing this quarter) a feature for load()-level enforcement of .bzl visibility.

This would not include separate visibility declarations for individual symbols within a .bzl file, but you could accomplish something similar by re-exporting these symbols through different facade files with different visibility specifications.

@brandjon brandjon added P1 I'll work on this now. (Assignee required) team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols and removed P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Starlark labels Feb 19, 2021
@brandjon brandjon self-assigned this Feb 19, 2021
@brandjon
Copy link
Member

Update: Currently working on a design doc, expect to share it in a week or so.

@coreywoodfield
Copy link

@brandjon any update on this?

@brandjon
Copy link
Member

Update: We're actively implementing a simplified prototype, but the real design is not finalized.

copybara-service bot pushed a commit that referenced this issue Jun 28, 2022
BazelStarlarkContext is a hodgepodge of Bazel-specific state needed for or manipulated by various Starlark evaluations. A minor refactoring goal is to break this class up into subclasses, each one representing a different kind of Starlark evaluation that needs different kinds of data. This avoids the proliferation of new @nullable fields that may or may not be set and used for a given evaluation.

This CL begins the process by breaking out the case of initializing a .bzl module (i.e. running its top-level code). The other cases should follow a similar pathway, but I don't intend to pursue that in the immediate future. My motivation for tackling .bzl initialization now is that I want to add a field needed for the .bzl visibility without feeling guilty.

I also took this opportunity to clarify that BazelStarlarkContext ought not to be shared or reused between multiple StarlarkThreads, and fixed a location where this was violated by StarlarkCallbackHelper.

Work towards #11261.

PiperOrigin-RevId: 457732549
Change-Id: I3ee869865c47457275f1b84819f0f154290aa22a
copybara-service bot pushed a commit that referenced this issue Jun 28, 2022
A follow-up CL will add the `visibility()` callable that will allow the user to declare a visibility. The present CL adds:

- the BzlVisibility type
- a field for the currently evaluating Starlark thread to store the value set by calling `visibility()`
- a field to save this value on the resulting BzlLoadValue, for use when validating (future CL) load statements

I finally inlined ConfiguredRuleClassProvider#setStarlarkThreadContext into its sole call site within BzlLoadFunction. This allows the caller of BzlLoadFunction#executeBzlFile to own the context object and retrieve the visibility directly from it. The alternative would require adding some kind of return value or out parameter to executeBzlFile. This change also required refining getConfigurationFragmentMap's return type and adding an accessor for the network allowlist (constrained by RuleDefinitionEnvironment inheritance to be an Optional).

Work towards #11261.

PiperOrigin-RevId: 457738773
Change-Id: I583d53f0440b3e63eeb207f9a178a2e4cfb512f5
copybara-service bot pushed a commit that referenced this issue Jul 1, 2022
This adds a `visibility()` callable to .bzl files, to be used during top-level evaluation (and ideally, immediately after `load()` statements at the top of the file). `visibility("public")` declares the .bzl to be loadable by anyone (the default), while `visibility("private")` ensures it can only be loaded by BUILD/.bzl files in the same package. A follow-up CL will add the ability to allowlist package paths.

This API is not final, and is guarded behind two experimental flags: --experimental_bzl_visibility to guard the feature overall, and --experimental_bzl_visibility_allowlist to only permit certain packages to declare a visibility. (For technical reasons the `visibility()` builtin itself is available unconditionally even though it can only be successfully called when those flag checks are satisfied.)

The allowlist implementation does not handle repository remapping correctly and therefore may not work in bzlmod. Our plan is to prototype the feature within Google and then remove allowlisting altogether, rather than fix the allowlisting to accommodate repository remapping.

Work towards #11261.

PiperOrigin-RevId: 458528142
Change-Id: I3d15d902edf10f6676eac08ebb1e02f6451f5c26
copybara-service bot pushed a commit that referenced this issue Jul 2, 2022
E.g., `visibility(["//foo", "//bar/subpkg"])`. Package globs like "//pkg/*" are not allowed, nor are anything that uses repo syntax ('@'). This is sufficient for prototyping bzl-visibility within Google.

For production usage in the final design, we likely will want to allow //pkg/* syntax, but not repo syntax since it makes little sense to allow something in another repo without also allowing the whole world (public visibility). This also matches how package_group does not currently permit including individual packages from other repos. We might still at some point allow "@the_current_repo_name//pkg" in order to make this check a semantic one on the package name rather than a syntactic one on the string itself.

Work towards #11261.

PiperOrigin-RevId: 458577792
Change-Id: I31605909580ebca13007be3d81e76dfc358e877f
copybara-service bot pushed a commit that referenced this issue Jul 21, 2022
This lets us use --experimental_bzl_visibility_allowlist to enable the .bzl load-visibility unconditionally. This eases migration for graveyarding the flag.

Also did a drive-by docs tweak.

Work towards #11261.

PiperOrigin-RevId: 462480270
Change-Id: Ia877bb5947db7655d91e4838f4b8a5162df9580b
copybara-service bot pushed a commit that referenced this issue Aug 24, 2022
This enables `//foo/...` wildcards both in the allowlists passed to `visibility()`, and in the allowlist guarding the experimental feature itself (`--experimental_bzl_visibility_allowlist`). Both limitations proved to be a major hurdle for onboarding experimental users: There are over four hundred packages owned by the relatively small number of teams that have expressed interest.

Changes:
- Ad hoc parsing of visibility() strings in BazelBuildApiGlobals is replaced by reusing PackageSpecification parsing logic, which governs package_group's `packages` attribute. (This is something we would've wanted to do eventually anyway.)
- To support this, a new fromString variant is added to PackageSpecification, since the existing one is not public, nor is its exception type, nor is the capability to distinguish (and thereby disallow) negative patterns.
- Updated the allowlist string / packageid logic to tolerate `/...` in BazelBuildApiGlobals.
- Drive-by cleanups to visibility tests: Use "everyone" allowlist in test cases that are not testing the allowlist feature. Flip a couple private/public visibilities that are incorrect/irrelevant for what is being tested.
- Drive-by cleanups to misleading comments in PathFragment-related code.

Work toward #11261.

PiperOrigin-RevId: 469723348
Change-Id: Ieaca652a921d95c033456f4bca05897714c16e78
@brandjon
Copy link
Member

Update: We've been working on a design and implementation, and are aiming to include this in 6.0 (to be cut in ~a week).

copybara-service bot pushed a commit that referenced this issue Sep 21, 2022
- Change test assertions to check for the presence/absence of a PackageIdentifier, rather than actually evaluating the Packages that a package_group refers to. This is more straightforward, requires less setup boilerplate, and also allows us to write assertions that talk about packages in external repos, which PackageLoadingTestCase can't evaluate.

- Reflow scratch file contents to follow BUILD formatting.

- Add TODO mentioning a cross-repo test case that'd be nice to have, but isn't worth figuring out how to add.

- Make the negative-everything test case a little more interesting by giving it something to exclude.

- Simplify the positive-everything case's assertion and check that it admits things from other repos.

Work toward #11261.

PiperOrigin-RevId: 475837392
Change-Id: Idcee6afa3a51de569979a2dde619dfa8fd23f1a1
@meteorcloudy meteorcloudy added this to the 6.0.0 release blockers milestone Sep 27, 2022
copybara-service bot pushed a commit that referenced this issue Oct 3, 2022
codebase.md missed some changes that were made to a Google-internal version fork of this file. This CL adds them back. (I am not the author of the added content.)

Work toward #11261.

PiperOrigin-RevId: 478489612
Change-Id: Ib51bde2a047c3845e3cb340bd89bd20c0c5eb8ef
copybara-service bot pushed a commit that referenced this issue Oct 3, 2022
This is a precursor to documenting the new changes to visibility -- "public", "private", and --incomaptible_fix_package_group_syntax; and to documenting the new bzl visibility feature.

package_group() function doc:
- `into text`: Clarify visibility levels. Replace legacy "rule" terminology with more modern "target", and be a little more pedantic about granting access to packages vs targets.
- `packages` attr: Be consistent about these being "package specifications", not an enumeration of packages. Say the allowed forms, then the semantics without respect to `includes`. Mention `//...` special case, and repo behavior.
- `includes` attr: Move interaction between negation and includes to here.

dependencies.md:
- Better hint that granting access to a package group can never remove access from the package to itself ("may grant broader visibility").
- Describe visibility as a "dual" of dependency rather than "opposite".
- Paragraph break.

visibility.md:
- Again be pedantic about access for package vs target, while factoring out "targets defined in" verbiage.
- Again be pedantic about not suggesting you can remove access from a package to itself.
- Factor out mention of how the syntax is weird.
- Expand wording for load() visibility a little.

codebase.md:
- Drive by rewrite of part of Skyframe section.
- Drive-by fixes for Starlark section.
- Streamline wording of visibility section.
- Add more mention of package_group implementation classes. Maybe a little more verbose than needed, but I figured it was better to include them than not.

Work toward #11261.

PiperOrigin-RevId: 478572996
Change-Id: I1edc9bc80337976cc5dc21cf48a1d7e00924c63f
@lberki
Copy link
Contributor

lberki commented Oct 4, 2022

@brandjon Do I understand correctly that these two issues are blockers for this one:

#16323
#16355

?

copybara-service bot pushed a commit that referenced this issue Oct 4, 2022
This CL gives us a pathway for fixing stringification of package_group's `packages` attribute, so that //foo/bar is printed as "//foo/bar" and not "foo/bar". This affects e.g. `bazel query --output=proto [...]`. This change is a prerequisite to adding support for "public" and "private" string constant package specifiers, since otherwise "public" would be ambiguous with the stringification of //public.

PackageSpecification currently has two forms of stringification. The toString() method is unambiguous but omits the leading double slash, while the toStringWithoutRepository() always includes the double slash but never includes the repo name. I factored toString() into asString(boolean includeDoubleSlash) to switch between the old form with and without the fix to include slashes, and made toString() and all current callers pass false for now. I just renamed toStringWithoutRepository to asStringWithoutRepository for symmetry. Both are now clearly documented.

In PackageGroupContents, I clarified its behavior w.r.t. duplicates and order, and renamed its member vars for simplicity and in anticipation of storing another type of package spec subclass in a follow-up CL. Also renamed some methods for clarity.

Fixed a bug in AllPackagesBeneath where //... parsed in another repo would stringify without the leading "@repo". However, this bug was never exercised since it would require the follow-up CL's behavior of enabling "//..." to parse as AllPackagesBeneath rather than AllPackages (#16323).

Added a test for all three forms of stringification to PackageGroupTest.

There should be no actual behavioral change enabled in this CL.

Work toward #11261.

PiperOrigin-RevId: 478828269
Change-Id: Ifc7c08e6e90b89d33bed1ee6a5a97b3a3ac07326
@brandjon
Copy link
Member

brandjon commented Oct 5, 2022

Yes, and I'm about to add a third blocking flag as well. (Logically these are all the same flag though, in terms of the amount of migration work they represent.)

Edit: Third flag is #16391.

copybara-service bot pushed a commit that referenced this issue Oct 6, 2022
This CL does three things:

1. It adds the special package specifications "public" and "private, guarded behind the flag `--incompatible_package_group_has_public_syntax`.

2. It fixes the behavior of package specification "//..." to mean "all packages in the current repo" rather than "all packages in any repo", guarded behind the flag `--incompatible_fix_package_group_reporoot_syntax`.

3. It fixes the behavior of `bazel query --output=proto` (and `--output=xml`) so that the `packages` attribute is serialized without dropping the leading `//` from package names -- it is now `"//foo/bar/..."` instead of `"foo/bar/..."`. This behavioral change is guarded behind `--incompatible_package_group_includes_double_slash`.

The .bzl `visibility()` builtin always acts as if the first two flags are enabled.

Work toward #11261.
Work toward #16323.
Work toward #16355.
Work toward #16391.

RELNOTES: Added three `package_group`-related flags: `--incompatible_package_group_includes_double_slash` (#16391), `--incompatible_package_group_has_public_syntax` (#16355), and `--incompatible_fix_package_group_reporoot_syntax` (#16323). With these flags, `package_group` can now easily specify "all packages", "no packages", and "all packages in the current repo".

In PackageSpecification:
- fromString(), fromStringPositive(): rewrote docstring, added bool args for flags, renamed or removed vars for simplicity, added handling of public/private and flag behavior
- AllPackagesBeneath(): fixed asStringWithoutRepository() for case of //..., which wasn't likely to arise before.
- normalize order of equals(), hashCode(), etc.
- AllPackages: change hash, and change str representation to "public"
- Add NoPackages, symmetric to AllPackages
- NegativePackageSpecification: don't reuse the delegate's hash as our own hash verbatim

In PackageGroupTest:
- Move `testEverythingSpecificationWorks` to above `testNegativeEverything`, and made it use "public" syntax in place of "//...".
- Add test for "private" syntax.
- Add test for flag guarding "public"/"private".
- Add tests for old and new "//..." behavior.
- Add tests that you can't negate "public" or "private".
- Add test for illegal combination of flags.
- Add assertions for stringification of "public"/"private".

Other changes:
- Update XmlOutputFormatterTest and ProtoOutputFormatterTest with test cases for the flag. In the latter case, parameterize a helper to allow setting per-test-case flags.
- Add test cases to BzlLoadFunctionTest for bzl visibility accepting the list forms `["public"]`/`["private"]`, and for its behavior being unaffected by the new flags.

PiperOrigin-RevId: 479347358
Change-Id: I124ca5406bff15615adaa1256e2d7bef69b9d9a5
@brandjon
Copy link
Member

brandjon commented Oct 6, 2022

Update: Incompatible flags are submitted, will run downstream pipeline to see about flipping them. Other implementation commits to come shortly.

copybara-service bot pushed a commit that referenced this issue Oct 6, 2022
BazelBuildApiGlobals:
- An arg of "some_string" is treated as syntactic sugar for ["some_string"]. This supports args of form "//foo".
- Deleted special casing for "public" and "private". These are now handled by the sugar and by the new PackageSpecification syntax added in unknown commit.

BzlLoadFunctionTest:
- Add tests for `visibility([])`, `visibility("//foo")`, and the unsupported case of negated package specs

BzlVisibility:
- Add interning of special public/private cases; lift construction to factory method.
- Rename PackageListBzlVisibility since it's really package specifications, not packages being listed. Make private since users don't need direct access to it. (Kept PUBLIC/PRIVATE singletons as public since it's reasonable for client code to directly refer to those.)

Since it's likely we won't support negation, we could even eliminate the BzlVisibility class altogether in favor of lists of PackageSpecifications. This would simplify interning of public/private (but complicate any hypothetical future interning of non-trivial allowlists). But having a separate class is more convenient/readable for the bzl machinery.

StarlarkBuildApiGlobals:
- Move description of valid visibility() arg values to the arg's doc.
- Condense the mention of the distinction between bzl-visibility and target visibility.
- Mention behavior relative to --incompatible_fix_package_group_reporoot_syntax.

Work toward #11261.

PiperOrigin-RevId: 479369691
Change-Id: Ib9b2844abc03b0596399cc3fe09e9b89c6416e90
copybara-service bot pushed a commit that referenced this issue Oct 6, 2022
This CL makes visibility() fail if it is called with a stack frame containing anything other than the entries for top-level module evaluation and for visibility() itself.

The motivation is to prohibit fancy framework-style .bzl logic that would frustrate automated refactorings. If static tooling can always locate where the `visibility(<foo>)` call in a file is, it can always mechanically modify it to `visibility([some_dep] + <foo>)`. This is handy for developer workflows that rely on tooling to help locate and update visibilities that need to be broadened, even if the mutated declaration is still not review-ready.

It is still possible to alias `visibility` to another symbol, e.g. `f = visibility; f(...)`. There's no real use for this so we're not worried about preempting that. (It'd also probably require elevating visibility to some kind of syntactic builtin, since we can't inspect whether/how a function gets aliased.)

Work toward #11261.

PiperOrigin-RevId: 479378704
Change-Id: I294bf975d8972c5f91bb5585fa118532bba37435
copybara-service bot pushed a commit that referenced this issue Oct 6, 2022
This is a break-glass flag analogous to --[no]check_visibility, designed to unblock developers who are prototyping changes locally.

Work toward #11261.

PiperOrigin-RevId: 479387465
Change-Id: I4a10a5e2f9616065ccd525d5f627d21773cd5032
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This enables `//foo/...` wildcards both in the allowlists passed to `visibility()`, and in the allowlist guarding the experimental feature itself (`--experimental_bzl_visibility_allowlist`). Both limitations proved to be a major hurdle for onboarding experimental users: There are over four hundred packages owned by the relatively small number of teams that have expressed interest.

Changes:
- Ad hoc parsing of visibility() strings in BazelBuildApiGlobals is replaced by reusing PackageSpecification parsing logic, which governs package_group's `packages` attribute. (This is something we would've wanted to do eventually anyway.)
- To support this, a new fromString variant is added to PackageSpecification, since the existing one is not public, nor is its exception type, nor is the capability to distinguish (and thereby disallow) negative patterns.
- Updated the allowlist string / packageid logic to tolerate `/...` in BazelBuildApiGlobals.
- Drive-by cleanups to visibility tests: Use "everyone" allowlist in test cases that are not testing the allowlist feature. Flip a couple private/public visibilities that are incorrect/irrelevant for what is being tested.
- Drive-by cleanups to misleading comments in PathFragment-related code.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 469723348
Change-Id: Ieaca652a921d95c033456f4bca05897714c16e78
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
- Change test assertions to check for the presence/absence of a PackageIdentifier, rather than actually evaluating the Packages that a package_group refers to. This is more straightforward, requires less setup boilerplate, and also allows us to write assertions that talk about packages in external repos, which PackageLoadingTestCase can't evaluate.

- Reflow scratch file contents to follow BUILD formatting.

- Add TODO mentioning a cross-repo test case that'd be nice to have, but isn't worth figuring out how to add.

- Make the negative-everything test case a little more interesting by giving it something to exclude.

- Simplify the positive-everything case's assertion and check that it admits things from other repos.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 475837392
Change-Id: Idcee6afa3a51de569979a2dde619dfa8fd23f1a1
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
codebase.md missed some changes that were made to a Google-internal version fork of this file. This CL adds them back. (I am not the author of the added content.)

Work toward bazelbuild#11261.

PiperOrigin-RevId: 478489612
Change-Id: Ib51bde2a047c3845e3cb340bd89bd20c0c5eb8ef
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This is a precursor to documenting the new changes to visibility -- "public", "private", and --incomaptible_fix_package_group_syntax; and to documenting the new bzl visibility feature.

package_group() function doc:
- `into text`: Clarify visibility levels. Replace legacy "rule" terminology with more modern "target", and be a little more pedantic about granting access to packages vs targets.
- `packages` attr: Be consistent about these being "package specifications", not an enumeration of packages. Say the allowed forms, then the semantics without respect to `includes`. Mention `//...` special case, and repo behavior.
- `includes` attr: Move interaction between negation and includes to here.

dependencies.md:
- Better hint that granting access to a package group can never remove access from the package to itself ("may grant broader visibility").
- Describe visibility as a "dual" of dependency rather than "opposite".
- Paragraph break.

visibility.md:
- Again be pedantic about access for package vs target, while factoring out "targets defined in" verbiage.
- Again be pedantic about not suggesting you can remove access from a package to itself.
- Factor out mention of how the syntax is weird.
- Expand wording for load() visibility a little.

codebase.md:
- Drive by rewrite of part of Skyframe section.
- Drive-by fixes for Starlark section.
- Streamline wording of visibility section.
- Add more mention of package_group implementation classes. Maybe a little more verbose than needed, but I figured it was better to include them than not.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 478572996
Change-Id: I1edc9bc80337976cc5dc21cf48a1d7e00924c63f
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This CL gives us a pathway for fixing stringification of package_group's `packages` attribute, so that //foo/bar is printed as "//foo/bar" and not "foo/bar". This affects e.g. `bazel query --output=proto [...]`. This change is a prerequisite to adding support for "public" and "private" string constant package specifiers, since otherwise "public" would be ambiguous with the stringification of //public.

PackageSpecification currently has two forms of stringification. The toString() method is unambiguous but omits the leading double slash, while the toStringWithoutRepository() always includes the double slash but never includes the repo name. I factored toString() into asString(boolean includeDoubleSlash) to switch between the old form with and without the fix to include slashes, and made toString() and all current callers pass false for now. I just renamed toStringWithoutRepository to asStringWithoutRepository for symmetry. Both are now clearly documented.

In PackageGroupContents, I clarified its behavior w.r.t. duplicates and order, and renamed its member vars for simplicity and in anticipation of storing another type of package spec subclass in a follow-up CL. Also renamed some methods for clarity.

Fixed a bug in AllPackagesBeneath where //... parsed in another repo would stringify without the leading "@repo". However, this bug was never exercised since it would require the follow-up CL's behavior of enabling "//..." to parse as AllPackagesBeneath rather than AllPackages (bazelbuild#16323).

Added a test for all three forms of stringification to PackageGroupTest.

There should be no actual behavioral change enabled in this CL.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 478828269
Change-Id: Ifc7c08e6e90b89d33bed1ee6a5a97b3a3ac07326
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This CL does three things:

1. It adds the special package specifications "public" and "private, guarded behind the flag `--incompatible_package_group_has_public_syntax`.

2. It fixes the behavior of package specification "//..." to mean "all packages in the current repo" rather than "all packages in any repo", guarded behind the flag `--incompatible_fix_package_group_reporoot_syntax`.

3. It fixes the behavior of `bazel query --output=proto` (and `--output=xml`) so that the `packages` attribute is serialized without dropping the leading `//` from package names -- it is now `"//foo/bar/..."` instead of `"foo/bar/..."`. This behavioral change is guarded behind `--incompatible_package_group_includes_double_slash`.

The .bzl `visibility()` builtin always acts as if the first two flags are enabled.

Work toward bazelbuild#11261.
Work toward bazelbuild#16323.
Work toward bazelbuild#16355.
Work toward bazelbuild#16391.

RELNOTES: Added three `package_group`-related flags: `--incompatible_package_group_includes_double_slash` (bazelbuild#16391), `--incompatible_package_group_has_public_syntax` (bazelbuild#16355), and `--incompatible_fix_package_group_reporoot_syntax` (bazelbuild#16323). With these flags, `package_group` can now easily specify "all packages", "no packages", and "all packages in the current repo".

In PackageSpecification:
- fromString(), fromStringPositive(): rewrote docstring, added bool args for flags, renamed or removed vars for simplicity, added handling of public/private and flag behavior
- AllPackagesBeneath(): fixed asStringWithoutRepository() for case of //..., which wasn't likely to arise before.
- normalize order of equals(), hashCode(), etc.
- AllPackages: change hash, and change str representation to "public"
- Add NoPackages, symmetric to AllPackages
- NegativePackageSpecification: don't reuse the delegate's hash as our own hash verbatim

In PackageGroupTest:
- Move `testEverythingSpecificationWorks` to above `testNegativeEverything`, and made it use "public" syntax in place of "//...".
- Add test for "private" syntax.
- Add test for flag guarding "public"/"private".
- Add tests for old and new "//..." behavior.
- Add tests that you can't negate "public" or "private".
- Add test for illegal combination of flags.
- Add assertions for stringification of "public"/"private".

Other changes:
- Update XmlOutputFormatterTest and ProtoOutputFormatterTest with test cases for the flag. In the latter case, parameterize a helper to allow setting per-test-case flags.
- Add test cases to BzlLoadFunctionTest for bzl visibility accepting the list forms `["public"]`/`["private"]`, and for its behavior being unaffected by the new flags.

PiperOrigin-RevId: 479347358
Change-Id: I124ca5406bff15615adaa1256e2d7bef69b9d9a5
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
BazelBuildApiGlobals:
- An arg of "some_string" is treated as syntactic sugar for ["some_string"]. This supports args of form "//foo".
- Deleted special casing for "public" and "private". These are now handled by the sugar and by the new PackageSpecification syntax added in unknown commit.

BzlLoadFunctionTest:
- Add tests for `visibility([])`, `visibility("//foo")`, and the unsupported case of negated package specs

BzlVisibility:
- Add interning of special public/private cases; lift construction to factory method.
- Rename PackageListBzlVisibility since it's really package specifications, not packages being listed. Make private since users don't need direct access to it. (Kept PUBLIC/PRIVATE singletons as public since it's reasonable for client code to directly refer to those.)

Since it's likely we won't support negation, we could even eliminate the BzlVisibility class altogether in favor of lists of PackageSpecifications. This would simplify interning of public/private (but complicate any hypothetical future interning of non-trivial allowlists). But having a separate class is more convenient/readable for the bzl machinery.

StarlarkBuildApiGlobals:
- Move description of valid visibility() arg values to the arg's doc.
- Condense the mention of the distinction between bzl-visibility and target visibility.
- Mention behavior relative to --incompatible_fix_package_group_reporoot_syntax.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 479369691
Change-Id: Ib9b2844abc03b0596399cc3fe09e9b89c6416e90
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This CL makes visibility() fail if it is called with a stack frame containing anything other than the entries for top-level module evaluation and for visibility() itself.

The motivation is to prohibit fancy framework-style .bzl logic that would frustrate automated refactorings. If static tooling can always locate where the `visibility(<foo>)` call in a file is, it can always mechanically modify it to `visibility([some_dep] + <foo>)`. This is handy for developer workflows that rely on tooling to help locate and update visibilities that need to be broadened, even if the mutated declaration is still not review-ready.

It is still possible to alias `visibility` to another symbol, e.g. `f = visibility; f(...)`. There's no real use for this so we're not worried about preempting that. (It'd also probably require elevating visibility to some kind of syntactic builtin, since we can't inspect whether/how a function gets aliased.)

Work toward bazelbuild#11261.

PiperOrigin-RevId: 479378704
Change-Id: I294bf975d8972c5f91bb5585fa118532bba37435
aiuto pushed a commit to aiuto/bazel that referenced this issue Oct 12, 2022
This is a break-glass flag analogous to --[no]check_visibility, designed to unblock developers who are prototyping changes locally.

Work toward bazelbuild#11261.

PiperOrigin-RevId: 479387465
Change-Id: I4a10a5e2f9616065ccd525d5f627d21773cd5032
copybara-service bot pushed a commit that referenced this issue Oct 14, 2022
- Rewrite "artifact" and "tree artifact" a little
- Rewrite "aspect-on-aspect" a little
- Add entry for "attribute"
- Flesh out "provider" a little bit, especially to distinguish between providers and provider instances
- Rewrite "rule". Emphasize its behavior from a BUILD author's perspective rather than starting with a brain-dump of the jargon a rule author needs to know. Include a brief description of its interaction with the three build phases, since rules are *the* central concept in Bazel. Add sheepish note that rules and and rule targets are sometimes conflated.
- Add separate entry for "rule target". We commonly use this term in other documentation, and a user might be confused whether we're talking about a "rule" or a "target".
- Rewrite "target" to emphasize its role in the build graph and prioritize discussion of rule targets and file targets.

Follow-up work will restructure the information in visibility.md, and add content for .bzl visibility.

Work toward #11261.

PiperOrigin-RevId: 481238358
Change-Id: I728e1be134e1619e1f3710328a111c45c06732e0
copybara-service bot pushed a commit that referenced this issue Oct 18, 2022
The three flipped flags are: --incompatible_package_group_includes_double_slash, --incompatible_package_group_has_public_syntax, and --incompatible_fix_package_group_reporoot_syntax. The first is a common query flag, and the latter two are stored in starlark semantics.

Logically, these are all one flag that migrates package_group to use `public` instead of `//...`, which now means "this repo only".

A side-effect of this change is that //src/main/java/net/starlark/java:clients is no longer publicly visible. In order to not break bootstrapping from prior Bazel versions, we will tolerate this regression until after the 6.0 release. The Java Starlark interpreter is not a publicly supported API anyway; its main users are Copybara and Stardoc, both of which can simply avoid vendoring-in the 6.0 version.

The //tools source tree is updated to move some allowlist definitions from BUILD files into BUILD.tools files. This works around a bootstrapping issue. The allowlist definitions are not needed at development time for Bazel itself, except for one test that incorrectly referred to a label under the main repo instead of @bazel_tools.

Updated a few test setups to use "public" in place of "//...".

These flags are not yet flipped in Blaze at Google. Our ordinary mechanism for controlling flags with a bazelrc file does not work in this particular case. Therefore, this CL takes the unusual step of factoring out the default values into string constants in stub files (FlagConstants.java) that differ between the internal and external source trees. This hack will be reverted once the flags are flipped in Blaze.

Fixes #16391.
Fixes #16355.
Fixes #16323.
Work toward #11261.

RELNOTES[INC]: In package_group's `packages` attribute, the syntax "//..." now refers to all packages in the same repository as the package group, rather than all packages everywhere. The new item "public" can be used instead to obtain the old behavior. In `bazel query --output=proto` (and `--output=xml`), the `packages` attribute now serializes with the leading double slash included (for instance, `//foo/bar/...` instead of `foo/bar/...`). See also #16355, #16323, and #16391.

PiperOrigin-RevId: 482023278
Change-Id: If86703d279dd3cd18b6b3948f37720816ada465f
@brandjon
Copy link
Member

Hm, best reopen this as release blocker until I submit the documentation as well.

@brandjon brandjon reopened this Oct 19, 2022
copybara-service bot pushed a commit that referenced this issue Oct 21, 2022
This is a major restructuring of the content in visibility.md.

- Make two main headings: target visibility and bzl visibility. The latter remains a stub, to be filled in by a different CL.

- Eliminate the best practices section, moving those bullet points inline into the relevant sections (called out as "Best practices:").

- Move more discussion/concepts to the top, before we get into visibility specifications.

- Move obscure detail about package_group into its own stub section. While tiny, this parallels the distinction between rule targets, file targets, and package groups as the three main kinds of targets.

- Turn warning about a cyclic dep error a formatted "Note:" callout.

- Moved config setting into its own section, since it's a legacy edge case and not really relevant to rule targets in general.

- Rephrased the sections on source file targets, config settings, and implicit deps, in an attempt to be clearer and more direct.

I'm not in love with the examples on this page, but didn't attempt to touch them in this CL. The diff's already big enough for sure.

Work toward #11261.

PiperOrigin-RevId: 482890214
Change-Id: I5b0c7454228ff457319aeed04465af982132106b
copybara-service bot pushed a commit that referenced this issue Oct 21, 2022
…ssages

See discussion in review of unknown commit for rationale. It boils down to the fact that ".bzl visibility" would be the preferred formatting, but this is awkward to write at the beginning of sentences. Both "bzl" and "load" are pieces of terminology the user already knows. The only downside of "load" is that it can be interpreted as a verb rather than as part of a compound noun.

Changes:
- Generally, I substituted mentions of "bzl visibility" and "bzl-visibility" to be "load visibility".
- In some cases I just made it a bare "visibility" where it was obvious, in error messages created by a call to `visibility()`.
- In other cases I specify ".bzl load visibility" for extra clarity where context is weaker.
- One instance of "load-visibility" was de-hyphenated.
- Deleted mention in `visibility()`'s documentation that the feature is experimental.

Instances of "bzlVisibility" occur in the actual java symbols. I didn't change those because it's not user-visible, is extra churn for us, and can be misread more easily ("load" as a verb). I also didn't change the names of user-facing flags because renaming flags is an absolute nightmare and we don't have time before the Bazel cut anyway.

A follow-up will add more user concepts documentation for load visibility.

(This is probably the longest commit message I've written for such a trivial change.)

Work toward #11261.

PiperOrigin-RevId: 482897281
Change-Id: Iabb21e8b00b9cbb5335302e494bc0b0af0fa59af
copybara-service bot pushed a commit that referenced this issue Oct 22, 2022
Work toward #11261.

PiperOrigin-RevId: 482956105
Change-Id: I04d53352d6368d377258687671dfe770642832a6
@meteorcloudy meteorcloudy removed this from the 6.0.0 nice to have milestone Jan 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P1 I'll work on this now. (Assignee required) team-Starlark-Integration Issues involving Bazel's integration with Starlark, excluding builtin symbols type: feature request
Projects
None yet
Development

No branches or pull requests

8 participants