Improve build and test experience for Make builds

[pdillinger]

Summary:
Local Make builds silently reuse object files even when build parameters (DEBUG_LEVEL, sanitizers, ASSERT_STATUS_CHECKED, RTTI, etc.) change, since all object files share the same paths. This leads to confusing linker errors, sanitizer false negatives, ODR violations, and "phantom" bugs that are easy to miss -- a pitfall for humans and especially for AI agents driving builds non-interactively.

This change improves the Make experience on three fronts:

1. Build-parameter change detection in the Makefile. After flags are fully resolved, we hash the effective compile/link inputs (CC|CXX|CFLAGS|CXXFLAGS|LDFLAGS|EXEC_LDFLAGS) into a per-OBJ_DIR .build_signature stamp and compare against the previous build:

   • Default: stop with a clear error when parameters changed. This is optimized for the expert who might not intend a full rebuild, and for avoiding CI inefficiency.
   • AUTO_CLEAN=1: automatically run clean-rocks and proceed.
   • ALLOW_BUILD_PARAMETER_CHANGE=1: skip the check (e.g. intentionally mixing DEBUG_LEVEL=1 and DEBUG_LEVEL=2 objects).
     The check is skipped for dry runs (-n) and for non-building / self-cleaning targets (clean*, format, check-, tags, gen-pc, check-progress, watch-log, release, coverage, the asan_/ubsan_* variants, etc.). make_config.mk is removed only by the top-level clean target, not by clean-rocks, so the auto-clean does not delete the make_config.mk already included by the running make. The signature is removed last in clean-rocks so an interrupted clean keeps change detection meaningful.

2. New build_tools/rockstest.sh and build_tools/rocksptest.sh helpers that build and run unit tests in one step. They set AUTO_CLEAN=1 and build with -j (computed like the Makefile). rocksptest.sh runs one or more binaries under the checked-in gtest-parallel (sharing one parallel worker pool); rockstest.sh runs a single binary directly for a small number of cases. Both reject a leading-option first argument; rockstest.sh install writes ~/bin/rockstest and ~/bin/rocksptest shims that defer to the in-tree scripts (with a friendly message when run outside a source root). This is a big win for AI-agent workflows: one command instead of a separate make then test-run invocation, removing the overhead of monitoring two long commands, while avoiding serial builds and serial test execution.

3. CLAUDE.md guidance updated to recommend AUTO_CLEAN=1 for manual make invocations and the rocks(p)test.sh helpers for running tests

Although there might be drive to consolidate around the BUCK build for internal use, I'll note that last I checked it was around ~20 minutes to run all the unit tests under buck and ~2 minutes under make check.

Bonus: the first revision of this had copyright/license mistakes, and this change corrects similar errors elsewhere, with CLAUDE.md advice updated.

Test Plan:
Manual verification of the Makefile change detection:

• Confirmed the resolved-flags hash is deterministic across runs for the same goal, and differs between configs (e.g. default/shared vs static_lib, and with/without ASSERT_STATUS_CHECKED).
• Unchanged parameters: rebuild does not error.
• Changed parameters: errors by default; AUTO_CLEAN=1 runs clean-rocks and proceeds; ALLOW_BUILD_PARAMETER_CHANGE=1 bypasses the check.
• Dry runs (make -n) and exempt targets (e.g. make check-progress, list_all_tests, gen-pc) do not trip the check and leave the stamp untouched.
• Reproduced the ASSERT_STATUS_CHECKED=1 auto-clean path end to end and confirmed it no longer fails with "No rule to make target 'make_config.mk'"; verified make clean removes make_config.mk while clean-rocks does not, and that the signature is deleted as the final clean-rocks step.

Manual verification of the helper scripts:

• shellcheck-clean and bash -n pass for both scripts.
• Usage/guard behavior: missing argument and leading-option first argument are rejected with a clear message.
• rockstest.sh install (validated against a temp HOME) generates both shims; running a shim outside a source root prints "(Not in a rocksdb source root directory?)" and exits non-zero.
• rocksptest.sh argument splitting verified for single binary, multiple binaries, and binaries followed by gtest-parallel/test args.
• .gitignore patterns verified with git check-ignore for .build_signature (root and jl/jls) and build_tools/pycache/.

[github-actions]

✅ clang-tidy: No findings on changed lines

Completed in 51.4s.

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D109713512.

[meta-codesync]

@pdillinger has imported this pull request. If you are a Meta employee, you can view this in D109713512.

[github-actions]

🟡 Codex Code Review

Auto-triggered after CI passed — reviewing commit 8ee16d9

---

❌ Codex review failed before producing findings.

WARNING: proceeding, even though we could not create PATH aliases: Refusing to create helper binaries under temporary dir "/tmp" (codex_home: AbsolutePathBuf("/tmp/codex-home"))
error: the argument '--base <BRANCH>' cannot be used with '[PROMPT]'

Usage: codex exec review --commit <SHA> --base <BRANCH> --title <TITLE> --model <MODEL> --config <key=value> --dangerously-bypass-approvals-and-sandbox --output-last-message <FILE> [PROMPT]

For more information, try '--help'.

---

ℹ️ About this response

Generated by Codex CLI.
Review methodology: claude_md/code_review.md

Limitations:

• Codex may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /codex-review [context] — Request a code review
• /codex-query <question> — Ask about the PR or codebase

[github-actions]

