Add a dry-run option to bazel mod tidy by numine777 · Pull Request #25540 · bazelbuild/bazel

numine777 · 2025-03-12T21:46:37Z

This PR addresses issue #24263

We needed this change for our CI as we are migrating to bzlmod. This PR now addresses the master branch to make the patch as useful as possible.

Please let me know if this PR needs any additional updates to documentation or testing before it is viable. I merely saw that the issue was tagged as a good first issue and thought I would jump in!

* Allow bazel mod tidy to take a `write` option that determines whether fixes are made to files in place -> Defaults to true

fmeum · 2025-03-13T08:31:54Z

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

+  private static BlazeCommandResult reportAndCreateTidyDryRunResult(
+    CommandEnvironment env, BazelModTidyValue modTidyValue) {
+    if (modTidyValue.fixups().isEmpty()) {
+      return BlazeCommandResult.success();


The second part of my comment from #25246 (comment) still applies. As is, this might pass even though bazel mod tidy would have made changes due to formatting. I think that you need to invoke buildozer in each case and use it as the source of truth for whether it changed a file.

Do you mean to say here that we may end up with false positives in cases where MODULE.bazel file(s) need to be formatted rather than actually having errors? Or are you pointing out that this implementation might return a success case even though bazel mod tidy would have made formatting only changes?

The latter. If all use_repo arguments are corrected but the module file is badly formatted, this would return success but performing a non-dry run would change the module file.

Is that something that we feel a need to include in dry-run? For instance, it's fairly straightforward to use buildifier to capture formatting errors, but nothing except this tool detects problems in use_repo calls. If we do want to include formatting, is the proposed flow that we call buildozer with stdout and pipe that output to a function to determine formatting needs?

Yes, see #25246 (comment). I think that we do need to capture stdout.

Implemented! Happy to hear constructive feedback

fmeum · 2025-03-19T20:13:22Z

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

+            .setWorkingDir(env.getWorkspace())
+            .addArg(modTidyValue.buildozer().getPathString())
+            .addArg("-f")
+            .addArg("-")


Could we share the common part of the construction of the builder between the branches?

fmeum · 2025-03-24T12:42:14Z

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

-          .get();
+          .get()
+          .getStdout();
+        return reportAndCreateTidyDryRunResult(env, modTidyValue, new String(out));


Instead of comparing stdout for differences, which requires reading the module files, could you check stderr for fixed <path> lines? https://github.com/bazelbuild/buildtools/blob/733b4a20350d044edbef66af669b1793cf8347e3/edit/buildozer.go#L1502

fmeum · 2025-03-24T12:44:05Z

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

+                fixup.usage().getExtensionBzlFile() + "%" + fixup.usage().getExtensionName();
+              return fixup.usage().getProxies().stream()
+                .map(p -> String.format("  %s: %s", p.getLocation().toString(), extensionId))
+                .collect(joining("\n"));


Instead of collecting here and then collecting again in the outer stream, use flatMap.

fmeum · 2025-03-24T12:44:17Z

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

+              return fixup.usage().getProxies().stream()
+                .map(p -> String.format("  %s: %s", p.getLocation().toString(), extensionId))
+                .collect(joining("\n"));
+            }).collect(joining("\n"))) + "\n";


joining has an overload that accepts a suffix.

JIRA Tracking ID: Description: Test Notes: Release Notes:

dws · 2025-06-13T17:33:02Z

+1 to get this landed

fmeum · 2025-06-21T13:02:54Z

I sent bazelbuild/buildtools#1373 to make it easier to implement this feature. Let's wait for that to land as well as a release, then our job here will become much easier.

Based on that change, I was able to simplify the current PR: https://github.com/bazelbuild/bazel/compare/master...fmeum:bazel:fork/numine777/feature/add-mody-tidy-nowrite?expand=1

numine777 · 2025-06-21T13:56:12Z

@fmeum Thanks much! I apologize for letting this get stale. My team pivoted back to using WORKSPACE for the time being, so the demand hasn't been as strong on our end

fmeum · 2025-06-21T15:50:44Z

Don't worry, thanks for kicking this off!

iancha1992 · 2025-09-12T22:09:29Z

@bazel-io fork 7.7.0

dws · 2025-10-26T18:48:03Z

Any update on getting this landed?

fmeum · 2025-10-26T19:20:33Z

This needs bazelbuild/buildtools#1373, which has been rolled back

github-actions · 2026-02-18T01:38:30Z

Thank you for contributing to the Bazel repository! This pull request has been marked as stale since it has not had any activity in the last 30 days. It will be closed in the next 30 days unless any other activity occurs. If you think this PR is still relevant and should stay open, please post any comment here and the PR will no longer be marked as stale.

archshift · 2026-02-24T17:44:05Z

This would be great to have!

Add a dry-run option to bazel mod tidy

d531676

* Allow bazel mod tidy to take a `write` option that determines whether fixes are made to files in place -> Defaults to true

numine777 requested review from Wyverald and meteorcloudy as code owners March 12, 2025 21:46

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

fmeum reviewed Mar 13, 2025

View reviewed changes

Use stdout to track format errors

b3facf7

fmeum reviewed Mar 24, 2025

View reviewed changes

Revisions

1fc800e

JIRA Tracking ID: Description: Test Notes: Release Notes:

meteorcloudy added awaiting-user-response Awaiting a response from the author and removed awaiting-review PR is awaiting review from an assigned reviewer labels Sep 9, 2025

bazel-io mentioned this pull request Sep 12, 2025

[7.7.0] Add a dry-run option to bazel mod tidy #26987

Open

github-actions bot added the stale Issues or PRs that are stale (no activity for 30 days) label Feb 18, 2026

iancha1992 added not stale Issues or PRs that are inactive but not considered stale and removed stale Issues or PRs that are stale (no activity for 30 days) labels Feb 24, 2026

meteorcloudy mentioned this pull request Feb 27, 2026

Also print fixed ... messages with -stdout bazelbuild/buildtools#1373

Merged

Conversation

numine777 commented Mar 12, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dws commented Jun 13, 2025

Uh oh!

fmeum commented Jun 21, 2025

Uh oh!

numine777 commented Jun 21, 2025

Uh oh!

fmeum commented Jun 21, 2025

Uh oh!

iancha1992 commented Sep 12, 2025

Uh oh!

dws commented Oct 26, 2025

Uh oh!

fmeum commented Oct 26, 2025

Uh oh!

github-actions bot commented Feb 18, 2026

Uh oh!

archshift commented Feb 24, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants