Fix lint source discovery without git#10357
Conversation
There was a problem hiding this comment.
Welcome to OpenROAD! Thanks for opening your first PR.
Before we review:
- Contribution Guide: https://openroad.readthedocs.io/en/latest/contrib/contributing.html
- Build Instructions: https://openroad.readthedocs.io/en/latest/contrib/BuildWithCMake.html
Please ensure:
- CI passes
- Code is properly formatted
- Tests are included where applicable
A maintainer will review shortly!
There was a problem hiding this comment.
Code Review
This pull request replaces the dependency on git ls-files for identifying source files in linting and formatting scripts with a custom list_sources.sh script. This allows these tools to function in environments where Git is unavailable. Feedback was provided to refine the find command's pruning logic in the new script to ensure it only targets directories and symlinks, preventing the accidental exclusion of files that might match the naming patterns.
Signed-off-by: naveenvenk17 <naveenvenkat1711@gmail.com>
ff5efb4 to
68e099d
Compare
|
@codex review |
|
clang-tidy review says "All clean, LGTM! 👍" |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 68e099d98f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| find . \ | ||
| \( \( -type d -o -type l \) \ | ||
| \( \ | ||
| -path "./.git" -o \ | ||
| -path "./src/sta" -o \ | ||
| -path "./third-party/abc" -o \ | ||
| -path "./bazel-*" -o \ | ||
| -path "./build" -o \ | ||
| -path "./build-*" \ | ||
| \) \ | ||
| \) -prune -o \ | ||
| \( -type f -o -type l \) \( "${patterns[@]}" \) -print0 |
There was a problem hiding this comment.
Keep lint source discovery limited to tracked files
The new find-based discovery now returns any matching filesystem entry, not just repository-tracked files like the previous git ls-files calls. In local workflows (//:lint_*, //:fmt_*, //:fix_lint), this causes untracked or ignored scratch/generated files (for example, temporary *.tcl files outside build*/bazel-*) to be linted/formatted and potentially fail the target, which is a behavior regression from the prior scripts. The helper should preserve tracked-file semantics when possible (or explicitly filter ignored/untracked files) to avoid false failures unrelated to committed sources.
Useful? React with 👍 / 👎.
|
I tried this on a local sandbox and it gave loads of errors on tmp/... which is a directory that is .gitignore'd but not excluded in the find command. This approach is rather brittle. I still prefer using git ls-files and just depending on git as I indicated in #10311 |
Signed-off-by: naveenvenk17 <naveenvenkat1711@gmail.com>
|
clang-tidy review says "All clean, LGTM! 👍" |
|
@hzeller do you prefer we use |
|
@hzeller How about this, add a command line option to inject the list of files if git is not available.
|
Fixes #10311.
This removes the
git ls-filesdependency from the Bazel lint/format scripts by adding a sharedbazel/list_sources.shhelper that usesfindinstead. The helper keeps the old behavior of skipping OpenROAD's upstream-owned submodule directories explicitly (src/sta,third-party/abc) and also skips VCS/build output directories.The Tcl and Bazel lint/format entrypoints now use the helper from Bazel runfiles, and
fix_lint.shskips its final workspace status report whengitis unavailable.Verification performed in a clean WSL-native checkout:
diff -u <(git ls-files '*.tcl' '*.sdc' '*.upf' | sort) <(./bazel/list_sources.sh '*.tcl' '*.sdc' '*.upf' | sort)diff -u <(git ls-files '*.bazel' '*.bzl' '**/BUILD' 'BUILD' '**/WORKSPACE' 'WORKSPACE' | sort) <(./bazel/list_sources.sh '*.bazel' '*.bzl' '**/BUILD' 'BUILD' '**/WORKSPACE' 'WORKSPACE' | sort)! ./bazel/list_sources.sh '*' | grep -E '^(src/sta|third-party/abc)/'npx --yes @bazel/bazelisk@latest test -c opt --test_output=errors //:fmt_bzl_test //:fmt_tcl_test //:lint_bzl_test //:lint_tcl_testgitfirst inPATH, using--nocache_test_results --action_env=PATH