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())