Formatting tests error out because they don't find `git`

[hzeller]

Now that soon ... to test should work I noticed that the formatting tests don't work yet: they error out as they seem to call git.
@oharboe since you just added that in another context, maybe it is quick to do here as well

bazel test -c opt --test_output=errors //:fmt_bzl_test //:fmt_tcl_test //:lint_bzl_test //:lint_tcl_test
INFO: Invocation ID: 0863b6e3-ba12-4a53-a942-f74c9269678d
INFO: Analyzed 4 targets (0 packages loaded, 0 targets configured).
INFO: Found 4 test targets...
INFO: Elapsed time: 0.955s, Critical Path: 0.02s
INFO: 1 process: 4 action cache hit, 1 internal.
INFO: Build completed, 4 tests FAILED, 1 total action
FAILED: //:fmt_bzl_test (see /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/fmt_bzl_test/test.log)
INFO: From Testing //:fmt_bzl_test
==================== Test output for //:fmt_bzl_test:
/home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/bin/fmt_bzl_test.runfiles/_main/fmt_bzl_test: line 17: git: command not found
================================================================================
FAILED: //:fmt_tcl_test (see /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/fmt_tcl_test/test.log)
INFO: From Testing //:fmt_tcl_test
==================== Test output for //:fmt_tcl_test:
/home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/bin/fmt_tcl_test.runfiles/_main/fmt_tcl_test: line 10: git: command not found
usage: tclfmt [-h] [--version] [--in-place | --check] [-d] [-c <path>]
              [--partial] [--exclude "path1, path2, ..."]
              [--extend-exclude "path1, path2, ..."]
              [--extensions "tcl, xdc, ..."] [--commands <path>]
              [--indent <indent>] [--max-blank-lines <max_blank_lines>]
              [--indent-namespace-eval | --no-indent-namespace-eval]
              [--spaces-in-braces | --no-spaces-in-braces]
              source [source ...]
tclfmt: error: the following arguments are required: source
================================================================================
FAILED: //:lint_bzl_test (see /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/lint_bzl_test/test.log)
INFO: From Testing //:lint_bzl_test
==================== Test output for //:lint_bzl_test:
/home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/bin/lint_bzl_test.runfiles/_main/lint_bzl_test: line 17: git: command not found
================================================================================
FAILED: //:lint_tcl_test (see /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/lint_tcl_test/test.log)
INFO: From Testing //:lint_tcl_test
==================== Test output for //:lint_tcl_test:
/home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/bin/lint_tcl_test.runfiles/_main/lint_tcl_test: line 10: git: command not found
usage: tclint [-h] [--version] [-d] [-c <path>] [--ignore "rule1, rule2, ..."]
              [--extend-ignore "rule1, rule2, ..."]
              [--exclude "path1, path2, ..."]
              [--extend-exclude "path1, path2, ..."]
              [--extensions "tcl, xdc, ..."] [--commands <path>]
              [--style-line-length <line_length>]
              source [source ...]
tclint: error: the following arguments are required: source
================================================================================
//:fmt_bzl_test                                                 (cached) FAILED in 0.1s
  /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/fmt_bzl_test/test.log
//:fmt_tcl_test                                                 (cached) FAILED in 0.7s
  /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/fmt_tcl_test/test.log
//:lint_bzl_test                                                (cached) FAILED in 0.1s
  /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/lint_bzl_test/test.log
//:lint_tcl_test                                                (cached) FAILED in 0.6s
  /home/hzeller/.cache/bazel/_bazel_hzeller/515c003e8af37d4c8564c2613da4cc17/execroot/_main/bazel-out/k8-opt/testlogs/lint_tcl_test/test.log

[oharboe]

@maliberty isnt this moot if you autoformat and lint post merge?

[maliberty]

Why would these need git? Formatting has no relation to git.

[hzeller]

Yeah, I was also surprised. I assume they use git to figure out all changed files ?
(I would just run the formatter over all files regardless)

[oharboe]

Claude plan to fix this. If we want Claude to fix this, then someone has to parse this.

decouple fix_lint from git ls-files

Problem

fix_lint (root BUILD.bazel:171-185) drives four shell scripts in bazel/:

