Skip to content

Fix long-running clang-format after running integration tests - #1406

Merged
allenss-amazon merged 2 commits into
valkey-io:mainfrom
boda26:fix/format-skip-build-dirs
Sep 17, 2026
Merged

allenss-amazon merged 2 commits into
valkey-io:mainfrom
boda26:fix/format-skip-build-dirs

Conversation

@boda26

@boda26 boda26 commented Sep 17, 2026

Copy link
Copy Markdown
Collaborator

Fix long-running clang-format after running integration tests

Skip build directories when formatting

./build.sh --format takes ~1m46s instead of ~2s once integration tests have been run.

Cause

format() walks testing/ with no exclusion for build directories. testing/integration/CMakeLists.txt creates its venv at ${CMAKE_BINARY_DIR}/venv, i.e. testing/integration/.build-<config>/venv, and requirements.txt installs pyarrow, whose wheel ships the Arrow C++ API — xxhash.h (7678 lines), date.h (6717), arrow/type.h, arrow_to_pandas.cc and more. clang-format, single-threaded, then reformats 570 files of third-party code on every invocation.

Measured on a tree that has run integration tests once:

files time
before 838 1m46s
after 268 2.3s

Two properties make this easy to miss: the venv survives until deleted, so formatting stays slow on days you never touch integration tests; and testing/integration/.gitignore ignores .build*, so git status stays clean while 258MB of headers sit in the working tree.

Fix

Prune .build* directories in the find. -type d keeps a source file whose name starts with .build eligible, and the parentheses around the -name alternation are required once -prune -o enters the expression.

Notes

  • The newer top-level integration/ suite is unaffected — it puts its venv in ${BUILD_DIR}/integration/env at the repo root, which format() never walks.
  • Existing stale venvs stay on disk; rm -rf testing/integration/.build-*/venv reclaims the space, and the fix makes it irrelevant to formatting either way.

Testing

Verified with a simulated testing/integration/.build-release/venv/.../xxhash.h: the glob goes from 270 to 268 paths with zero venv entries, all tracked sources still included, bash -n build.sh clean.

Signed-off-by: Miles Song <bodasong@amazon.com>
@github-actions

Copy link
Copy Markdown

Reviewers for this PR

  • First Pass Reviewer: @neerajr0 — Please do your best to do a detailed review on the PR and get a response on your feedback. Once the first pass is done, notify the maintainer assigned to this PR to follow up on the final review and getting the PR merged. You can reach out to the people owning the relevant code paths for more help on the review.
  • Maintainer Reviewer: @allenss-amazon — Once the first review is done, please follow up with a final review and help to merge the change in.

Assigned automatically to the least-assigned members of the reviewer pools in .github/reviewer-pools.json. Use /reviewer or /remove-reviewer to adjust.

@coderabbitai

coderabbitai Bot commented Sep 17, 2026

Copy link
Copy Markdown
Contributor

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: 1eaa7a9f-a7a3-4a6d-a334-71b3c3b185c7

📥 Commits

Reviewing files that changed from the base of the PR and between 4f6ef4b and 99b9eaa.

📒 Files selected for processing (1)
  • build.sh

Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The format command now excludes .build* directories when it recursively locates C/C++ files. It continues to preserve the existing RAX path exclusion.

Changes

Formatting traversal

Layer / File(s) Summary
Exclude build directories
build.sh
The recursive formatter prunes .build* directories before selecting .h and .cc files. The existing RAX exclusion remains active.

Suggested reviewers: karthiksubbarao

Priority: ⬇️ Low

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 99b9e

The formatting traversal change is narrowly scoped and presents no established merge-blocking risk.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 1 functions across 1 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly describes the main change: preventing long-running clang-format after integration tests create build directories.
Description check ✅ Passed The description directly explains the formatting slowdown, its cause, the directory-pruning fix, and the validation performed.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@greptile-apps

greptile-apps Bot commented Sep 17, 2026

Copy link
Copy Markdown

RetriggerConfidence Score: 5/5

Safe to merge; no blocking issues remain.

Summary

The formatting command now excludes generated .build* directories before selecting C/C++ files, while retaining formatting for eligible source files and the existing Rax exclusion. No blocking issues were identified; the change is safe to merge.

Reviews (2) · Last reviewed commit: "Merge branch 'main' into fix/format-skip..."

@allenss-amazon
allenss-amazon merged commit ef57713 into valkey-io:main Sep 17, 2026
7 of 8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants