From e899d85302b4a51ce27a650fa5691ee2388a0c14 Mon Sep 17 00:00:00 2001 From: Googler Date: Tue, 18 Oct 2022 14:28:47 -0700 Subject: [PATCH] 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 --- .../build/lib/packages/semantics/BUILD | 5 ++- .../semantics/BuildLanguageOptions.java | 8 ++--- .../lib/packages/semantics/FlagConstants.java | 31 ++++++++++++++++++ .../devtools/build/lib/query2/common/BUILD | 1 + .../lib/query2/common/CommonQueryOptions.java | 2 +- .../lib/query2/common/FlagConstants.java | 25 +++++++++++++++ src/main/java/net/starlark/java/BUILD | 3 ++ .../lib/analysis/mock/BazelAnalysisMock.java | 10 +++--- .../build/lib/packages/PackageGroupTest.java | 6 +++- .../lib/rules/android/AndroidDeviceTest.java | 3 ++ src/test/shell/bazel/local_repository_test.sh | 32 +++++++++++-------- src/test/shell/bazel/platform_mapping_test.sh | 4 +-- tools/allowlists/config_feature_flag/BUILD | 5 --- .../config_feature_flag/BUILD.tools | 14 ++++++++ .../function_transition_allowlist/BUILD | 8 ++--- .../function_transition_allowlist/BUILD.tools | 14 ++++++++ tools/android/BUILD.tools | 6 ++-- tools/whitelists/config_feature_flag/BUILD | 5 --- .../config_feature_flag/BUILD.tools | 14 ++++++++ .../function_transition_whitelist/BUILD | 8 ++--- .../function_transition_whitelist/BUILD.tools | 14 ++++++++ 21 files changed, 166 insertions(+), 52 deletions(-) create mode 100644 src/main/java/com/google/devtools/build/lib/packages/semantics/FlagConstants.java create mode 100644 src/main/java/com/google/devtools/build/lib/query2/common/FlagConstants.java create mode 100644 tools/allowlists/config_feature_flag/BUILD.tools create mode 100644 tools/allowlists/function_transition_allowlist/BUILD.tools create mode 100644 tools/whitelists/config_feature_flag/BUILD.tools create mode 100644 tools/whitelists/function_transition_whitelist/BUILD.tools diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/BUILD b/src/main/java/com/google/devtools/build/lib/packages/semantics/BUILD index c16aff183a1277..e6cc4100bd55a7 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/semantics/BUILD +++ b/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", 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 47444580bf52ed..4eef9b6f010f84 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 @@ -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 = diff --git a/src/main/java/com/google/devtools/build/lib/packages/semantics/FlagConstants.java b/src/main/java/com/google/devtools/build/lib/packages/semantics/FlagConstants.java new file mode 100644 index 00000000000000..973474357c678a --- /dev/null +++ b/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"; +} diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/BUILD b/src/main/java/com/google/devtools/build/lib/query2/common/BUILD index 8cddf7ea135325..110aa501f7e55e 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/common/BUILD +++ b/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", diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java b/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java index eb44883d47703f..09483e52cb7d2d 100644 --- a/src/main/java/com/google/devtools/build/lib/query2/common/CommonQueryOptions.java +++ b/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}, diff --git a/src/main/java/com/google/devtools/build/lib/query2/common/FlagConstants.java b/src/main/java/com/google/devtools/build/lib/query2/common/FlagConstants.java new file mode 100644 index 00000000000000..785927a6892219 --- /dev/null +++ b/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"; +} diff --git a/src/main/java/net/starlark/java/BUILD b/src/main/java/net/starlark/java/BUILD index 78a9b855f7f037..e163e8404bf019 100644 --- a/src/main/java/net/starlark/java/BUILD +++ b/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 ) diff --git a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java index 77d3ef84eca4fc..5d196a2e8c3004 100644 --- a/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java +++ b/src/test/java/com/google/devtools/build/lib/analysis/mock/BazelAnalysisMock.java @@ -362,8 +362,8 @@ public void setupMockClient(MockToolsConfig config, List 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 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'])"); diff --git a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java b/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java index f91a8492252171..e27ba519778108 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/PackageGroupTest.java +++ b/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", // diff --git a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidDeviceTest.java b/src/test/java/com/google/devtools/build/lib/rules/android/AndroidDeviceTest.java index ccd389db42b53e..a5732a8b7620a3 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/android/AndroidDeviceTest.java +++ b/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(); diff --git a/src/test/shell/bazel/local_repository_test.sh b/src/test/shell/bazel/local_repository_test.sh index d9829e7b0d543a..6d946f7c7dc386 100755 --- a/src/test/shell/bazel/local_repository_test.sh +++ b/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 < blue/do-the-thing.sh < green/BUILD <