Flip package_group incompatible flags

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

Author: brandjon

src/main/java/com/google/devtools/build/lib/packages/semantics/BUILD

@@ -15,7 +15,10 @@ filegroup(
 # so it may not depend on (say) Label.
 java_library(
     name = "semantics",
-    srcs = ["BuildLanguageOptions.java"],
+    srcs = [
+        "BuildLanguageOptions.java",
+        "FlagConstants.java",
+    ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/concurrent",
         "//src/main/java/com/google/devtools/common/options",

src/main/java/com/google/devtools/build/lib/packages/semantics/BuildLanguageOptions.java

@@ -452,7 +452,7 @@ public final class BuildLanguageOptions extends OptionsBase {
 
   @Option(
       name = "incompatible_package_group_has_public_syntax",
-      defaultValue = "false",
+      defaultValue = FlagConstants.DEFAULT_INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX,
       documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
       effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
       metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
@@ -463,7 +463,7 @@ public final class BuildLanguageOptions extends OptionsBase {
 
   @Option(
       name = "incompatible_fix_package_group_reporoot_syntax",
-      defaultValue = "false",
+      defaultValue = FlagConstants.DEFAULT_INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX,
       documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS,
       effectTags = {OptionEffectTag.BUILD_FILE_SEMANTICS},
       metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},
@@ -803,9 +803,9 @@ public StarlarkSemantics toStarlarkSemantics() {
   public static final String INCOMPATIBLE_DISALLOW_STRUCT_PROVIDER_SYNTAX =
       "-incompatible_disallow_struct_provider_syntax";
   public static final String INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX =
-      "-incompatible_package_group_has_public_syntax";
+      FlagConstants.INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX;
   public static final String INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX =
-      "-incompatible_fix_package_group_reporoot_syntax";
+      FlagConstants.INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX;
   public static final String INCOMPATIBLE_DO_NOT_SPLIT_LINKING_CMDLINE =
       "+incompatible_do_not_split_linking_cmdline";
   public static final String INCOMPATIBLE_JAVA_COMMON_PARAMETERS =

src/main/java/com/google/devtools/build/lib/packages/semantics/FlagConstants.java

@@ -0,0 +1,31 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.packages.semantics;
+
+/** This file holds hardcoded flag defaults that vary between Bazel and Blaze. */
+// TODO(b/254084490): This file is a temporary hack. Eliminate once we've flipped the incompatible
+// flag in Blaze.
+class FlagConstants {
+
+  private FlagConstants() {}
+
+  public static final String DEFAULT_INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX = "true";
+  public static final String DEFAULT_INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX = "true";
+
+  public static final String INCOMPATIBLE_PACKAGE_GROUP_HAS_PUBLIC_SYNTAX =
+      "+incompatible_package_group_has_public_syntax";
+  public static final String INCOMPATIBLE_FIX_PACKAGE_GROUP_REPOROOT_SYNTAX =
+      "+incompatible_fix_package_group_reporoot_syntax";
+}

src/main/java/com/google/devtools/build/lib/query2/common/BUILD

@@ -32,6 +32,7 @@ java_library(
     name = "options",
     srcs = [
         "CommonQueryOptions.java",
+        "FlagConstants.java",
     ],
     deps = [
         "//src/main/java/com/google/devtools/build/lib/query2/engine",

src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java

@@ -139,7 +139,7 @@ public String getLineTerminator() {
 
   @Option(
       name = "incompatible_package_group_includes_double_slash",
-      defaultValue = "false",
+      defaultValue = FlagConstants.DEFAULT_INCOMPATIBLE_PACKAGE_GROUP_INCLUDES_DOUBLE_SLASH,
       documentationCategory = OptionDocumentationCategory.QUERY,
       effectTags = {OptionEffectTag.TERMINAL_OUTPUT},
       metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE},

src/main/java/com/google/devtools/build/lib/query2/common/FlagConstants.java

@@ -0,0 +1,25 @@
+// Copyright 2022 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+package com.google.devtools.build.lib.query2.common;
+
+/** This file holds hardcoded flag defaults that vary between Bazel and Blaze. */
+// TODO(b/254084490): This file is a temporary hack. Eliminate once we've flipped the incompatible
+// flag in Blaze.
+class FlagConstants {
+
+  private FlagConstants() {}
+
+  static final String DEFAULT_INCOMPATIBLE_PACKAGE_GROUP_INCLUDES_DOUBLE_SLASH = "true";
+}

src/main/java/net/starlark/java/BUILD

@@ -26,5 +26,8 @@ package_group(
 # Approved clients of java.starlark.net.
 package_group(
     name = "clients",
+    # TODO(#11261, 16355): The meaning of `//...` changed from "public" to "this
+    # repo only" in Bazel 6.0. Switch this to `public` in a subsequent release.
+    # (This must remain `//...` in 6.0 to allow bootstrapping Bazel with 5.x.)
     packages = ["//..."],  # anyone
 )

src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java

@@ -362,8 +362,8 @@ public void setupMockClient(MockToolsConfig config, List<String> workspaceConten
 
     config.create(
         "tools/allowlists/config_feature_flag/BUILD",
-        "package_group(name='config_feature_flag', packages=['//...'])",
-        "package_group(name='config_feature_flag_Setter', packages=['//...'])");
+        "package_group(name='config_feature_flag', packages=['public'])",
+        "package_group(name='config_feature_flag_Setter', packages=['public'])");
 
     config.create(
         "embedded_tools/tools/proto/BUILD",
@@ -541,10 +541,10 @@ private ImmutableList<String> createAndroidBuildContents() {
         .add("    generates_api = 1,")
         .add("    processor_class = 'android.databinding.annotationprocessor.ProcessDataBinding')")
         .add("sh_binary(name = 'instrumentation_test_check', srcs = ['empty.sh'])")
-        .add("package_group(name = 'android_device_allowlist', packages = ['//...'])")
-        .add("package_group(name = 'export_deps_allowlist', packages = ['//...'])")
+        .add("package_group(name = 'android_device_allowlist', packages = ['public'])")
+        .add("package_group(name = 'export_deps_allowlist', packages = ['public'])")
         .add("package_group(name = 'allow_android_library_deps_without_srcs_allowlist',")
-        .add("    packages=['//...'])")
+        .add("    packages=['public'])")
         .add("android_tools_defaults_jar(name = 'android_jar')")
         .add("sh_binary(name = 'dex_list_obfuscator', srcs = ['empty.sh'])");
 

src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java

@@ -255,7 +255,11 @@ public void testNothingSpecificationWorks() throws Exception {
 
   @Test
   public void testPublicPrivateAreNotAccessibleWithoutFlag() throws Exception {
-    setBuildLanguageOptions("--incompatible_package_group_has_public_syntax=false");
+    setBuildLanguageOptions(
+        // Flag being tested
+        "--incompatible_package_group_has_public_syntax=false",
+        // Must also be disabled in order to disable the above
+        "--incompatible_fix_package_group_reporoot_syntax=false");
 
     scratch.file(
         "foo/BUILD", //

src/test/java/com/google/devtools/build/lib/rules/android/AndroidDeviceTest.java

@@ -638,6 +638,9 @@ public void testPackageWhitelist() throws Exception {
     String mockedAndroidToolsContent =
         scratch
             .readFile(mockedAndroidToolsBuildFileLocation)
+            .replaceAll(Pattern.quote("packages = ['public']"), "packages = ['//bar/...']")
+            // TODO(b/254084490): Migrate Google-internal usage of "//..." in test mock to be
+            // "public" instead.
             .replaceAll(Pattern.quote("packages = ['//...']"), "packages = ['//bar/...']");
     scratch.overwriteFile(mockedAndroidToolsBuildFileLocation, mockedAndroidToolsContent);
     invalidatePackages();

src/test/shell/bazel/local_repository_test.sh

@@ -1387,7 +1387,7 @@ EOF
   expect_log "@x_repo//x to @x_repo//a"
 }
 
-# This test verifies that the //... pattern includes external dependencies
+# This test verifies that the `public` pattern includes external dependencies.
 #
 # ${WORKSPACE_DIR}/
 #     WORKSPACE
@@ -1397,22 +1397,26 @@ EOF
 #   blue/
 #     BUILD
 #
-# repo2 contains a .sh file whose visibility is set to //...
-# we verify that we can use this file from ${WORKSPACE_DIR} by running it as
+# repo2 contains a .sh file whose visibility is set to public.
+# We verify that we can use this file from ${WORKSPACE_DIR} by running it as
 # part of the "run-the-thing" binary.
-function test_slashslashdotdotdot_includes_external_dependencies() {
+#
+# TODO(brandjon): Can this test be deleted in favor of an analysis-time unit
+# test? Ideally PackageGroupTest should cover it, but that suite can't handle
+# external repos.
+function test_public_includes_external_dependencies() {
   create_new_workspace
   repo2=${new_workspace_dir}
   mkdir -p blue
   cat > blue/BUILD <<EOF
 package_group(
-    name = "slash-slash-dot-dot-dot",
-    packages = ["//..."],
+    name = "everyone",
+    packages = ["public"],
 )
 filegroup(
     name = "do-the-thing",
     srcs = ["do-the-thing.sh"],
-    visibility = [":slash-slash-dot-dot-dot"]
+    visibility = [":everyone"]
 )
 EOF
   cat > blue/do-the-thing.sh <<EOF
@@ -1437,9 +1441,11 @@ EOF
   expect_log "WE DID IT FAM"
 }
 
-## Like test above, but testing an external dep can depend on a local target
-## with //... visibility
-function test_slashslashdotdotdot_includes_main_repo_from_external_dep() {
+# Like test above, but testing an external dep can depend on a local target with
+# with `public` visibility.
+#
+# TODO(brandjon): Eliminate this test, as described above?
+function test_public_includes_main_repo_from_external_dep() {
   create_new_workspace
   repo2=${new_workspace_dir}
   mkdir -p blue
@@ -1457,13 +1463,13 @@ local_repository(name = 'blue', path = "${repo2}")
 EOF
   cat > green/BUILD <<EOF
 package_group(
-    name = "slash-slash-dot-dot-dot",
-    packages = ["//..."],
+    name = "everyone",
+    packages = ["public"],
 )
 filegroup(
     name = "do-the-thing",
     srcs = ["do-the-thing.sh"],
-    visibility = [":slash-slash-dot-dot-dot"]
+    visibility = [":everyone"]
 )
 
 EOF

src/test/shell/bazel/platform_mapping_test.sh

@@ -184,7 +184,7 @@ my_rule = rule(
   attrs = {
       "deps": attr.label_list(cfg = my_transition),
       "_allowlist_function_transition": attr.label(
-          default = "@//tools/allowlists/function_transition_allowlist"),
+          default = "@bazel_tools//tools/allowlists/function_transition_allowlist"),
   }
 )
 EOF
@@ -256,7 +256,7 @@ transitioning_rule = rule(
   attrs = {
       "deps": attr.label_list(cfg = my_transition),
       "_allowlist_function_transition": attr.label(
-          default = "@//tools/allowlists/function_transition_allowlist"),
+          default = "@bazel_tools//tools/allowlists/function_transition_allowlist"),
   }
 )
 

tools/allowlists/config_feature_flag/BUILD

@@ -2,11 +2,6 @@
 #   Package groups for restricting access to config_feature_flag to specific
 #   packages, allowing for careful rollout as it is an experimental feature.
 
-package_group(
-    name = "config_feature_flag",
-    packages = ["//..."],
-)
-
 filegroup(
     name = "srcs",
     srcs = glob(["**"]),

tools/allowlists/config_feature_flag/BUILD.tools

@@ -0,0 +1,14 @@
+# Description:
+#   Package groups for restricting access to config_feature_flag to specific
+#   packages, allowing for careful rollout as it is an experimental feature.
+
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+    visibility = ["//tools/allowlists:__pkg__"],
+)
+
+package_group(
+    name = "config_feature_flag",
+    packages = ["public"],
+)

tools/allowlists/function_transition_allowlist/BUILD

@@ -1,10 +1,6 @@
 # Description:
-#   Package group restricting access to starlark-defined transitions, allowing for careful rollout as it is an experimental feature.
-
-package_group(
-    name = "function_transition_allowlist",
-    packages = ["//..."],
-)
+#   Package group restricting access to starlark-defined transitions, allowing
+#   for careful rollout as it is an experimental feature.
 
 filegroup(
     name = "srcs",

tools/allowlists/function_transition_allowlist/BUILD.tools

@@ -0,0 +1,14 @@
+# Description:
+#   Package group restricting access to starlark-defined transitions, allowing
+#   for careful rollout as it is an experimental feature.
+
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+    visibility = ["//tools/allowlists:__pkg__"],
+)
+
+package_group(
+    name = "function_transition_allowlist",
+    packages = ["public"],
+)

tools/android/BUILD.tools

@@ -523,17 +523,17 @@ config_setting(
 
 package_group(
     name = "android_device_allowlist",
-    packages = ["//..."],
+    packages = ["public"],
 )
 
 package_group(
     name = "export_deps_allowlist",
-    packages = ["//..."],
+    packages = ["public"],
 )
 
 package_group(
     name = "allow_android_library_deps_without_srcs_allowlist",
-    packages = ["//..."],
+    packages = ["public"],
 )
 
 sh_binary(

tools/whitelists/config_feature_flag/BUILD

@@ -2,11 +2,6 @@
 #   Package groups for restricting access to config_feature_flag to specific
 #   packages, allowing for careful rollout as it is an experimental feature.
 
-package_group(
-    name = "config_feature_flag",
-    includes = ["//tools/allowlists/config_feature_flag"],
-)
-
 filegroup(
     name = "srcs",
     srcs = glob(["**"]),

tools/whitelists/config_feature_flag/BUILD.tools

@@ -0,0 +1,14 @@
+# Description:
+#   Package groups for restricting access to config_feature_flag to specific
+#   packages, allowing for careful rollout as it is an experimental feature.
+
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+    visibility = ["//tools/whitelists:__pkg__"],
+)
+
+package_group(
+    name = "config_feature_flag",
+    includes = ["//tools/allowlists/config_feature_flag"],
+)

tools/whitelists/function_transition_whitelist/BUILD

@@ -1,10 +1,6 @@
 # Description:
-#   Package group restricting access to starlark-defined transitions, allowing for careful rollout as it is an experimental feature.
-
-package_group(
-    name = "function_transition_whitelist",
-    includes = ["//tools/allowlists/function_transition_allowlist"],
-)
+#   Package group restricting access to starlark-defined transitions, allowing
+#   for careful rollout as it is an experimental feature.
 
 filegroup(
     name = "srcs",

tools/whitelists/function_transition_whitelist/BUILD.tools

@@ -0,0 +1,14 @@
+# Description:
+#   Package group restricting access to starlark-defined transitions, allowing
+#   for careful rollout as it is an experimental feature.
+
+filegroup(
+    name = "srcs",
+    srcs = glob(["**"]),
+    visibility = ["//tools/whitelists:__pkg__"],
+)
+
+package_group(
+    name = "function_transition_whitelist",
+    includes = ["//tools/allowlists/function_transition_allowlist"],
+)