From 0d5eb3c6b43cd028f51af6e727f6787c1c0de18b Mon Sep 17 00:00:00 2001 From: Googler Date: Mon, 3 Oct 2022 12:21:47 -0700 Subject: [PATCH] Rewrite some visibility documentation 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 --- site/en/basics/dependencies.md | 17 +-- site/en/concepts/visibility.md | 47 +++--- site/en/contribute/codebase.md | 142 +++++++++--------- .../build/docgen/templates/be/functions.vm | 103 ++++++++----- 4 files changed, 171 insertions(+), 138 deletions(-) diff --git a/site/en/basics/dependencies.md b/site/en/basics/dependencies.md index 2c7f22010b7f15..0b3af8455f0bcc 100644 --- a/site/en/basics/dependencies.md +++ b/site/en/basics/dependencies.md @@ -73,18 +73,17 @@ Bazel in the ## Minimizing Module Visibility -Bazel and other build systems allow each target to specify a visibility: a -property that specifies which other targets may depend on it. Targets can be -public, in which case they can be referenced by any other target in the -workspace; private, in which case they can be referenced only from within the -same `BUILD` file; or visible to only an explicitly defined list of other -targets. A visibility is essentially the opposite of a dependency: if target A -wants to depend on target B, target B must make itself visible to target A. As -with most programming languages, it is usually best to minimize visibility as +Bazel and other build systems allow each target to specify a visibility — a +property that determines which other targets may depend on it. A private target +can only be referenced within its own `BUILD` file. A target may grant broader +visibility to the targets of an explicitly defined list of `BUILD` files, or, in +the case of public visibility, to every target in the workspace. + +As with most programming languages, it is usually best to minimize visibility as much as possible. Generally, teams at Google will make targets public only if those targets represent widely used libraries available to any team at Google. Teams that require others to coordinate with them before using their code will -maintain an allow list of customer targets as their target’s visibility. Each +maintain an allowlist of customer targets as their target’s visibility. Each team’s internal implementation targets will be restricted to only directories owned by the team, and most `BUILD` files will have only one target that isn’t private. diff --git a/site/en/concepts/visibility.md b/site/en/concepts/visibility.md index cd414756694c9b..89efcfbd6d4ba3 100644 --- a/site/en/concepts/visibility.md +++ b/site/en/concepts/visibility.md @@ -19,32 +19,33 @@ For more details on package and subpackages, see ## Visibility specifications {:#visibility-specifications} All rule targets have a `visibility` attribute that takes a list of labels. One -target is visible to another if they are in the same package, or if they are -granted visibility by one of the labels. +target is visible to another if they are in the same package, or if visibility +is granted to the depending target's package. -Each label has one of the following forms: +Each label has one of the following forms. With the exception of the last form, +these are just syntactic placeholders that do not correspond to any actual +target. -* `"//visibility:public"`: Anyone can use this target. (May not be combined +* `"//visibility:public"`: Grants access to all packages. (May not be combined with any other specification.) -* `"//visibility:private"`: Only targets in this package can use this - target. (May not be combined with any other specification.) +* `"//visibility:private"`: Does not grant any additional access; only targets + in this package can use this target. (May not be combined with any other + specification.) -* `"//foo/bar:__pkg__"`: Grants access to targets defined in `//foo/bar` (but - not its subpackages). Here, `__pkg__` is a special piece of syntax - representing all of the targets in a package. +* `"//foo/bar:__pkg__"`: Grants access to `//foo/bar` (but not its + subpackages). -* `"//foo/bar:__subpackages__"`: Grants access to targets defined in - `//foo/bar`, or any of its direct or indirect subpackages. Again, - `__subpackages__` is special syntax. +* `"//foo/bar:__subpackages__"`: Grants access `//foo/bar` and all of its + direct and indirect subpackages. -* `"//foo/bar:my_package_group"`: Grants access to all of the packages named - by the given [package group](/reference/be/functions#package_group). +* `"//some_pkg:my_package_group"`: Grants access to all of the packages that + are part of the given [`package_group`](/reference/be/functions#package_group). - * Package groups do not support the special `__pkg__` and - `__subpackages__` syntax. Within a package group, `"//foo/bar"` is - equivalent to `"//foo/bar:__pkg__"` and `"//foo/bar/..."` is equivalent - to `"//foo/bar:__subpackages__"`. + * Package groups use a different syntax for specifying packages. Within a + package group, the forms `"//foo/bar:__pkg__"` and + `"//foo/bar:__subpackages__"` are respectively replaced by `"//foo/bar"` + and `"//foo/bar/..."`. For example, if `//some/package:mytarget` has its `visibility` set to `[":__subpackages__", "//tests:__pkg__"]`, then it could be used by any target @@ -187,12 +188,14 @@ for more details. ## Visibility of bzl files {:#visibility-bzl-files} -`load` statements are currently not subject to visibility. It is possible to -load a `bzl` file anywhere in the workspace. +`BUILD` and `.bzl` files, as processed by Bazel during loading, are not +considered to be targets and therefore are not subject to visibility. It is +therefore possible to load a `.bzl` file from anywhere in the workspace. However, users may choose to run the Buildifier linter. -The [bzl-visibility](https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#bzl-visibility) check -provides a warning if users `load` from beneath a subdirectory named `internal` or `private`. +The [bzl-visibility](https://github.com/bazelbuild/buildtools/blob/master/WARNINGS.md#bzl-visibility) +check provides a warning if users `load` from beneath a subdirectory named +`internal` or `private`. ## Visibility of implicit dependencies {:#visibility-implicit-dependencies} diff --git a/site/en/contribute/codebase.md b/site/en/contribute/codebase.md index 59351cd24af45e..95f7fb72a22b35 100644 --- a/site/en/contribute/codebase.md +++ b/site/en/contribute/codebase.md @@ -246,17 +246,17 @@ it) Every repository is composed of packages, a collection of related files and a specification of the dependencies. These are specified by a file called `BUILD` or `BUILD.bazel`. If both exist, Bazel prefers `BUILD.bazel`; the reason -why BUILD files are still accepted is that Bazel’s ancestor, Blaze, used this +why `BUILD` files are still accepted is that Bazel’s ancestor, Blaze, used this file name. However, it turned out to be a commonly used path segment, especially on Windows, where file names are case-insensitive. -Packages are independent of each other: changes to the BUILD file of a package -cannot cause other packages to change. The addition or removal of BUILD files +Packages are independent of each other: changes to the `BUILD` file of a package +cannot cause other packages to change. The addition or removal of `BUILD` files _can _change other packages, since recursive globs stop at package boundaries -and thus the presence of a BUILD file stops the recursion. +and thus the presence of a `BUILD` file stops the recursion. -The evaluation of a BUILD file is called "package loading". It's implemented in -the class `PackageFactory`, works by calling the Starlark interpreter and +The evaluation of a `BUILD` file is called "package loading". It's implemented +in the class `PackageFactory`, works by calling the Starlark interpreter and requires knowledge of the set of available rule classes. The result of package loading is a `Package` object. It's mostly a map from a string (the name of a target) to the target itself. @@ -304,7 +304,7 @@ Packages are composed of targets, which have the following types: The name of a target is called a _Label_. The syntax of labels is `@repo//pac/kage:name`, where `repo` is the name of the repository the Label is -in, `pac/kage` is the directory its BUILD file is in and `name` is the path of +in, `pac/kage` is the directory its `BUILD` file is in and `name` is the path of the file (if the label refers to a source file) relative to the directory of the package. When referring to a target on the command line, some parts of the label can be omitted: @@ -321,9 +321,9 @@ be implemented either in Starlark (the `rule()` function) or in Java (so called rule will be implemented in Starlark, but some legacy rule families (such as Java or C++) are still in Java for the time being. -Starlark rule classes need to be imported at the beginning of BUILD files using -the `load()` statement, whereas Java rule classes are "innately" known by Bazel, -by virtue of being registered with the `ConfiguredRuleClassProvider`. +Starlark rule classes need to be imported at the beginning of `BUILD` files +using the `load()` statement, whereas Java rule classes are "innately" known by +Bazel, by virtue of being registered with the `ConfiguredRuleClassProvider`. Rule classes contain information such as: @@ -365,41 +365,39 @@ similarly-named package `com.google.devtools.build.lib.skyframe` contains the implementation of Bazel on top of Skyframe. More information about Skyframe is available [here](/reference/skyframe). -Generating a new `SkyValue` involves the following steps: - -1. Running the associated `SkyFunction` -2. Declaring the dependencies (such as `SkyValue`s) that the `SkyFunction` needs - to do its job. This is done by calling the various overloads of - `SkyFunction.Environment.getValue()`. -3. If a dependency is not available, Skyframe signals that by returning null - from `getValue()`. In this case, the `SkyFunction` is expected to yield - control to Skyframe by returning null, then Skyframe evaluates the - dependencies that haven't been evaluated yet and calls the `SkyFunction` - again, thus going back to (1). -4. Constructing the resulting `SkyValue` - -A consequence of this is that if not all dependencies are available in (3), the -function needs to be completely restarted and thus computation needs to be -re-done, which is obviously inefficient. `SkyFunction.Environment.getState()` -lets us directly work around this issue by having Skyframe maintain the -`SkyKeyComputeState` instance between calls to `SkyFunction.compute` for the -same `SkyKey`. Check out the example in the javadoc for -`SkyFunction.Environment.getState()`, as well as real usages in the Bazel -codebase. - -Other indirect workarounds: - -1. Declaring dependencies of `SkyFunction`s in groups so that if a function - has, say, 10 dependencies, it only needs to restart once instead of ten - times. -2. Splitting `SkyFunction`s so that one function does not need to be restarted - many times. This has the side effect of interning data into Skyframe that - may be internal to the `SkyFunction`, thus increasing memory use. - -These are all just workarounds for the limitations of Skyframe, which -is mostly a consequence of the fact that Java doesn't support lightweight -threads and that we routinely have hundreds of thousands of in-flight Skyframe -nodes. +To evaluate a given `SkyKey` into a `SkyValue`, Skyframe will invoke the +`SkyFunction` corresponding to the type of the key. During the function's +evaluation, it may request other dependencies from Skyframe by calling the +various overloads of `SkyFunction.Environment.getValue()`. This has the +side-effect of registering those dependencies into Skyframe's internal graph, so +that Skyframe will know to re-evaluate the function when any of its dependencies +change. In other words, Skyframe's caching and incremental computation work at +the granularity of `SkyFunction`s and `SkyValue`s. + +Whenever a `SkyFunction` requests a dependency that is unavailable, `getValue()` +will return null. The function should then yield control back to Skyframe by +itself returning null. At some later point, Skyframe will evaluate the +unavailable dependency, then restart the function from the beginning — only this +time the `getValue()` call will succeed with a non-null result. + +A consequence of this is that any computation performed inside the `SkyFunction` +prior to the restart must be repeated. But this does not include work done to +evaluate dependency `SkyValues`, which are cached. Therefore, we commonly work +around this issue by: + +1. Declaring dependencies in batches (by using `getValuesAndExceptions()`) to + limit the number of restarts. +2. Breaking up a `SkyValue` into separate pieces computed by different + `SkyFunction`s, so that they can be computed and cached independently. This + should be done strategically, since it has the potential to increases memory + usage. +3. Storing state between restarts, either using + `SkyFunction.Environment.getState()`, or keeping an ad hoc static cache + "behind the back of Skyframe". + +Fundamentally, we need these types of workarounds because we routinely have +hundreds of thousands of in-flight Skyframe nodes, and Java doesn't support +lightweight threads. ## Starlark {:#starlark} @@ -410,16 +408,16 @@ guarantees to enable concurrent reads. It is not Turing-complete, which discourages some (but not all) users from trying to accomplish general programming tasks within the language. -Starlark is implemented in the `com.google.devtools.build.lib.syntax` package. +Starlark is implemented in the `net.starlark.java` package. It also has an independent Go implementation [here](https://github.com/google/starlark-go){: .external}. The Java implementation used in Bazel is currently an interpreter. -Starlark is used in four contexts: +Starlark is used in several contexts, including: -1. **The BUILD language.** This is where new rules are defined. Starlark code - running in this context only has access to the contents of the BUILD file - itself and Starlark files loaded by it. +1. **The `BUILD` language.** This is where new rules are defined. Starlark code + running in this context only has access to the contents of the `BUILD` file + itself and `.bzl` files loaded by it. 2. **Rule definitions.** This is how new rules (such as support for a new language) are defined. Starlark code running in this context has access to the configuration and data provided by its direct dependencies (more on this @@ -430,8 +428,8 @@ Starlark is used in four contexts: are defined. Starlark code running in this context can run arbitrary code on the machine where Bazel is running, and reach outside the workspace. -The dialects available for BUILD and .bzl files are slightly different because -they express different things. A list of differences is available +The dialects available for `BUILD` and `.bzl` files are slightly different +because they express different things. A list of differences is available [here](/rules/language#differences-between-build-and-bzl-files). More information about Starlark is available @@ -446,7 +444,7 @@ quite sensibly, a (target, configuration) pair. It's called the "loading/analysis phase" because it can be split into two distinct parts, which used to be serialized, but they can now overlap in time: -1. Loading packages, that is, turning BUILD files into the `Package` objects +1. Loading packages, that is, turning `BUILD` files into the `Package` objects that represent them 2. Analyzing configured targets, that is, running the implementation of the rules to produce the action graph @@ -831,18 +829,17 @@ It's under review in pull request ### Visibility {:#visibility} If you work on a large codebase with a lot of developers (like at Google), you -don't necessarily want everyone else to be able to depend on your code so that -you retain the liberty to change things that you deem to be implementation -details (otherwise, as per [Hyrum's law](https://www.hyrumslaw.com/){: .external}, -people _will_ come to depend on all parts of your code). - -Bazel supports this by the mechanism called _visibility: _you can declare that a -particular rule can only be depended on using the visibility attribute -(documentation -[here](/reference/be/common-definitions#common-attributes)). -This attribute is a little special because unlike every other attribute, the set -of dependencies it generates is not simply the set of labels listed (yes, this -is a design flaw). +want to take care to prevent everyone else from arbitrarily depending on your +code. Otherwise, as per [Hyrum's law](https://www.hyrumslaw.com/){: .external}, +people _will_ come to rely on behaviors that you considered to be implementation +details. + +Bazel supports this by the mechanism called _visibility_: you can declare that a +particular target can only be depended on using the +[visibility](/reference/be/common-definitions#common-attributes) attribute. This +attribute is a little special because, although it holds a list of labels, these +labels may encode a pattern over package names rather than a pointer to any +particular target. (Yes, this is a design flaw.) This is implemented in the following places: @@ -852,9 +849,14 @@ This is implemented in the following places: packages directly (`//pkg:__pkg__`) or subtrees of packages (`//pkg:__subpackages__`). This is different from the command line syntax, which uses `//pkg:*` or `//pkg/...`. -* Package groups are implemented as their own target and configured target - types (`PackageGroup` and `PackageGroupConfiguredTarget`). We could probably - replace these with simple rules if we wanted to. +* Package groups are implemented as their own target (`PackageGroup`) and + configured target (`PackageGroupConfiguredTarget`). We could probably + replace these with simple rules if we wanted to. Their logic is implemented + with the help of: `PackageSpecification`, which corresponds to a + single pattern like `//pkg/...`; `PackageGroupContents`, which corresponds + to a single `package_group`'s `packages` attribute; and + `PackageSpecificationProvider`, which aggregates over a `package_group` and + its transitive `includes`. * The conversion from visibility label lists to dependencies is done in `DependencyResolver.visitTargetVisibility` and a few other miscellaneous places. @@ -915,7 +917,7 @@ built). Derived artifacts can themselves be multiple kinds: There is no fundamental reason why source artifacts cannot be tree artifacts or unresolved symlink artifacts, it's just that we haven't implemented it yet (we -should, though -- referencing a source directory in a BUILD file is one of the +should, though -- referencing a source directory in a `BUILD` file is one of the few known long-standing incorrectness issues with Bazel; we have an implementation that kind of works which is enabled by the `BAZEL_TRACK_SOURCE_DIRECTORIES=1` JVM property) @@ -1672,6 +1674,6 @@ tools. There are many examples of `BuildIntegrationTestCase` classes in the Bazel repository. Analysis tests are implemented as subclasses of `BuildViewTestCase`. There is a -scratch file system you can use to write BUILD files, then various helper +scratch file system you can use to write `BUILD` files, then various helper methods can request configured targets, change the configuration and assert various things about the result of the analysis. diff --git a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm index 6a0c08b8504d2a..164ece718cf14d 100644 --- a/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm +++ b/src/main/java/com/google/devtools/build/docgen/templates/be/functions.vm @@ -119,13 +119,25 @@ package(default_visibility = ["//foo:target"]) 
package_group(name, packages, includes)
-

This function defines a set of packages and assigns a label to the -group. The label can be referenced in visibility attributes.

- -

Package groups are used for visibility control. You can grant access to a rule -to one or more package groups, every rule in the entire source tree, or only to rules declared -in the same package. For more detailed description of the visibility system, see -the visibility attribute. +

This function defines a set of packages +and associates a label with the set. The label can be referenced in +visibility attributes.

+ +

Package groups are primarily used for visibility control. A publicly visible +target can be referenced from every package in the source tree. A privately +visible target can only be referenced within its own package (not subpackages). +In between these extremes, a target may allow access to its own package plus any +of the packages described by one or more package groups. For a more detailed +explanation of the visibility system, see the +visibility +attribute.

+ +

A given package is considered to be in the group if it either matches the +packages attribute, or is already contained in one of the other +package groups mentioned in the includes attribute.

+ +

Package groups are technically targets, but are not created by rules, and do +not themselves have any visibility protection.

Arguments

@@ -151,30 +163,41 @@ the visibility attribute packages -

List of Package; optional

-

A complete enumeration of packages in this group.

- -

Packages should be referred to using their full names, - starting with a double slash. For - example, //foo/bar/main is a valid element - of this list.

- -

You can also specify wildcards: the specification - //foo/... specifies every package under - //foo, including //foo itself.