✅ Claude Code Review

Auto-triggered after CI passed — reviewing commit 8ee16d9

---

Summary

Well-designed build infrastructure improvement that addresses a real pain point (stale object files from mixed build modes). The approach of hashing resolved compile/link flags is sound and the implementation is thorough, with good handling of edge cases (dry runs, sub-makes, clean targets, Java OBJ_DIRs). The helper scripts are clean and well-guarded.

High-severity findings (1):

• [Makefile:~2505] Shell injection via single quotes in CFLAGS/CXXFLAGS/LDFLAGS breaks the signature computation and can cause parse-time shell errors or silent signature mismatches.

Full review (click to expand)

Findings

🔴 HIGH

H1. Shell injection / breakage via single quotes in build flags -- Makefile:~2505

• Issue: The signature computation uses single-quoted shell strings:

  BUILD_SIG := $(shell printf '%s' '$(CC)|$(CXX)|$(CFLAGS)|$(CXXFLAGS)|$(LDFLAGS)|$(EXEC_LDFLAGS)' | $(SHA256_CMD) | cut -d ' ' -f 1)

  If any of these variables contain single quotes (e.g., CXXFLAGS="-DFOO='bar'" or -DNAME='"hello"'), the single-quoted shell string is broken. This can cause:
  1. A shell syntax error during Makefile parse, aborting the entire build
  2. Silent truncation of the hash input, causing false "parameters changed" errors
  3. In adversarial cases, shell command injection (low risk since these are developer-set, but the PR mentions AI agents driving builds)
• Root cause: $(shell printf '%s' '...') does not escape single quotes in the Make variable expansions.
• Suggested fix: Use a here-string or pipe from echo without quoting, or use Make's $(subst ','\'',...) to escape single quotes. A simpler and safer approach:

  BUILD_SIG := $(shell printf '%s' $(subst ','"'"',$(CC)|$(CXX)|$(CFLAGS)|$(CXXFLAGS)|$(LDFLAGS)|$(EXEC_LDFLAGS)) | $(SHA256_CMD) | cut -d ' ' -f 1)

  Or avoid shell quoting entirely by piping via heredoc or using env to pass the value.

🟡 MEDIUM

M1. Incomplete signature: AR/ARFLAGS not included -- Makefile:~2505

• Issue: The hash covers CC|CXX|CFLAGS|CXXFLAGS|LDFLAGS|EXEC_LDFLAGS but not AR or ARFLAGS. Changing the archiver (e.g., ar vs llvm-ar) while building static libraries could produce incompatible archives without triggering the change detection.
• Suggested fix: Include $(AR)|$(ARFLAGS) in the hash input.

M2. Race condition on concurrent make invocations -- Makefile:~2515

• Issue: Concurrent make processes race on reading/writing .build_signature (TOCTOU). In practice both processes typically have the same flags, making this benign. Failure mode is a spurious "parameters changed" error on next build, not data corruption.
• Suggested fix: Use atomic write (write to temp file, then mv).

M3. CLAUDE.md loses useful detail about ABI break causes -- CLAUDE.md

• Issue: The old guidance explicitly listed what causes ABI breaks (ASSERT_STATUS_CHECKED=1 changes Status layout, sanitizer toggles, DEBUG_LEVEL changes). The new text is generic. Educational value is lost.
• Suggested fix: Keep a brief note about ASSERT_STATUS_CHECKED ABI break even in the simplified guidance.

🟢 LOW / NIT

L1. BUILD_SIG_CLEAN_OUTPUT variable is unused -- Makefile:~2504

• Issue: Captures stdout (which is redirected to stderr) into a never-referenced variable. Harmless but confusing.
• Suggested fix: Use _dummy := or add a comment.

L2. analyze_incremental in NONBUILD_GOALS may be wrong -- Makefile:~2449

• Issue: Unlike analyze, analyze_incremental does NOT depend on clean. It builds incrementally and would benefit from the signature check. Low impact since it's a static analysis target.

Cross-Component Analysis

Context	Affected?	Assessment
CI (pr-jobs.yml)	Yes	Safe. Fresh checkout has no .build_signature.
make check	Yes	Correct. Sub-makes skip via MAKELEVEL guard.
release/asan_check/ubsan_check	No	Correctly in NONBUILD_GOALS (clean before build).
Java builds (jl/jls)	Yes	Per-OBJ_DIR signatures handle correctly.
make -n (dry run)	No	Correctly detected via MAKEFLAGS parsing.
macOS (old Make 3.81)	Pre-existing	.SHELLSTATUS already used at line 271.

Positive Observations

• Thorough edge case handling: MAKELEVEL guard, dry-run detection, per-OBJ_DIR signatures.
• Clean-rocks ordering: Removing .build_signature LAST preserves detection across interrupted cleans.
• make_config.mk separation: Prevents AUTO_CLEAN from breaking the running Makefile -- subtle but important.
• Script quality: Well-guarded with set -euo pipefail, argument validation, and consistent NCORES computation.

---

ℹ️ About this response

Generated by Claude Code.
Review methodology: claude_md/code_review.md

Limitations:

• Claude may miss context from files not in the diff
• Large PRs may be truncated
• Always apply human judgment to AI suggestions

Commands:

• /claude-review [context] — Request a code review
• /claude-query <question> — Ask about the PR or codebase

[meta-codesync]

@pdillinger merged this pull request in f6fabdb.