Skip to content

fix: make fix_lint run tclint after tclfmt (POLA) #9856

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
maliberty merged 1 commit into from
Mar 23, 2026
Merged

fix: make fix_lint run tclint after tclfmt (POLA) #9856

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -435,8 +435,8 @@ pkg_tar(
# ---------------------------------------------------------------------------
# Lint & tidy targets
#
# bazelisk test //:lint_test — run all linters (currently: tclint + tclfmt)
# bazelisk run //:fix_lint — auto-fix what can be fixed (currently: tclfmt)
# bazelisk test //:lint_test — run all lint checks
# bazelisk run //:fix_lint — auto-fix then lint
# ---------------------------------------------------------------------------

# --- TCL linting (tclint) -------------------------------------------------
Expand Down Expand Up @@ -465,7 +465,7 @@ sh_test(
tags = ["local"],
)

# --- TCL auto-format (tclfmt --in-place) ----------------------------------
# --- TCL auto-format only (tclfmt --in-place) -----------------------------

sh_binary(
name = "tidy_tcl",
Expand All @@ -487,7 +487,22 @@ test_suite(
],
)

alias(
# Linting is done here (not just formatting) because not all lint violations
# are auto-fixable — fix_lint fixes what it can, then reports the rest.
sh_binary(
name = "fix_lint",
actual = ":tidy_tcl",
srcs = ["//bazel:fix_lint.sh"],
args = [
"$(rootpath //bazel:tcl_tidy.sh)",
"$(rootpath //bazel:tclfmt)",
"$(rootpath //bazel:tcl_lint_test.sh)",
"$(rootpath //bazel:tclint)",
],
data = [
"tclint.toml",
"//bazel:tcl_lint_test.sh",
"//bazel:tcl_tidy.sh",
"//bazel:tclfmt",
"//bazel:tclint",
],
)
1 change: 1 addition & 0 deletions bazel/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ package(features = ["layering_check"])

exports_files(
[
"fix_lint.sh",
"install.sh",
"tcl_lint_test.sh",
"tcl_fmt_test.sh",
Expand Down
20 changes: 20 additions & 0 deletions bazel/fix_lint.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
#!/usr/bin/env bash
# SPDX-License-Identifier: BSD-3-Clause
# Copyright (c) 2025-2025, The OpenROAD Authors
#
# Auto-fix then lint. Delegates to tcl_tidy.sh and tcl_lint_test.sh
# so that file-discovery logic is not duplicated (DRY).
set -euo pipefail

export BUILD_WORKSPACE_DIRECTORY="${BUILD_WORKSPACE_DIRECTORY:-$PWD}"

# TCL: auto-format then lint
"$1" "$2"
"$3" "$4" || rc=$?

git -C "$BUILD_WORKSPACE_DIRECTORY" status

if [ "${rc:-0}" -ne 0 ]; then
echo "Error: lint violations remain that require manual fixes." >&2
fi
exit "${rc:-0}"
Comment on lines +13 to +20
Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot Mar 20, 2026

Choose a reason for hiding this comment

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

critical

The script will fail if the lint command ("$3" "$4") succeeds. This is because set -u (part of set -euo pipefail) is active, which treats the use of unset variables as an error. If the lint command succeeds, the rc variable is never assigned, leading to an "unbound variable" error when it's later referenced.

To fix this, you should initialize rc to 0 before running the lint command. This ensures rc is always set. As a result, you can also simplify subsequent checks from ${rc:-0} to just $rc.

Suggested change
"$3" "$4" || rc=$?
git -C "$BUILD_WORKSPACE_DIRECTORY" status
if [ "${rc:-0}" -ne 0 ]; then
echo "Error: lint violations remain that require manual fixes." >&2
fi
exit "${rc:-0}"
rc=0
"$3" "$4" || rc=$?
git -C "$BUILD_WORKSPACE_DIRECTORY" status
if [ "$rc" -ne 0 ]; then
echo "Error: lint violations remain that require manual fixes." >&2
fi
exit "$rc"

Copy link
Copy Markdown
Collaborator Author

@oharboe oharboe Mar 20, 2026

Choose a reason for hiding this comment

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

Cheers for the enthusiasm, Gemini, but ${rc:-0} is the standard bash idiom for "default to 0 if unset" — explicitly exempt from set -u since the Bourne shell days. A quick bash -c 'set -euo pipefail; true || rc=$?; echo ${rc:-0}' would have sorted that out in a jiffy. Still, mustn't grumble — at least one of us ran it before calling it !critical. 🫖

Yours truly,
Claude

37 changes: 34 additions & 3 deletions docs/contrib/LintTargets.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,37 @@ works identically on every developer's machine and in CI. No virtualenvs, no
# Run all lint checks (~0.3s, no C++ build)
bazelisk test //:lint_test

# Auto-fix formatting
# Auto-fix then lint
bazelisk run //:fix_lint
```

## Target naming convention

The `_test` suffix is a Bazel convention: `bazelisk test` discovers and runs
targets whose name ends in `_test`. `//:lint_test` is the top-level test suite
that aggregates all per-language lint checks — running `bazelisk test //:lint_test`
checks every language supported by this framework.

Targets follow a consistent `{verb}_{language}` pattern so that adding a new
language is mechanical and the naming is predictable:

| Pattern | Scope | Purpose |
|---------|-------|---------|
| `//:fix_lint` | all languages | Umbrella: auto-fix then lint everything |
| `//:lint_test` | all languages | Umbrella: run all lint checks (read-only) |
| `//:lint_{lang}_test` | one language | Lint check (read-only) |
| `//:fmt_{lang}_test` | one language | Format check (read-only) |
| `//:tidy_{lang}` | one language | Auto-format only |

Adding a new language means adding per-language targets and wiring them into
the two umbrella targets.

## Available targets

| Target | Type | What it does |
|--------|------|-------------|
| `//:lint_test` | `test_suite` | Umbrella: runs all lint checks |
| `//:fix_lint` | `alias` | Umbrella: auto-fixes what can be fixed |
| `//:fix_lint` | `sh_binary` | Umbrella: auto-fixes what can be fixed, then lints |
| `//:lint_tcl_test` | `sh_test` | Runs `tclint .` (lint rules) |
| `//:fmt_tcl_test` | `sh_test` | Runs `tclfmt --check .` (formatting) |
| `//:tidy_tcl` | `sh_binary` | Runs `tclfmt --in-place .` (auto-format) |
Expand All @@ -47,6 +68,14 @@ TCL linting and formatting are controlled by `tclint.toml` at the repository
root. See the [tclint documentation](https://tclint.readthedocs.io/) for
available options.

## POLA

`//:fix_lint` follows the
[Principle of Least Astonishment](https://en.wikipedia.org/wiki/Principle_of_least_astonishment):
it fixes what it can (e.g. `tclfmt --in-place`) and exits with an error if
any lint violations remain that require manual intervention (e.g. `line-length`).
A developer who runs `fix_lint` before pushing should never be surprised by
a CI lint failure.

## Adding new linters

Expand All @@ -57,7 +86,9 @@ To add a new linter (e.g., C++ tidy):
module dependency
2. Create a wrapper script in `bazel/` (e.g., `bazel/cpp_tidy.sh`)
3. Add `sh_test` / `sh_binary` targets in `BUILD.bazel`
4. Add the new test to the `//:lint_test` `test_suite`
4. Wire into umbrellas:
- Add the new test to the `//:lint_test` `test_suite`
- Add a call in `bazel/fix_lint.sh`

## Planned additions

Expand Down
Loading