From d09077ee91e646382fb901e256978b6f660600a2 Mon Sep 17 00:00:00 2001 From: wyv Date: Fri, 18 Jun 2021 06:27:50 -0700 Subject: [PATCH] bzlmod: Remove override_dep in favor of having module_name on each override type (https://github.com/bazelbuild/bazel/issues/13316) We're "denormalizing" the override API: whereas you'd write "override_dep(name='X', Y_override(...))" before, you'd now write "Y_override(module_name='X', ...)". PiperOrigin-RevId: 380173439 --- .../devtools/build/lib/bazel/bzlmod/BUILD | 1 + .../lib/bazel/bzlmod/ModuleFileFunction.java | 7 +- .../lib/bazel/bzlmod/ModuleFileGlobals.java | 69 +++++++++-------- .../lib/bazel/bzlmod/ModuleFileValue.java | 11 ++- .../bzlmod/ModuleOverride.java} | 19 ++--- .../lib/bazel/bzlmod/NonRegistryOverride.java | 4 +- .../lib/bazel/bzlmod/RegistryOverride.java | 4 +- .../repository/ModuleFileGlobalsApi.java | 75 +++++++++++-------- .../bazel/bzlmod/ModuleFileFunctionTest.java | 6 +- 9 files changed, 105 insertions(+), 91 deletions(-) rename src/main/java/com/google/devtools/build/lib/{starlarkbuildapi/repository/StarlarkOverrideApi.java => bazel/bzlmod/ModuleOverride.java} (58%) 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 25e2bae093b32c..51468b10d69c85 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 @@ -40,6 +40,7 @@ java_library( "ModuleFileFunction.java", "ModuleFileGlobals.java", "ModuleFileValue.java", + "ModuleOverride.java", "NonRegistryOverride.java", "RegistryOverride.java", "SingleVersionOverride.java", diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java index 67dc0b89699be2..69b80ab21d40e6 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunction.java @@ -22,7 +22,6 @@ import com.google.devtools.build.lib.packages.NoSuchThingException; import com.google.devtools.build.lib.skyframe.PrecomputedValue; import com.google.devtools.build.lib.skyframe.PrecomputedValue.Precomputed; -import com.google.devtools.build.lib.starlarkbuildapi.repository.StarlarkOverrideApi; import com.google.devtools.build.lib.vfs.FileSystemUtils; import com.google.devtools.build.lib.vfs.Path; import com.google.devtools.build.lib.vfs.PathFragment; @@ -121,8 +120,8 @@ private SkyValue computeForRootModule(StarlarkSemantics starlarkSemantics, Envir Module module = moduleFileGlobals.buildModule(null); // Check that overrides don't contain the root module itself. - ImmutableMap overrides = moduleFileGlobals.buildOverrides(); - StarlarkOverrideApi rootOverride = overrides.get(module.getName()); + ImmutableMap overrides = moduleFileGlobals.buildOverrides(); + ModuleOverride rootOverride = overrides.get(module.getName()); if (rootOverride != null) { throw errorf("invalid override for the root module found: %s", rootOverride); } @@ -162,7 +161,7 @@ private static class GetModuleFileResult { @Nullable private GetModuleFileResult getModuleFile( - ModuleKey key, @Nullable StarlarkOverrideApi override, Environment env) + ModuleKey key, @Nullable ModuleOverride override, Environment env) throws ModuleFileFunctionException, InterruptedException { // If there is a non-registry override for this module, we need to fetch the corresponding repo // first and read the module file from there. 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 3eec8546c97998..0f6f074241ed13 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 @@ -19,7 +19,6 @@ import com.google.common.collect.ImmutableMap; import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileFunction.ModuleFileFunctionException; import com.google.devtools.build.lib.starlarkbuildapi.repository.ModuleFileGlobalsApi; -import com.google.devtools.build.lib.starlarkbuildapi.repository.StarlarkOverrideApi; import java.util.HashMap; import java.util.LinkedHashMap; import java.util.Map; @@ -33,7 +32,7 @@ public class ModuleFileGlobals implements ModuleFileGlobalsApi deps = new LinkedHashMap<>(); - private final Map overrides = new HashMap<>(); + private final Map overrides = new HashMap<>(); public ModuleFileGlobals() {} @@ -57,11 +56,10 @@ public void bazelDep(String name, String version, String repoName) throws EvalEx } } - @Override - public void overrideDep(String name, StarlarkOverrideApi override) throws EvalException { - StarlarkOverrideApi existingOverride = overrides.putIfAbsent(name, override); + private void addOverride(String moduleName, ModuleOverride override) throws EvalException { + ModuleOverride existingOverride = overrides.putIfAbsent(moduleName, override); if (existingOverride != null) { - throw Starlark.errorf("multiple overrides for dep %s found", name); + throw Starlark.errorf("multiple overrides for dep %s found", moduleName); } } @@ -82,18 +80,25 @@ private static ImmutableList checkAllStrings(Iterable iterable, Strin } @Override - public StarlarkOverrideApi singleVersionOverride( - String version, String registry, Iterable patches, StarlarkInt patchStrip) + public void singleVersionOverride( + String moduleName, + String version, + String registry, + Iterable patches, + StarlarkInt patchStrip) throws EvalException { - return SingleVersionOverride.create( - version, - registry, - checkAllStrings(patches, "patches"), - patchStrip.toInt("single_version_override.patch_strip")); + addOverride( + moduleName, + SingleVersionOverride.create( + version, + registry, + checkAllStrings(patches, "patches"), + patchStrip.toInt("single_version_override.patch_strip"))); } @Override - public StarlarkOverrideApi archiveOverride( + public void archiveOverride( + String moduleName, Object urls, String integrity, String stripPrefix, @@ -104,35 +109,39 @@ public StarlarkOverrideApi archiveOverride( urls instanceof String ? ImmutableList.of((String) urls) : checkAllStrings((Iterable) urls, "urls"); - return ArchiveOverride.create( - urlList, - checkAllStrings(patches, "patches"), - integrity, - stripPrefix, - patchStrip.toInt("archive_override.patch_strip")); + addOverride( + moduleName, + ArchiveOverride.create( + urlList, + checkAllStrings(patches, "patches"), + integrity, + stripPrefix, + patchStrip.toInt("archive_override.patch_strip"))); } @Override - public StarlarkOverrideApi gitOverride( - String remote, String commit, Iterable patches, StarlarkInt patchStrip) + public void gitOverride( + String moduleName, String remote, String commit, Iterable patches, StarlarkInt patchStrip) throws EvalException { - return GitOverride.create( - remote, - commit, - checkAllStrings(patches, "patches"), - patchStrip.toInt("git_override.patch_strip")); + addOverride( + moduleName, + GitOverride.create( + remote, + commit, + checkAllStrings(patches, "patches"), + patchStrip.toInt("git_override.patch_strip"))); } @Override - public StarlarkOverrideApi localPathOverride(String path) { - return LocalPathOverride.create(path); + public void localPathOverride(String moduleName, String path) throws EvalException { + addOverride(moduleName, LocalPathOverride.create(path)); } public Module buildModule(Registry registry) { return module.setDeps(ImmutableMap.copyOf(deps)).setRegistry(registry).build(); } - public ImmutableMap buildOverrides() { + public ImmutableMap buildOverrides() { return ImmutableMap.copyOf(overrides); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java index d7e1a0d95cd857..37a80add67948e 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileValue.java @@ -22,7 +22,6 @@ import com.google.devtools.build.lib.concurrent.BlazeInterners; import com.google.devtools.build.lib.skyframe.SkyFunctions; import com.google.devtools.build.lib.skyframe.serialization.autocodec.AutoCodec; -import com.google.devtools.build.lib.starlarkbuildapi.repository.StarlarkOverrideApi; import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; @@ -45,14 +44,14 @@ public abstract class ModuleFileValue implements SkyValue { * The overrides specified by the evaluated module file. The key is the module name and the value * is the override itself. */ - public abstract ImmutableMap getOverrides(); + public abstract ImmutableMap getOverrides(); public static ModuleFileValue create( - Module module, ImmutableMap overrides) { + Module module, ImmutableMap overrides) { return new AutoValue_ModuleFileValue(module, overrides); } - public static Key key(ModuleKey moduleKey, @Nullable StarlarkOverrideApi override) { + public static Key key(ModuleKey moduleKey, @Nullable ModuleOverride override) { return Key.create(moduleKey, override); } @@ -75,11 +74,11 @@ abstract static class Key implements SkyKey { abstract ModuleKey getModuleKey(); @Nullable - abstract StarlarkOverrideApi getOverride(); + abstract ModuleOverride getOverride(); @AutoCodec.VisibleForSerialization @AutoCodec.Instantiator - static Key create(ModuleKey moduleKey, @Nullable StarlarkOverrideApi override) { + static Key create(ModuleKey moduleKey, @Nullable ModuleOverride override) { return interner.intern(new AutoValue_ModuleFileValue_Key(moduleKey, override)); } diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/StarlarkOverrideApi.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleOverride.java similarity index 58% rename from src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/StarlarkOverrideApi.java rename to src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleOverride.java index 54f27f2ff39ca7..15ddd720565e68 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/StarlarkOverrideApi.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleOverride.java @@ -13,17 +13,14 @@ // limitations under the License. // -package com.google.devtools.build.lib.starlarkbuildapi.repository; +package com.google.devtools.build.lib.bazel.bzlmod; -import com.google.devtools.build.docgen.annot.DocCategory; -import net.starlark.java.annot.StarlarkBuiltin; -import net.starlark.java.eval.StarlarkValue; - -/** Base interface for override objects. */ -@StarlarkBuiltin( - name = "override", - category = DocCategory.BUILTIN, - doc = "Overrides some information about a Bazel module dependency.") -public interface StarlarkOverrideApi extends StarlarkValue { +/** + * Represents an "override" applied to a particular module in the dependency graph. Must be of one + * of two categories: {@link RegistryOverride} and {@link NonRegistryOverride}. See there for + * further details. + */ +// This interface is not named "Override" because of the Java @Override annotation. +public interface ModuleOverride { // This space intentionally left blank } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/NonRegistryOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/NonRegistryOverride.java index 821efde5772a8b..86b280037e61db 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/NonRegistryOverride.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/NonRegistryOverride.java @@ -15,15 +15,13 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import com.google.devtools.build.lib.starlarkbuildapi.repository.StarlarkOverrideApi; - /** * An override specifying that the module should not be retrieved from a registry or participate in * version resolution, and should be retrieved from another given source instead. To evaluate the * module file of such modules, we need to first fetch the entire module contents and find the * module file in the root of the module. */ -public interface NonRegistryOverride extends StarlarkOverrideApi { +public interface NonRegistryOverride extends ModuleOverride { // TODO(wyv): getRepoSpec } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryOverride.java index f5fe39e82f1409..7c8848d1167e76 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryOverride.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/RegistryOverride.java @@ -15,13 +15,11 @@ package com.google.devtools.build.lib.bazel.bzlmod; -import com.google.devtools.build.lib.starlarkbuildapi.repository.StarlarkOverrideApi; - /** * An override specifying that the module should still come from a registry, albeit with some other * properties overridden (such as which registry it comes from, whether patches are applied, etc.) */ -public interface RegistryOverride extends StarlarkOverrideApi { +public interface RegistryOverride extends ModuleOverride { /** * The registry that should be used instead of the default list. Can be empty if there is no diff --git a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java index 7c32ca328e55e3..d227d1f639c2df 100644 --- a/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java +++ b/src/main/java/com/google/devtools/build/lib/starlarkbuildapi/repository/ModuleFileGlobalsApi.java @@ -91,33 +91,18 @@ void bazelDep(String name, String version, String repoName) throws EvalException, InterruptedException; @StarlarkMethod( - name = "override_dep", + name = "single_version_override", doc = - " Specifies that a direct or indirect dependency should be overridden to act in some way" - + " other than the default. This directive can only be used by the root module; in" - + " other words, if a module specifies any overrides, it cannot be used as a" - + " dependency by others.", + "Specifies that a dependency should still come from a registry, but its version should" + + " be pinned, or its registry overridden, or a list of patches applied. This" + + " directive can only be used by the root module; in other words, if a module" + + " specifies any overrides, it cannot be used as a dependency by others.", parameters = { @Param( - name = "name", - doc = "The name of the Bazel module dependency to apply an override to.", + name = "module_name", + doc = "The name of the Bazel module dependency to apply this override to.", named = true, positional = false), - @Param( - name = "override", - doc = "The actual override to apply.", - named = true, - positional = false), - }) - void overrideDep(String name, StarlarkOverrideApi override) - throws EvalException, InterruptedException; - - @StarlarkMethod( - name = "single_version_override", - doc = - "Specifies that the dependency should still come from a registry, but its version should" - + " be pinned, or its registry overridden, or a list of patches applied.", - parameters = { @Param( name = "version", doc = @@ -153,16 +138,27 @@ void overrideDep(String name, StarlarkOverrideApi override) positional = false, defaultValue = "0"), }) - StarlarkOverrideApi singleVersionOverride( - String version, String registry, Iterable patches, StarlarkInt patchStrip) + void singleVersionOverride( + String moduleName, + String version, + String registry, + Iterable patches, + StarlarkInt patchStrip) throws EvalException; @StarlarkMethod( name = "archive_override", doc = "Specifies that this dependency should come from an archive file (zip, gzip, etc) at a" - + " certain location, instead of from a registry.", + + " certain location, instead of from a registry. This directive can only be used by" + + " the root module; in other words, if a module specifies any overrides, it cannot" + + " be used as a dependency by others.", parameters = { + @Param( + name = "module_name", + doc = "The name of the Bazel module dependency to apply this override to.", + named = true, + positional = false), @Param( name = "urls", allowedTypes = { @@ -201,7 +197,8 @@ StarlarkOverrideApi singleVersionOverride( positional = false, defaultValue = "0"), }) - StarlarkOverrideApi archiveOverride( + void archiveOverride( + String moduleName, Object urls, String integrity, String stripPrefix, @@ -211,8 +208,16 @@ StarlarkOverrideApi archiveOverride( @StarlarkMethod( name = "git_override", - doc = "Specifies that this dependency should come from a certain commit of a Git repository.", + doc = + "Specifies that a dependency should come from a certain commit of a Git repository. This" + + " directive can only be used by the root module; in other words, if a module" + + " specifies any overrides, it cannot be used as a dependency by others.", parameters = { + @Param( + name = "module_name", + doc = "The name of the Bazel module dependency to apply this override to.", + named = true, + positional = false), @Param( name = "remote", doc = "The URL of the remote Git repository.", @@ -241,21 +246,29 @@ StarlarkOverrideApi archiveOverride( positional = false, defaultValue = "0"), }) - StarlarkOverrideApi gitOverride( - String remote, String commit, Iterable patches, StarlarkInt patchStrip) + void gitOverride( + String moduleName, String remote, String commit, Iterable patches, StarlarkInt patchStrip) throws EvalException, ModuleFileFunctionExceptionT; @StarlarkMethod( name = "local_path_override", - doc = "Specifies that this dependency should come from a certain directory on local disk.", + doc = + "Specifies that a dependency should come from a certain directory on local disk. This" + + " directive can only be used by the root module; in other words, if a module" + + " specifies any overrides, it cannot be used as a dependency by others.", parameters = { + @Param( + name = "module_name", + doc = "The name of the Bazel module dependency to apply this override to.", + named = true, + positional = false), @Param( name = "path", doc = "The path to the directory where this module is.", named = true, positional = false), }) - StarlarkOverrideApi localPathOverride(String path); + void localPathOverride(String moduleName, String path) throws EvalException; // TODO(wyv): multiple_version_override } 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 d89ad152075aa5..8fcfa1e84e93c3 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 @@ -116,8 +116,8 @@ public void testRootModule() throws Exception { "module(name='A',version='0.1')", "bazel_dep(name='B',version='1.0')", "bazel_dep(name='C',version='2.0',repo_name='see')", - "override_dep(name='D', override=single_version_override(version='18'))", - "override_dep(name='E', override=local_path_override(path='somewhere/else'))"); + "single_version_override(module_name='D',version='18')", + "local_path_override(module_name='E',path='somewhere/else')"); FakeRegistry registry = registryFactory.newFakeRegistry(); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -146,7 +146,7 @@ public void testRootModule_badSelfOverride() throws Exception { scratch.file( rootDirectory.getRelative("MODULE.bazel").getPathString(), "module(name='A')", - "override_dep(name='A', override=single_version_override(version='7'))"); + "single_version_override(module_name='A',version='7')"); FakeRegistry registry = registryFactory.newFakeRegistry(); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl()));