From 698626ff99c637016d23b7e5b24635dd67f49ca3 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 8 May 2024 11:36:10 -0700 Subject: [PATCH] Optionally restrict toolchain registration to single packages This restriction is tied to an experimental feature flag --experimental_single_package_toolchain_binding. This flag will exist as an option for projects to restrict more complex bindings in WORKSPACE and MODULE files, but will not be flipped true-by-default for the foreseeable future. PiperOrigin-RevId: 631870945 Change-Id: I93f1eda65c2d8f6af34f7e43bc15dca0e6a0d616 --- .../devtools/build/lib/bazel/bzlmod/BUILD | 1 + .../lib/bazel/bzlmod/ModuleFileGlobals.java | 19 +++++-- .../build/lib/packages/WorkspaceGlobals.java | 19 ++++++- .../semantics/BuildLanguageOptions.java | 16 ++++++ .../bazel/bzlmod/ModuleFileFunctionTest.java | 51 +++++++++++++++++++ .../packages/semantics/ConsistencyTest.java | 3 ++ .../skyframe/WorkspaceFileFunctionTest.java | 36 +++++++++++++ 7 files changed, 140 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD index 77292109787de7..7229de6e3e74ba 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BUILD @@ -162,6 +162,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/cmdline", "//src/main/java/com/google/devtools/build/lib/events", "//src/main/java/com/google/devtools/build/lib/packages", + "//src/main/java/com/google/devtools/build/lib/packages/semantics", "//src/main/java/com/google/devtools/build/lib/rules:repository/repo_recorded_input", "//src/main/java/com/google/devtools/build/lib/skyframe:sky_functions", "//src/main/java/com/google/devtools/build/lib/skyframe/serialization:visible-for-serialization", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java index 1ee8e2df30f9db..efc449178f9584 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java @@ -33,6 +33,7 @@ import com.google.devtools.build.lib.cmdline.RepositoryName; import com.google.devtools.build.lib.events.EventHandler; import com.google.devtools.build.lib.packages.StarlarkExportable; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.Map; import java.util.regex.Pattern; @@ -369,9 +370,21 @@ public void registerToolchains( if (context.shouldIgnoreDevDeps() && devDependency) { return; } - context - .getModuleBuilder() - .addToolchainsToRegister(checkAllAbsolutePatterns(toolchainLabels, "register_toolchains")); + ImmutableList checkedToolchainLabels = + checkAllAbsolutePatterns(toolchainLabels, "register_toolchains"); + if (thread + .getSemantics() + .getBool(BuildLanguageOptions.EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING)) { + for (String label : checkedToolchainLabels) { + if (label.contains("...")) { + throw Starlark.errorf( + "invalid target pattern \"%s\": register_toolchain target patterns may only refer to " + + "targets within a single package", + label); + } + } + } + context.getModuleBuilder().addToolchainsToRegister(checkedToolchainLabels); } @StarlarkMethod( diff --git a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java index 26326711b652bf..1089b77204d77b 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java +++ b/src/main/java/com/google/devtools/build/lib/packages/WorkspaceGlobals.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.cmdline.TargetPattern; import com.google.devtools.build.lib.packages.RuleFactory.InvalidRuleException; import com.google.devtools.build.lib.packages.TargetDefinitionContext.NameConflictException; +import com.google.devtools.build.lib.packages.semantics.BuildLanguageOptions; import com.google.devtools.build.lib.starlarkbuildapi.WorkspaceGlobalsApi; import com.google.devtools.build.lib.vfs.PathFragment; import java.util.List; @@ -118,9 +119,23 @@ public void registerToolchains(Sequence toolchainLabels, StarlarkThread threa Package.Builder builder = Package.Builder.fromOrFailDisallowingSymbolicMacros(thread, "register_toolchains()"); List patterns = Sequence.cast(toolchainLabels, String.class, "toolchain_labels"); + + ImmutableList targetPatterns = parsePatterns(patterns, builder, thread); + + if (thread + .getSemantics() + .getBool(BuildLanguageOptions.EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING)) { + for (TargetPattern tp : targetPatterns) { + if (tp.getType() == TargetPattern.Type.TARGETS_BELOW_DIRECTORY) { + throw Starlark.errorf( + "invalid target pattern \"%s\": register_toolchain target patterns may only refer to " + + "targets within a single package", + tp.getOriginalPattern()); + } + } + } builder.addRegisteredToolchains( - parsePatterns(patterns, builder, thread), - originatesInWorkspaceSuffix(thread.getCallStack())); + targetPatterns, originatesInWorkspaceSuffix(thread.getCallStack())); } @Override 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 73cf13a2f02fec..4d885b0fc765a0 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 @@ -157,6 +157,17 @@ public final class BuildLanguageOptions extends OptionsBase { + " evaluation to set their visibility for the purpose of load() statements.") public boolean experimentalBzlVisibility; + @Option( + name = "experimental_single_package_toolchain_binding", + defaultValue = "false", + documentationCategory = OptionDocumentationCategory.STARLARK_SEMANTICS, + effectTags = {OptionEffectTag.LOADING_AND_ANALYSIS}, + metadataTags = {OptionMetadataTag.INCOMPATIBLE_CHANGE}, + help = + "If enabled, the register_toolchain function may not include target patterns which may " + + "refer to more than one package.") + public boolean experimentalSinglePackageToolchainBinding; + @Option( name = "check_bzl_visibility", defaultValue = "true", @@ -748,6 +759,9 @@ public StarlarkSemantics toStarlarkSemantics() { .setBool(CHECK_BZL_VISIBILITY, checkBzlVisibility) .setBool( EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS, experimentalEnableAndroidMigrationApis) + .setBool( + EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING, + experimentalSinglePackageToolchainBinding) .setBool(EXPERIMENTAL_ENABLE_FIRST_CLASS_MACROS, experimentalEnableFirstClassMacros) .setBool(EXPERIMENTAL_ENABLE_SCL_DIALECT, experimentalEnableSclDialect) .setBool(ENABLE_BZLMOD, enableBzlmod) @@ -853,6 +867,8 @@ public StarlarkSemantics toStarlarkSemantics() { "-experimental_disable_external_package"; public static final String EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS = "-experimental_enable_android_migration_apis"; + public static final String EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING = + "-experimental_single_package_toolchain_binding"; public static final String EXPERIMENTAL_ENABLE_FIRST_CLASS_MACROS = "-experimental_enable_first_class_macros"; public static final String EXPERIMENTAL_ENABLE_SCL_DIALECT = "-experimental_enable_scl_dialect"; diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java index bf4d985c03e506..a83126a0e51751 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java @@ -1476,4 +1476,55 @@ public void isolatedUsage_notEnabled() throws Exception { + "and thus unavailable with the current flags. It may be enabled by setting " + "--experimental_isolated_extension_usages"); } + + @Test + public void testRegisterToolchains_singlePackageRestriction_error() throws Exception { + // Test intentionally introduces errors. + reporter.removeHandler(failFastHandler); + PrecomputedValue.STARLARK_SEMANTICS.set( + differencer, + StarlarkSemantics.builder() + .setBool(BuildLanguageOptions.EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING, true) + .build()); + + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + "module(name='aaa')", + "register_toolchains('//bar/...')"); + + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl())); + + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isTrue(); + + assertContainsEvent( + "invalid target pattern \"//bar/...\": register_toolchain target patterns " + + "may only refer to targets within a single package"); + } + + @Test + public void testRegisterToolchains_singlePackageRestriction() throws Exception { + PrecomputedValue.STARLARK_SEMANTICS.set( + differencer, + StarlarkSemantics.builder() + .setBool(BuildLanguageOptions.EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING, true) + .build()); + + scratch.overwriteFile( + rootDirectory.getRelative("MODULE.bazel").getPathString(), + "module(name='aaa')", + "register_toolchains('//:whatever')", + "register_toolchains('//bar:all')", + "register_toolchains('//qux:baz')"); + FakeRegistry registry = registryFactory.newFakeRegistry("/foo"); + ModuleFileFunction.REGISTRIES.set(differencer, ImmutableSet.of(registry.getUrl())); + + EvaluationResult result = + evaluator.evaluate( + ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext); + assertThat(result.hasError()).isFalse(); + } } diff --git a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java index ce23adc1a87f6d..609e570ed92e9f 100644 --- a/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java +++ b/src/test/java/com/google/devtools/build/lib/packages/semantics/ConsistencyTest.java @@ -123,6 +123,7 @@ private static BuildLanguageOptions buildRandomOptions(Random rand) throws Excep "--experimental_builtins_dummy=" + rand.nextBoolean(), "--experimental_bzl_visibility=" + rand.nextBoolean(), "--experimental_enable_android_migration_apis=" + rand.nextBoolean(), + "--experimental_single_package_toolchain_binding=" + rand.nextBoolean(), "--enable_bzlmod=" + rand.nextBoolean(), "--enable_workspace=" + rand.nextBoolean(), "--experimental_isolated_extension_usages=" + rand.nextBoolean(), @@ -167,6 +168,8 @@ private static StarlarkSemantics buildRandomSemantics(Random rand) { .setBool(BuildLanguageOptions.EXPERIMENTAL_BZL_VISIBILITY, rand.nextBoolean()) .setBool( BuildLanguageOptions.EXPERIMENTAL_ENABLE_ANDROID_MIGRATION_APIS, rand.nextBoolean()) + .setBool( + BuildLanguageOptions.EXPERIMENTAL_SINGLE_PACKAGE_TOOLCHAIN_BINDING, rand.nextBoolean()) .setBool(BuildLanguageOptions.ENABLE_BZLMOD, rand.nextBoolean()) .setBool(BuildLanguageOptions.ENABLE_WORKSPACE, rand.nextBoolean()) .setBool(BuildLanguageOptions.EXPERIMENTAL_ISOLATED_EXTENSION_USAGES, rand.nextBoolean()) diff --git a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java index 15937d4d453cec..ed7557eb1164a3 100644 --- a/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/skyframe/WorkspaceFileFunctionTest.java @@ -253,6 +253,42 @@ public void testRegisterToolchainsInvalidPattern() throws Exception { assertContainsEvent("error parsing target pattern"); } + @Test + public void testRegisterToolchains_singlePackageRestriction() throws Exception { + setBuildLanguageOptions("--experimental_single_package_toolchain_binding"); + + String[] lines = { + "register_toolchains('//foo:all')", + "register_toolchains('//bar:bar')", + "register_toolchains('//pkg/to/baz')" + }; + createWorkspaceFile(lines); + + SkyKey key = ExternalPackageFunction.key(); + EvaluationResult evaluationResult = eval(key); + Package pkg = evaluationResult.get(key).getPackage(); + assertThat(pkg.containsErrors()).isFalse(); + } + + @Test + public void testRegisterToolchains_singlePackageRestriction_error() throws Exception { + // Test intentionally introduces errors. + reporter.removeHandler(failFastHandler); + + setBuildLanguageOptions("--experimental_single_package_toolchain_binding"); + + String[] lines = {"register_toolchains('//foo/...')"}; + createWorkspaceFile(lines); + + SkyKey key = ExternalPackageFunction.key(); + EvaluationResult evaluationResult = eval(key); + Package pkg = evaluationResult.get(key).getPackage(); + assertThat(pkg.containsErrors()).isTrue(); + assertContainsEvent( + "invalid target pattern \"//foo/...\": register_toolchain target patterns " + + "may only refer to targets within a single package"); + } + @Test public void testNoWorkspaceFile() throws Exception { // Create and immediately delete to make sure we got the right file.