From ac0f83041f82aea7a8b72bf41c8021de167a641a Mon Sep 17 00:00:00 2001 From: "Ian (Hee) Cha" Date: Wed, 21 Jun 2023 16:33:29 -0700 Subject: [PATCH] Allow module extension usages to be isolated (#18727) If `isolate = True` is specified on `use_extension`, that particular usage will be isolated from all other usages, both in the same and other modules. Module extensions can check whether they are isolated (e.g. in case they can only be used in this way) via `module_ctx.is_isolated`. Closes #18529. PiperOrigin-RevId: 541823020 Change-Id: I68a7b49914bbc1fd50df2fda7a0af1e47421bb92 Co-authored-by: Fabian Meumertzheim --- .../bazel/bzlmod/BazelDepGraphFunction.java | 27 +- .../lib/bazel/bzlmod/GsonTypeAdapterUtil.java | 59 +++ .../bazel/bzlmod/ModuleExtensionContext.java | 12 +- .../lib/bazel/bzlmod/ModuleExtensionId.java | 30 +- .../bazel/bzlmod/ModuleExtensionMetadata.java | 73 +++- .../bazel/bzlmod/ModuleExtensionUsage.java | 9 + .../lib/bazel/bzlmod/ModuleFileGlobals.java | 71 +++- .../bzlmod/BazelDepGraphFunctionTest.java | 16 +- .../bzlmod/ModuleExtensionResolutionTest.java | 359 +++++++++++++++++- .../bazel/bzlmod/ModuleFileFunctionTest.java | 5 + .../bazel/bzlmod/StarlarkBazelModuleTest.java | 2 + 11 files changed, 627 insertions(+), 36 deletions(-) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java index 14bfbdafc730f0..f99b33b627a371 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunction.java @@ -214,7 +214,9 @@ private ImmutableTable getEx try { moduleExtensionId = ModuleExtensionId.create( - labelConverter.convert(usage.getExtensionBzlFile()), usage.getExtensionName()); + labelConverter.convert(usage.getExtensionBzlFile()), + usage.getExtensionName(), + usage.getIsolationKey()); } catch (LabelSyntaxException e) { throw new BazelDepGraphFunctionException( ExternalDepsException.withCauseAndMessage( @@ -248,12 +250,31 @@ private ImmutableBiMap calculateUniqueNameForUsedExte // not start with a tilde. RepositoryName repository = id.getBzlFileLabel().getRepository(); String nonEmptyRepoPart = repository.isMain() ? "_main" : repository.getName(); - String bestName = nonEmptyRepoPart + "~" + id.getExtensionName(); + // When using a namespace, prefix the extension name with "_" to distinguish the prefix from + // those generated by non-namespaced extension usages. Extension names are identified by their + // Starlark identifier, which in the case of an exported symbol cannot start with "_". + // We also include whether the isolated usage is a dev usage as well as its index in the + // MODULE.bazel file to ensure that canonical repository names don't change depending on + // whether dev dependencies are ignored. This removes potential for confusion and also + // prevents unnecessary refetches when --ignore_dev_dependency is toggled. + String bestName = + id.getIsolationKey() + .map( + namespace -> + String.format( + "%s~_%s~%s~%s~%s%d", + nonEmptyRepoPart, + id.getExtensionName(), + namespace.getModule().getName(), + namespace.getModule().getVersion(), + namespace.isDevUsage() ? "dev" : "", + namespace.getIsolatedUsageIndex())) + .orElse(nonEmptyRepoPart + "~" + id.getExtensionName()); if (extensionUniqueNames.putIfAbsent(bestName, id) == null) { continue; } int suffix = 2; - while (extensionUniqueNames.putIfAbsent(bestName + suffix, id) != null) { + while (extensionUniqueNames.putIfAbsent(bestName + "~" + suffix, id) != null) { suffix++; } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java index 4a7a8b43908914..15518989781133 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GsonTypeAdapterUtil.java @@ -20,17 +20,25 @@ import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_MAP; import static com.google.devtools.build.lib.bazel.bzlmod.DelegateTypeAdapterFactory.IMMUTABLE_SET; +import com.google.common.base.Preconditions; import com.google.common.base.Splitter; import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; import com.google.gson.Gson; import com.google.gson.GsonBuilder; import com.google.gson.JsonParseException; import com.google.gson.TypeAdapter; +import com.google.gson.TypeAdapterFactory; +import com.google.gson.reflect.TypeToken; import com.google.gson.stream.JsonReader; +import com.google.gson.stream.JsonToken; import com.google.gson.stream.JsonWriter; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; import java.io.IOException; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; import java.util.List; +import java.util.Optional; +import javax.annotation.Nullable; /** * Utility class to hold type adapters and helper methods to get gson registered with type adapters @@ -88,6 +96,56 @@ public ModuleKey read(JsonReader jsonReader) throws IOException { } }; + public static final TypeAdapterFactory OPTIONAL = + new TypeAdapterFactory() { + @Nullable + @Override + @SuppressWarnings("unchecked") + public TypeAdapter create(Gson gson, TypeToken typeToken) { + if (typeToken.getRawType() != Optional.class) { + return null; + } + Type type = typeToken.getType(); + if (!(type instanceof ParameterizedType)) { + return null; + } + Type elementType = ((ParameterizedType) typeToken.getType()).getActualTypeArguments()[0]; + var elementTypeAdapter = gson.getAdapter(TypeToken.get(elementType)); + if (elementTypeAdapter == null) { + return null; + } + return (TypeAdapter) new OptionalTypeAdapter<>(elementTypeAdapter); + } + }; + + private static final class OptionalTypeAdapter extends TypeAdapter> { + private final TypeAdapter elementTypeAdapter; + + public OptionalTypeAdapter(TypeAdapter elementTypeAdapter) { + this.elementTypeAdapter = elementTypeAdapter; + } + + @Override + public void write(JsonWriter jsonWriter, Optional t) throws IOException { + Preconditions.checkNotNull(t); + if (t.isEmpty()) { + jsonWriter.nullValue(); + } else { + elementTypeAdapter.write(jsonWriter, t.get()); + } + } + + @Override + public Optional read(JsonReader jsonReader) throws IOException { + if (jsonReader.peek() == JsonToken.NULL) { + jsonReader.nextNull(); + return Optional.empty(); + } else { + return Optional.of(elementTypeAdapter.read(jsonReader)); + } + } + } + public static final Gson LOCKFILE_GSON = new GsonBuilder() .setPrettyPrinting() @@ -97,6 +155,7 @@ public ModuleKey read(JsonReader jsonReader) throws IOException { .registerTypeAdapterFactory(IMMUTABLE_LIST) .registerTypeAdapterFactory(IMMUTABLE_BIMAP) .registerTypeAdapterFactory(IMMUTABLE_SET) + .registerTypeAdapterFactory(OPTIONAL) .registerTypeAdapter(Version.class, VERSION_TYPE_ADAPTER) .registerTypeAdapter(ModuleKey.class, MODULE_KEY_TYPE_ADAPTER) .registerTypeAdapter(AttributeValues.class, new AttributeValuesAdapter()) diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java index c27fbef23a6425..369a833c07db66 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java @@ -120,6 +120,16 @@ public boolean isDevDependency(TypeCheckedTag tag) { return tag.isDevDependency(); } + @StarlarkMethod( + name = "is_isolated", + doc = + "Whether this particular usage of the extension had isolate = True " + + "specified and is thus isolated from all other usages.", + structField = true) + public boolean isIsolated() { + return extensionId.getIsolationKey().isPresent(); + } + @StarlarkMethod( name = "extension_metadata", doc = @@ -181,6 +191,6 @@ public ModuleExtensionMetadata extensionMetadata( Object rootModuleDirectDepsUnchecked, Object rootModuleDirectDevDepsUnchecked) throws EvalException { return ModuleExtensionMetadata.create( - rootModuleDirectDepsUnchecked, rootModuleDirectDevDepsUnchecked); + rootModuleDirectDepsUnchecked, rootModuleDirectDevDepsUnchecked, extensionId); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java index 9fef1ef77aaf3c..8d667614832f8c 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionId.java @@ -17,15 +17,41 @@ import com.google.auto.value.AutoValue; import com.google.devtools.build.lib.cmdline.Label; +import java.util.Optional; /** A unique identifier for a {@link ModuleExtension}. */ @AutoValue public abstract class ModuleExtensionId { + + /** A unique identifier for a single isolated usage of a fixed module extension. */ + @AutoValue + abstract static class IsolationKey { + /** The module which contains this isolated usage of a module extension. */ + public abstract ModuleKey getModule(); + + /** Whether this isolated usage specified {@code dev_dependency = True}. */ + public abstract boolean isDevUsage(); + + /** + * The 0-based index of this isolated usage within the module's isolated usages of the same + * module extension and with the same {@link #isDevUsage()} value. + */ + public abstract int getIsolatedUsageIndex(); + + public static IsolationKey create( + ModuleKey module, boolean isDevUsage, int isolatedUsageIndex) { + return new AutoValue_ModuleExtensionId_IsolationKey(module, isDevUsage, isolatedUsageIndex); + } + } + public abstract Label getBzlFileLabel(); public abstract String getExtensionName(); - public static ModuleExtensionId create(Label bzlFileLabel, String extensionName) { - return new AutoValue_ModuleExtensionId(bzlFileLabel, extensionName); + public abstract Optional getIsolationKey(); + + public static ModuleExtensionId create( + Label bzlFileLabel, String extensionName, Optional isolationKey) { + return new AutoValue_ModuleExtensionId(bzlFileLabel, extensionName, isolationKey); } } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java index d680b3100402c3..5a38c3445f5fbd 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionMetadata.java @@ -69,7 +69,9 @@ private ModuleExtensionMetadata( } static ModuleExtensionMetadata create( - Object rootModuleDirectDepsUnchecked, Object rootModuleDirectDevDepsUnchecked) + Object rootModuleDirectDepsUnchecked, + Object rootModuleDirectDevDepsUnchecked, + ModuleExtensionId extensionId) throws EvalException { if (rootModuleDirectDepsUnchecked == Starlark.NONE && rootModuleDirectDevDepsUnchecked == Starlark.NONE) { @@ -80,11 +82,23 @@ static ModuleExtensionMetadata create( // root_module_direct_dev_deps = [], but not root_module_direct_dev_deps = ["some_repo"]. if (rootModuleDirectDepsUnchecked.equals("all") && rootModuleDirectDevDepsUnchecked.equals(StarlarkList.immutableOf())) { + if (extensionId.getIsolationKey().isPresent() + && extensionId.getIsolationKey().get().isDevUsage()) { + throw Starlark.errorf( + "root_module_direct_deps must be empty for an isolated extension usage with " + + "dev_dependency = True"); + } return new ModuleExtensionMetadata(null, null, UseAllRepos.REGULAR); } if (rootModuleDirectDevDepsUnchecked.equals("all") && rootModuleDirectDepsUnchecked.equals(StarlarkList.immutableOf())) { + if (extensionId.getIsolationKey().isPresent() + && !extensionId.getIsolationKey().get().isDevUsage()) { + throw Starlark.errorf( + "root_module_direct_dev_deps must be empty for an isolated extension usage with " + + "dev_dependency = False"); + } return new ModuleExtensionMetadata(null, null, UseAllRepos.DEV); } @@ -114,6 +128,20 @@ static ModuleExtensionMetadata create( Sequence.cast( rootModuleDirectDevDepsUnchecked, String.class, "root_module_direct_dev_deps"); + if (extensionId.getIsolationKey().isPresent()) { + ModuleExtensionId.IsolationKey isolationKey = extensionId.getIsolationKey().get(); + if (isolationKey.isDevUsage() && !rootModuleDirectDeps.isEmpty()) { + throw Starlark.errorf( + "root_module_direct_deps must be empty for an isolated extension usage with " + + "dev_dependency = True"); + } + if (!isolationKey.isDevUsage() && !rootModuleDirectDevDeps.isEmpty()) { + throw Starlark.errorf( + "root_module_direct_dev_deps must be empty for an isolated extension usage with " + + "dev_dependency = False"); + } + } + Set explicitRootModuleDirectDeps = new LinkedHashSet<>(); for (String dep : rootModuleDirectDeps) { try { @@ -257,13 +285,33 @@ private static Optional generateFixupMessage( var fixupCommands = Stream.of( makeUseRepoCommand( - "use_repo_add", false, importsToAdd, extensionBzlFile, extensionName), + "use_repo_add", + false, + importsToAdd, + extensionBzlFile, + extensionName, + firstUsage.getIsolationKey()), makeUseRepoCommand( - "use_repo_remove", false, importsToRemove, extensionBzlFile, extensionName), + "use_repo_remove", + false, + importsToRemove, + extensionBzlFile, + extensionName, + firstUsage.getIsolationKey()), makeUseRepoCommand( - "use_repo_add", true, devImportsToAdd, extensionBzlFile, extensionName), + "use_repo_add", + true, + devImportsToAdd, + extensionBzlFile, + extensionName, + firstUsage.getIsolationKey()), makeUseRepoCommand( - "use_repo_remove", true, devImportsToRemove, extensionBzlFile, extensionName)) + "use_repo_remove", + true, + devImportsToRemove, + extensionBzlFile, + extensionName, + firstUsage.getIsolationKey())) .flatMap(Optional::stream); return Optional.of( @@ -284,17 +332,28 @@ private static Optional makeUseRepoCommand( boolean devDependency, Collection repos, String extensionBzlFile, - String extensionName) { + String extensionName, + Optional isolationKey) { + if (repos.isEmpty()) { return Optional.empty(); } + + String extensionUsageIdentifier = extensionName; + if (isolationKey.isPresent()) { + // We verified in create() that the extension did not report root module deps of a kind that + // does not match the isolated (and hence only) usage. It also isn't possible for users to + // specify repo usages of the wrong kind, so we can't get here. + Preconditions.checkState(isolationKey.get().isDevUsage() == devDependency); + extensionUsageIdentifier += ":" + isolationKey.get().getIsolatedUsageIndex(); + } return Optional.of( String.format( "buildozer '%s%s %s %s %s' //MODULE.bazel:all", cmd, devDependency ? " dev" : "", extensionBzlFile, - extensionName, + extensionUsageIdentifier, String.join(" ", repos))); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java index 31cff67ecace10..a73ee46cd47d7f 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionUsage.java @@ -20,6 +20,7 @@ import com.google.common.collect.ImmutableSet; import com.google.errorprone.annotations.CanIgnoreReturnValue; import com.ryanharter.auto.value.gson.GenerateTypeAdapter; +import java.util.Optional; import net.starlark.java.syntax.Location; /** @@ -35,6 +36,12 @@ public abstract class ModuleExtensionUsage { /** The name of the extension. */ public abstract String getExtensionName(); + /** + * The isolation key of this module extension usage. This is present if and only if the usage is + * created with {@code isolate = True}. + */ + public abstract Optional getIsolationKey(); + /** The module that contains this particular extension usage. */ public abstract ModuleKey getUsingModule(); @@ -73,6 +80,8 @@ public abstract static class Builder { public abstract Builder setExtensionName(String value); + public abstract Builder setIsolationKey(Optional value); + public abstract Builder setUsingModule(ModuleKey value); public abstract Builder setLocation(Location value); 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 876293b60df06d..f21fbda0ef044e 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 @@ -35,6 +35,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Optional; import java.util.regex.Pattern; import javax.annotation.Nullable; import net.starlark.java.annot.Param; @@ -70,6 +71,8 @@ public class ModuleFileGlobals { private final List extensionUsageBuilders = new ArrayList<>(); private final Map overrides = new HashMap<>(); private final Map repoNameUsages = new HashMap<>(); + private int nextIsolatedNonDevUsageIndex = 0; + private int nextIsolatedDevUsageIndex = 0; public ModuleFileGlobals( ImmutableMap builtinModules, @@ -426,31 +429,59 @@ public void registerToolchains(boolean devDependency, Sequence toolchainLabel named = true, positional = false, defaultValue = "False"), + @Param( + name = "isolate", + doc = + "If true, this usage of the module extension will be isolated from all other " + + "usages, both in this and other modules. Tags created for this usage do not " + + "affect other usages and the repositories generated by the extension for " + + "this usage will be distinct from all other repositories generated by the " + + "extension.", + named = true, + positional = false, + defaultValue = "False"), }, useStarlarkThread = true) public ModuleExtensionProxy useExtension( String rawExtensionBzlFile, String extensionName, boolean devDependency, + boolean isolate, StarlarkThread thread) { hadNonModuleCall = true; String extensionBzlFile = normalizeLabelString(rawExtensionBzlFile); + Optional isolationKey; + if (isolate) { + isolationKey = + Optional.of( + ModuleExtensionId.IsolationKey.create( + module.getKey(), + devDependency, + devDependency ? nextIsolatedDevUsageIndex++ : nextIsolatedNonDevUsageIndex++)); + } else { + isolationKey = Optional.empty(); + } + ModuleExtensionUsageBuilder newUsageBuilder = new ModuleExtensionUsageBuilder( - extensionBzlFile, extensionName, thread.getCallerLocation()); + extensionBzlFile, extensionName, isolationKey, thread.getCallerLocation()); if (ignoreDevDeps && devDependency) { // This is a no-op proxy. return newUsageBuilder.getProxy(devDependency); } - // Find an existing usage builder corresponding to this extension. - for (ModuleExtensionUsageBuilder usageBuilder : extensionUsageBuilders) { - if (usageBuilder.extensionBzlFile.equals(extensionBzlFile) - && usageBuilder.extensionName.equals(extensionName)) { - return usageBuilder.getProxy(devDependency); + // Find an existing usage builder corresponding to this extension. Isolated usages need to get + // their own proxy. + if (!isolate) { + for (ModuleExtensionUsageBuilder usageBuilder : extensionUsageBuilders) { + if (usageBuilder.extensionBzlFile.equals(extensionBzlFile) + && usageBuilder.extensionName.equals(extensionName) + && usageBuilder.isolationKey.isEmpty()) { + return usageBuilder.getProxy(devDependency); + } } } @@ -478,14 +509,20 @@ private String normalizeLabelString(String rawExtensionBzlFile) { class ModuleExtensionUsageBuilder { private final String extensionBzlFile; private final String extensionName; + private final Optional isolationKey; private final Location location; private final HashBiMap imports; private final ImmutableSet.Builder devImports; private final ImmutableList.Builder tags; - ModuleExtensionUsageBuilder(String extensionBzlFile, String extensionName, Location location) { + ModuleExtensionUsageBuilder( + String extensionBzlFile, + String extensionName, + Optional isolationKey, + Location location) { this.extensionBzlFile = extensionBzlFile; this.extensionName = extensionName; + this.isolationKey = isolationKey; this.location = location; this.imports = HashBiMap.create(); this.devImports = ImmutableSet.builder(); @@ -493,15 +530,17 @@ class ModuleExtensionUsageBuilder { } ModuleExtensionUsage buildUsage() { - return ModuleExtensionUsage.builder() - .setExtensionBzlFile(extensionBzlFile) - .setExtensionName(extensionName) - .setUsingModule(module.getKey()) - .setLocation(location) - .setImports(ImmutableBiMap.copyOf(imports)) - .setDevImports(devImports.build()) - .setTags(tags.build()) - .build(); + var builder = + ModuleExtensionUsage.builder() + .setExtensionBzlFile(extensionBzlFile) + .setExtensionName(extensionName) + .setIsolationKey(isolationKey) + .setUsingModule(module.getKey()) + .setLocation(location) + .setImports(ImmutableBiMap.copyOf(imports)) + .setDevImports(devImports.build()) + .setTags(tags.build()); + return builder.build(); } /** diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java index 3e10c004b73f03..6894c77a50fe03 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BazelDepGraphFunctionTest.java @@ -64,6 +64,7 @@ import com.google.devtools.build.skyframe.SkyFunctionName; import com.google.devtools.build.skyframe.SkyKey; import com.google.devtools.build.skyframe.SkyValue; +import java.util.Optional; import java.util.concurrent.atomic.AtomicReference; import javax.annotation.Nullable; import net.starlark.java.eval.StarlarkSemantics; @@ -214,6 +215,7 @@ private static ModuleExtensionUsage createModuleExtensionUsage( return ModuleExtensionUsage.builder() .setExtensionBzlFile(bzlFile) .setExtensionName(name) + .setIsolationKey(Optional.empty()) .setImports(importsBuilder.buildOrThrow()) .setDevImports(ImmutableSet.of()) .setUsingModule(ModuleKey.ROOT) @@ -253,14 +255,16 @@ public void createValue_moduleExtensions() throws Exception { ModuleExtensionId maven = ModuleExtensionId.create( - Label.parseCanonical("@@rules_jvm_external~1.0//:defs.bzl"), "maven"); + Label.parseCanonical("@@rules_jvm_external~1.0//:defs.bzl"), "maven", Optional.empty()); ModuleExtensionId pip = - ModuleExtensionId.create(Label.parseCanonical("@@rules_python~2.0//:defs.bzl"), "pip"); + ModuleExtensionId.create( + Label.parseCanonical("@@rules_python~2.0//:defs.bzl"), "pip", Optional.empty()); ModuleExtensionId myext = - ModuleExtensionId.create(Label.parseCanonical("@@dep~2.0//:defs.bzl"), "myext"); + ModuleExtensionId.create( + Label.parseCanonical("@@dep~2.0//:defs.bzl"), "myext", Optional.empty()); ModuleExtensionId myext2 = ModuleExtensionId.create( - Label.parseCanonical("@@dep~2.0//incredible:conflict.bzl"), "myext"); + Label.parseCanonical("@@dep~2.0//incredible:conflict.bzl"), "myext", Optional.empty()); resolutionFunctionMock.setDepGraph(depGraph); EvaluationResult result = @@ -287,7 +291,7 @@ public void createValue_moduleExtensions() throws Exception { maven, "rules_jvm_external~1.0~maven", pip, "rules_python~2.0~pip", myext, "dep~2.0~myext", - myext2, "dep~2.0~myext2"); + myext2, "dep~2.0~myext~2"); assertThat(value.getFullRepoMapping(ModuleKey.ROOT)) .isEqualTo( @@ -318,7 +322,7 @@ public void createValue_moduleExtensions() throws Exception { "oneext", "dep~2.0~myext~myext", "twoext", - "dep~2.0~myext2~myext")); + "dep~2.0~myext~2~myext")); } @Test diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java index d94389a6f7ecac..a2f78bd8b11405 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java @@ -603,6 +603,104 @@ public void multipleModules_ignoreDevDependency() throws Exception { .isEqualTo("modules: bar@2.0 False"); } + @Test + public void multipleModules_isolatedUsages() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "module(name='root',version='1.0')", + "bazel_dep(name='ext',version='1.0')", + "bazel_dep(name='foo',version='1.0')", + "ext = use_extension('@ext//:defs.bzl','ext')", + "ext.tag(data='root',expect_isolated=False)", + "use_repo(ext,'ext_repo')", + "isolated_ext = use_extension('@ext//:defs.bzl','ext',isolate=True)", + "isolated_ext.tag(data='root_isolated',expect_isolated=True)", + "use_repo(isolated_ext,isolated_ext_repo='ext_repo')", + "isolated_dev_ext =" + + " use_extension('@ext//:defs.bzl','ext',isolate=True,dev_dependency=True)", + "isolated_dev_ext.tag(data='root_isolated_dev',expect_isolated=True)", + "use_repo(isolated_dev_ext,isolated_dev_ext_repo='ext_repo')", + "ext2 = use_extension('@ext//:defs.bzl','ext')", + "ext2.tag(data='root_2',expect_isolated=False)"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + "load('@ext_repo//:data.bzl', ext_data='data')", + "load('@isolated_ext_repo//:data.bzl', isolated_ext_data='data')", + "load('@isolated_dev_ext_repo//:data.bzl', isolated_dev_ext_data='data')", + "data=ext_data", + "isolated_data=isolated_ext_data", + "isolated_dev_data=isolated_dev_ext_data"); + + registry.addModule( + createModuleKey("foo", "1.0"), + "module(name='foo',version='1.0')", + "bazel_dep(name='ext',version='1.0')", + "isolated_ext = use_extension('@ext//:defs.bzl','ext',isolate=True)", + "isolated_ext.tag(data='foo@1.0_isolated',expect_isolated=True)", + "use_repo(isolated_ext,isolated_ext_repo='ext_repo')", + "isolated_dev_ext =" + + " use_extension('@ext//:defs.bzl','ext',isolate=True,dev_dependency=True)", + "isolated_dev_ext.tag(data='foo@1.0_isolated_dev',expect_isolated=True)", + "use_repo(isolated_dev_ext,isolated_dev_ext_repo='ext_repo')", + "ext = use_extension('@ext//:defs.bzl','ext')", + "ext.tag(data='foo@1.0',expect_isolated=False)", + "use_repo(ext,'ext_repo')"); + scratch.file(modulesRoot.getRelative("foo~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("foo~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("foo~1.0/data.bzl").getPathString(), + "load('@ext_repo//:data.bzl', ext_data='data')", + "load('@isolated_ext_repo//:data.bzl', isolated_ext_data='data')", + "data=ext_data", + "isolated_data=isolated_ext_data"); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')"); + scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), + "load('@data_repo//:defs.bzl','data_repo')", + "def _ext_impl(ctx):", + " data_str = ''", + " for mod in ctx.modules:", + " data_str += mod.name + '@' + mod.version + (' (root): ' if mod.is_root else ': ')", + " for tag in mod.tags.tag:", + " data_str += tag.data", + " if tag.expect_isolated != ctx.is_isolated:", + " fail()", + " data_str += '\\n'", + " data_repo(name='ext_repo',data=data_str)", + "tag=tag_class(attrs={'data':attr.string(),'expect_isolated':attr.bool()})", + "ext=module_extension(implementation=_ext_impl,tag_classes={'tag':tag})"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + if (result.hasError()) { + throw result.getError().getException(); + } + assertThat(result.get(skyKey).getModule().getGlobal("data")) + .isEqualTo("root@1.0 (root): rootroot_2\nfoo@1.0: foo@1.0\n"); + assertThat(result.get(skyKey).getModule().getGlobal("isolated_data")) + .isEqualTo("root@1.0 (root): root_isolated\n"); + assertThat(result.get(skyKey).getModule().getGlobal("isolated_dev_data")) + .isEqualTo("root@1.0 (root): root_isolated_dev\n"); + + skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("@foo~1.0//:data.bzl")); + result = evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + if (result.hasError()) { + throw result.getError().getException(); + } + assertThat(result.get(skyKey).getModule().getGlobal("data")) + .isEqualTo("root@1.0 (root): rootroot_2\nfoo@1.0: foo@1.0\n"); + assertThat(result.get(skyKey).getModule().getGlobal("isolated_data")) + .isEqualTo("foo@1.0: foo@1.0_isolated\n"); + } + @Test public void labels_readInModuleExtension() throws Exception { scratch.file( @@ -1592,6 +1690,66 @@ public void extensionMetadata_duplicateRepoAcrossTypes() throws Exception { "in root_module_direct_dev_deps: entry 'dep' is also in root_module_direct_deps"); } + @Test + public void extensionMetadata_isolate_devUsageWithAllDirectNonDevDeps() throws Exception { + var result = + evaluateIsolatedModuleExtension( + "return" + + " ctx.extension_metadata(root_module_direct_deps=\"all\"," + + "root_module_direct_dev_deps=[])", + /* devDependency= */ true); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "root_module_direct_deps must be empty for an isolated extension usage with dev_dependency" + + " = True"); + } + + @Test + public void extensionMetadata_isolate_nonDevUsageWithAllDirectDevDeps() throws Exception { + var result = + evaluateIsolatedModuleExtension( + "return" + + " ctx.extension_metadata(root_module_direct_deps=[]," + + "root_module_direct_dev_deps=\"all\")", + /* devDependency= */ false); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "root_module_direct_dev_deps must be empty for an isolated extension usage with " + + "dev_dependency = False"); + } + + @Test + public void extensionMetadata_isolate_devUsageWithDirectNonDevDeps() throws Exception { + var result = + evaluateIsolatedModuleExtension( + "return" + + " ctx.extension_metadata(root_module_direct_deps=['dep1']," + + "root_module_direct_dev_deps=['dep2'])", + /* devDependency= */ true); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "root_module_direct_deps must be empty for an isolated extension usage with dev_dependency" + + " = True"); + } + + @Test + public void extensionMetadata_isolate_nonDevUsageWithDirectDevDeps() throws Exception { + var result = + evaluateIsolatedModuleExtension( + "return" + + " ctx.extension_metadata(root_module_direct_deps=['dep1']," + + "root_module_direct_dev_deps=['dep2'])", + /* devDependency= */ false); + + assertThat(result.hasError()).isTrue(); + assertContainsEvent( + "root_module_direct_dev_deps must be empty for an isolated extension usage with " + + "dev_dependency = False"); + } + @Test public void extensionMetadata() throws Exception { scratch.file( @@ -1883,6 +2041,205 @@ public void extensionMetadata_noRootUsage() throws Exception { assertEventCount(0, eventCollector); } + @Test + public void extensionMetadata_isolated() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "bazel_dep(name='ext', version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext1 = use_extension('@ext//:defs.bzl', 'ext', isolate = True)", + "use_repo(", + " ext1,", + " 'indirect_dep',", + ")", + "ext2 = use_extension('@ext//:defs.bzl', 'ext', isolate = True)", + "use_repo(", + " ext2,", + " 'direct_dep',", + ")"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + "load('@direct_dep//:data.bzl', data_1='data')", + "load('@indirect_dep//:data.bzl', data_2='data')"); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')"); + scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), + "load('@data_repo//:defs.bzl','data_repo')", + "def _ext_impl(ctx):", + " data_repo(name='direct_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='indirect_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps=['direct_dep', 'missing_direct_dep'],", + " root_module_direct_dev_deps=[],", + " )", + "ext=module_extension(implementation=_ext_impl)"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + // Evaluation fails due to the import of a repository not generated by the extension, but we + // only want to assert that the warning is emitted. + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isFalse(); + + assertEventCount(2, eventCollector); + assertContainsEvent( + "WARNING /ws/MODULE.bazel:3:21: The module extension ext defined in @ext//:defs.bzl" + + " reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the" + + " build to fail):\n" + + " direct_dep, missing_direct_dep\n" + + "\n" + + "Imported, but reported as indirect dependencies by the extension:\n" + + " indirect_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + + " issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_add @ext//:defs.bzl ext:0 direct_dep missing_direct_dep'" + + " //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove @ext//:defs.bzl ext:0 indirect_dep' //MODULE.bazel:all", + ImmutableSet.of(EventKind.WARNING)); + assertContainsEvent( + "WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl" + + " reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the" + + " build to fail):\n" + + " missing_direct_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + + " issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_add @ext//:defs.bzl ext:1 missing_direct_dep'" + + " //MODULE.bazel:all", + ImmutableSet.of(EventKind.WARNING)); + } + + @Test + public void extensionMetadata_isolatedDev() throws Exception { + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + "bazel_dep(name='ext', version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext1 = use_extension('@ext//:defs.bzl', 'ext', isolate = True, dev_dependency = True)", + "use_repo(", + " ext1,", + " 'indirect_dep',", + ")", + "ext2 = use_extension('@ext//:defs.bzl', 'ext', isolate = True, dev_dependency = True)", + "use_repo(", + " ext2,", + " 'direct_dep',", + ")"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + scratch.file( + workspaceRoot.getRelative("data.bzl").getPathString(), + "load('@direct_dep//:data.bzl', data_1='data')", + "load('@indirect_dep//:data.bzl', data_2='data')"); + + registry.addModule( + createModuleKey("ext", "1.0"), + "module(name='ext',version='1.0')", + "bazel_dep(name='data_repo',version='1.0')", + "ext = use_extension('//:defs.bzl', 'ext')", + "use_repo(ext, 'indirect_dep')"); + scratch.file(modulesRoot.getRelative("ext~1.0/WORKSPACE").getPathString()); + scratch.file(modulesRoot.getRelative("ext~1.0/BUILD").getPathString()); + scratch.file( + modulesRoot.getRelative("ext~1.0/defs.bzl").getPathString(), + "load('@data_repo//:defs.bzl','data_repo')", + "def _ext_impl(ctx):", + " data_repo(name='direct_dep')", + " data_repo(name='missing_direct_dep')", + " data_repo(name='indirect_dep')", + " return ctx.extension_metadata(", + " root_module_direct_deps=[],", + " root_module_direct_dev_deps=['direct_dep', 'missing_direct_dep'],", + " )", + "ext=module_extension(implementation=_ext_impl)"); + + SkyKey skyKey = BzlLoadValue.keyForBuild(Label.parseCanonical("//:data.bzl")); + // Evaluation fails due to the import of a repository not generated by the extension, but we + // only want to assert that the warning is emitted. + reporter.removeHandler(failFastHandler); + EvaluationResult result = + evaluator.evaluate(ImmutableList.of(skyKey), evaluationContext); + assertThat(result.hasError()).isFalse(); + + assertEventCount(2, eventCollector); + assertContainsEvent( + "WARNING /ws/MODULE.bazel:3:21: The module extension ext defined in @ext//:defs.bzl" + + " reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the" + + " build to fail):\n" + + " direct_dep, missing_direct_dep\n" + + "\n" + + "Imported, but reported as indirect dependencies by the extension:\n" + + " indirect_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + + " issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_add dev @ext//:defs.bzl ext:0 direct_dep missing_direct_dep'" + + " //MODULE.bazel:all\n" + + "buildozer 'use_repo_remove dev @ext//:defs.bzl ext:0 indirect_dep'" + + " //MODULE.bazel:all", + ImmutableSet.of(EventKind.WARNING)); + assertContainsEvent( + "WARNING /ws/MODULE.bazel:8:21: The module extension ext defined in @ext//:defs.bzl" + + " reported incorrect imports of repositories via use_repo():\n" + + "\n" + + "Not imported, but reported as direct dependencies by the extension (may cause the" + + " build to fail):\n" + + " missing_direct_dep\n" + + "\n" + + "\033[35m\033[1m ** You can use the following buildozer command(s) to fix these" + + " issues:\033[0m\n" + + "\n" + + "buildozer 'use_repo_add dev @ext//:defs.bzl ext:1 missing_direct_dep'" + + " //MODULE.bazel:all", + ImmutableSet.of(EventKind.WARNING)); + } + + private EvaluationResult evaluateIsolatedModuleExtension( + String returnStatement, boolean devDependency) throws Exception { + String devDependencyStr = devDependency ? "True" : "False"; + String isolateStr = "True"; + scratch.file( + workspaceRoot.getRelative("MODULE.bazel").getPathString(), + String.format( + "ext = use_extension('//:defs.bzl', 'ext', dev_dependency = %s, isolate = %s)", + devDependencyStr, isolateStr)); + scratch.file( + workspaceRoot.getRelative("defs.bzl").getPathString(), + "def _ext_impl(ctx):", + " " + returnStatement, + "ext = module_extension(implementation=_ext_impl)"); + scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); + + ModuleExtensionId extensionId = + ModuleExtensionId.create( + Label.parseCanonical("//:defs.bzl"), + "ext", + Optional.of(ModuleExtensionId.IsolationKey.create(ModuleKey.ROOT, devDependency, 0))); + reporter.removeHandler(failFastHandler); + return evaluator.evaluate( + ImmutableList.of(SingleExtensionEvalValue.key(extensionId)), evaluationContext); + } + private EvaluationResult evaluateSimpleModuleExtension( String returnStatement) throws Exception { scratch.file( @@ -1896,7 +2253,7 @@ private EvaluationResult evaluateSimpleModuleExtension scratch.file(workspaceRoot.getRelative("BUILD").getPathString()); ModuleExtensionId extensionId = - ModuleExtensionId.create(Label.parseCanonical("//:defs.bzl"), "ext"); + ModuleExtensionId.create(Label.parseCanonical("//:defs.bzl"), "ext", Optional.empty()); reporter.removeHandler(failFastHandler); return evaluator.evaluate( ImmutableList.of(SingleExtensionEvalValue.key(extensionId)), evaluationContext); 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 1e29e61d276022..129094a98bd422 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 @@ -476,6 +476,7 @@ public void testModuleExtensions_good() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext1") + .setIsolationKey(Optional.empty()) .setUsingModule(myMod) .setLocation( Location.fromFileLineColumn( @@ -500,6 +501,7 @@ public void testModuleExtensions_good() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext2") + .setIsolationKey(Optional.empty()) .setUsingModule(myMod) .setLocation( Location.fromFileLineColumn( @@ -537,6 +539,7 @@ public void testModuleExtensions_good() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@rules_jvm_external//:defs.bzl") .setExtensionName("maven") + .setIsolationKey(Optional.empty()) .setUsingModule(myMod) .setLocation( Location.fromFileLineColumn( @@ -607,6 +610,7 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@//:defs.bzl") .setExtensionName("myext") + .setIsolationKey(Optional.empty()) .setUsingModule(ModuleKey.ROOT) .setLocation(Location.fromFileLineColumn("/workspace/MODULE.bazel", 1, 23)) .setImports( @@ -704,6 +708,7 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("@mymod//:defs.bzl") .setExtensionName("myext") + .setIsolationKey(Optional.empty()) .setUsingModule(myMod) .setLocation( Location.fromFileLineColumn( diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java index 9dbb6904cba911..5782eaa2a96461 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/StarlarkBazelModuleTest.java @@ -30,6 +30,7 @@ import com.google.devtools.build.lib.packages.BuildType; import com.google.devtools.build.lib.packages.Type; import com.google.devtools.build.lib.util.FileTypeSet; +import java.util.Optional; import net.starlark.java.eval.StarlarkList; import net.starlark.java.syntax.Location; import org.junit.Test; @@ -45,6 +46,7 @@ private static ModuleExtensionUsage.Builder getBaseUsageBuilder() { return ModuleExtensionUsage.builder() .setExtensionBzlFile("//:rje.bzl") .setExtensionName("maven") + .setIsolationKey(Optional.empty()) .setUsingModule(ModuleKey.ROOT) .setLocation(Location.BUILTIN) .setImports(ImmutableBiMap.of())