Merge settings one place instead of five places #15934
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
There was a problem hiding this comment.
I had a refactor (which I did't submit) that supported chaining ArgsManager. The idea was to support changing some args when calling some RPC, so a ArgsManager is created with the "overridden" args and passed thru. Is this something you are considering supporting or do you see a different approach?
Concept ACK.
19e84b7 to
1d543ad
Compare
|
re: #15934 (review) from promag
This change does make it easier to add new settings sources (with consistent handling of negated args and things), so it should be compatible with your idea and maybe helpful. Depending on the situation, I think having chained or scoped settings could be a good idea or not. I do think that in wallet code and application code generally it's good to get away from using key-value storage classes like |
|
Concept ACK |
There was a problem hiding this comment.
Tested ACK 1d543ad
Generated and reviewed the test output locally. Mucked around with various argument formulations using the following config file:
dbcache=100
[main]
dbcache=200
[test]
dbcache=300
and commandline invocations e.g.
./src/bitcoind -conf=$(pwd)/test.conf -dbcache=1000 -dbcache=500 | grep Usingto verify dbcache being set as expected.
This is a well-written change that cleans up a lot of gnarly, duplicated settings munging. It explicitly outlines surprising corner cases in existing behavior (with docs too), and makes reasoning about settings merge order easier. This change also introduces substantial test coverage to settings management (util::Settings).
After this is merged, adding a read-write settings file (whether it's JSON or something else) will be much easier.
src/util/system.cpp
Outdated
| // Weird behavior preserved for backwards compatibility: command line | ||
| // options with section prefixes are allowed but ignored. It would be | ||
| // better if these options triggered the IsArgKnown error below, or were | ||
| // actually used instead of silently ignored. |
There was a problem hiding this comment.
Thanks for the nice comment. Potentially out of scope: could we log warnings for this instead of silently ignoring?
There was a problem hiding this comment.
re: #15934 (comment)
Potentially out of scope: could we log warnings for this instead of silently ignoring?
I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.
| // test parses and merges settings, representing the results as strings that get | ||
| // compared against an expected hash. To debug, the result strings can be dumped | ||
| // to a file (see comments below). | ||
| BOOST_FIXTURE_TEST_CASE(Merge, MergeTestingSetup) |
There was a problem hiding this comment.
Cool test! The formatted output is really helpful. Encourage other reviewers to run and inspect with
SETTINGS_MERGE_TEST_OUT=results.txt ./src/test/test_bitcoin --run_test=settings_tests/MergeThere was a problem hiding this comment.
Updated 1d543ad -> 2dfeff1 (pr/mergeset.6 -> pr/mergeset.7), compare) with suggested changes.
Rebased 2dfeff1 -> 955c782 (pr/mergeset.7 -> pr/mergeset.8) to share common code with #15988.
src/util/system.cpp
Outdated
| // Weird behavior preserved for backwards compatibility: command line | ||
| // options with section prefixes are allowed but ignored. It would be | ||
| // better if these options triggered the IsArgKnown error below, or were | ||
| // actually used instead of silently ignored. |
There was a problem hiding this comment.
re: #15934 (comment)
Potentially out of scope: could we log warnings for this instead of silently ignoring?
I'm planning on making followup PRs that simplify and clean up all these "Weird behavior preserved for backwards compatibility" instances. I'd rather not add warnings in this PR partly because I'm disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.
|
re-tACK 955c782 based on the interdiff and running an abbreviated version of the testing above. |
|
Rebased 955c782 -> 14a6dfc (pr/mergeset.8 -> pr/mergeset.9) due to conflict with #16278 |
|
reACK 14a6dfc based on interdiff. Only change since |
Suggestion from John Newbery <john@johnnewbery.com> in bitcoin#15934 (comment)
includeconf -> conf_file_names to_include -> conf_file_name include_config -> conf_file_stream Suggestion from John Newbery <john@johnnewbery.com> in bitcoin#15934 (comment)
Easier to review ignoring whitespace Suggestion from John Newbery <john@johnnewbery.com> in bitcoin#15934 (comment)
Suggested by James O'Beirne <james.obeirne@gmail.com> bitcoin#15934 (comment) This commit does not change behavior.
Suggested by Antoine Riard <ariard@student.42.fr> bitcoin#15934 (comment) and John Newbery <john@johnnewbery.com> bitcoin#15934 (comment) This commit does not change behavior.
Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from e03483f bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (bitcoin#16545) This commit does not change behavior.
Suggested by John Newbery <john@johnnewbery.com> bitcoin#15934 (comment) This commit does not change behavior.
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
backport: bitcoin#15934, bitcoin#15864, bitcoin#19188, bitcoin#18338, bitcoin#19413, bitcoin#18571, bitcoin#18575 (deglobalization part 3)
Co-authored-by: UdjinM6 <UdjinM6@users.noreply.github.com>
…etArgFlags Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528 bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (bitcoin#16545) This commit does not change behavior.
…etArgFlags Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528 bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (bitcoin#16545) This commit does not change behavior.
…etArgFlags Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528 bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (bitcoin#16545) This commit does not change behavior.
…etArgFlags Rename suggested by João Barbosa <joao.paulo.barbosa@gmail.com> bitcoin#16545 (comment) This also gets rid of ArgsManager::NONE constant, which was an implementation detail not meant to be used by ArgsManager callers. Finally this reverts a change from 7f40528 bitcoin#15934 adding "-" characters to argument names. Better for GetArgFlags to require "-" prefixes for consistency with other ArgsManager methods, and to be more efficient later when GetArg functions need to call GetArgFlags (bitcoin#16545) This commit does not change behavior.
This is a refactoring-only change that makes it easier to add a new settings source.
This PR doesn't change behavior. The
util_ArgsMergeandutil_ChainMergetests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.This change:
The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent
-walletsetting are pretty straightforward and show how the new code can be extended: