-
Notifications
You must be signed in to change notification settings - Fork 37.7k
Move GetDataDir to ArgsManager #21244
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Move GetDataDir to ArgsManager #21244
Conversation
e4882c2
to
c136daf
Compare
c136daf
to
bb4a251
Compare
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. |
624a01e
to
daa523c
Compare
daa523c
to
3544cd2
Compare
Concept ACK |
3544cd2
to
bf451ff
Compare
Concept ACK. |
@sipa Yes, you are right. I have modified that. |
9d5a4bd
to
82b8119
Compare
82b8119
to
3aa3326
Compare
d148137
to
47f8894
Compare
-BEGIN VERIFY SCRIPT- git ls-files src/test/getarg_tests.cpp | xargs sed -i "s/m_args/m_local_args/g"; -END VERIFY SCRIPT-
…estingSetup class instead of implicitly relying on gArgs. -BEGIN VERIFY SCRIPT- git ls-files src/test/dbwrapper_tests.cpp src/test/denialofservice_tests.cpp src/test/flatfile_tests.cpp src/test/fs_tests.cpp src/test/settings_tests.cpp src/test/util_tests.cpp | xargs sed -i 's/GetDataDir()/m_args.GetDataDirPath()/g'; -END VERIFY SCRIPT-
I addessed outstanding suggestions. Diff since 69b8c4a is minimal. |
69b8c4a
to
bb8d1c6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review ACK bb8d1c6. Just minor const/naming changes and splitting/scripting commits since last review
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
review ACK bb8d1c6 📓
Show signature and timestamp
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK bb8d1c6e029a2fd01387599d2ff3bfc879a8ada5 📓
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYEACgkQzit1aX5p
pUgg4wv/VxpdghXhej+GNGTyDSSqYsXme76aDsRmpsxCgFsPO1mpnR9KzmCQNYN7
M9MBJ8+1Slsa8an8P5pk9cFYmGciu0Iu26hVOrZ/lESZYZ4j9+qWbN5dN2VkSDEg
GEt1P1RnE5qyqLQZmrrafkk0EcGl8ErBc56kjlYyciugjKX8KNqdvDSJAAdnMCmB
h+TIlGuCXrdG2TEngv0neHJ5nrp4JG0hdkD73u8QeVnszs3KuHbXzWenng5jJKwC
CidMrDLjLtc/B/I31GlXeWD3pYwlOkxrtC5rAObaThRtU0NlQ5Pz3DsrcvWLuEPA
vzyBa/ru6TUHxGPpq4IMTjdf0v3GVYyjJc0xbXiGGLQygJw6ff0YzJm8F0hqw7J+
/6xvt8hVz+10z887VNIBQLKgasLiDB5R1us+XprxYcwGW3GljiDtKCFUcIBvSlbn
D3yODkhg6jYIoiLUi2xp8B2e9AWqlCSx09dySUl3Gn+Q9srvkHIu5ZOfUg1vM6rx
XhwN4wTz
=q1yj
-----END PGP SIGNATURE-----
Timestamp of file with hash 977786d72da2ca0c8d5b6be0fa8686d3e4ab5578eca910bb84af1e639ee23595 -
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
aca0e5d Remove `GetDataDir(bool fNetSpecific = true)` function (Kiminuo) b3e67f2 scripted-diff: Replace `GetDataDir(true)` calls with `gArgs.GetDataDirNet()` calls (Kiminuo) 4c3a5dc scripted-diff: Replace `GetDataDir()` calls with `gArgs.GetDataDirNet()` calls (Kiminuo) 13bd8bb Make `ArgsManager.GetDataDirPath` private and drop needless suffix (Kiminuo) 4d8189f scripted-diff: Change `ArgsManager.GetDataDirPath()` to `ArgsManager.GetDataDirBase()` in tests (Kiminuo) 0f53df4 Add `ArgsManager.GetDataDirBase()` and `ArgsManager.GetDataDirNet()` as an intended replacement for `ArgsManager.GetDataDirPath(net_identifier)` (Kiminuo) 716de29 Make `m_cached_blocks_path` mutable. Make `ArgsManager::GetBlocksDirPath()` const. (Kiminuo) Pull request description: This PR is a follow up PR to #21244. The PR attempts to move us an inch towards the [goal](bitcoin/bitcoin#21244 (comment)) by removing `GetDataDir(net_specific)` and replacing it by `gArgs.GetDataDir(net_specific)` calls. The approach of this PR attempts to be similar to the one chosen in "De-globalize ChainstateManager" (#20158). The goal is to pass `ArgsManager` to functions (or ideally to have `ArgsManager` as a member of a class where needed; inspiration from here: #21789) instead of having it as a global variable (i.e. `gArgs`). **Notes:** * First commit makes `m_cached_blocks_path` `mutable` as was suggested [here](bitcoin/bitcoin#21244 (comment)) but not fully applied in #21244. (`m_cached_datadir_path` and `m_cached_network_datadir_path` were marked as `mutable` in #21244) This commit can be in a separate PR too. * Other commits deal with removing of `GetDataDir(net_specific)` function. * This was originally part of #21244 but it was [left]((bitcoin/bitcoin#21244 (review))) for a follow up PR. * I think that the proposed changes show nicely where there is reliance on `gArgs` which is IMO a good thing. If you know about a better approach how to do this, please share it here. ACKs for top commit: hebasto: ACK aca0e5d MarcoFalke: re-ACK aca0e5d 👃 Tree-SHA512: deec4d88edb32d7f4c818c3a74ffbb64709685819b88242dcf5dbaa1fb611f3ce2b29d2576ddb9e0dc5e75288e43538968224008c0a80e7149fc81c309f7c9da
…et.cpp` c3c2132 Use `context.args` in `src/wallet/load.cpp`. (Kiminuo) 25de4e7 Use `context.args` in `CWallet::Create` instead of `gArgs`. (Kiminuo) aa5e7c9 Fix typo in bitcoin-cli.cpp (Kiminuo) Pull request description: The PR attempts to move us an inch towards the [goal](bitcoin/bitcoin#21244 (comment)) by using `WalletContext` in `wallet.{h|cpp}` code instead of relying on the global state (i.e. `gArgs`). Edit: The PR builds on #19101. ACKs for top commit: ryanofsky: Code review ACK c3c2132. Changes since last review: just rebasing and adding wallet load commit Tree-SHA512: 2b436f5a219e32c2d529f55a89edca086d949396cebf9e089a21aa7b1c180e3c0fb17468f415dfc834f8e1614f8b3914c7e9a0bd33b95e7e0199c0dfe5ca9490
c3c2132 Use `context.args` in `src/wallet/load.cpp`. (Kiminuo) 25de4e7 Use `context.args` in `CWallet::Create` instead of `gArgs`. (Kiminuo) aa5e7c9 Fix typo in bitcoin-cli.cpp (Kiminuo) Pull request description: The PR attempts to move us an inch towards the [goal](bitcoin#21244 (comment)) by using `WalletContext` in `wallet.{h|cpp}` code instead of relying on the global state (i.e. `gArgs`). Edit: The PR builds on bitcoin#19101. ACKs for top commit: ryanofsky: Code review ACK c3c2132. Changes since last review: just rebasing and adding wallet load commit Tree-SHA512: 2b436f5a219e32c2d529f55a89edca086d949396cebf9e089a21aa7b1c180e3c0fb17468f415dfc834f8e1614f8b3914c7e9a0bd33b95e7e0199c0dfe5ca9490
Summary: PR description: > This PR attempts to contribute to "Remove gArgs" (#21005). > > Main changes: > > `GetDataDir()` function is moved to `ArgsManager.GetDataDirPath()`. > `GetBlocksDir()` function is moved to `ArgsManager.GetBlocksDirPath()`. This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [1/8] bitcoin/bitcoin@70cdf67 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10749
Summary: This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [2/8] bitcoin/bitcoin@1add318 Note: due to out of order backport, I also had to remove `private:` in `setup_common.h` which is otherwise removed in [[bitcoin/bitcoin#19806 | core#19806]]. Depends on D10749 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10750
Summary: This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [3/8] bitcoin/bitcoin@1cb52ba Depends on D10750 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10751
Summary: ``` -BEGIN VERIFY SCRIPT- git ls-files src/test/getarg_tests.cpp | xargs sed -i "s/m_args/m_local_args/g"; arc lint -END VERIFY SCRIPT- ``` This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [4/8] bitcoin/bitcoin@55c68e6 Depends on D10751 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10753
Summary: This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [5/8] bitcoin/bitcoin@511ce3a Depends on D10753 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Subscribers: Fabien Differential Revision: https://reviews.bitcoinabc.org/D10752
…estingSetup class instead of implicitly relying on gArgs Summary: ``` -BEGIN VERIFY SCRIPT- git ls-files src/test/dbwrapper_tests.cpp src/test/denialofservice_tests.cpp src/test/flatfile_tests.cpp src/test/fs_tests.cpp src/test/settings_tests.cpp src/test/util_tests.cpp | xargs sed -i 's/GetDataDir()/m_args.GetDataDirPath()/g'; arc lint -END VERIFY SCRIPT- ``` This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [6/8] bitcoin/bitcoin@83292e2 Depends on D10752 Test Plan: `ninja check` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10755
Summary: This is a backport of [[bitcoin/bitcoin#21244 | core#21244]] [7/8] bitcoin/bitcoin@b4190ef Note: The few minor difference from Core are explained by D4495 (Range-based `for` loop) and D7579 (blockdb.cpp) Depends on D10755 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10756
Summary: This concludes backport of [[bitcoin/bitcoin#21244 | core#21244]] [8/8] bitcoin/bitcoin@bb8d1c6 Depends on D10756 Test Plan: `ninja all check-all` Reviewers: #bitcoin_abc, Fabien Reviewed By: #bitcoin_abc, Fabien Differential Revision: https://reviews.bitcoinabc.org/D10757
This PR attempts to contribute to "Remove gArgs" (#21005).
Main changes:
GetDataDir()
function is moved toArgsManager.GetDataDirPath()
.GetBlocksDir()
function is moved toArgsManager.GetBlocksDirPath()
.