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

    }

    // 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);


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

    }

    // 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);


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

    }

    // 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);


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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `module_ctx.is_dev_dependency` #17909

Add `module_ctx.is_dev_dependency` #17909

fmeum commented Mar 29, 2023

Wyverald Mar 29, 2023

fmeum Mar 29, 2023

Wyverald Mar 29, 2023

Wyverald Mar 29, 2023

fmeum Mar 29, 2023

Wyverald Mar 29, 2023

Wyverald Mar 29, 2023

Wyverald left a comment

Wyverald left a comment

fmeum commented Mar 30, 2023

Add module_ctx.is_dev_dependency #17909

Add module_ctx.is_dev_dependency #17909

Conversation

fmeum commented Mar 29, 2023

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

fmeum Mar 29, 2023

Choose a reason for hiding this comment

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

fmeum Mar 29, 2023

Choose a reason for hiding this comment

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Wyverald Mar 29, 2023

Choose a reason for hiding this comment

Wyverald left a comment

Choose a reason for hiding this comment

Wyverald left a comment

Choose a reason for hiding this comment

fmeum commented Mar 30, 2023

Add `module_ctx.is_dev_dependency` #17909

Add `module_ctx.is_dev_dependency` #17909