scripted-diff: Type-safe settings retrieval #31260
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31260. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
|
Current status of this PR is that |
47e30b4 to
d8a4a0a
Compare
|
Updated 416860f -> 47e30b4 ( |
|
🚧 At least one of the CI tasks failed. HintsTry to run the tests locally, according to the documentation. However, a CI failure may still
Leave a comment here, if you need help tracking down a confusing failure. |
d8a4a0a to
ad32a27
Compare
ad32a27 to
534f971
Compare
Avoid string concatenation and add missing namespace names so AddArg calls can be moved to header files in an upcoming scripted-diff.
Move constant declarations referenced in AddArg calls to headers so AddArg calls be moved to header files in an upcoming scripted-diff.
Drop hidden_args vector so all AddHiddenArgs calls can be replaced in upcoming scripted diff.
Linter changes to deal with _settings.h headers added in the next commit. In circular-dependencies.py, prevent _settings.h headers from being included transitively. Add a new check that makes it an error to include _settings.h from another header file, rather than a .cpp file. Use this fact to avoid errors about .cpp files including _settings.h files being circularly dependent based on each other. In check-doc.py update test to look for Setting::Get and Setting::Register instead of GetArg and AddArg.
Add univalue include directory to libraries that need it it because they will
include it indirectly through setting.h files, and add libevent include
directory to libraries that need it because they will include it indirectly
through init_settings.h which includes torcontrol.h which includes
event2/util.h.
The missing cmake library dependencies trigger CI errors that may not happen on
all systems depending in how package include paths are laid out. In particular
the torcontrol / libevent errors only seem to happen on macos with homebrew.
It would probably be good to streamline headers to avoid these dependencies in
the future. It would be good if the torcontrol header did not have a pulic
dependency on libevent or, if it was just split up and the DEFAULT_TOR_CONTROL
and DEFAULT_TOR_CONTROL_PORT constants needed by init_settings.h to a different
file.
Errors look like:
D:\a\bitcoin\bitcoin\src\common\setting.h(10,1): error C1083: Cannot open include file: 'univalue.h': No such file or directory [D:\a\bitcoin\bitcoin\build\src\bitcoin.vcxproj]
In file included from /home/admin/actions-runner/_work/_temp/src/bitcoin.cpp:7:
In file included from /home/admin/actions-runner/_work/_temp/src/bitcoin_settings.h:4:
/home/admin/actions-runner/_work/_temp/src/common/setting.h:10:10: fatal error: 'univalue.h' file not found
10 | #include <univalue.h>
| ^~~~~~~~~~~~
1 error generated.
D:\a\bitcoin\bitcoin\src\txmempool.h(27,1): error C1083: Cannot open include file: 'boost/multi_index/hashed_index.hpp': No such file or directory [D:\a\bitcoin\bitcoin\build\src\bitcoin.vcxproj]
In file included from /home/admin/actions-runner/_work/_temp/src/validation.h:29,
from /home/admin/actions-runner/_work/_temp/src/node/chainstatemanager_args.h:9,
from /home/admin/actions-runner/_work/_temp/src/init_settings.h:21,
from /home/admin/actions-runner/_work/_temp/src/bitcoin.cpp:10:
/home/admin/actions-runner/_work/_temp/src/txmempool.h:27:10: fatal error: boost/multi_index/hashed_index.hpp: No such file or directory
27 | #include <boost/multi_index/hashed_index.hpp>
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /Users/runner/work/bitcoin/bitcoin/src/qt/bitcoin.cpp:14:
In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30:
/Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found
14 | #include <event2/util.h>
| ^~~~~~~~~~~~~~~
1 error generated.
ninja: build stopped: subcommand failed.
In file included from /Users/runner/work/bitcoin/bitcoin/src/test/util/setup_common.cpp:17:
In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30:
/Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found
14 | #include <event2/util.h>
| ^~~~~~~~~~~~~~~
1 error generated.
In file included from /Users/runner/work/bitcoin/bitcoin/src/common/config.cpp:6:
In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30:
/Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found
14 | #include <event2/util.h>
| ^~~~~~~~~~~~~~~
1 error generated.
In file included from /Users/runner/work/bitcoin/bitcoin/src/bitcoin.cpp:10:
In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30:
/Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found
14 | #include <event2/util.h>
| ^~~~~~~~~~~~~~~
1 error generated.
In file included from /Users/runner/work/bitcoin/bitcoin/src/bitcoind.cpp:15:
In file included from /Users/runner/work/bitcoin/bitcoin/src/init_settings.h:30:
/Users/runner/work/bitcoin/bitcoin/src/torcontrol.h:14:10: fatal error: 'event2/util.h' file not found
14 | #include <event2/util.h>
| ^~~~~~~~~~~~~~~
1 error generated.
…egister / Get calls Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
…/ Get calls This commit is a pure refactoring and does not change behavior in any way. -BEGIN VERIFY SCRIPT- python contrib/devtools/reg-settings.py git add -N src/bench/bench_bitcoin_settings.h src/bitcoin_settings.h src/bitcoin-tx_settings.h src/bitcoin-util_settings.h src/bitcoin-wallet_settings.h src/chainparamsbase_settings.h src/common/args_settings.h src/init/common_settings.h src/init_settings.h src/qt/bitcoin_settings.h src/test/argsman_tests_settings.h src/test/logging_tests_settings.h src/wallet/init_settings.h src/dummywallet_settings.h src/qt/test/optiontests_settings.h src/test/getarg_tests_settings.h git rm contrib/devtools/reg-settings.py -END VERIFY SCRIPT-
fc2591a to
cc61e18
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
This PR changes the way settings are registered and retrieved to provide more compile-time safety. Currently settings are registered and retrieved like:
But this is not ideal because nothing checks that setting names are spelled correctly, or that default values (
BITCOIN_PID_FILENAME) are used consistently in the help string and retrieval sites, or that settings are retrieved with consistent types (bool, int, string, path, or list). This PR addresses these issues by adding setting declarations which allow settings to be registered and retrieved like:Suggestions for review
All the real changes in this PR are in the last scripted-diff commit:
If you take a few minutes to look at the changes applied in this scripted-diff commit, you'll understand everything this PR is doing. A good place to start looking around in this commit is the generated
src/init_settings.hfile which declares the settings that get registered in src/init.cpp. Then you can look at the surrounding diffs and see they are just replacingAddArgandGetArgcalls.The other notable commit is second commit, which implements the
Settingclass:The other commits do minor things like moving code or updating linters. The python script that implements the scripted diff is a temporary artifact that gets deleted. The python script is complicated because it does things like parsing c++ code to extract help strings, and figuring out the right types to declare settings with so code compiles. But the entire scope of the script is to (1) generate Setting type definitions, (2) add #includes, and (3) replace
AddArg()calls withRegister()calls andGetArg()calls withGet()calls. So there is not much the script can actually do wrong without triggering build and test failures.Extensions
This PR only adds the ability to declare individual settings with built-in types. It doesn't provide any new runtime behavior, but a branch in issue #22978 extends the
Settingclass implemented here to support runtime setting validation, additional types likestd::variant, additional conversion options, custom types, custom validation, and groups of settings declared as options structs.This change is mostly orthogonal to #16545. #16545 only provides runtime type checking while this PR only provides compile-time checking with no new runtime behavior. But this change does allow a nicer way of declaring types in #16545, using c++ types like
intinstead of flags likeALLOW_INT, orstd::vector<std::string>instead ofALLOW_STRING | ALLOW_LIST.