Add `module_ctx.is_dev_dependency` #17909

fmeum · 2023-03-29T06:20:45Z

Allows module extensions to determine whether a given tag represents a dev dependency.

Fixes #17101
Work towards #17908

Allows module extensions to determine whether a given tag represents a dev dependency. Fixes bazelbuild#17101 Work towards bazelbuild#17908

Wyverald · 2023-03-29T11:31:32Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

      if (proxy.extensionBzlFile.equals(extensionBzlFile)
          && proxy.extensionName.equals(extensionName)) {
-        return proxy;
+        return proxy.withDevDependency(devDependency);


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.

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?

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

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.

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

Wyverald · 2023-03-29T11:39:37Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

      if (proxy.extensionBzlFile.equals(extensionBzlFile)
          && proxy.extensionName.equals(extensionName)) {
-        return proxy;
+        return proxy.withDevDependency(devDependency);


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

Wyverald · 2023-03-29T11:42:28Z

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

      if (proxy.extensionBzlFile.equals(extensionBzlFile)
          && proxy.extensionName.equals(extensionName)) {
-        return proxy;
+        return proxy.withDevDependency(devDependency);


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

thanks, I like this much more :)

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

Wyverald

thanks!

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java

fmeum · 2023-03-30T15:28:11Z

@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 <[email protected]> Co-authored-by: keertk <[email protected]>

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

a18f6cc

Allows module extensions to determine whether a given tag represents a dev dependency. Fixes bazelbuild#17101 Work towards bazelbuild#17908

fmeum force-pushed the is-dev-dependency branch from ad59144 to a18f6cc Compare March 29, 2023 07:50

fmeum marked this pull request as ready for review March 29, 2023 07:50

fmeum requested review from Wyverald and meteorcloudy as code owners March 29, 2023 07:51

github-actions bot added awaiting-review PR is awaiting review from an assigned reviewer team-ExternalDeps External dependency handling, remote repositiories, WORKSPACE file. labels Mar 29, 2023

Wyverald reviewed Mar 29, 2023

View reviewed changes

Use a dedicated proxy builder class

9a3bdc8

fmeum requested a review from Wyverald March 29, 2023 12:25

Wyverald reviewed Mar 29, 2023

View reviewed changes

src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java Outdated Show resolved Hide resolved

Wyverald approved these changes Mar 29, 2023

View reviewed changes

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

Rename builder

11ab28c

fmeum force-pushed the is-dev-dependency branch from 03fa693 to 11ab28c Compare March 29, 2023 14:41

Wyverald approved these changes Mar 29, 2023

View reviewed changes

Wyverald added awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally and removed awaiting-review PR is awaiting review from an assigned reviewer labels Mar 29, 2023

fmeum mentioned this pull request Mar 29, 2023

Canonicalize use_extension label #17920

Closed

copybara-service bot closed this in 1a719ce Mar 30, 2023

fmeum deleted the is-dev-dependency branch March 30, 2023 15:28

bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Mar 30, 2023

sgowroji removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Mar 30, 2023

Wyverald removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Mar 30, 2023

ShreeM01 mentioned this pull request Mar 30, 2023

[6.2.0]Add module_ctx.is_dev_dependency #17934

Merged

Add module_ctx.is_dev_dependency #17909

Add module_ctx.is_dev_dependency #17909

Uh oh!

Conversation

fmeum commented Mar 29, 2023

Uh oh!

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

fmeum Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

fmeum Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Wyverald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Wyverald left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

fmeum commented Mar 30, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add `module_ctx.is_dev_dependency` #17909

Add `module_ctx.is_dev_dependency` #17909