Add module_ctx.is_dev_dependency#17909
Conversation
Allows module extensions to determine whether a given tag represents a dev dependency. Fixes bazelbuild#17101 Work towards bazelbuild#17908
ad59144 to
a18f6cc
Compare
| if (proxy.extensionBzlFile.equals(extensionBzlFile) | ||
| && proxy.extensionName.equals(extensionName)) { | ||
| return proxy; | ||
| return proxy.withDevDependency(devDependency); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
oh, I see! I missed the fact that the proxy-proxies share the accumulated tags. Sorry for the noise :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I agree that's the safer design, implemented it.
| if (proxy.extensionBzlFile.equals(extensionBzlFile) | ||
| && proxy.extensionName.equals(extensionName)) { | ||
| return proxy; | ||
| return proxy.withDevDependency(devDependency); |
There was a problem hiding this comment.
oh, I see! I missed the fact that the proxy-proxies share the accumulated tags. Sorry for the noise :)
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
Outdated
Show resolved
Hide resolved
| if (proxy.extensionBzlFile.equals(extensionBzlFile) | ||
| && proxy.extensionName.equals(extensionName)) { | ||
| return proxy; | ||
| return proxy.withDevDependency(devDependency); |
There was a problem hiding this comment.
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.
Wyverald
left a comment
There was a problem hiding this comment.
thanks, I like this much more :)
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
Outdated
Show resolved
Hide resolved
src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
Outdated
Show resolved
Hide resolved
03fa693 to
11ab28c
Compare
|
@bazel-io flag |
Allows module extensions to determine whether a given tag represents a dev dependency. Fixes bazelbuild#17101 Work towards bazelbuild#17908 Closes bazelbuild#17909. PiperOrigin-RevId: 520645663 Change-Id: I3e3136a09d01d25fc706bcd0dfd7e53b6e7d5285
* Add `module_ctx.is_dev_dependency` Allows module extensions to determine whether a given tag represents a dev dependency. Fixes #17101 Work towards #17908 Closes #17909. PiperOrigin-RevId: 520645663 Change-Id: I3e3136a09d01d25fc706bcd0dfd7e53b6e7d5285 * Revert section that was accidentally cherry-picked --------- Co-authored-by: Fabian Meumertzheim <fabian@meumertzhe.im> Co-authored-by: keertk <keerthanakumar@google.com>
Allows module extensions to determine whether a given tag represents a dev dependency. Fixes bazelbuild#17101 Work towards bazelbuild#17908 Closes bazelbuild#17909. PiperOrigin-RevId: 520645663 Change-Id: I3e3136a09d01d25fc706bcd0dfd7e53b6e7d5285
Allows module extensions to determine whether a given tag represents a dev dependency.
Fixes #17101
Work towards #17908