bazel: Migrate Buildifier to Bazel Lint Umbrella (#9858) by Divinesoumyadip · Pull Request #9865 · The-OpenROAD-Project/OpenROAD

Divinesoumyadip · 2026-03-21T08:22:57Z

Fixes Issue #9858

This migrates Buildifier into the Bazel lint umbrella, following the same pattern already established for TCL linting. Previously, Buildifier only ran as a standalone GitHub Actions workflow, meaning developers had to push to CI just to discover formatting violations in BUILD and .bzl files.
The change adds buildifier_prebuilt (v8.2.1) as a Bzlmod dev dependency and introduces three new shell scripts bzl_fmt_test.sh, bzl_lint_test.sh, and bzl_tidy.sh , mirroring the existing TCL lint pattern exactly. The two read-only check targets (fmt_bzl_test and lint_bzl_test) are wired into the //:lint_test umbrella, and fix_lint has been updated to run both tidy_bzl and tidy_tcl so developers get a single command that auto-fixes both Starlark and TCL files locally. The old .github/workflows/buildifier.yaml workflow has been retired. Both new tests pass locally.

gemini-code-assist

Code Review

This pull request migrates Buildifier into the Bazel lint umbrella, which is a great improvement for the local development workflow. The changes follow the existing pattern for TCL linting by adding new test and tidy targets.

My main feedback is to make the new Buildifier test targets fully consistent with the TCL ones by using a non-sandboxed execution (tags = ["local"]) and git ls-files for discovering files. This approach is more robust than using glob() in a sandboxed test, as it correctly discovers all tracked files without being sensitive to the Bazel cache state. I've left specific suggestions on the sh_test targets in BUILD.bazel and on the new shell scripts to implement this.

Overall, this is a valuable change that will improve developer experience.

gemini-code-assist · 2026-03-21T08:25:28Z

BUILD.bazel

+sh_test(
+    name = "fmt_bzl_test",
+    size = "small",
+    srcs = ["//bazel:bzl_fmt_test.sh"],
+    args = ["$(rootpath @buildifier_prebuilt//:buildifier)"],
+    data = [
+        "BUILD.bazel",
+        "MODULE.bazel",
+        "@buildifier_prebuilt//:buildifier",
+    ] + glob(
+        ["**/*.bzl"],
+        allow_empty = True,
+    ),
+)


To improve consistency with the existing TCL linting pattern and make file discovery more robust, I suggest a few changes. The current glob() can miss newly added files until a bazel clean is performed. The TCL tests avoid this by using tags = ["local"] and git ls-files in the test script.

I recommend applying the same pattern here:

Add tags = ["local"].

Simplify data to only include the buildifier binary.

Update bzl_fmt_test.sh to use git ls-files.

This same feedback applies to lint_bzl_test. The duplication of the data attribute between the two tests could also be addressed by extracting it to a variable if you stick with the glob approach.

sh_test( name = "fmt_bzl_test", size = "small", srcs = ["//bazel:bzl_fmt_test.sh"], args = ["$(rootpath @buildifier_prebuilt//:buildifier)"], data = ["@buildifier_prebuilt//:buildifier"], tags = ["local"], )

gemini-code-assist · 2026-03-21T08:25:28Z

BUILD.bazel

+sh_test(
+    name = "lint_bzl_test",
+    size = "small",
+    srcs = ["//bazel:bzl_lint_test.sh"],
+    args = ["$(rootpath @buildifier_prebuilt//:buildifier)"],
+    data = [
+        "BUILD.bazel",
+        "MODULE.bazel",
+        "@buildifier_prebuilt//:buildifier",
+    ] + glob(
+        ["**/*.bzl"],
+        allow_empty = True,
+    ),
+)


Similar to my comment on fmt_bzl_test, I recommend using tags = ["local"] and updating the script to use git ls-files for consistency with the TCL linting setup and for more robust file discovery.

sh_test( name = "lint_bzl_test", size = "small", srcs = ["//bazel:bzl_lint_test.sh"], args = ["$(rootpath @buildifier_prebuilt//:buildifier)"], data = ["@buildifier_prebuilt//:buildifier"], tags = ["local"], )

gemini-code-assist · 2026-03-21T08:25:28Z

bazel/bzl_fmt_test.sh

+#!/bin/bash
+set -e
+# Capture absolute path BEFORE changing directories
+BUILDIFIER_BIN=$(realpath "$1")
+
+if [ -n "$BUILD_WORKSPACE_DIRECTORY" ]; then
+  cd "$BUILD_WORKSPACE_DIRECTORY"
+fi
+
+"$BUILDIFIER_BIN" -r -mode=check -lint=off .


This script can be made more robust and consistent with the existing TCL linting scripts. Using git ls-files ensures that only tracked files are checked, and it's more reliable for discovering all relevant files than running on . in a sandboxed environment populated by glob. Also, set -euo pipefail is generally safer for shell scripts.

This change assumes the corresponding sh_test in BUILD.bazel is updated with tags = ["local"].

Suggested change

#!/bin/bash

set -e

# Capture absolute path BEFORE changing directories

BUILDIFIER_BIN=$(realpath "$1")

if [ -n "$BUILD_WORKSPACE_DIRECTORY" ]; then

cd "$BUILD_WORKSPACE_DIRECTORY"

fi

"$BUILDIFIER_BIN" -r -mode=check -lint=off .

#!/bin/bash

set -euo pipefail

# Capture absolute path BEFORE changing directories

BUILDIFIER_BIN=$(realpath "$1")

if [ -n "$BUILD_WORKSPACE_DIRECTORY" ]; then

cd "$BUILD_WORKSPACE_DIRECTORY"

fi

# Find all Starlark, BUILD, and WORKSPACE files tracked by git.

git ls-files -- '*.bzl' 'BUILD' 'BUILD.bazel' 'MODULE.bazel' 'WORKSPACE' 'WORKSPACE.bazel' -z | xargs -0 "$BUILDIFIER_BIN" -mode=check -lint=off

gemini-code-assist · 2026-03-21T08:25:28Z

bazel/bzl_lint_test.sh

+#!/bin/bash
+set -e
+BUILDIFIER_BIN=$(realpath "$1")
+
+if [ -n "$BUILD_WORKSPACE_DIRECTORY" ]; then
+  cd "$BUILD_WORKSPACE_DIRECTORY"
+fi
+
+"$BUILDIFIER_BIN" -r -mode=check -lint=warn .


Similar to my comment on bzl_fmt_test.sh, I suggest using git ls-files for robust file discovery and set -euo pipefail for safer script execution. This aligns with the pattern used by the TCL linting scripts.

Suggested change

#!/bin/bash

set -e

BUILDIFIER_BIN=$(realpath "$1")

if [ -n "$BUILD_WORKSPACE_DIRECTORY" ]; then

cd "$BUILD_WORKSPACE_DIRECTORY"

fi

"$BUILDIFIER_BIN" -r -mode=check -lint=warn .

#!/bin/bash

set -euo pipefail

BUILDIFIER_BIN=$(realpath "$1")

if [ -n "$BUILD_WORKSPACE_DIRECTORY" ]; then

cd "$BUILD_WORKSPACE_DIRECTORY"

fi

git ls-files -- '*.bzl' 'BUILD' 'BUILD.bazel' 'MODULE.bazel' 'WORKSPACE' 'WORKSPACE.bazel' -z | xargs -0 "$BUILDIFIER_BIN" -mode=check -lint=warn

gemini-code-assist · 2026-03-21T08:25:28Z

bazel/bzl_tidy.sh

+#!/bin/bash
+set -e
+BUILDIFIER_BIN=$(realpath "$1")
+
+if [ -z "$BUILD_WORKSPACE_DIRECTORY" ]; then
+  echo "Error: This script must be run via 'bazelisk run'"
+  exit 1
+fi
+
+cd "$BUILD_WORKSPACE_DIRECTORY"
+"$BUILDIFIER_BIN" -r -mode=fix -lint=fix .
+echo "Buildifier tidy complete!"


For consistency with the TCL tidy script (tcl_tidy.sh) and to ensure only tracked files are modified, I recommend using git ls-files to find the files to format. Also, set -euo pipefail is a good practice for safer script execution.

Suggested change

#!/bin/bash

set -e

BUILDIFIER_BIN=$(realpath "$1")

if [ -z "$BUILD_WORKSPACE_DIRECTORY" ]; then

echo "Error: This script must be run via 'bazelisk run'"

exit 1

fi

cd "$BUILD_WORKSPACE_DIRECTORY"

"$BUILDIFIER_BIN" -r -mode=fix -lint=fix .

echo "Buildifier tidy complete!"

#!/bin/bash

set -euo pipefail

BUILDIFIER_BIN=$(realpath "$1")

if [ -z "$BUILD_WORKSPACE_DIRECTORY" ]; then

echo "Error: This script must be run via 'bazelisk run'"

exit 1

fi

cd "$BUILD_WORKSPACE_DIRECTORY"

git ls-files -- '*.bzl' 'BUILD' 'BUILD.bazel' 'MODULE.bazel' 'WORKSPACE' 'WORKSPACE.bazel' -z | xargs -0 "$BUILDIFIER_BIN" -mode=fix -lint=fix

echo "Buildifier tidy complete!"

Thanks for the suggestion. The git ls-files approach fails in Bazel's sandboxed test environment with fatal: not a git repository (exit 128) since the sandbox doesn't have access to the .git directory. The tags = ["local"] has been added for consistency with the TCL pattern, and the -r recursive flag on buildifier correctly discovers all Starlark files without needing git. Both tests pass locally.

github-actions · 2026-03-21T08:25:41Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-03-21T08:27:17Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-03-21T08:36:17Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>

github-actions · 2026-03-21T08:37:46Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-03-21T08:39:17Z

clang-tidy review says "All clean, LGTM! 👍"

github-actions · 2026-03-21T08:39:54Z

clang-tidy review says "All clean, LGTM! 👍"

Divinesoumyadip · 2026-03-21T11:01:09Z

@maliberty all CI checks including Jenkins are passing. Would appreciate a review when you get a chance!

oharboe · 2026-03-21T11:02:36Z

Nice! I have not reviewed it, but just a note on buildifier versions and operating systems. I am not sure why the MODULE.bazel.lock is in git, but I think it is a security issue, users can tell bazel to check for supply chain attacks.

I wonder if it is possible to make the lint check ignore non security concerns in .lock diffs. Would require some unittests in python also under CI tests as such a script cant be checked on any specific dev machine and would need to be curated to encode all these headaches. The upside to making this script is reduced cognitive load of contributors.

oharboe · 2026-03-21T11:05:50Z

I usually just point Claude at the PR and ask it to create new commits to address concerns in the PR, which it happily does.

Except when Gemmini and Claude disagrees, in which case I ask Claude to give a snarky explanation why Gemmini is hallucinating with a record of tests or evidence that Gemmini is wrong.

Divinesoumyadip · 2026-03-21T11:45:34Z

Nice! I have not reviewed it, but just a note on buildifier versions and operating systems. I am not sure why the MODULE.bazel.lock is in git, but I think it is a security issue, users can tell bazel to check for supply chain attacks.

I wonder if it is possible to make the lint check ignore non security concerns in .lock diffs. Would require some unittests in python also under CI tests as such a script cant be checked on any specific dev machine and would need to be curated to encode all these headaches. The upside to making this script is reduced cognitive load of contributors.

Thanks @oharboe! The MODULE.bazel.lock is standard Bzlmod practice for reproducible builds and supply chain verification. The lock diff noise in lint is a good point i think.

luarss · 2026-03-21T13:11:55Z

bazel/bzl_fmt_test.sh

@@ -0,0 +1,7 @@
+#!/bin/bash
+set -e


Suggested change

set -e

set -euo pipefail

luarss · 2026-03-21T13:12:18Z

bazel/bzl_fmt_test.sh

@@ -0,0 +1,7 @@
+#!/bin/bash
+set -e
+BUILDIFIER_BIN=$(realpath "$1")


is realpath portable across all of our supported platforms?

luarss · 2026-03-21T13:12:46Z

bazel/bzl_fmt_test.sh

+#!/bin/bash
+set -e
+BUILDIFIER_BIN=$(realpath "$1")
+if [ -n "${BUILD_WORKSPACE_DIRECTORY:-}" ]; then


Refer to how other .sh locates WORKSPACE, let's keep things consistent.

Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>

github-actions · 2026-03-22T18:47:02Z

clang-tidy review says "All clean, LGTM! 👍"

Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>

Divinesoumyadip · 2026-03-22T18:50:56Z

@luarss I've addressed all three suggestions in the latest commits.

Please let me know if anything else needs changing.

github-actions · 2026-03-22T18:52:52Z

clang-tidy review says "All clean, LGTM! 👍"

Divinesoumyadip force-pushed the buildifier-migration branch from 097a416 to 4935d3d Compare March 21, 2026 08:24

gemini-code-assist bot reviewed Mar 21, 2026

View reviewed changes

Divinesoumyadip force-pushed the buildifier-migration branch 3 times, most recently from 793a68c to 46106d3 Compare March 21, 2026 08:35

bazel: integrate buildifier for starlark formatting and linting

b389365

Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>

Divinesoumyadip force-pushed the buildifier-migration branch from 46106d3 to b389365 Compare March 21, 2026 08:37

luarss suggested changes Mar 21, 2026

View reviewed changes

bazel: fix bzl scripts to use readlink-f and git ls-files pattern

67246b4

Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>

bazel: remove glob from bzl test targets, use git ls-files instead

10daede

Signed-off-by: Divinesoumyadip <soumyacode7@gmail.com>

Conversation

Divinesoumyadip commented Mar 21, 2026

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

Divinesoumyadip Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

github-actions bot commented Mar 21, 2026

Uh oh!

Divinesoumyadip commented Mar 21, 2026

Uh oh!

oharboe commented Mar 21, 2026

Uh oh!

oharboe commented Mar 21, 2026

Uh oh!

Divinesoumyadip commented Mar 21, 2026

Uh oh!

luarss Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

luarss Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

luarss Mar 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Mar 22, 2026

Uh oh!

Divinesoumyadip commented Mar 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions bot commented Mar 22, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

luarss Mar 21, 2026 •

edited

Loading

Divinesoumyadip commented Mar 22, 2026 •

edited

Loading