Add `bazel mod tidy` #20483

fmeum · 2023-12-09T09:51:01Z

Implements https://docs.google.com/document/d/13LbK_1WhA4la0eH7yISjnMvXs2cKFXD-adKPu0i0RK0/edit

RELNOTES: The new bazel mod tidy subcommand automatically updates use_repo calls in the MODULE.bazel file for extensions that use module_ctx.extension_metadata.

fmeum · 2023-12-13T19:20:01Z

@aherrmann @cgrindel in case you want to give this an early try

The CI failure does appear to be a flake.

aherrmann · 2023-12-20T13:00:48Z

@fmeum Thanks! I gave it a quick shot on rules_nixpkgs_core and it seems to be working there. (I tested removing a use_repo entry and it regenerated it)

fmeum · 2024-01-28T13:37:46Z

@meteorcloudy @Wyverald I resolved all conflicts and also fixed a bug in the buildozer module that resulted in a platform-dependent lockfile entry.

Wyverald · 2024-01-30T00:42:12Z

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

+                .equals(FailureDetails.ExternalDeps.Code.INVALID_EXTENSION_IMPORT)) {
+          throw new BazelModTidyFunctionException(e, SkyFunctionException.Transience.PERSISTENT);
+        }
+        // This is an error bazel mod tidy can fix, so don't fail.


does this actually work? last time I checked, we can't actually recover from errors in any Skyframe node evaluation. So even if we don't throw anything here, the evaluation would end in error (presumably at ModCommand.java:234)

As far as I understand this, we can't recover in a SkyFunction, but here we perform a top-level evaluation (implicitly in keep going mode: https://cs.opensource.google/bazel/bazel/+/master:src/main/java/com/google/devtools/build/lib/skyframe/SkyframeExecutor.java;l=2262;drc=0e1dd2afc8dfe69033abbc7852bb23bd0a9237f6). The individual values will still be marked as failed, but we should be able to extract the individual exceptions in this way. There is a test case covering this case, but if I misunderstood how this works it might of course not catch every issue due to the inherent non-determinism of Skyframe evaluation order.

ah! I think the 'implicitly in keep-going mode' bit is crucial. But it still surprises me a bit that the whole evaluation doesn't end up in error (even though we can go through all the failed nodes). Anyhow, this is definitely not blocking the whole PR, especially as you do have a test case

But it still surprises me a bit that the whole evaluation doesn't end up in error (even though we can go through all the failed nodes).

You are right, that is indeed surprising. Maybe this is because the nodes we actually requested did not fail themselves and we are in "keep going" mode? It would be nice to get confirmation on this.

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

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

src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java

Wyverald · 2024-01-30T01:45:57Z

src/main/java/com/google/devtools/build/lib/bazel/commands/ModCommand.java

