Skip to content

Conversation

kiminuo
Copy link
Contributor

@kiminuo kiminuo commented Feb 20, 2021

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().

@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch from e4882c2 to c136daf Compare February 20, 2021 11:17
@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch from c136daf to bb4a251 Compare February 20, 2021 12:22
@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 20, 2021

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, 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.

@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch 2 times, most recently from 624a01e to daa523c Compare February 21, 2021 13:05
@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch from daa523c to 3544cd2 Compare February 22, 2021 21:53
@laanwj
Copy link
Member

laanwj commented Feb 23, 2021

Concept ACK

@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch from 3544cd2 to bf451ff Compare February 23, 2021 20:59
@hebasto
Copy link
Member

hebasto commented Feb 23, 2021

Concept ACK.

@kiminuo
Copy link
Contributor Author

kiminuo commented Feb 24, 2021

@sipa Yes, you are right. I have modified that.

@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch 3 times, most recently from 9d5a4bd to 82b8119 Compare February 25, 2021 21:25
@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch 2 times, most recently from d148137 to 47f8894 Compare February 27, 2021 12:40
@kiminuo kiminuo marked this pull request as ready for review February 27, 2021 12:46
kiminuo added 5 commits April 18, 2021 11:59
-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-
@kiminuo
Copy link
Contributor Author

kiminuo commented Apr 18, 2021

I addessed outstanding suggestions. Diff since 69b8c4a is minimal.

@kiminuo kiminuo force-pushed the feature/2021-02-get-data-dir-args branch from 69b8c4a to bb8d1c6 Compare April 18, 2021 10:09
Copy link
Contributor

@ryanofsky ryanofsky left a 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

Copy link
Member

@maflcko maflcko left a 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 -

Copy link
Member

@hebasto hebasto left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re-ACK bb8d1c6, addressed comments, and two commits made scripted-diffs since my previous review.

@fanquake fanquake merged commit a839303 into bitcoin:master Apr 20, 2021
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 20, 2021
@kiminuo kiminuo deleted the feature/2021-02-get-data-dir-args branch April 20, 2021 08:46
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request May 24, 2021
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
maflcko pushed a commit to bitcoin-core/gui that referenced this pull request Aug 26, 2021
…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
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Aug 28, 2021
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
…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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
Fabcien pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request Jan 4, 2022
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
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants