Skip to content

bazel: add Python format lint target#10368

Open
naveenvenk17 wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
naveenvenk17:worker1-bazel-python-lint-9859
Open

bazel: add Python format lint target#10368
naveenvenk17 wants to merge 1 commit into
The-OpenROAD-Project:masterfrom
naveenvenk17:worker1-bazel-python-lint-9859

Conversation

@naveenvenk17
Copy link
Copy Markdown
Contributor

Fixes #9859.

Summary:

  • add Black to the Bazel-managed Python tool requirements
  • add //:fmt_py_test and //:tidy_py using Bazel's Black console script
  • wire Python formatting into //:lint_test and //:fix_lint

Notes:

  • this keeps the existing .github/workflows/black.yaml workflow in place, matching the concern in the issue discussion that PRs still need a formatting guardrail
  • the Bazel target checks Python files changed relative to OPENROAD_LINT_BASE_REF, origin/master, or master, and skips tracked symlinks so Windows worktrees do not feed symlink placeholder files to Black

Tests:

  • PASS: wsl bash -lc 'cd /mnt/d/startup/openroad-project/OpenROAD && npx --yes @bazel/bazelisk@latest run --workspace_status_command= //bazel:requirements.update'
  • PASS: wsl bash -lc 'cd /mnt/d/startup/openroad-project/OpenROAD && npx --yes @bazel/bazelisk@latest test --workspace_status_command= --test_output=errors //:fmt_py_test'
  • PASS: wsl bash -lc 'cd /mnt/d/startup/openroad-project/OpenROAD && npx --yes @bazel/bazelisk@latest run --workspace_status_command= //bazel:black -- --version'
  • PASS: repo-pinned buildifier check on BUILD.bazel and bazel/BUILD
  • PASS: bash -n for bazel/py_fmt_test.sh, bazel/py_tidy.sh, and bazel/fix_lint.sh
  • PASS: git diff --check
  • ATTEMPTED: //:lint_test in the Windows-backed WSL checkout; //:fmt_py_test passed, but existing full-tree Bazel/Tcl formatting checks reported CRLF/worktree formatting noise unrelated to this PR

@github-actions github-actions Bot added the size/M label May 9, 2026
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request integrates Python formatting and linting into the Bazel build system using the black tool. It introduces new targets for checking and fixing Python formatting, along with the necessary shell scripts and dependency updates. Review feedback focuses on improving the robustness of the scripts by supporting 'main' as a default branch and ensuring uncommitted changes are included in the formatting process to improve the local development experience.

Comment thread bazel/py_fmt_test.sh Outdated
cd "$WORKSPACE"

base_ref=""
for candidate in "${OPENROAD_LINT_BASE_REF:-}" origin/master master; do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Many repositories have transitioned from master to main as the default branch name. Adding origin/main and main to the candidate list ensures the linting logic works correctly in environments using the newer naming convention, preventing silent skips of the formatting checks.

Suggested change
for candidate in "${OPENROAD_LINT_BASE_REF:-}" origin/master master; do
for candidate in "${OPENROAD_LINT_BASE_REF:-}" origin/main main origin/master master; do

Comment thread bazel/py_fmt_test.sh
changed_files+=("$file")
done < <(
if [ -n "$base_ref" ]; then
git diff --name-only --diff-filter=d -z "$base_ref" HEAD -- "*.py"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Explicitly specifying HEAD as the second argument to git diff limits the check to committed changes only. Removing it allows the script to also identify and check uncommitted changes in the working tree and index, providing faster feedback during local development. Since $base_ref is determined using git merge-base, this will correctly identify all changes introduced by the current branch plus any pending work.

Suggested change
git diff --name-only --diff-filter=d -z "$base_ref" HEAD -- "*.py"
git diff --name-only --diff-filter=d -z "$base_ref" -- "*.py"

Comment thread bazel/py_tidy.sh Outdated
cd "${BUILD_WORKSPACE_DIRECTORY:-$PWD}"

base_ref=""
for candidate in "${OPENROAD_LINT_BASE_REF:-}" origin/master master; do
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Adding origin/main and main to the candidate list ensures the auto-formatting logic works correctly in repositories that use main as the default branch.

Suggested change
for candidate in "${OPENROAD_LINT_BASE_REF:-}" origin/master master; do
for candidate in "${OPENROAD_LINT_BASE_REF:-}" origin/main main origin/master master; do

Comment thread bazel/py_tidy.sh
changed_files+=("$file")
done < <(
if [ -n "$base_ref" ]; then
git diff --name-only --diff-filter=d -z "$base_ref" HEAD -- "*.py"
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

Removing the explicit HEAD argument allows black to format uncommitted changes in the working tree. This is typically the desired behavior when running a 'tidy' script locally before committing code.

Suggested change
git diff --name-only --diff-filter=d -z "$base_ref" HEAD -- "*.py"
git diff --name-only --diff-filter=d -z "$base_ref" -- "*.py"

Signed-off-by: naveenvenk17 <naveenvenkat1711@gmail.com>
@naveenvenk17 naveenvenk17 force-pushed the worker1-bazel-python-lint-9859 branch from d385144 to 73ccf19 Compare May 9, 2026 10:09
@naveenvenk17
Copy link
Copy Markdown
Contributor Author

Follow-up on Gemini feedback:

  • Applied the origin/main / main fallback in both Python lint scripts in 73ccf198.
  • Kept the explicit HEAD in the git diff $base_ref HEAD calls intentionally. In this Windows-backed WSL checkout, git diff -- *.py reports untouched Python files as dirty due local line-ending conversion, which makes //:fmt_py_test check unrelated files and fail for environment noise. The Bazel test is therefore scoped to committed branch diffs; CI/local callers can still choose the base with OPENROAD_LINT_BASE_REF.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature Request: Migrate Python Linting to Bazel Lint Umbrella

1 participant