Bazel: Add unified hermetic:tidy target for workspace formatting

[alokkumardalei-wq]

What does this PR resolves ?

Fixes #8495
Adds a unified bazelisk run :tidy target to formatting the entire codebase consistently without requiring developers to manage system-level tool installations. This significantly simplifies onboarding and standardizes code style checks.

What are key changes made??

• C++: Leverages the Bazel-managed LLVM toolchain (@llvm_toolchain_llvm//:bin/clang-format) for hermetic C++ formatting.
  Bazel: Added buildifier_prebuilt to
• MODULE.bazel to format BUILDand .bzl files automatically.
• Python / Tcl: Locked the black and tclint pip dependencies in bazel/requirements.in.
• Engine: Created etc/tidy.sh wrapper and exposed it via an alias in the root BUILD.bazel for native workspace traversal.

Testing/ Verification

bazelisk run :tidy

Expected output:

INFO: Invocation ID: ef4b208c-5209-43ad-9fc7-d5796ab22311
INFO: Analyzed target //:tidy (132 packages loaded, 11213 targets configured).
INFO: Found 1 target...
Target //etc:tidy up-to-date:
  bazel-bin/etc/tidy
INFO: Elapsed time: 0.545s, Critical Path: 0.19s
INFO: 1 process: 13 action cache hit, 1 internal.
INFO: Build completed successfully, 1 total action
INFO: Running command line: bazel-bin/etc/tidy ../toolchains_llvm++llvm+llvm_toolchain_llvm/bin/clang-format ../buildifier_prebuilt+/buildifier/buildifier etc/black_wrapper etc/tclint_wrapper
Starting OpenROAD codebase tidy...
[1/4] Formatting C++ files...
[2/4] Formatting Bazel files...
[3/4] Formatting Python files...
All done! ✨ 🍰 ✨
417 files left unchanged.

[gemini-code-assist]

Code Review

This pull request introduces a new tidy script and associated Bazel infrastructure to automate code formatting and linting for C++, Bazel, Python, and Tcl files, by adding buildifier_prebuilt, black, and tclint as dependencies. The bazel/install.sh script's default installation path was updated, and several minor C++ formatting adjustments were made. Review comments suggest improving the tidy.sh script by handling tclint errors more robustly instead of suppressing all of them, simplifying the tool path resolution logic, and making the install.sh destination directory a named constant for better maintainability. Additionally, a minor stylistic inconsistency in a C++ parameter declaration was noted.

[gemini-code-assist]

The || true at the end of the tclint command suppresses any errors from tclint. It would be better to check the exit code of tclint and only suppress the error if it's a specific, non-critical error (e.g., a style violation). Otherwise, a genuine error could be masked.

Suggested change

	if command -v "$TCLINT" &> /dev/null || [ -f "$TCLINT" ]; then
	"$TCLINT" src etc tools test || true
	"$TCLINT" src etc tools test
	TCLINT_STATUS=$?
	if [ $TCLINT_STATUS -ne 0 ]; then
	echo "Tclint found errors, but continuing anyway."
	fi

[gemini-code-assist]

The destination directory is hardcoded. Consider defining this as a named constant to improve maintainability and allow for easier customization.

[gemini-code-assist]

The script checks if the tools are not absolute paths and not equal to the tool names, and then prepends $PWD. This logic can be simplified by checking if the tool is an absolute path, and if not, using command -v to resolve the tool path. This approach would be more robust and easier to understand.

Suggested change

	if [[ "$CLANG_FORMAT" != /* ]] && [[ "$CLANG_FORMAT" != "clang-format" ]]; then
	CLANG_FORMAT="$PWD/$CLANG_FORMAT"
	fi
	if [[ "$BUILDIFIER" != /* ]] && [[ "$BUILDIFIER" != "buildifier" ]]; then
	BUILDIFIER="$PWD/$BUILDIFIER"
	fi
	if [[ "$BLACK" != /* ]] && [[ "$BLACK" != "black" ]]; then
	BLACK="$PWD/$BLACK"
	fi
	if [[ "$TCLINT" != /* ]] && [[ "$TCLINT" != "tclint" ]]; then
	TCLINT="$PWD/$TCLINT"
	fi
	if ! [[ "$CLANG_FORMAT" == /* ]]; then
	CLANG_FORMAT=$(command -v "$CLANG_FORMAT")
	fi
	if ! [[ "$BUILDIFIER" == /* ]]; then
	BUILDIFIER=$(command -v "$BUILDIFIER")
	fi
	if ! [[ "$BLACK" == /* ]]; then
	BLACK=$(command -v "$BLACK")
	fi
	if ! [[ "$TCLINT" == /* ]]; then
	TCLINT=$(command -v "$TCLINT")
	fi

[gemini-code-assist]

There's a typo in the parameter name T_impl::*module_field. It should be T_impl::* module_field to be consistent with the naming convention used in the other parameter.

    dbId<_dbModule> T_impl::* module_field,

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[github-actions]

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

[maliberty]

@oharboe does this add anything beyond your linting PRs?

[oharboe]

@alokkumardalei-wq nice work getting multiple formatters wired up under Bazel —
that's the right direction. This PR bundles four tools (clang-format, buildifier,
black, tclint) behind a single shell script, which makes it hard to review and
merge. To get something landed quickly, I'd suggest reworking this into a focused
buildifier-only PR, then following up with black in a second.

Why buildifier first

• BUILD/bzl files are edited frequently and formatting isn't enforced in CI yet.
• The buildifier_prebuilt dep you added in MODULE.bazel is the right approach —
  that's the hard part done.
• A focused buildifier PR is small, easy to review, and immediately useful.

Reference implementation

bazel-orfs already has a
working buildifier + black setup you can use as a reference:

• MODULE.bazel —
  buildifier_prebuilt v6.4.0 as dev_dependency (same version you picked)
• BUILD —
  sh_binary target with @buildifier_prebuilt//:buildifier as data dep
• fix_lint.sh —
  resolves buildifier from Bazel runfiles, runs on files changed since origin/main,
  includes bazelisk mod tidy for lock file regeneration

Key patterns from bazel-orfs worth reusing:

• Runfiles-based tool resolution ($RUNFILES/buildifier_prebuilt+/buildifier/buildifier)
  instead of the resolve_tool() helper in your etc/tidy.sh
• Only formatting files changed since origin/main (via git diff --name-only)
  instead of the whole repo — much faster for developers

What's already in place in OpenROAD

PRs #9734 and #9856 merged a Bazel lint framework with these entry points:

bazelisk test //:lint_test   # umbrella: all lint checks
bazelisk run  //:fix_lint    # umbrella: auto-fix

Currently it covers TCL only (tclint + tclfmt). The design is documented in
docs/contrib/LintTargets.md and explicitly set up for extension — each new
linter is a sh_test (check) + sh_binary (fix) pair added to the umbrella
targets. That way Bazel handles tool paths, caching, and parallelism natively.

What to do for this PR

Keep: buildifier as a Bazel lint target

Following the pattern in BUILD.bazel (lines 442-493) and the bazel-orfs reference:

• Create bazel/buildifier_test.sh — runs buildifier -lint warn -mode check
• Create bazel/buildifier_fix.sh — runs buildifier -lint fix -mode fix
• Add sh_test and sh_binary targets in root BUILD.bazel
• Add the test to //:lint_test, the binary to //:fix_lint
• Keep the buildifier_prebuilt dep in MODULE.bazel

Drop from this PR

Component	Why
etc/tidy.sh orchestrator	Bazel umbrellas already handle this
etc/BUILD targets	Targets go in root BUILD.bazel
etc/*_wrapper.py	py_console_script_binary handles entry points
tclint changes	Already merged in PR #9734 (your lock file also downgrades tclint from 0.7.0 → 0.2.2, which is a regression)
clang-format	Separate concern, separate PR
black / Python formatting	Good follow-up PR using the same pattern (bazel-orfs fix_lint.sh shows how)

Follow-up PR

After buildifier lands, add black as a second PR using the same
sh_test/sh_binary pattern. bazel-orfs fix_lint.sh already demonstrates
black integration via the same runfiles approach.

Let me know if you have questions — happy to help work through the details.

[github-actions]

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

[github-actions]

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

[alokkumardalei-wq]

Thanks @oharboe for the detailed review and guidance!

You were totally right—the monolithic script was getting out of hand. I've completely wiped the massive 600-file diff from this branch and stripped out tidy.sh and the other formatters so this PR is now exclusively focused on buildifier.

I also wired bazel/buildifier_test.sh and bazel/buildifier_fix.sh
directly into your native //:lint_test and //:fix_lint umbrellas in BUILD.bazel
, adapting the exact same runfiles architecture you set up in bazel-orfs. Everything runs natively now!

I'll open a fresh, isolated follow-up PR for the black Python formatter using this exact same native Bazel pattern!
and very soon notify you here.

[oharboe]

Good progress stripping this down to buildifier-only — that took discipline and was the right call.

You're close, but the implementation has drifted from the spec. Please read #9858 carefully — it defines exactly what targets are needed and what they should be called. Here's what needs to change:

1. Naming: use bzl, not buildifier

The naming convention uses language names, not tool names. Look at TCL: targets are lint_tcl_test, fmt_tcl_test, tidy_tcl — not lint_tclint_test. Same principle here:

Current	Should be
lint_buildifier_test	lint_bzl_test
tidy_buildifier	tidy_bzl
buildifier_test.sh	bzl_lint_test.sh
buildifier_fix.sh	bzl_tidy.sh

2. Missing fmt_bzl_test target

Issue #9858 specifies three targets, matching the TCL pattern:

Target	Command
//:lint_bzl_test	buildifier -mode=check -lint=warn
//:fmt_bzl_test	buildifier -mode=check -lint=off
//:tidy_bzl	buildifier -mode=fix -lint=fix

The format-only check (-lint=off) separates "your indentation is wrong" from "you have a lint warning" — same reason TCL has both lint_tcl_test and fmt_tcl_test. Look at how tcl_lint_test.sh, tcl_fmt_test.sh, and tcl_tidy.sh are structured and mirror that pattern.

3. Drop the src/sta submodule pointer change — and never touch src/sta

Your diff includes a src/sta submodule change (35aef386d6 → 2d7fc5c82b). Revert it. src/sta is the OpenSTA submodule — it is managed upstream and changes to it require careful coordination. As a rule: never include src/sta changes in a PR unless that PR is specifically about updating OpenSTA.

This also matters for your buildifier scripts: src/sta/BUILD exists and must not be reformatted by your scripts. Your scripts must exclude submodule paths. The simplest approach is to filter out paths listed in .gitmodules (currently src/sta and third-party/abc), or hard-code the exclusions. The bazel-orfs fix_lint.sh shows how to filter .bazelignore paths — adapt that pattern.

4. Add buildifier: disable comments

The existing TCL targets have # buildifier: disable=native-sh-test and # buildifier: disable=native-sh-binary comments. Add the same to your new targets — copy the pattern from the TCL section directly above yours in BUILD.bazel.

5. .buildifier.json — read it, your scripts must override it

There's a .buildifier.json at the repo root with mode: fix, lint: fix, warnings: all. Buildifier picks this up automatically. That means if your test scripts don't explicitly pass -mode=check, buildifier will fix files in place instead of just checking them — defeating the purpose of a read-only test. Your test scripts must pass explicit flags that override the config (-mode=check -lint=warn for lint, -mode=check -lint=off for fmt).

6. Update docs/contrib/LintTargets.md

Move buildifier from the "Planned additions" list to the "Available targets" table. Add all three new targets with descriptions.

7. Positional args in fix_lint.sh

The $5-$8 extension is fine for now — don't worry about it.

In short

I know this is a lot of feedback for what sounds like "just add a formatter." That's exactly the point — getting this right is shockingly complex compared to what it sounds like on the surface. Submodule exclusions, config file interactions, naming conventions, the split between read-only checks and auto-fix — there are real landmines here. But that complexity is precisely why this work is so valuable: once //:fix_lint handles buildifier correctly, every developer just runs one command and never has to think about any of this. You're absorbing the complexity so hundreds of contributors don't have to.

Take your time. The architecture is sound. The changes above are about matching the spec in #9858 and mirroring the TCL pattern exactly. Open BUILD.bazel at the TCL lint section (lines 442-479) and replicate that structure with bzl instead of tcl.

[github-actions]

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

[alokkumardalei-wq]

Good progress stripping this down to buildifier-only — that took discipline and was the right call.

You're close, but the implementation has drifted from the spec. Please read #9858 carefully — it defines exactly what targets are needed and what they should be called. Here's what needs to change:

1. Naming: use bzl, not buildifier

The naming convention uses language names, not tool names. Look at TCL: targets are lint_tcl_test, fmt_tcl_test, tidy_tcl — not lint_tclint_test. Same principle here:

Current Should be
lint_buildifier_test lint_bzl_test
tidy_buildifier tidy_bzl
buildifier_test.sh bzl_lint_test.sh
buildifier_fix.sh bzl_tidy.sh

2. Missing fmt_bzl_test target

Issue #9858 specifies three targets, matching the TCL pattern:

Target Command
//:lint_bzl_test buildifier -mode=check -lint=warn
//:fmt_bzl_test buildifier -mode=check -lint=off
//:tidy_bzl buildifier -mode=fix -lint=fix
The format-only check (-lint=off) separates "your indentation is wrong" from "you have a lint warning" — same reason TCL has both lint_tcl_test and fmt_tcl_test. Look at how tcl_lint_test.sh, tcl_fmt_test.sh, and tcl_tidy.sh are structured and mirror that pattern.

3. Drop the src/sta submodule pointer change — and never touch src/sta

Your diff includes a src/sta submodule change (35aef386d6 → 2d7fc5c82b). Revert it. src/sta is the OpenSTA submodule — it is managed upstream and changes to it require careful coordination. As a rule: never include src/sta changes in a PR unless that PR is specifically about updating OpenSTA.

This also matters for your buildifier scripts: src/sta/BUILD exists and must not be reformatted by your scripts. Your scripts must exclude submodule paths. The simplest approach is to filter out paths listed in .gitmodules (currently src/sta and third-party/abc), or hard-code the exclusions. The bazel-orfs fix_lint.sh shows how to filter .bazelignore paths — adapt that pattern.

4. Add buildifier: disable comments

The existing TCL targets have # buildifier: disable=native-sh-test and # buildifier: disable=native-sh-binary comments. Add the same to your new targets — copy the pattern from the TCL section directly above yours in BUILD.bazel.

5. .buildifier.json — read it, your scripts must override it

There's a .buildifier.json at the repo root with mode: fix, lint: fix, warnings: all. Buildifier picks this up automatically. That means if your test scripts don't explicitly pass -mode=check, buildifier will fix files in place instead of just checking them — defeating the purpose of a read-only test. Your test scripts must pass explicit flags that override the config (-mode=check -lint=warn for lint, -mode=check -lint=off for fmt).

6. Update docs/contrib/LintTargets.md

Move buildifier from the "Planned additions" list to the "Available targets" table. Add all three new targets with descriptions.

7. Positional args in fix_lint.sh

The $5-$8 extension is fine for now — don't worry about it.

In short

I know this is a lot of feedback for what sounds like "just add a formatter." That's exactly the point — getting this right is shockingly complex compared to what it sounds like on the surface. Submodule exclusions, config file interactions, naming conventions, the split between read-only checks and auto-fix — there are real landmines here. But that complexity is precisely why this work is so valuable: once //:fix_lint handles buildifier correctly, every developer just runs one command and never has to think about any of this. You're absorbing the complexity so hundreds of contributors don't have to.

Take your time. The architecture is sound. The changes above are about matching the spec in #9858 and mirroring the TCL pattern exactly. Open BUILD.bazel at the TCL lint section (lines 442-479) and replicate that structure with bzl instead of tcl.

Hello @oharboe thank you for the feedback again I will definitely work on this and actually I am very happy to address this , and I shall ask any doubts and issues if I faced while working here and I will notify you very soon here with your suggested changes.

[github-actions]

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

[alokkumardalei-wq]

Hello @oharboe thank you so much for your time to consider this and I have addressed all 7 points from your review in the commit 75aac70:

1. Naming - renamed everything to bzl convention: lint_bzl_test, fmt_bzl_test, tidy_bzl for targets, and bzl_lint_test.sh, bzl_fmt_test.sh, bzl_tidy.sh for scripts.

2. Missing fmt_bzl_test - added the third target with -mode=check -lint=off, so format and lint checks are separate. All three now mirror the TCL pattern exactly.

3. src/sta submodule - dropped completely. Also rewrote scripts to use git ls-files for file discovery (same as the TCL scripts), which naturally excludes submodule paths like src/sta/BUILD and third-party/abc/ — no hardcoded exclusion list needed.

4. buildifier: disable comments - added # buildifier: disable=native-sh-test and # buildifier: disable=native-sh-binary on all new targets, matching the TCL section above.

5. .buildifier.json override - both test scripts now explicitly pass -mode=check so they stay read-only regardless of the repo-root config (mode: fix). Lint test uses -lint=warn, format test uses -lint=off.

6. LintTargets.md- moved buildifier from "Planned additions" to "Available targets" table with all three targets and descriptions. Also added a note about how the test scripts override .buildifier.json.

7. Positional args - kept the $5–$8 pattern in fix_lint.sh, no changes there as you suggested.

Please have a look again when you have time and let me if anything I missed or need improvement.

Thank you !

[alokkumardalei-wq]

Hello @maliberty and @oharboe, all checks are passing please have look when you have time . I would be happy to address any feedback further.

Thank you !

[maliberty]

why 6.4.0 when the current is 8.5.1.2 ?

[maliberty]

@oharboe any further comments?

[github-actions]

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

[github-actions]

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

[alokkumardalei-wq]

Hello @maliberty and @oharboe , updated version from 6.4.0 to 8.5.1.2. Please have look when you have time .

Thank you !