- -

Package specifications can be prefixed with - to - indicate negation: the specification -//foo/bar/... - excludes all packages under //foo/bar that would - otherwise have been matched by the package patterns in the - current package_group. When used together with - includes, the set of packages for each package group - is computed and then the results are unioned: negative patterns - in one package group do not affect the result of included package - groups. - -

If this attribute is missing, the package group itself will contain - no packages (but it can still include other package groups).

+

List of strings; optional

+

A list of zero or more package specifications.

+ +

Each package specification string can have one of the following + forms:

+ +
    + +
  1. The full name of a package, without its repository, starting with a + double slash. For example, //foo/bar specifies the package + having that name and which lives in the same repository as the package + group. + +
  2. As above, but with a trailing /.... For example, + //foo/... specifies the set of //foo and all its + subpackages. As a special case, //... specifies all + packages in any repository (in other words, public). + +
+ +

In addition, a package specification may also be prefixed with + - to indicate that it is negated.

+ +

The package group contains any package that matches at least one of + its positive specifications and none of its negative specifications + For instance, the value [//foo/..., -//foo/tests/...] + includes all subpackages of //foo that are not also + subpackages of //foo/tests. (//foo itself is + included while //foo/tests itself is not.)

+ +

Aside from //..., there is no way to directly specify + packages outside the current repository.

+ +

If this attribute is missing, it is the same as setting it to an + empty list (in other words, private). @@ -183,12 +206,18 @@ the visibility attribute

List of labels; optional

Other package groups that are included in this one.

-

The labels in this attribute must refer to other package - groups. Packages in referenced package groups are taken to be part - of this package group. This is transitive, that is, if package - group a contains package group b, - and b contains package group c, every - package in c will also be a member of a.

+

The labels in this attribute must refer to other package groups. + Packages in referenced package groups are taken to be part of this + package group. This is transitive — if package group + a includes package group b, and b + includes package group c, then every package in + c will also be a member of a.

+ +

When used together with negated package specifications, note that the + set of packages for each group is first computed independently and the + results are then unioned together. This means that negated + specifications in one group have no effect on the specifications in + another group.