Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add module_ctx.is_dev_dependency #17909

Closed
wants to merge 3 commits into from
Closed

Add module_ctx.is_dev_dependency #17909

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
a18f6cc
Add `module_ctx.is_dev_dependency`
fmeum Mar 29, 2023
9a3bdc8
Use a dedicated proxy builder class
fmeum Mar 29, 2023
11ab28c
Rename builder
fmeum Mar 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -99,4 +101,19 @@ protected ImmutableMap<String, String> getRemoteExecProperties() throws EvalExce
public StarlarkList<StarlarkBazelModule> getModules() {
return modules;
}

@StarlarkMethod(
name = "is_dev_dependency",
doc = "Returns whether the given tag was specified on the result of a <a "
+ "href=\"globals.html#use_extension\">use_extension</a> call with "
+ "<code>devDependency = True</code>.",
parameters = {
@Param(
name = "tag",
doc = "A tag obtained from <a href=\"bazel_module.html#tags\">bazel_module.tags</a>.",
allowedTypes = {@ParamType(type = TypeCheckedTag.class)})
})
public boolean isDevDependency(TypeCheckedTag tag) {
return tag.isDevDependency();
}
}
29 changes: 24 additions & 5 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,27 +375,26 @@ public void registerToolchains(Sequence<?> toolchainLabels) throws EvalException
},
useStarlarkThread = true)
public ModuleExtensionProxy useExtension(
String extensionBzlFile, String extensionName, boolean devDependency, StarlarkThread thread)
throws EvalException {
String extensionBzlFile, String extensionName, boolean devDependency, StarlarkThread thread) {
ModuleExtensionProxy newProxy =
new ModuleExtensionProxy(extensionBzlFile, extensionName, thread.getCallerLocation());

if (ignoreDevDeps && devDependency) {
// This is a no-op proxy.
return newProxy;
return newProxy.withDevDependency(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;
return proxy.withDevDependency(devDependency);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this works. If you add a test like the following:

proxy1 = use_extension(":foo.bzl", "proxy")
proxy1.tag()
proxy2 = use_extension(":foo.bzl", "proxy", dev_dependency=True)
proxy2.tag()

I believe the tag on proxy2 will be lost. (This is because the Java object backing proxy2 was never recorded anywhere; normally all proxies are recorded in extensionProxies)

What you need to do is make devDependency a parameter to the constructor of ModuleExtensionProxy, and in the if condition here, also check if devDependency matches. Then at the very end in #buildModule, you need to somehow merge two ModuleExtensionUsage objects if they only differ in devDependency. This is somewhat unwieldy but it's my first reaction.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your suggested approach is actually what I tried first, but merging the usages while preserving the orders of imports and tags proved to be rather difficult.

Instead, I am essentially creating proxy-proxies here (phew): Both proxy.withDevDependency(true) and proxy.withDevDependency(false) share the imports and tags of the original proxy and thus should make that test pass (see https://github.com/bazelbuild/bazel/pull/17909/files#diff-99af257732180398dadf7c29772bd194ca9518e59625e9ca10e3b1a31c5d9afaL553, albeit that one has a different order of dev/non-dev). Do you still see an issue here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, I see! I missed the fact that the proxy-proxies share the accumulated tags. Sorry for the noise :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

alternatively we could formulate this a bit differently. Rename ModuleExtensionProxy to ModuleExtensionUsageBuilder or something and let it have a method ModuleExtensionProxy getProxy(boolean devDependency). This way you cannot nest proxies in arbitrary depths.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that's the safer design, implemented it.

}
}

// If no such proxy exists, we can just use a new one.
extensionProxies.add(newProxy);
return newProxy;
return newProxy.withDevDependency(devDependency);
}

@StarlarkBuiltin(name = "module_extension_proxy", documented = false)
Expand All @@ -405,13 +404,32 @@ class ModuleExtensionProxy implements Structure {
private final Location location;
private final HashBiMap<String, String> imports;
private final ImmutableList.Builder<Tag> tags;
private final boolean devDependency;

ModuleExtensionProxy(String extensionBzlFile, String extensionName, Location location) {
this.extensionBzlFile = extensionBzlFile;
this.extensionName = extensionName;
this.location = location;
this.imports = HashBiMap.create();
this.tags = ImmutableList.builder();
this.devDependency = false;
}

private ModuleExtensionProxy(ModuleExtensionProxy other, boolean devDependency) {
this.extensionBzlFile = other.extensionBzlFile;
this.extensionName = other.extensionName;
this.location = other.location;
this.imports = other.imports;
this.tags = other.tags;
this.devDependency = devDependency;
}

/**
* Creates a proxy with the specified devDependency bit that shares accumulated imports and tags
* with the current one, thus preserving tag specification order.
*/
ModuleExtensionProxy withDevDependency(boolean devDependency) {
return devDependency ? new ModuleExtensionProxy(this, true) : this;
fmeum marked this conversation as resolved.
Show resolved Hide resolved
}

ModuleExtensionUsage buildUsage() {
Expand Down Expand Up @@ -453,6 +471,7 @@ public void call(Dict<String, Object> kwargs, StarlarkThread thread) {
Tag.builder()
.setTagName(tagName)
.setAttributeValues(kwargs)
.setDevDependency(devDependency)
.setLocation(thread.getCallerLocation())
.build());
}
Expand Down
5 changes: 5 additions & 0 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/Tag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,9 @@ public abstract class Tag {
/** All keyword arguments supplied to the tag instance. */
public abstract Dict<String, Object> 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();

Expand All @@ -48,6 +51,8 @@ public abstract static class Builder {

public abstract Builder setAttributeValues(Dict<String, Object> value);

public abstract Builder setDevDependency(boolean value);

public abstract Builder setLocation(Location value);

public abstract Tag build();
Expand Down
14 changes: 12 additions & 2 deletions src/main/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTag.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}. */
Expand Down Expand Up @@ -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
* <code>dev_dependency=True</code>.
*/
public boolean isDevDependency() {
return devDependency;
}

@Override
Expand Down
8 changes: 8 additions & 0 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/BzlmodTestUtil.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -283,6 +283,7 @@ public static TagClass createTagClass(Attribute... attrs) {
public static class TestTagBuilder {
private final Dict.Builder<String, Object> attrValuesBuilder = Dict.builder();
private final String tagName;
private boolean devDependency = false;

private TestTagBuilder(String tagName) {
this.tagName = tagName;
Expand All @@ -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();
}
}
Expand Down
8 changes: 4 additions & 4 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleExtensionResolutionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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})");
Expand All @@ -457,7 +457,7 @@ 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
Expand Down Expand Up @@ -497,7 +497,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})");
Expand All @@ -511,7 +511,7 @@ 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
Expand Down
57 changes: 56 additions & 1 deletion src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -484,6 +484,7 @@ public void testModuleExtensions_good() throws Exception {
Dict.<String, Object>builder()
.put("key", "val")
.buildImmutable())
.setDevDependency(false)
.setLocation(
Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 4, 11))
.build())
Expand All @@ -501,6 +502,7 @@ public void testModuleExtensions_good() throws Exception {
Dict.<String, Object>builder()
.put("key1", "val1")
.buildImmutable())
.setDevDependency(false)
.setLocation(
Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 7, 12))
.build())
Expand All @@ -511,6 +513,7 @@ public void testModuleExtensions_good() throws Exception {
Dict.<String, Object>builder()
.put("key2", "val2")
.buildImmutable())
.setDevDependency(false)
.setLocation(
Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 8, 12))
.build())
Expand All @@ -529,6 +532,7 @@ public void testModuleExtensions_good() throws Exception {
Dict.<String, Object>builder()
.put("coord", "junit")
.buildImmutable())
.setDevDependency(false)
.setLocation(
Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 12, 10))
.build())
Expand All @@ -539,6 +543,7 @@ public void testModuleExtensions_good() throws Exception {
Dict.<String, Object>builder()
.put("coord", "guava")
.buildImmutable())
.setDevDependency(false)
.setLocation(
Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 14, 10))
.build())
Expand All @@ -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());

Expand All @@ -580,6 +589,34 @@ public void testModuleExtensions_duplicateProxy_asRoot() throws Exception {
ImmutableBiMap.of(
"alpha", "alpha", "beta", "beta", "gamma", "gamma", "delta",
"delta"))
.addTag(Tag.builder()
.setTagName("tag")
.setAttributeValues(
Dict.<String, Object>builder().put("name", "tag1").buildImmutable())
.setDevDependency(true)
.setLocation(Location.fromFileLineColumn("<root>/MODULE.bazel", 2, 11))
.build())
.addTag(Tag.builder()
.setTagName("tag")
.setAttributeValues(
Dict.<String, Object>builder().put("name", "tag2").buildImmutable())
.setDevDependency(false)
.setLocation(Location.fromFileLineColumn("<root>/MODULE.bazel", 5, 11))
.build())
.addTag(Tag.builder()
.setTagName("tag")
.setAttributeValues(
Dict.<String, Object>builder().put("name", "tag3").buildImmutable())
.setDevDependency(true)
.setLocation(Location.fromFileLineColumn("<root>/MODULE.bazel", 8, 11))
.build())
.addTag(Tag.builder()
.setTagName("tag")
.setAttributeValues(
Dict.<String, Object>builder().put("name", "tag4").buildImmutable())
.setDevDependency(false)
.setLocation(Location.fromFileLineColumn("<root>/MODULE.bazel", 11, 11))
.build())
.build())
.build());
}
Expand All @@ -593,12 +630,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()));

Expand All @@ -617,8 +658,22 @@ 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.<String, Object>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.<String, Object>builder().put("name", "tag4").buildImmutable())
.setDevDependency(false)
.setLocation(Location.fromFileLineColumn("mymod@1.0/MODULE.bazel", 12, 11))
.build())
.build())
.build());
}
Expand Down
9 changes: 7 additions & 2 deletions src/test/java/com/google/devtools/build/lib/bazel/bzlmod/TypeCheckedTagTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
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
Expand All @@ -87,6 +88,7 @@ public void label() throws Exception {
Label.parseCanonicalUnchecked("@myrepo//mypkg:thing1"),
Label.parseCanonicalUnchecked("@myrepo//pkg:thing2"),
Label.parseCanonicalUnchecked("@other_repo//pkg:thing3")));
assertThat(typeCheckedTag.isDevDependency()).isFalse();
}

@Test
Expand All @@ -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
Expand All @@ -119,6 +122,7 @@ public void stringListDict_default() throws Exception {
Dict.builder()
.put("key", StarlarkList.immutableOf("value1", "value2"))
.buildImmutable());
assertThat(typeCheckedTag.isDevDependency()).isFalse();
}

@Test
Expand All @@ -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
Expand Down