• bazel/tcl_tidy.sh:9 — git ls-files '*.tcl'
• bazel/tcl_lint_test.sh:10 — git ls-files '*.tcl' '*.sdc' '*.upf'
• bazel/bzl_tidy.sh:11-12 — git ls-files '*.bazel' '*.bzl' '**/BUILD' 'BUILD' '**/WORKSPACE' 'WORKSPACE'
• bazel/bzl_lint_test.sh:17-18 — same patterns

All four enumerate sources via git ls-files. That's a Bazel target reaching out to git to discover its inputs. Three concrete consequences:

1. Doesn't run on a source tarball.
2. Doesn't run in a Nix sandbox or any minimal CI image without git.
3. Result depends on working-tree state (untracked files, submodule init), not on what's actually present on disk. Two contributors with the same files can get different lint sets.

The submodule-skipping side effect of git ls-files is the only reason src/sta/ and third-party/abc/ aren't reformatted today. That's load-bearing but implicit. CLAUDE.md rule 1 says don't touch src/sta/; nothing in the build enforces it. Remove git ls-files and the rule becomes enforceable in code.

Goal

bazelisk run //:fix_lint works without git on PATH. Submodules and other upstream-owned trees stay excluded by an explicit list in the build, not by an accident of VCS.

Alternatives

Listed worst to best for this codebase. All seven were considered.

A. glob() in BUILD.bazel

Aggregate sources with glob(['**/*.tcl']) in the root BUILD, pass as runfiles.

Doesn't work. Bazel glob() doesn't cross package boundaries. We have BUILD.bazel in most src/<module>/ directories; a root-level glob can't see into them. Fixing it means adding filegroups in every package and aggregating via deps. Large mechanical change, ongoing maintenance, and easy to forget when adding a new package.

B. Aspect over existing cc_library / tcl_library srcs

Walk the dep graph, collect declared sources.

Doesn't work for our file mix. Many .tcl files are not declared in any target (test fixtures, ORFS shims, doc examples, helpers). Aspect silently misses them and lint coverage shrinks.

C. bazel query 'kind("source file", deps(//...))'

Same problem as B — only sees declared files — plus bazel-in-bazel ergonomics.

D. Generate list at edit time, check it in

tools/refresh_sources.py writes sources.bzl; build reads the static list. Refresh requires git, build doesn't.

Works, but the list now drifts every time someone adds a file and forgets to refresh. Pre-commit hooks paper over it but don't fix it. Extra step in every contributor's workflow for no real gain over (E) or (F).

E. Optional git, fallback inside the lint scripts

if command -v git >/dev/null && [ -d .git ]; then
  git ls-files '*.tcl'
else
  find . -name '*.tcl' ...
fi

Two code paths. Two sets of bugs. The fallback is the interesting case (it's what runs in the broken-without-git environment) and it's also the path almost nobody exercises locally. Don't ship two implementations of the same enumeration.

F. Accept a file list as args, fall back to walking the tree

bazelisk run //:fix_lint -- $(git ls-files '*.tcl') if you want git's view, plain bazelisk run //:fix_lint if you don't. The build target itself takes a list and runs the linter on it; producing the list is the caller's problem.

Architecturally clean — separation between "what to lint" and "how to lint" — and matches the Bazel philosophy that drove --workspace_status_command for stamping. With no args, fall back to walking the tree (option G's mechanism).

Two downsides for this repo:

1. Changes the contract. Today CI runs bazelisk run //:fix_lint with no args. Either CI keeps doing that and uses the fallback (in which case F reduces to G), or CI starts piping git ls-files into the run (in which case the simple invocation regresses). The win only materializes if downstream callers actually pass a list — and we have no callers that want to.
2. Bazel arg passing through sh_binary runfiles is fiddly when the list is large. Workable, but more rope.

Worth keeping in mind as a future extension once a caller actually wants to lint a subset. Not the right primary mechanism today.

G. find with explicit exclusions, in one helper

A single bazel/list_sources.sh that wraps find with the prune list:

find . \
  \( -path './.git' -o \
     -path './src/sta' -o \
     -path './third-party/abc' -o \
     -name 'bazel-*' -o \
     -path './build' \) -prune -o \
  -type f \( <patterns> \) -print

Four lint scripts call it instead of git ls-files. Submodule exclusion becomes data — a list at the top of the helper with a comment pointing at .gitmodules as the source of truth. Adding a submodule means adding a line. The mistake is loud: lint touches upstream files, diff is obvious.

This is the minimum-disruption replacement. POSIX find is everywhere Bazel runs. No new tooling. No new BUILD-file ceremony. The semantics that matter (skip submodules, skip output trees, skip .git) are stated once, in code, next to the comment that explains them.

Recommendation

Ship G now. Keep F in mind as a follow-up if a caller ever wants to pass a subset — it's a strict superset of G (G is the no-arg path of F). Don't ship E. Don't ship D. A and B/C don't fit this codebase.

Out of scope

• tools/workspace_status.sh (git describe for STABLE_GIT_VERSION). That's the Bazel-blessed git escape hatch via --workspace_status_command; --nostamp disables it. Separate concern.
• Wiring C++ clang-format into fix_lint. No C++ lint runs today.
• The *.i / src/sta exclusion rule from CLAUDE.md — relevant only once C++ lint exists.

Files affected (for reviewer orientation, not a checklist)

• bazel/list_sources.sh — new, the find wrapper
• bazel/tcl_tidy.sh, bazel/tcl_lint_test.sh, bazel/bzl_tidy.sh, bazel/bzl_lint_test.sh — call the helper
• BUILD.bazel:171-185 — helper into data / runfiles of the four sh_binary targets
• bazel/fix_lint.sh:19 — guard the trailing git status (command -v git >/dev/null && git status || true) so the wrapper degrades on git-less environments

Verification

1. diff <(git ls-files '*.tcl' | sort) <(bazel/list_sources.sh '*.tcl' | sort) — empty in a normal checkout. Investigate any divergence.
2. bazel/list_sources.sh '*' | grep -E '^\./(src/sta|third-party/abc)/' — empty.
3. bazelisk run //:fix_lint on a clean tree: no diff. On a deliberately misformatted file: fixes it.
4. Extract the repo to a directory with no .git, submodules present. bazelisk run //:fix_lint works. This is the regression the change fixes.
5. Existing CI passes without CI-side changes.

[hzeller]

find in suggestion (G) sounds like the most natural but the suggested syntax with path pruning sounds fragile. I'd just start with the paths we're interested in (src, test, bazel) and the filter the file types we're interested in and then do pruning with a grep (which in our case is only src/sta)

e.g.

# for c++ type files
find src bazel test -type f -name "*.cc" -o -name "*.cpp" -o -name "*.cxx'" -o -name "*.inl" \
            -name "*.h"  -o -name "*.hh" -o -name "*.hpp" -o -name "*.hxx" \
            | grep -v ^src/sta
# for tcl files
find src bazel test -name "*.tcl" | grep -v ^src/sta

[hzeller]

(is the other place where git was needed also using git ls-files ? That should probably also be replaced with a bazel glob or a find.

Remember when people download a tarball of OpenROAD, they will not have git directories, so git will not yield anything.

[maliberty]

Formatting is a developer activity not a user one. Not having git would be odd for a developer. I am ok to use git for this purpose. clang-format could be run on everything as it is quite fast but clang-tidy is too slow for that. I think we will need to discover the modified files and I don't see another method.

[oharboe]

tarballs are idiomatic way to comply with gpl and POLA, so I think build should work wo git.

[maliberty]

This is about formatting not building.

[oharboe]

This is about formatting not building.

True.

I think bazel has the information, which files to format, but lacks a way to use it for formatting(developer) whereas for building things are much more established.

bazel handles build dependencies and we have decided that we want it to handle developer dependencies too(linting/formatting w dependencies and pre PR testing/fixing hell).

These days I dont care about formatting, linting or AI code improvements suggestions. I am just trying to whackamole it.

I also care less about if nits are fixed pre or post merge. Claude does it. All I do is to point Claude to the PR and ask it to fix it. 95% of the time it gets it right. 95%+ of the time it gets it right on a second try with some more feedback from others commenting on the PR.

@hzeller I feel like I am "sharing password" to my Claude a la sharing password to Netflix in the family :-) I think this can be a good thing, not everybody needs or wants a Claude account. Some of my colleagues prefer "hipster coding", work on what Claude cant do and let others interact with Claude. This works great. There are some tough problems that require quiet thinking time, not whackamoling. Algorithmic improvements. Knuth said he wanted to be at the bottom of things and ditched email.

Perhaps Claude will refuse to fix requests/nits that are not from the same household in the future. :-)

[maliberty]

I think bazel knows which files to build but not which files to format. You need to know which are modified relative to what is committed.