+    }
+
+    if (modTidyValue.lockfileMode().equals(LockfileMode.UPDATE)) {
+      // We cannot safely rerun Skyframe evaluation here to pick up the updated module file.


this part seems rather tricky so I'd just like to flag it so I can think about it more. My immediate reaction was that we should just trigger whatever mechanism we have for detecting diffs/dirtiness (there seem to be quite a few of those... DiffAwareness, DirtinessChecker, etc). But some half-hearted digging revealed that it might be even hackier than what you're doing here. So I'd like to come back later with a clearer head and review this part more carefully.

I went with this approach based on @lberki's comments on the design doc. I don't know whether triggering any of the diff awareness logic directly could help work around the various file system caches.

Another argument in favor of the current approach is that it could be fully dropped with the proposed new lockfile format: The lockfile wouldn't change at all if only imports are changed.

Another argument in favor of the current approach is that it could be fully dropped with the proposed new lockfile format: The lockfile wouldn't change at all if only imports are changed.

To confirm, this is because we send out the lockfile event (or whatever it's called...) before we check the validity of use_repo, right?

I'm not sure how your question relates to the quote (did you mean to respond on the thread about Skyframe evaluation failing?). The fixup events are emitted before we check the validity of use_repo, which is indeed crucial, as otherwise we would exit the SkyFunction with a failure before we learn of the fixup command. However, this was already necessary before this PR, simply to show the user-facing warning before running into the failure.

good, good -- nevermind the quote, not important :)

meteorcloudy · 2024-02-05T14:34:35Z

@fmeum Can you please resolve the conflict?

fmeum · 2024-02-05T22:04:52Z

Done!

fmeum · 2024-02-06T15:14:31Z

I resolved the new conflict.

fmeum · 2024-02-06T20:44:20Z

The remaining failure looks like a flake, but a retry didn't fix it.

fmeum · 2024-02-07T18:47:57Z

@bazel-io fork 7.1.0

Implements https://docs.google.com/document/d/13LbK_1WhA4la0eH7yISjnMvXs2cKFXD-adKPu0i0RK0/edit RELNOTES: The new `bazel mod tidy` subcommand automatically updates `use_repo` calls in the `MODULE.bazel` file for extensions that use `module_ctx.extension_metadata`. Closes bazelbuild#20483. PiperOrigin-RevId: 605021386 Change-Id: Idb1f22c51e126328b9efd6a5a6d6f89d77b9308d

Implements https://docs.google.com/document/d/13LbK_1WhA4la0eH7yISjnMvXs2cKFXD-adKPu0i0RK0/edit RELNOTES: The new `bazel mod tidy` subcommand automatically updates `use_repo` calls in the `MODULE.bazel` file for extensions that use `module_ctx.extension_metadata`. Closes #20483. PiperOrigin-RevId: 605021386 Change-Id: Idb1f22c51e126328b9efd6a5a6d6f89d77b9308d Closes #21241

iancha1992 · 2024-02-22T22:42:50Z

The changes in this PR have been included in Bazel 7.1.0 RC1. Please test out the release candidate and report any issues as soon as possible. If you're using Bazelisk, you can point to the latest RC by setting USE_BAZEL_VERSION=last_rc.
Thanks!

aherrmann · 2024-02-23T09:29:07Z

Thanks! I repeated my test from above. bazel mod tidy correctly re-created a deleted use_repo item.
I noticed that it also changed the ordering of attributes in a tag invocation

diff --git a/core/MODULE.bazel b/core/MODULE.bazel
index 52953ec..5202e78 100644
--- a/core/MODULE.bazel
+++ b/core/MODULE.bazel
@@ -9,7 +9,7 @@ bazel_dep(name = "bazel_skylib", version = "1.0.3")
 nix_repo = use_extension("//extensions:repository.bzl", "nix_repo")
 nix_repo.github(
     name = "nixpkgs",
-    tag = "22.11",
     sha256 = "ddc3428d9e1a381b7476750ac4dbea7a42885cbbe6e1af44b21d6447c9609a6f",
+    tag = "22.11",
 )
 use_repo(nix_repo, "nixpkgs")

Is that intentional? IIRC this did not happen when I last tested it and the proposal only speaks about use_repo.
I personally am happy with this behavior, but I just wanted to check if this is not an unintended side effect.

fmeum · 2024-02-23T09:40:41Z

This is a result of buildifier formatting the module file, which is a side effect of running buildozer on it. Since buildozer by default only reformats if it made any changes, the bazel mod tidy even forces unconditional reformatting.

fmeum force-pushed the bazel-mod-fix branch 10 times, most recently from ebabd9e to 389e53c Compare December 13, 2023 18:32

fmeum marked this pull request as ready for review December 13, 2023 19:19

fmeum requested review from Wyverald and meteorcloudy as code owners December 13, 2023 19:19

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

fmeum force-pushed the bazel-mod-fix branch 2 times, most recently from f3fc1fe to 8039efa Compare January 27, 2024 23:55

fmeum assigned fmeum and unassigned fmeum Jan 27, 2024

fmeum force-pushed the bazel-mod-fix branch from 8039efa to 03d5ea6 Compare January 28, 2024 12:43

Wyverald reviewed Jan 30, 2024

View reviewed changes

fmeum force-pushed the bazel-mod-fix branch 3 times, most recently from 1fdcced to d644e21 Compare January 30, 2024 22:35

fmeum requested a review from Wyverald January 30, 2024 22:35

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 Jan 30, 2024

Wyverald added the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 1, 2024

fmeum force-pushed the bazel-mod-fix branch from dd16424 to c863a45 Compare February 5, 2024 20:02

fmeum requested a review from meteorcloudy February 5, 2024 20:02

fmeum force-pushed the bazel-mod-fix branch from c863a45 to d40a11e Compare February 6, 2024 15:14

Add bazel mod tidy

3f5709d

fmeum force-pushed the bazel-mod-fix branch from d40a11e to 3f5709d Compare February 6, 2024 19:15

copybara-service bot closed this in 9f0f232 Feb 7, 2024

github-actions bot removed the awaiting-PR-merge PR has been approved by a reviewer and is ready to be merge internally label Feb 7, 2024

fmeum deleted the bazel-mod-fix branch February 7, 2024 18:45

bazel-io mentioned this pull request Feb 7, 2024

[7.1.0] Add bazel mod tidy #21241

Closed

stefanpenner mentioned this pull request Feb 7, 2024

[Feature] bzlmod & go.work bazel-contrib/bazel-gazelle#1731

Merged

iancha1992 mentioned this pull request Feb 7, 2024

Add bazel mod tidy #21242

Closed

iancha1992 mentioned this pull request Feb 7, 2024

[7.1.0] Add bazel mod tidy #21247

Closed

fmeum mentioned this pull request Feb 8, 2024

[7.1.0] Add bazel mod tidy #21265

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `bazel mod tidy` #20483

Add `bazel mod tidy` #20483

fmeum commented Dec 9, 2023 •

edited

Loading

fmeum commented Dec 13, 2023

aherrmann commented Dec 20, 2023

fmeum commented Jan 28, 2024

Wyverald Jan 30, 2024

fmeum Jan 30, 2024

Wyverald Jan 30, 2024

fmeum Jan 30, 2024

Wyverald Jan 30, 2024

fmeum Jan 30, 2024 •

edited

Loading

Wyverald Jan 30, 2024

fmeum Jan 30, 2024

Wyverald Jan 30, 2024

meteorcloudy commented Feb 5, 2024

fmeum commented Feb 5, 2024

fmeum commented Feb 6, 2024

fmeum commented Feb 6, 2024

fmeum commented Feb 7, 2024

iancha1992 commented Feb 22, 2024

aherrmann commented Feb 23, 2024

fmeum commented Feb 23, 2024

Add bazel mod tidy #20483

Add bazel mod tidy #20483

Conversation

fmeum commented Dec 9, 2023 • edited Loading

fmeum commented Dec 13, 2023

aherrmann commented Dec 20, 2023

fmeum commented Jan 28, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fmeum Jan 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meteorcloudy commented Feb 5, 2024

fmeum commented Feb 5, 2024

fmeum commented Feb 6, 2024

fmeum commented Feb 6, 2024

fmeum commented Feb 7, 2024

iancha1992 commented Feb 22, 2024

aherrmann commented Feb 23, 2024

fmeum commented Feb 23, 2024

Add `bazel mod tidy` #20483

Add `bazel mod tidy` #20483

fmeum commented Dec 9, 2023 •

edited

Loading

fmeum Jan 30, 2024 •

edited

Loading