From 0511bb4b73a4b566636ffdcc930cbb682313de97 Mon Sep 17 00:00:00 2001 From: Googler Date: Wed, 17 May 2023 14:34:51 -0700 Subject: [PATCH] Eliminate PackageArgument The package() function is now implemented by a @StarlarkMethod rather than an anonymous subclass of StarlarkCallable. The PackageArgument machinery is deleted. The convert-and-process approach is instead inlined into a series of if-elses for each argument (with the convert step using a helper to reduce repetition). The big if-else is factored out into an overridable method, so other implementations of package() can add their own parameters on top. The package() symbol is now registered on the ConfiguredRuleClassProvider like any other BUILD toplevel. Removed its special casing in BazelStarlarkEnvironment, and the ability to register PackageArguments with the CRCP. Fixed a copy-paste error where the "default_applicable_licenses" param would claim to be "default_package_metadata" for the purposes of error messages. PiperOrigin-RevId: 532911239 Change-Id: I957354e0a4e32f7922b510c5537f56c6aff72ab9 --- .../analysis/ConfiguredRuleClassProvider.java | 15 - .../devtools/build/lib/bazel/rules/BUILD | 1 + .../bazel/rules/BazelRuleClassProvider.java | 10 + .../packages/BazelStarlarkEnvironment.java | 17 +- .../build/lib/packages/PackageArgument.java | 57 ---- .../build/lib/packages/PackageCallable.java | 290 +++++------------- 6 files changed, 84 insertions(+), 306 deletions(-) delete mode 100644 src/main/java/com/google/devtools/build/lib/packages/PackageArgument.java diff --git a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java index 25187978c69e6f..8f8e5d64347368 100644 --- a/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/analysis/ConfiguredRuleClassProvider.java @@ -49,8 +49,6 @@ import com.google.devtools.build.lib.graph.Node; import com.google.devtools.build.lib.packages.BazelStarlarkEnvironment; import com.google.devtools.build.lib.packages.NativeAspectClass; -import com.google.devtools.build.lib.packages.PackageArgument; -import com.google.devtools.build.lib.packages.PackageCallable; import com.google.devtools.build.lib.packages.RuleClass; import com.google.devtools.build.lib.packages.RuleClassProvider; import com.google.devtools.build.lib.packages.RuleFactory; @@ -157,7 +155,6 @@ public static final class Builder implements RuleDefinitionEnvironment { private final Map ruleClassMap = new HashMap<>(); private final Map ruleDefinitionMap = new HashMap<>(); private final Map nativeAspectClassMap = new HashMap<>(); - private final List> packageArgs = new ArrayList<>(); private final Map, RuleClass> ruleMap = new HashMap<>(); private final Digraph> dependencyGraph = new Digraph<>(); private final List> universalFragments = new ArrayList<>(); @@ -306,12 +303,6 @@ public Builder addNativeAspectClass(NativeAspectClass aspectFactoryClass) { return this; } - @CanIgnoreReturnValue - public Builder addPackageArg(PackageArgument arg) { - packageArgs.add(arg); - return this; - } - /** * Adds a configuration fragment and all build options required by its fragment. * @@ -608,7 +599,6 @@ public ConfiguredRuleClassProvider build() { ImmutableMap.copyOf(ruleClassMap), ImmutableMap.copyOf(ruleDefinitionMap), ImmutableMap.copyOf(nativeAspectClassMap), - ImmutableList.copyOf(packageArgs), FragmentRegistry.create( configurationFragmentClasses, universalFragments, configurationOptions), defaultWorkspaceFilePrefix.toString(), @@ -728,7 +718,6 @@ private ConfiguredRuleClassProvider( ImmutableMap ruleClassMap, ImmutableMap ruleDefinitionMap, ImmutableMap nativeAspectClassMap, - ImmutableList> packageArgs, FragmentRegistry fragmentRegistry, String defaultWorkspaceFilePrefix, String defaultWorkspaceFileSuffix, @@ -780,10 +769,6 @@ private ConfiguredRuleClassProvider( this.bazelStarlarkEnvironment = new BazelStarlarkEnvironment( /* ruleFunctions= */ RuleFactory.buildRuleFunctions(ruleClassMap), - // TODO(b/280446865): Instead of exposing the {@code package()} symbol separately, just - // keep it part of the BUILD/native environments. Requires migrating {@code package()} - // to be a @StarlarkMethod that gets registered as a BUILD toplevel. - /* packageCallable= */ PackageCallable.newPackageCallable(packageArgs), buildFileToplevels, /* bzlToplevels= */ environment, /* nativeRuleSpecificBindings= */ nativeRuleSpecificBindings, diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD b/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD index 158b0257f83326..ddb8968dcbb2fd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BUILD @@ -64,6 +64,7 @@ java_library( "//src/main/java/com/google/devtools/build/lib/exec:module_action_context_registry", "//src/main/java/com/google/devtools/build/lib/exec:spawn_cache", "//src/main/java/com/google/devtools/build/lib/exec:spawn_strategy_registry", + "//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/remote", "//src/main/java/com/google/devtools/build/lib/remote/options", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java index b1b3ee498774be..0b640057aaf120 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/rules/BazelRuleClassProvider.java @@ -59,6 +59,7 @@ import com.google.devtools.build.lib.bazel.rules.python.BazelPythonConfiguration; import com.google.devtools.build.lib.cmdline.LabelConstants; import com.google.devtools.build.lib.cmdline.RepositoryName; +import com.google.devtools.build.lib.packages.PackageCallable; import com.google.devtools.build.lib.rules.android.AarImportBaseRule; import com.google.devtools.build.lib.rules.android.AndroidApplicationResourceInfo; import com.google.devtools.build.lib.rules.android.AndroidAssetsInfo; @@ -308,6 +309,15 @@ public void init(ConfiguredRuleClassProvider.Builder builder) { builder.addStarlarkBuiltinsInternal(CcStarlarkInternal.NAME, new CcStarlarkInternal()); builder.addStarlarkBuiltinsInternal( BazelObjcStarlarkInternal.NAME, new BazelObjcStarlarkInternal()); + + // Add the package() function. + // TODO(bazel-team): Factor this into a group of similar BUILD definitions, or add a more + // convenient way of obtaining a BuiltinFunction than addMethods(). + ImmutableMap.Builder symbols = ImmutableMap.builder(); + Starlark.addMethods(symbols, PackageCallable.INSTANCE); + for (Map.Entry entry : symbols.buildOrThrow().entrySet()) { + builder.addBuildFileToplevel(entry.getKey(), entry.getValue()); + } } }; diff --git a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java index f17325db878536..b8b5f08c4690b1 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java +++ b/src/main/java/com/google/devtools/build/lib/packages/BazelStarlarkEnvironment.java @@ -81,7 +81,6 @@ public final class BazelStarlarkEnvironment { * * @param ruleFunctions a map from a rule class name (e.g. "java_library") to the (uninjected) * Starlark callable that instantiates it - * @param packageCallable the symbol implementing the {@code package()} function in BUILD files * @param buildFileToplevels the map of non-{@link Starlark#UNIVERSE} top-level symbols available * to BUILD files, prior to builtins injection * @param bzlToplevels the map of non-universe top-level symbols available to .bzl files, prior to @@ -97,7 +96,6 @@ public final class BazelStarlarkEnvironment { */ public BazelStarlarkEnvironment( ImmutableMap ruleFunctions, - Object packageCallable, ImmutableMap buildFileToplevels, ImmutableMap bzlToplevels, ImmutableMap nativeRuleSpecificBindings, @@ -110,7 +108,7 @@ public BazelStarlarkEnvironment( this.workspaceBzlNativeBindings = workspaceBzlNativeBindings; this.uninjectedBuildBzlNativeBindings = - createUninjectedBuildBzlNativeBindings(ruleFunctions, packageCallable, buildFileToplevels); + createUninjectedBuildBzlNativeBindings(ruleFunctions, buildFileToplevels); this.uninjectedBuildBzlEnv = createUninjectedBuildBzlEnv(bzlToplevels, uninjectedBuildBzlNativeBindings); this.uninjectedWorkspaceBzlEnv = @@ -126,8 +124,7 @@ public BazelStarlarkEnvironment( builtinsInternals, uninjectedBuildBzlNativeBindings, uninjectedBuildBzlEnv); - this.uninjectedBuildEnv = - createUninjectedBuildEnv(ruleFunctions, packageCallable, buildFileToplevels); + this.uninjectedBuildEnv = createUninjectedBuildEnv(ruleFunctions, buildFileToplevels); } /** @@ -186,13 +183,10 @@ public ImmutableMap getBzlmodBzlEnv() { * injection didn't happen. */ private static ImmutableMap createUninjectedBuildBzlNativeBindings( - Map ruleFunctions, - Object packageCallable, - Map buildFileToplevels) { + Map ruleFunctions, Map buildFileToplevels) { ImmutableMap.Builder env = new ImmutableMap.Builder<>(); env.putAll(StarlarkNativeModule.BINDINGS_FOR_BUILD_FILES); env.putAll(ruleFunctions); - env.put("package", packageCallable); env.putAll(buildFileToplevels); return env.buildOrThrow(); } @@ -217,13 +211,10 @@ private static ImmutableMap createUninjectedBuildBzlEnv( } private static ImmutableMap createUninjectedBuildEnv( - Map ruleFunctions, - Object packageCallable, - Map buildFileToplevels) { + Map ruleFunctions, Map buildFileToplevels) { ImmutableMap.Builder env = ImmutableMap.builder(); env.putAll(StarlarkLibrary.BUILD); // e.g. select, depset env.putAll(StarlarkNativeModule.BINDINGS_FOR_BUILD_FILES); - env.put("package", packageCallable); env.putAll(ruleFunctions); env.putAll(buildFileToplevels); return env.buildOrThrow(); diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageArgument.java b/src/main/java/com/google/devtools/build/lib/packages/PackageArgument.java deleted file mode 100644 index a23150a64c411c..00000000000000 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageArgument.java +++ /dev/null @@ -1,57 +0,0 @@ -// Copyright 2020 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; - -import net.starlark.java.eval.EvalException; -import net.starlark.java.syntax.Location; - -/** Defines an argument to the {@code package()} function. */ -public abstract class PackageArgument { - private final String name; - private final Type type; - - protected PackageArgument(String name, Type type) { - this.name = name; - this.type = type; - } - - String getName() { - return name; - } - - /** - * Converts an untyped argument to a typed one, then calls the user-provided {@link #process}. - * - * Note that the location is used not just for exceptions (for which null would do), but also for - * reporting events. - */ - final void convertAndProcess( - Package.Builder pkgBuilder, Location location, Object value) - throws EvalException { - T typedValue = type.convert(value, "'package' argument", pkgBuilder.getLabelConverter()); - process(pkgBuilder, location, typedValue); - } - - /** - * Processes an argument. - * - * @param pkgBuilder the package builder to be mutated - * @param location the location of the {@code package} function for error reporting - * @param value the value of the argument. Typically passed to {@link Type#convert} - */ - protected abstract void process( - Package.Builder pkgBuilder, Location location, T value) - throws EvalException; -} diff --git a/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java b/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java index 63909262a4ada0..3f8db53ca1fdd8 100644 --- a/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java +++ b/src/main/java/com/google/devtools/build/lib/packages/PackageCallable.java @@ -14,276 +14,124 @@ package com.google.devtools.build.lib.packages; -import com.google.common.collect.ImmutableList; -import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.cmdline.Label; import com.google.devtools.build.lib.packages.License.DistributionType; import com.google.devtools.build.lib.server.FailureDetails.PackageLoading.Code; import java.util.List; import java.util.Map; import java.util.Set; -import net.starlark.java.eval.Dict; +import net.starlark.java.annot.Param; +import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.EvalException; -import net.starlark.java.eval.Printer; import net.starlark.java.eval.Starlark; -import net.starlark.java.eval.StarlarkCallable; import net.starlark.java.eval.StarlarkThread; -import net.starlark.java.eval.Tuple; import net.starlark.java.syntax.Location; /** - * Utility class encapsulating the definition of the {@code package()} function of BUILD files. - * - *

Also includes the definitions of those arguments to {@code package()} that are available in - * all Bazel environments. + * Utility class encapsulating the standard definition of the {@code package()} function of BUILD + * files. */ public class PackageCallable { - private PackageCallable() {} + protected PackageCallable() {} - /** - * Returns a {@link StarlarkCallable} that implements the {@code package()} functionality. - * - * @param packageArgs a list of {@link PackageArgument}s to support, beyond the standard ones - * included in every Bazel environment - */ - // TODO(b/280446865): Consider eliminating the package() extensibility mechanism altogether. - // There is currently only one use case: distinguishing the set of package() arguments available - // in OSS Bazel vs internally to Google. Instead of registering these arguments and passing them - // to this factory method to obtain a package() callable, we could instead define two - // @StarlarkMethod-annotated Java functions implementing the two versions of package(), and - // register the appropriate one with the ConfiguredRuleClassProvider.Builder. - public static StarlarkCallable newPackageCallable(List> packageArgs) { - // Construct a map of PackageArguments, which the returned callable closes over. - ImmutableMap.Builder> argsBuilder = ImmutableMap.builder(); - for (PackageArgument arg : getCommonArguments()) { - argsBuilder.put(arg.getName(), arg); - } - for (PackageArgument arg : packageArgs) { - argsBuilder.put(arg.getName(), arg); - } - final ImmutableMap> packageArguments = argsBuilder.buildOrThrow(); - - return new StarlarkCallable() { - @Override - public String getName() { - return "package"; - } - - @Override - public String toString() { - return "package(...)"; - } - - @Override - public boolean isImmutable() { - return true; - } + public static final PackageCallable INSTANCE = new PackageCallable(); - @Override - public void repr(Printer printer) { - printer.append(""); - } - - @Override - public Object call(StarlarkThread thread, Tuple args, Dict kwargs) - throws EvalException { - if (!args.isEmpty()) { - throw new EvalException("unexpected positional arguments"); - } - Package.Builder pkgBuilder = PackageFactory.getContext(thread).pkgBuilder; - - // Validate parameter list - if (pkgBuilder.isPackageFunctionUsed()) { - throw new EvalException("'package' can only be used once per BUILD file"); - } - pkgBuilder.setPackageFunctionUsed(); - - // Each supplied argument must name a PackageArgument. - if (kwargs.isEmpty()) { - throw new EvalException("at least one argument must be given to the 'package' function"); - } - Location loc = thread.getCallerLocation(); - for (Map.Entry kwarg : kwargs.entrySet()) { - String name = kwarg.getKey(); - PackageArgument pkgarg = packageArguments.get(name); - if (pkgarg == null) { - throw Starlark.errorf("unexpected keyword argument: %s", name); - } - pkgarg.convertAndProcess(pkgBuilder, loc, kwarg.getValue()); - } - return Starlark.NONE; - } - }; - } - - /** Returns the basic set of {@link PackageArgument}s. */ - private static ImmutableList> getCommonArguments() { - return ImmutableList.of( - new DefaultDeprecation(), - new DefaultDistribs(), - new DefaultApplicableLicenses(), - new DefaultPackageMetadata(), - new DefaultLicenses(), - new DefaultTestOnly(), - new DefaultVisibility(), - new Features(), - new DefaultCompatibleWith(), - new DefaultRestrictedTo()); - } + @StarlarkMethod( + name = "package", + documented = false, // documented in docgen/templates/be/functions.vm + extraKeywords = @Param(name = "kwargs", defaultValue = "{}"), + useStarlarkThread = true) + public Object packageCallable(Map kwargs, StarlarkThread thread) + throws EvalException { + Package.Builder pkgBuilder = PackageFactory.getContext(thread).pkgBuilder; + if (pkgBuilder.isPackageFunctionUsed()) { + throw new EvalException("'package' can only be used once per BUILD file"); + } + pkgBuilder.setPackageFunctionUsed(); - private static class DefaultVisibility extends PackageArgument> { - private DefaultVisibility() { - super("default_visibility", BuildType.LABEL_LIST); + if (kwargs.isEmpty()) { + throw new EvalException("at least one argument must be given to the 'package' function"); } - @Override - protected void process(Package.Builder pkgBuilder, Location location, List