fix: make fix_lint run tclint after tclfmt (POLA)#9856
fix: make fix_lint run tclint after tclfmt (POLA)#9856oharboe wants to merge 1 commit intoThe-OpenROAD-Project:masterfrom
Conversation
fix_lint was an alias for tidy_tcl (tclfmt only), so developers who ran it before pushing were still surprised by tclint CI failures like line-length. Replace the alias with a script that delegates to tcl_tidy.sh and tcl_lint_test.sh (DRY), shows git status, and exits with a clear error if lint violations remain unfixed. Update docs with POLA rationale and naming convention for umbrella and per-language targets. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Øyvind Harboe <oyvind.harboe@zylin.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
Code Review
This pull request correctly addresses an issue where fix_lint only performed formatting and not linting, by replacing the Bazel alias with a shell script that runs both steps. The accompanying documentation updates are clear and helpful. However, I've found a critical bug in the new bazel/fix_lint.sh script: it will fail with an 'unbound variable' error if the linting step is successful. This is due to set -u being active. I've provided a code suggestion to resolve this issue.
| "$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}" |
There was a problem hiding this comment.
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.
| "$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" |
There was a problem hiding this comment.
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
fix_lint was an alias for tidy_tcl (tclfmt only), so developers who ran it before pushing were still surprised by tclint CI failures like line-length.
Replace the alias with a script that delegates to tcl_tidy.sh and tcl_lint_test.sh (DRY), shows git status, and exits with a clear error if lint violations remain unfixed.
Update docs with POLA rationale and naming convention for umbrella and per-language targets.