Add three more cmake presets to the project-wide CMakePresets.json file:
dev-mode: enables all features and dependencieslibfuzzer: builds for fuzzing with libfuzzer and the typical sanitizers (but not the optional ones that require suppressions) enabled.libfuzzer-nosan: builds for fuzzing with libfuzzer and no (other) sanitizers… and then uses these in some documentation.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, TheCharlatan |
| Concept ACK | ismaelsadeeq |
| Stale ACK | maflcko |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
6@@ -7,11 +7,7 @@ To quickly get started fuzzing Bitcoin Core using [libFuzzer](https://llvm.org/d
7 ```sh
8 $ git clone https://github.com/bitcoin/bitcoin
9 $ cd bitcoin/
10-$ cmake -B build_fuzz \
11- -DCMAKE_C_COMPILER="clang" \
12- -DCMAKE_CXX_COMPILER="clang++" \
13- -DBUILD_FOR_FUZZING=ON \
14- -DSANITIZERS=undefined,address,fuzzer
15+$ cmake --preset=libfuzzer
And then uses these … in CI.
I’m not sure about doing this. At least, not if it’s going to use the same presets as the “general” dev presets. Then we’ve spread the CI config into 2 different places, made it harder to change the CI (as that might break/make the preset less useful for devs), and opened ourselves up to changes in the presets (silently) breaking the CI. i.e devs decide they no-longer want BUILD_GUI_TESTS in the preset, that gets removed, and the CI silently stops running the GUI tests. I think if we want to use presets in the CI, they should encapsulate all config, and exist under /ci/.
87+ "WITH_MINIUPNPC": "ON",
88+ "WITH_MULTIPROCESS": "ON",
89+ "WITH_NATPMP": "ON",
90+ "WITH_QRENCODE": "ON",
91+ "WITH_SQLITE": "ON",
92+ "WITH_USDT": "ON",
“WITH_USDT”: “ON”,
Something to keep in mind with a generic “dev mode”, is that this wont work for all devs. i.e WITH_USDT=ON will always fail to configure for devs building on macOS, trying to use this preset.
-DWITH_USDT=OFF still together with the preset.
I think if we want to use presets in the CI, they should encapsulate all config, and exist under
/ci/.
From https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html:
CMakePresets.jsonandCMakeUserPresets.jsonlive in the project’s root directory.
ci/test/03_test_script.sh script does not set the build dir, but uses cd to navigate to it and set it implicitly. With a preset build dir, this will fail. A fix would have to make the ci build dir explicit, or remove the preset build dir.
From https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html: CMakePresets.json and CMakeUserPresets.json live in the project’s root directory.
Ok? We can put files where ever we want and just include them. i.e:
0 "include": [
1 "ci/ci_presets.json"
2 ],
From https://cmake.org/cmake/help/latest/manual/cmake-presets.7.html: CMakePresets.json and CMakeUserPresets.json live in the project’s root directory.
Ok? We can put files where ever we want and just include them. i.e:
0 "include": [ 1 "ci/ci_presets.json" 2 ],
Which in turn requires CMake >= 3.23.
review ACK 1171871f63261e0c06d34baf216a54bfa747d75f
Seems fine to add recommended presets for common stuff like development or fuzzing.
Nit: Add build: prefix to pull request title and remove “CI” mention from the pull request description?
Nit: Add
build:prefix to pull request title and remove “CI” mention from the pull request description?
Done.
Code review ACK f15e817811e328423ea489870ead089128a6ef8c. This change is much needed to simplify my command line.
Could consider the name “all” or “enable-all” for the preset, and “build_all” for the directory name, but current “dev-mode” and “build_dev_mode” also seems ok.
