Skip to content

Commit

Permalink
Bzl-visibility: Add "everyone" value for allowlist
Browse files Browse the repository at this point in the history
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
  • Loading branch information
brandjon authored and Copybara-Service committed Jul 21, 2022
1 parent ddcbe58 commit f069347
Show file tree
Hide file tree
Showing 4 changed files with 33 additions and 5 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,11 @@ private void checkVisibilityAllowlist(PackageIdentifier pkgId, List<String> allo
// TODO(b/22193153): This seems incorrect since parse doesn't take into account any repository
// map. (This shouldn't matter within Google's monorepo, which doesn't use a repo map.)
try {
// Special constant to disable allowlisting. For migration to enable the feature globally.
if (allowedPkgString.equals("everyone")) {
foundMatch = true;
break;
}
PackageIdentifier allowedPkgId = PackageIdentifier.parse(allowedPkgString);
if (pkgId.equals(allowedPkgId)) {
foundMatch = true;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -147,7 +147,8 @@ public final class BuildLanguageOptions extends OptionsBase {
"A comma-separated list of packages (sans \"//\") which, if --experimental_bzl_visibility"
+ " is enabled, are permitted to contain .bzl files that set a bzl-visibility by"
+ " calling the `visibility()` function. (Known issue: This flag may not work"
+ " correctly in the presence of repository remapping, which is used by bzlmod.)")
+ " correctly in the presence of repository remapping, which is used by bzlmod.)"
+ " If the list includes the special item \"everyone\", all packages are permitted.")
public List<String> experimentalBzlVisibilityAllowlist;

@Option(
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ public interface StarlarkBuildApiGlobals {
+ "<li>a list of package paths (e.g. <code>[\"//pkg1\", \"//pkg2/subpkg\","
+ " ...]</code>): the .bzl can be loaded by files in any of the listed packages. Only"
+ " packages in the current repository may be specified; the repository \"@\" syntax"
+ " is disallowed."
+ " is disallowed. Subpackage globs (<code>\"//pkg/...\"</code>) are not supported."
+ "</ul>"
+ "<p>Generally, <code>visibility()</code> is called at the top of the .bzl file,"
+ " immediately after its <code>load()</code> statements. (It is poor style to put"
Expand All @@ -59,8 +59,8 @@ public interface StarlarkBuildApiGlobals {
name = "value",
named = false,
doc =
"The bzl-visibility level to set. May be <code>\"public\"</code> or"
+ " <code>\"private\"</code>.")
"The bzl-visibility level to set. May be <code>\"public\"</code>,"
+ " <code>\"private\"</code>, or a list of packages.")
},
// Ordinarily we'd use enableOnlyWithFlag here to gate access on
// --experimental_bzl_visibility. However, the StarlarkSemantics isn't available at the point
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -423,6 +423,28 @@ public void testBzlVisibility_disabledWithoutAllowlist() throws Exception {
assertContainsEvent("`visibility() is not enabled for package //b");
}

@Test
public void testBzlVisibility_enabledWhenAllowlistDisabled() throws Exception {
setBuildLanguageOptions(
"--experimental_bzl_visibility=true",
// Put unrelated c in allowlist, but not b, to show that "everyone" disables checking when
// included as a list item
"--experimental_bzl_visibility_allowlist=c,everyone");

scratch.file("a/BUILD");
scratch.file(
"a/foo.bzl", //
"load(\"//b:bar.bzl\", \"x\")");
scratch.file("b/BUILD");
scratch.file(
"b/bar.bzl", //
"visibility(\"public\")",
"x = 1");

checkSuccessfulLookup("//a:foo.bzl");
assertNoEvents();
}

@Test
public void testBzlVisibility_malformedAllowlist() throws Exception {
setBuildLanguageOptions(
Expand Down Expand Up @@ -889,7 +911,7 @@ public FileStatus statIfFound(PathFragment path, boolean followSymlinks) throws
}

@Override
protected InputStream getInputStream(PathFragment path) throws IOException {
protected synchronized InputStream getInputStream(PathFragment path) throws IOException {
if (badPathForRead != null && badPathForRead.asFragment().equals(path)) {
throw new IOException("bad");
}
Expand Down

0 comments on commit f069347

Please sign in to comment.