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 6dbf5e3e4a6748..0cf2fd58a9a15d 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 @@ -24,6 +24,8 @@ import com.google.devtools.build.skyframe.SkyFunction.Environment; import java.util.Map; import javax.annotation.Nullable; +import net.starlark.java.annot.Param; +import net.starlark.java.annot.ParamType; import net.starlark.java.annot.StarlarkBuiltin; import net.starlark.java.annot.StarlarkMethod; import net.starlark.java.eval.EvalException; @@ -99,4 +101,20 @@ protected ImmutableMap getRemoteExecProperties() throws EvalExce public StarlarkList getModules() { return modules; } + + @StarlarkMethod( + name = "is_dev_dependency", + doc = + "Returns whether the given tag was specified on the result of a use_extension call with " + + "devDependency = True.", + parameters = { + @Param( + name = "tag", + doc = "A tag obtained from bazel_module.tags.", + allowedTypes = {@ParamType(type = TypeCheckedTag.class)}) + }) + public boolean isDevDependency(TypeCheckedTag tag) { + return tag.isDevDependency(); + } } 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 3ff8e9e65933a8..d48359d212a605 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 @@ -25,6 +25,7 @@ import com.google.common.collect.ImmutableList; import com.google.common.collect.ImmutableMap; import com.google.devtools.build.docgen.annot.DocumentMethods; +import com.google.devtools.build.lib.bazel.bzlmod.ModuleFileGlobals.ModuleExtensionUsageBuilder.ModuleExtensionProxy; import com.google.devtools.build.lib.bazel.bzlmod.Version.ParseException; import com.google.devtools.build.lib.cmdline.RepositoryName; import java.util.ArrayList; @@ -63,7 +64,7 @@ public class ModuleFileGlobals { private final boolean ignoreDevDeps; private final Module.Builder module; private final Map deps = new LinkedHashMap<>(); - private final List extensionProxies = new ArrayList<>(); + private final List extensionUsageBuilders = new ArrayList<>(); private final Map overrides = new HashMap<>(); private final Map repoNameUsages = new HashMap<>(); @@ -373,38 +374,37 @@ public void registerToolchains(Sequence toolchainLabels) throws EvalException }, useStarlarkThread = true) public ModuleExtensionProxy useExtension( - String extensionBzlFile, String extensionName, boolean devDependency, StarlarkThread thread) - throws EvalException { - ModuleExtensionProxy newProxy = - new ModuleExtensionProxy(extensionBzlFile, extensionName, thread.getCallerLocation()); + String extensionBzlFile, String extensionName, boolean devDependency, StarlarkThread thread) { + ModuleExtensionUsageBuilder newUsageBuilder = + new ModuleExtensionUsageBuilder( + extensionBzlFile, extensionName, thread.getCallerLocation()); if (ignoreDevDeps && devDependency) { // This is a no-op proxy. - return newProxy; + return newUsageBuilder.getProxy(devDependency); } - // Find an existing proxy object corresponding to this extension. - for (ModuleExtensionProxy proxy : extensionProxies) { - if (proxy.extensionBzlFile.equals(extensionBzlFile) - && proxy.extensionName.equals(extensionName)) { - return proxy; + // 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); } } // If no such proxy exists, we can just use a new one. - extensionProxies.add(newProxy); - return newProxy; + extensionUsageBuilders.add(newUsageBuilder); + return newUsageBuilder.getProxy(devDependency); } - @StarlarkBuiltin(name = "module_extension_proxy", documented = false) - class ModuleExtensionProxy implements Structure { + class ModuleExtensionUsageBuilder { private final String extensionBzlFile; private final String extensionName; private final Location location; private final HashBiMap imports; private final ImmutableList.Builder tags; - ModuleExtensionProxy(String extensionBzlFile, String extensionName, Location location) { + ModuleExtensionUsageBuilder(String extensionBzlFile, String extensionName, Location location) { this.extensionBzlFile = extensionBzlFile; this.extensionName = extensionName; this.location = location; @@ -422,50 +422,69 @@ ModuleExtensionUsage buildUsage() { .build(); } - void addImport(String localRepoName, String exportedName, Location location) - throws EvalException { - RepositoryName.validateUserProvidedRepoName(localRepoName); - RepositoryName.validateUserProvidedRepoName(exportedName); - addRepoNameUsage(localRepoName, "by a use_repo() call", location); - if (imports.containsValue(exportedName)) { - String collisionRepoName = imports.inverse().get(exportedName); - throw Starlark.errorf( - "The repo exported as '%s' by module extension '%s' is already imported at %s", - exportedName, extensionName, repoNameUsages.get(collisionRepoName).getWhere()); - } - imports.put(localRepoName, exportedName); + /** + * Creates a proxy with the specified dev_dependency bit that shares accumulated imports and + * tags with all other such proxies, thus preserving their order across dev/non-dev deps. + */ + ModuleExtensionProxy getProxy(boolean devDependency) { + return new ModuleExtensionProxy(devDependency); } - @Nullable - @Override - public Object getValue(String tagName) throws EvalException { - return new StarlarkValue() { - @StarlarkMethod( - name = "call", - selfCall = true, - documented = false, - extraKeywords = @Param(name = "kwargs"), - useStarlarkThread = true) - public void call(Dict kwargs, StarlarkThread thread) { - tags.add( - Tag.builder() - .setTagName(tagName) - .setAttributeValues(kwargs) - .setLocation(thread.getCallerLocation()) - .build()); + @StarlarkBuiltin(name = "module_extension_proxy", documented = false) + class ModuleExtensionProxy implements Structure { + + private final boolean devDependency; + + private ModuleExtensionProxy(boolean devDependency) { + this.devDependency = devDependency; + } + + void addImport(String localRepoName, String exportedName, Location location) + throws EvalException { + RepositoryName.validateUserProvidedRepoName(localRepoName); + RepositoryName.validateUserProvidedRepoName(exportedName); + addRepoNameUsage(localRepoName, "by a use_repo() call", location); + if (imports.containsValue(exportedName)) { + String collisionRepoName = imports.inverse().get(exportedName); + throw Starlark.errorf( + "The repo exported as '%s' by module extension '%s' is already imported at %s", + exportedName, extensionName, repoNameUsages.get(collisionRepoName).getWhere()); } - }; - } + imports.put(localRepoName, exportedName); + } - @Override - public ImmutableCollection getFieldNames() { - return ImmutableList.of(); - } + @Nullable + @Override + public Object getValue(String tagName) throws EvalException { + return new StarlarkValue() { + @StarlarkMethod( + name = "call", + selfCall = true, + documented = false, + extraKeywords = @Param(name = "kwargs"), + useStarlarkThread = true) + public void call(Dict kwargs, StarlarkThread thread) { + tags.add( + Tag.builder() + .setTagName(tagName) + .setAttributeValues(kwargs) + .setDevDependency(devDependency) + .setLocation(thread.getCallerLocation()) + .build()); + } + }; + } - @Nullable - @Override - public String getErrorMessageForUnknownField(String field) { - return null; + @Override + public ImmutableCollection getFieldNames() { + return ImmutableList.of(); + } + + @Nullable + @Override + public String getErrorMessageForUnknownField(String field) { + return null; + } } } @@ -821,8 +840,8 @@ public Module buildModule() { .setDeps(ImmutableMap.copyOf(deps)) .setOriginalDeps(ImmutableMap.copyOf(deps)) .setExtensionUsages( - extensionProxies.stream() - .map(ModuleExtensionProxy::buildUsage) + extensionUsageBuilders.stream() + .map(ModuleExtensionUsageBuilder::buildUsage) .collect(toImmutableList())) .build(); } diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java index 6d89b5a778cc29..498aacd427c49a 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java @@ -33,6 +33,9 @@ public abstract class Tag { /** All keyword arguments supplied to the tag instance. */ public abstract Dict getAttributeValues(); + /** Whether this tag was created using a proxy created with dev_dependency = True. */ + public abstract boolean isDevDependency(); + /** The source location in the module file where this tag was created. */ public abstract Location getLocation(); @@ -48,6 +51,8 @@ public abstract static class Builder { public abstract Builder setAttributeValues(Dict value); + public abstract Builder setDevDependency(boolean value); + public abstract Builder setLocation(Location value); public abstract Tag build(); diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java index 30c52dc3b97a77..54261a2349f863 100644 --- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java +++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java @@ -34,10 +34,12 @@ public class TypeCheckedTag implements Structure { private final TagClass tagClass; private final Object[] attrValues; + private final boolean devDependency; - private TypeCheckedTag(TagClass tagClass, Object[] attrValues) { + private TypeCheckedTag(TagClass tagClass, Object[] attrValues, boolean devDependency) { this.tagClass = tagClass; this.attrValues = attrValues; + this.devDependency = devDependency; } /** Creates a {@link TypeCheckedTag}. */ @@ -95,7 +97,15 @@ public static TypeCheckedTag create(TagClass tagClass, Tag tag, LabelConverter l attrValues[i] = Attribute.valueToStarlark(attr.getDefaultValueUnchecked()); } } - return new TypeCheckedTag(tagClass, attrValues); + return new TypeCheckedTag(tagClass, attrValues, tag.isDevDependency()); + } + + /** + * Whether the tag was specified on an extension proxy created with dev_dependency=True + * . + */ + public boolean isDevDependency() { + return devDependency; } @Override diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java index cc4dbaba79f53b..2af63ead6d9d35 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java @@ -283,6 +283,7 @@ public static TagClass createTagClass(Attribute... attrs) { public static class TestTagBuilder { private final Dict.Builder attrValuesBuilder = Dict.builder(); private final String tagName; + private boolean devDependency = false; private TestTagBuilder(String tagName) { this.tagName = tagName; @@ -294,11 +295,18 @@ public TestTagBuilder addAttr(String attrName, Object attrValue) { return this; } + @CanIgnoreReturnValue + public TestTagBuilder setDevDependency() { + devDependency = true; + return this; + } + public Tag build() { return Tag.builder() .setTagName(tagName) .setLocation(Location.BUILTIN) .setAttributeValues(attrValuesBuilder.buildImmutable()) + .setDevDependency(devDependency) .build(); } } 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 f8ad86388377d7..69b3adba649dc4 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 @@ -446,7 +446,7 @@ public void multipleModules_devDependency() throws Exception { " data_str = 'modules:'", " for mod in ctx.modules:", " for tag in mod.tags.tag:", - " data_str += ' ' + tag.data", + " data_str += ' ' + tag.data + ' ' + str(ctx.is_dev_dependency(tag))", " data_repo(name='ext_repo',data=data_str)", "tag=tag_class(attrs={'data':attr.string()})", "ext=module_extension(implementation=_ext_impl,tag_classes={'tag':tag})"); @@ -457,7 +457,8 @@ public void multipleModules_devDependency() throws Exception { if (result.hasError()) { throw result.getError().getException(); } - assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("modules: root bar@2.0"); + assertThat(result.get(skyKey).getModule().getGlobal("data")) + .isEqualTo("modules: root True bar@2.0 False"); } @Test @@ -497,7 +498,7 @@ public void multipleModules_ignoreDevDependency() throws Exception { " data_str = 'modules:'", " for mod in ctx.modules:", " for tag in mod.tags.tag:", - " data_str += ' ' + tag.data", + " data_str += ' ' + tag.data + ' ' + str(ctx.is_dev_dependency(tag))", " data_repo(name='ext_repo',data=data_str)", "tag=tag_class(attrs={'data':attr.string()})", "ext=module_extension(implementation=_ext_impl,tag_classes={'tag':tag})"); @@ -511,7 +512,8 @@ public void multipleModules_ignoreDevDependency() throws Exception { if (result.hasError()) { throw result.getError().getException(); } - assertThat(result.get(skyKey).getModule().getGlobal("data")).isEqualTo("modules: bar@2.0"); + assertThat(result.get(skyKey).getModule().getGlobal("data")) + .isEqualTo("modules: bar@2.0 False"); } @Test 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 7da2e62ffe4098..bbdeb0d4d4d33a 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 @@ -484,6 +484,7 @@ public void testModuleExtensions_good() throws Exception { Dict.builder() .put("key", "val") .buildImmutable()) + .setDevDependency(false) .setLocation( Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 4, 11)) .build()) @@ -501,6 +502,7 @@ public void testModuleExtensions_good() throws Exception { Dict.builder() .put("key1", "val1") .buildImmutable()) + .setDevDependency(false) .setLocation( Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 7, 12)) .build()) @@ -511,6 +513,7 @@ public void testModuleExtensions_good() throws Exception { Dict.builder() .put("key2", "val2") .buildImmutable()) + .setDevDependency(false) .setLocation( Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 8, 12)) .build()) @@ -529,6 +532,7 @@ public void testModuleExtensions_good() throws Exception { Dict.builder() .put("coord", "junit") .buildImmutable()) + .setDevDependency(false) .setLocation( Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 12, 10)) .build()) @@ -539,6 +543,7 @@ public void testModuleExtensions_good() throws Exception { Dict.builder() .put("coord", "guava") .buildImmutable()) + .setDevDependency(false) .setLocation( Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 14, 10)) .build()) @@ -551,12 +556,16 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { scratch.file( rootDirectory.getRelative("MODULE.bazel").getPathString(), "myext1 = use_extension('//:defs.bzl','myext',dev_dependency=True)", + "myext1.tag(name = 'tag1')", "use_repo(myext1, 'alpha')", "myext2 = use_extension('//:defs.bzl','myext')", + "myext2.tag(name = 'tag2')", "use_repo(myext2, 'beta')", "myext3 = use_extension('//:defs.bzl','myext',dev_dependency=True)", + "myext3.tag(name = 'tag3')", "use_repo(myext3, 'gamma')", "myext4 = use_extension('//:defs.bzl','myext')", + "myext4.tag(name = 'tag4')", "use_repo(myext4, 'delta')"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of()); @@ -580,6 +589,50 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception { ImmutableBiMap.of( "alpha", "alpha", "beta", "beta", "gamma", "gamma", "delta", "delta")) + .addTag( + Tag.builder() + .setTagName("tag") + .setAttributeValues( + Dict.builder() + .put("name", "tag1") + .buildImmutable()) + .setDevDependency(true) + .setLocation( + Location.fromFileLineColumn("/MODULE.bazel", 2, 11)) + .build()) + .addTag( + Tag.builder() + .setTagName("tag") + .setAttributeValues( + Dict.builder() + .put("name", "tag2") + .buildImmutable()) + .setDevDependency(false) + .setLocation( + Location.fromFileLineColumn("/MODULE.bazel", 5, 11)) + .build()) + .addTag( + Tag.builder() + .setTagName("tag") + .setAttributeValues( + Dict.builder() + .put("name", "tag3") + .buildImmutable()) + .setDevDependency(true) + .setLocation( + Location.fromFileLineColumn("/MODULE.bazel", 8, 11)) + .build()) + .addTag( + Tag.builder() + .setTagName("tag") + .setAttributeValues( + Dict.builder() + .put("name", "tag4") + .buildImmutable()) + .setDevDependency(false) + .setLocation( + Location.fromFileLineColumn("/MODULE.bazel", 11, 11)) + .build()) .build()) .build()); } @@ -593,12 +646,16 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { createModuleKey("mymod", "1.0"), "module(name='mymod',version='1.0')", "myext1 = use_extension('//:defs.bzl','myext',dev_dependency=True)", + "myext1.tag(name = 'tag1')", "use_repo(myext1, 'alpha')", "myext2 = use_extension('//:defs.bzl','myext')", + "myext2.tag(name = 'tag2')", "use_repo(myext2, 'beta')", "myext3 = use_extension('//:defs.bzl','myext',dev_dependency=True)", + "myext3.tag(name = 'tag3')", "use_repo(myext3, 'gamma')", "myext4 = use_extension('//:defs.bzl','myext')", + "myext4.tag(name = 'tag4')", "use_repo(myext4, 'delta')"); ModuleFileFunction.REGISTRIES.set(differencer, ImmutableList.of(registry.getUrl())); @@ -617,8 +674,30 @@ public void testModuleExtensions_duplicateProxy_asDep() throws Exception { ModuleExtensionUsage.builder() .setExtensionBzlFile("//:defs.bzl") .setExtensionName("myext") - .setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 4, 23)) + .setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 5, 23)) .setImports(ImmutableBiMap.of("beta", "beta", "delta", "delta")) + .addTag( + Tag.builder() + .setTagName("tag") + .setAttributeValues( + Dict.builder() + .put("name", "tag2") + .buildImmutable()) + .setDevDependency(false) + .setLocation( + Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 6, 11)) + .build()) + .addTag( + Tag.builder() + .setTagName("tag") + .setAttributeValues( + Dict.builder() + .put("name", "tag4") + .buildImmutable()) + .setDevDependency(false) + .setLocation( + Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 12, 11)) + .build()) .build()) .build()); } diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java index f6eec3267f87d8..1151480aaee6e3 100644 --- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java +++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java @@ -61,10 +61,11 @@ public void basic() throws Exception { TypeCheckedTag typeCheckedTag = TypeCheckedTag.create( createTagClass(attr("foo", Type.INTEGER).build()), - buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).build(), - /*labelConverter=*/ null); + buildTag("tag_name").addAttr("foo", StarlarkInt.of(3)).setDevDependency().build(), + /* labelConverter= */ null); assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo"); assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(StarlarkInt.of(3)); + assertThat(typeCheckedTag.isDevDependency()).isTrue(); } @Test @@ -87,6 +88,7 @@ public void label() throws Exception { Label.parseAbsoluteUnchecked("@myrepo//mypkg:thing1"), Label.parseAbsoluteUnchecked("@myrepo//pkg:thing2"), Label.parseAbsoluteUnchecked("@other_repo//pkg:thing3"))); + assertThat(typeCheckedTag.isDevDependency()).isFalse(); } @Test @@ -95,12 +97,13 @@ public void label_withoutDefaultValue() throws Exception { TypeCheckedTag.create( createTagClass( attr("foo", BuildType.LABEL).allowedFileTypes(FileTypeSet.ANY_FILE).build()), - buildTag("tag_name").build(), + buildTag("tag_name").setDevDependency().build(), new LabelConverter( PackageIdentifier.parse("@myrepo//mypkg"), createRepositoryMapping(createModuleKey("test", "1.0"), "repo", "other_repo"))); assertThat(typeCheckedTag.getFieldNames()).containsExactly("foo"); assertThat(getattr(typeCheckedTag, "foo")).isEqualTo(Starlark.NONE); + assertThat(typeCheckedTag.isDevDependency()).isTrue(); } @Test @@ -119,6 +122,7 @@ public void stringListDict_default() throws Exception { Dict.builder() .put("key", StarlarkList.immutableOf("value1", "value2")) .buildImmutable()); + assertThat(typeCheckedTag.isDevDependency()).isFalse(); } @Test @@ -139,6 +143,7 @@ public void multipleAttributesAndDefaults() throws Exception { assertThat(getattr(typeCheckedTag, "bar")).isEqualTo(StarlarkInt.of(3)); assertThat(getattr(typeCheckedTag, "quux")) .isEqualTo(StarlarkList.immutableOf("quuxValue1", "quuxValue2")); + assertThat(typeCheckedTag.isDevDependency()).isFalse(); } @Test