Skip to content

Commit f6fabdb

Browse files
pdillingermeta-codesync[bot]
authored andcommitted
Improve build and test experience for Make builds (#14883)
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. 3. 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<NCORES> (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. 4. 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. Pull Request resolved: #14883 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__/. Reviewed By: xingbowang Differential Revision: D109713512 Pulled By: pdillinger fbshipit-source-id: a801d29afa91f53a2dce717db9fe5e5d485cc154
1 parent 8fae7ff commit f6fabdb

14 files changed

Lines changed: 1293 additions & 41 deletions

File tree

‎.gitignore‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
make_config.mk
22
rocksdb.pc
3+
.build_signature
4+
build_tools/__pycache__/
35

46
*.a
57
*.arc

‎CLAUDE.md‎

Lines changed: 51 additions & 35 deletions
Original file line numberDiff line numberDiff line change
@@ -244,26 +244,15 @@ from an implementation detail instead of an explicit option.
244244
/usr/local/bin/python3 buckifier/buckify_rocksdb.py to update it
245245
* For -j in make command, use the number of CPU cores to decide it.
246246

247-
### When to run `make clean` (avoid mixing build modes)
247+
### Avoiding mixed build modes with Make (use `AUTO_CLEAN=1`)
248248

249-
The Makefile does **not** track build mode, so object files from a prior
250-
build are silently reused even when compiled with different flags, leading
251-
to confusing linker errors, sanitizer false negatives, ODR violations, or
252-
"phantom" bugs.
253-
254-
Run `make clean` before switching any of these:
255-
* **`ASSERT_STATUS_CHECKED=1` ↔ unset** — changes the `Status` class layout (ABI break).
256-
* **Sanitizer builds** — toggling any of `COMPILE_WITH_ASAN=1`,
257-
`COMPILE_WITH_UBSAN=1`, `COMPILE_WITH_TSAN=1` on/off.
258-
* `DEBUG_LEVEL=0` (release) ↔ `DEBUG_LEVEL=1` (debug, default for `make dbg`).
259-
* Different compilers, `OPT` levels, or other flags affecting codegen/ABI.
260-
261-
**Notable exception:** `DEBUG_LEVEL=2` can be safely mixed with
262-
`DEBUG_LEVEL=1` — rebuild a subset of files with `DEBUG_LEVEL=2` to get
263-
extra/more accurate runtime checks for those files without a full clean.
264-
265-
When in doubt, `make clean` is cheap insurance compared to chasing a
266-
phantom bug.
249+
Object files are written to the same paths regardless of build flags, so
250+
reusing objects from a prior build with different flags causes confusing
251+
linker errors, etc. This problem is essentially avoidable by ALWAYS using
252+
`AUTO_CLEAN=1 make -j<n> <something>` for manual make invocations. This
253+
will automatically clean object files if the build parameters/flavor have
254+
changed. The `build_tools/rockstest.sh` / `rocksptest.sh` helpers described
255+
below set `AUTO_CLEAN=1` for you.
267256

268257
### Source checks
269258
* Run `make check-sources` before committing. This catches non-ASCII
@@ -272,6 +261,22 @@ phantom bug.
272261
smart quotes, etc.) in comments or strings -- use ASCII equivalents
273262
(`--` instead of em dash, `'` instead of smart quote, etc.).
274263

264+
### License headers
265+
* Every new source file needs a license header. For a file that does **not**
266+
carry an outside/third-party copyright, use the standard Meta dual-licensed
267+
header (the dual-license designation is required -- a bare
268+
"All Rights Reserved" copyright is not an acceptable open-source header):
269+
```
270+
// Copyright (c) Meta Platforms, Inc. and affiliates.
271+
// This source code is licensed under both the GPLv2 (found in the
272+
// COPYING file in the root directory) and Apache 2.0 License
273+
// (found in the LICENSE.Apache file in the root directory).
274+
```
275+
Use a `#` comment prefix instead of `//` for shell, Python, and Makefile
276+
fragments.
277+
* Files derived from an external source (e.g. LevelDB) keep their original
278+
upstream copyright line in addition to the header above.
279+
275280
### RTTI and dynamic_cast
276281
* Production code and `db_stress` must build in **release mode
277282
(`-fno-rtti`)**. Do not use `dynamic_cast` anywhere except unit tests.
@@ -309,13 +314,22 @@ rather than relying on libstdc++ transitive includes.
309314
* Don't use sleep to wait for certain events to happen. This will cause test to
310315
be flaky. Instead, use sync point to synchronize thread progress.
311316
* Cap unit test execution with 60 seconds timeout.
312-
* When there are multiple unit tests need to be executed, try to use
313-
gtest_parallel.py if available. E.g.
314-
python3 ${GTEST_PARALLEL}/gtest_parallel.py ./table_test
315-
* After writing a test, stress-test for flakiness:
317+
* To build and run unit tests locally, prefer these helper scripts:
318+
* `build_tools/rocksptest.sh <test_binary> [more_binaries...] [args...]`
319+
builds the binary(ies) with parallel make and `AUTO_CLEAN=1` and runs
320+
them under gtest-parallel, sharding the test cases across CPUs. Prefer
321+
this whenever running more than a couple of test cases, e.g.
322+
`build_tools/rocksptest.sh table_test` or
323+
`build_tools/rocksptest.sh db_test env_test --gtest_filter=*Foo*`.
324+
* `build_tools/rockstest.sh <test_binary> [args...]` builds with parallel
325+
make and `AUTO_CLEAN=1` and runs the binary directly (serially).
326+
Use it only for a very small number of test cases, e.g.
327+
`build_tools/rockstest.sh db_test --gtest_filter=*MixedSlowdown*`.
328+
* After writing a test, stress-test for flakiness (AUTO_CLEAN handles the
329+
rebuild needed by the `COERCE_CONTEXT_SWITCH=1` flag change):
316330
```bash
317-
COERCE_CONTEXT_SWITCH=1 make {test_binary}
318-
./{test_binary} --gtest_filter="*YourTestName*" --gtest_repeat=5
331+
COERCE_CONTEXT_SWITCH=1 build_tools/rockstest.sh {test_binary} -r100 \
332+
--gtest_filter="*YourTestName*"
319333
```
320334
* For CI-style flaky tests that do not reproduce with `gtest_parallel.py`,
321335
`--gtest_repeat`, or normal coerce-mode runs, inspect
@@ -372,20 +386,21 @@ rather than relying on libstdc++ transitive includes.
372386
* Blog post authors must be defined in `docs/_data/authors.yml` to be displayed
373387
374388
### Final verification of the change
375-
* Execute make clean to clean all of the changes.
376-
* Execute make check to build all of the changes and execute all of the tests.
377-
Note that executing all of the tests could take multiple minutes.
378-
* Run `ASSERT_STATUS_CHECKED=1 make check` to verify all Status objects are
379-
properly checked. This catches missing error handling that can lead to
380-
silent data corruption.
389+
* Execute `AUTO_CLEAN=1 make check` to build all of the changes and execute all
390+
of the tests. `AUTO_CLEAN=1` ensures a clean rebuild if your previous build
391+
used different parameters. Note that executing all of the tests could take
392+
multiple minutes.
393+
* Run `AUTO_CLEAN=1 ASSERT_STATUS_CHECKED=1 make check` to verify all Status
394+
objects are properly checked. This catches missing error handling that can
395+
lead to silent data corruption.
381396
382397
### Monitoring make check progress
383398
* Use `make check-progress` to get machine-parseable JSON progress while
384399
`make check` is running. This is useful for Claude Code to monitor long
385400
builds without timeout issues.
386401
* Run `make check` in background, then poll progress:
387402
```bash
388-
make check &
403+
AUTO_CLEAN=1 make check &
389404
# Poll periodically:
390405
make check-progress
391406
```
@@ -410,9 +425,10 @@ rather than relying on libstdc++ transitive includes.
410425
411426
### Executing benchmark using db_bench
412427
* Since the goal is to measure performance, we need to build a release binary
413-
using `make clean && DEBUG_LEVEL=0 make db_bench`. If there is an engine
414-
crash due to bug, we need to switch back to debug build. Make sure to run
415-
`make clean` before running `make dbg`.
428+
using `AUTO_CLEAN=1 DEBUG_LEVEL=0 make db_bench`. If there is an engine
429+
crash due to a bug, switch back to a debug build with
430+
`AUTO_CLEAN=1 make dbg`; `AUTO_CLEAN=1` handles the release<->debug rebuild
431+
automatically.
416432
417433
### Formatting code
418434
* After making change, use `make format-auto` to auto-apply formatting without

‎Makefile‎

Lines changed: 106 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -288,7 +288,7 @@ endif
288288
endif
289289

290290
export JAVAC_ARGS
291-
CLEAN_FILES += make_config.mk rocksdb.pc
291+
CLEAN_FILES += rocksdb.pc
292292

293293
ifeq ($(V), 1)
294294
$(info $(shell uname -a))
@@ -1359,8 +1359,13 @@ rocksdb.h rocksdb.cc: build_tools/amalgamate.py Makefile $(LIB_SOURCES) unity.cc
13591359
build_tools/amalgamate.py -I. -i./include unity.cc -x include/rocksdb/c.h -H rocksdb.h -o rocksdb.cc
13601360

13611361
clean: clean-ext-libraries-all clean-rocks clean-rocksjava
1362+
# Removed here rather than in clean-rocks (via CLEAN_FILES) so the build-parameter
1363+
# auto-clean, which runs clean-rocks, doesn't delete the make_config.mk already
1364+
# included by the in-progress make.
1365+
@rm -f make_config.mk
13621366

13631367
clean-not-downloaded: clean-ext-libraries-bin clean-rocks clean-not-downloaded-rocksjava
1368+
@rm -f make_config.mk
13641369

13651370
clean-rocks:
13661371
# Not practical to exactly match all versions/variants in naming (e.g. debug or not)
@@ -1375,6 +1380,11 @@ clean-rocks:
13751380
else \
13761381
$(FIND) . -name "*.[oda]" -exec rm -f {} \; ; \
13771382
fi
1383+
# Remove the build signature(s) LAST. Done after the object files are gone so
1384+
# that a Ctrl+C partway through clean leaves the old signature in place: it
1385+
# still matches the leftover objects, so a later build with different
1386+
# parameters is still detected (signature usefulness is preserved).
1387+
@rm -f $(BUILD_SIG_FILE) jl/.build_signature jls/.build_signature
13781388

13791389
clean-rocksjava: clean-rocks
13801390
rm -rf jl jls
@@ -2408,6 +2418,101 @@ ifeq ($(PLATFORM), OS_OPENBSD)
24082418
endif
24092419
export SHA256_CMD
24102420

2421+
# ----------------------------------------------------------------------------
2422+
# Build parameter change detection
2423+
# ----------------------------------------------------------------------------
2424+
# RocksDB writes object files to the same $(OBJ_DIR) paths regardless of
2425+
# DEBUG_LEVEL, sanitizers (ASAN/TSAN/UBSAN), ASSERT_STATUS_CHECKED, RTTI, LTO,
2426+
# COERCE_CONTEXT_SWITCH, etc. Most of those are applied in this Makefile after
2427+
# `include make_config.mk` and are therefore NOT reflected in make_config.mk,
2428+
# so switching them and rebuilding silently mixes incompatible object files.
2429+
# To guard against that, we hash the fully-resolved compile/link parameters
2430+
# (which already embed make_config.mk via PLATFORM_*) and compare against the
2431+
# value recorded from the previous build. On mismatch the build stops so the
2432+
# user can `make clean`. Knobs:
2433+
# AUTO_CLEAN=1
2434+
# run `make clean-rocks` automatically on mismatch
2435+
# ALLOW_BUILD_PARAMETER_CHANGE=1
2436+
# skip the check entirely (e.g. intentionally mixing DEBUG_LEVEL=1 and
2437+
# DEBUG_LEVEL=2)
2438+
# The signature is stored per-$(OBJ_DIR), so Java (jl/jls) builds are tracked
2439+
# independently from the default build.
2440+
BUILD_SIG_FILE := $(OBJ_DIR)/.build_signature
2441+
# NOTE: deliberately NOT added to CLEAN_FILES. The signature is removed as the
2442+
# very last step of clean-rocks so that an interrupted clean keeps it (see
2443+
# clean-rocks), preserving change detection across a Ctrl+C'd clean.
2444+
2445+
# Goals that do not compile object files into the tree (pure utilities,
2446+
# informational, source/config checks): the change check is irrelevant for them.
2447+
BUILD_SIG_NONBUILD_GOALS := \
2448+
clean clean-rocks clean-not-downloaded clean-rocksjava \
2449+
clean-not-downloaded-rocksjava clean-ext-libraries-all \
2450+
clean-ext-libraries-bin \
2451+
format format-auto check-format check-buck-targets check-headers \
2452+
check-sources check-workflow-yaml check-progress clang-tidy \
2453+
tags tags0 package jclean checkout_folly \
2454+
watch-log dump-log suggest-slow-tests list_all_tests gen-pc \
2455+
gen_parallel_tests check-c-api-gen \
2456+
setup-hooks install-hooks uninstall-hooks uninstall db_crashtest_tests
2457+
# Goals that clean before building (depend on or invoke `clean`): they manage
2458+
# their own freshness, so the check must not block them (it would error before
2459+
# their built-in clean runs).
2460+
BUILD_SIG_NONBUILD_GOALS += \
2461+
release coverage build_size analyze analyze_incremental \
2462+
asan_check asan_crash_test asan_crash_test_with_atomic_flush \
2463+
asan_crash_test_with_txn asan_crash_test_with_best_efforts_recovery \
2464+
whitebox_asan_crash_test blackbox_asan_crash_test \
2465+
ubsan_check ubsan_crash_test ubsan_crash_test_with_atomic_flush \
2466+
ubsan_crash_test_with_txn ubsan_crash_test_with_best_efforts_recovery \
2467+
whitebox_ubsan_crash_test blackbox_ubsan_crash_test
2468+
2469+
# Goals that explicitly clean; never trip the check when one is requested.
2470+
BUILD_SIG_CLEAN_GOALS := \
2471+
clean clean-rocks clean-not-downloaded clean-rocksjava \
2472+
clean-not-downloaded-rocksjava clean-ext-libraries-all \
2473+
clean-ext-libraries-bin
2474+
2475+
BUILD_SIG_GOALS := $(if $(MAKECMDGOALS),$(MAKECMDGOALS),all)
2476+
BUILD_SIG_DO_BUILD := $(filter-out $(BUILD_SIG_NONBUILD_GOALS),$(BUILD_SIG_GOALS))
2477+
BUILD_SIG_CLEANING := $(filter $(BUILD_SIG_CLEAN_GOALS),$(MAKECMDGOALS))
2478+
# Dry runs (-n/--just-print) must not write the stamp, clean, or error; the
2479+
# parse-time $(shell) below would otherwise run even under -n. Note that
2480+
# actual options are canonicalized and shorted as possible such that all
2481+
# short options are in the first word of MAKEFLAGS.
2482+
BUILD_SIG_DRYRUN := $(findstring n,$(firstword -$(MAKEFLAGS)))
2483+
2484+
# Only enforce at the top level. Sub-makes (e.g. `check` invokes
2485+
# $(MAKE) gen_parallel_tests / check-c-api-gen / check_0) inherit the parent's
2486+
# build flags, so the top-level check already covers them; re-checking inside a
2487+
# sub-make only causes spurious failures.
2488+
ifneq ($(ALLOW_BUILD_PARAMETER_CHANGE),1)
2489+
ifeq ($(MAKELEVEL),0)
2490+
ifeq ($(BUILD_SIG_DRYRUN),)
2491+
ifeq ($(BUILD_SIG_CLEANING),)
2492+
ifneq ($(BUILD_SIG_DO_BUILD),)
2493+
BUILD_SIG := $(shell printf '%s' '$(CC)|$(CXX)|$(CFLAGS)|$(CXXFLAGS)|$(LDFLAGS)|$(EXEC_LDFLAGS)' | $(SHA256_CMD) | cut -d ' ' -f 1)
2494+
BUILD_SIG_OLD := $(shell cat $(BUILD_SIG_FILE) 2>/dev/null)
2495+
ifneq ($(BUILD_SIG_OLD),)
2496+
ifneq ($(BUILD_SIG_OLD),$(BUILD_SIG))
2497+
ifeq ($(AUTO_CLEAN),1)
2498+
$(info *** Build parameters changed since last build (OBJ_DIR=$(OBJ_DIR)); running 'make clean-rocks' because AUTO_CLEAN=1)
2499+
BUILD_SIG_CLEAN_OUTPUT := $(shell $(MAKE) clean-rocks 1>&2)
2500+
ifneq ($(.SHELLSTATUS),0)
2501+
$(error AUTO_CLEAN: 'make clean-rocks' failed (exit $(.SHELLSTATUS)); not building against a partially-cleaned tree)
2502+
endif
2503+
else
2504+
$(error Build parameters changed since the last build (OBJ_DIR=$(OBJ_DIR)). Existing object files are stale and must be removed. Run 'make clean', or set AUTO_CLEAN=1 to clean automatically, or ALLOW_BUILD_PARAMETER_CHANGE=1 to build anyway)
2505+
endif
2506+
endif
2507+
endif
2508+
# Record the current signature for the next build.
2509+
BUILD_SIG_WRITE := $(shell mkdir -p $(OBJ_DIR) 2>/dev/null; printf '%s\n' '$(BUILD_SIG)' > $(BUILD_SIG_FILE))
2510+
endif
2511+
endif
2512+
endif
2513+
endif
2514+
endif
2515+
24112516
zlib-$(ZLIB_VER).tar.gz:
24122517
curl --fail --output zlib-$(ZLIB_VER).tar.gz --location ${ZLIB_DOWNLOAD_BASE}/zlib-$(ZLIB_VER).tar.gz
24132518
ZLIB_SHA256_ACTUAL=`$(SHA256_CMD) zlib-$(ZLIB_VER).tar.gz | cut -d ' ' -f 1`; \

‎build_tools/check-public-header.sh‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,8 @@
11
#!/usr/bin/env bash
2-
# Copyright (c) Meta Platforms, Inc. and affiliates. All Rights Reserved.
2+
# Copyright (c) Meta Platforms, Inc. and affiliates.
3+
# This source code is licensed under both the GPLv2 (found in the
4+
# COPYING file in the root directory) and Apache 2.0 License
5+
# (found in the LICENSE.Apache file in the root directory).
36
#
47
# Check for some simple mistakes in public headers (on the command line)
58
# that should prevent commit or push

‎build_tools/gtest-parallel‎

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,18 @@
1+
#!/usr/bin/env python3
2+
# Copyright 2017 Google Inc. All rights reserved.
3+
#
4+
# Licensed under the Apache License, Version 2.0 (the "License");
5+
# you may not use this file except in compliance with the License.
6+
# You may obtain a copy of the License at
7+
#
8+
# http://www.apache.org/licenses/LICENSE-2.0
9+
#
10+
# Unless required by applicable law or agreed to in writing, software
11+
# distributed under the License is distributed on an "AS IS" BASIS,
12+
# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
# See the License for the specific language governing permissions and
14+
# limitations under the License.
15+
import gtest_parallel
16+
import sys
17+
18+
sys.exit(gtest_parallel.main())

0 commit comments

Comments
 (0)