move-only: Extract common/args from util/system #27419

pull TheCharlatan wants to merge 1 commits into bitcoin:master from TheCharlatan:splitSystemArgs changing 113 files +1655 −1545
  1. TheCharlatan commented at 1:12 pm on April 4, 2023: contributor

    This pull request is part of the libbitcoinkernel project #24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its “Step 2: Decouple most non-consensus code from libbitcoinkernel”. It is part of a series of patches splitting up the util/system files. Its preceding pull request is #27254.

    The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file.

    The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See doc/design/libraries.md for more information on this rationale.

  2. DrahtBot commented at 1:12 pm on April 4, 2023: contributor

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

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK MarcoFalke, ryanofsky, hebasto
    Concept ACK fanquake

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #27491 (refactor: Move chain constants to the util library by TheCharlatan)
    • #27409 (Make GUI and CLI tools use the same datadir by ryanofsky)
    • #27385 (net, refactor: extract Network and BIP155Network logic to node/network by jonatack)
    • #27302 (init: Error if ignored bitcoin.conf file is found by ryanofsky)
    • #27125 (refactor, kernel: Decouple ArgsManager from blockstorage by TheCharlatan)
    • #27064 (system: use %LOCALAPPDATA% as default datadir on windows by pinheadmz)
    • #26951 (wallet: improve rescan performance 640% by pstratem)
    • #26762 (refactor: Make CCheckQueue RAII-styled by hebasto)
    • #25908 (p2p: remove adjusted time by fanquake)
    • #24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg by luke-jr)
    • #24313 (Improve display address handling for external signer by Sjors)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
    • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
    • #10443 (Add fee_est tool for debugging fee estimation code by ryanofsky)
    • #10102 (Multiprocess bitcoin by ryanofsky)

    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.

  3. DrahtBot added the label Refactoring on Apr 4, 2023
  4. TheCharlatan marked this as ready for review on Apr 4, 2023
  5. in src/test/util_tests.cpp:1688 in 93c56f4f20 outdated 
    1684@@ -1685,7 +1685,7 @@ BOOST_AUTO_TEST_CASE(message_hash)
1685 
1686 BOOST_AUTO_TEST_CASE(remove_prefix)
1687 {
1688-    BOOST_CHECK_EQUAL(RemovePrefix("./util/system.h", "./"), "util/system.h");
1689+    BOOST_CHECK_EQUAL(RemovePrefix("./<<common/args.h>>", "./"), "<<common/args.h>>");
    maflcko commented at 5:04 pm on April 4, 2023:
    Can you explain this change?
    TheCharlatan commented at 8:34 pm on April 4, 2023:
    No I cannot, because it doesn’t make sense. Must have overlooked this after scripting the header moves. Will revert.
  6. TheCharlatan force-pushed on Apr 4, 2023
  7. TheCharlatan commented at 8:43 pm on April 4, 2023: contributor

    Updated 93c56f4f203f3bf3a616863b4ec6443aa36b2f9d -> c02e451882bb1d220b944af3b444d4b529ac351a (splitSystemArgs_0 -> splitSystemArgs_1, compare):

  8. hebasto commented at 6:51 am on April 5, 2023: member
    Concept ACK.
  9. in src/common/args.h:5 in c02e451882 outdated 
    0@@ -0,0 +1,455 @@
1+#ifndef BITCOIN_COMMON_ARGS_H
    hebasto commented at 7:33 am on April 5, 2023:
    Add a copyright header?
  10. hebasto commented at 7:46 am on April 5, 2023: member

    Approach ACK c02e451882bb1d220b944af3b444d4b529ac351a.

    Suggesting to pass the new common/args.cpp to IWYU:

     0diff --git a/ci/test/06_script_b.sh b/ci/test/06_script_b.sh
 1index c2cf2a8720..0cef4683ac 100755
 2--- a/ci/test/06_script_b.sh
 3+++ b/ci/test/06_script_b.sh
 4@@ -42,6 +42,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
 5   ( CI_EXEC run-clang-tidy-15 -quiet "${MAKEJOBS}" ) | grep -C5 "error"
 6   export P_CI_DIR="${BASE_BUILD_DIR}/bitcoin-$HOST/"
 7   CI_EXEC "python3 ${DIR_IWYU}/include-what-you-use/iwyu_tool.py"\
 8+          " src/common/args.cpp"\
 9           " src/common/init.cpp"\
10           " src/common/url.cpp"\
11           " src/compat"\
12diff --git a/src/common/args.cpp b/src/common/args.cpp
13index 42ea171a45..2aa378016b 100644
14--- a/src/common/args.cpp
15+++ b/src/common/args.cpp
16@@ -15,7 +15,6 @@
17 #include <util/settings.h>
18 #include <util/strencodings.h>
19 #include <util/string.h>
20-#include <util/time.h>
21 
22 #ifndef WIN32
23 #include <algorithm>
24@@ -28,17 +27,16 @@
25 
26 #include <univalue.h>
27 
28-#include <cstdint>
29 #include <cstdlib>
30 #include <cstring>
31 #include <filesystem>
32 #include <fstream>
33 #include <iostream>
34 #include <map>
35-#include <memory>
36 #include <optional>
37 #include <stdexcept>
38 #include <string>
39+#include <string_view>
40 #include <utility>
41 
42 const char * const BITCOIN_CONF_FILENAME = "bitcoin.conf";
43diff --git a/src/common/args.h b/src/common/args.h
44index 638bb7034e..35115e2dc4 100644
45--- a/src/common/args.h
46+++ b/src/common/args.h
47@@ -1,10 +1,6 @@
48 #ifndef BITCOIN_COMMON_ARGS_H
49 #define BITCOIN_COMMON_ARGS_H
50 
51-#if defined(HAVE_CONFIG_H)
52-#include <config/bitcoin-config.h>
53-#endif
54-
55 #include <compat/compat.h>
56 #include <sync.h>
57 #include <util/fs.h>
  11. in src/util/system.h:34 in c02e451882 outdated 
    29 #include <stdint.h>
30 #include <string>
31-#include <utility>
32-#include <vector>
33-
34-class ArgsManager;
    ryanofsky commented at 4:29 pm on April 5, 2023:

    In commit “refactor: Extract common/args from util/system” (c02e451882bb1d220b944af3b444d4b529ac351a)

    Would be good to drop the coment above line 7 “Server/client environment: argument handling, config file parsing”

  12. ryanofsky approved
  13. ryanofsky commented at 4:45 pm on April 5, 2023: contributor

    Code review ACK c02e451882bb1d220b944af3b444d4b529ac351a

    Just an option to consider, but I think could be good to move GetConfigFile, GetConfigOptions, ArgsManager::ReadConfigStream, and ArgsManager::ReadConfigFiles functions to src/common/config.cpp instead of src/common/args.cpp, since in longer run, it would be nice if it would be nice if there were a src/common/settings.cpp file to parse the settings file, src/common/config.cpp to parse the config file, and src/common/args.cpp that would parse arguments and depend on the settings and config modules. If we move the 4 config functions above now to src/common/config.cpp, it would avoid having to move them again later.

  14. fanquake commented at 7:19 pm on April 5, 2023: member
    Concept ACK
  15. TheCharlatan commented at 9:01 pm on April 5, 2023: contributor

    RE #27419#pullrequestreview-1373289140

    Just an option to consider, but I think could be good to move GetConfigFile, GetConfigOptions, ArgsManager::ReadConfigStream, and ArgsManager::ReadConfigFiles functions to src/common/config.cpp instead of src/common/args.cpp, since in longer run, it would be nice if it would be nice if there were a src/common/settings.cpp file to parse the settings file, src/common/config.cpp to parse the config file, and src/common/args.cpp that would parse arguments and depend on the settings and config modules. If we move the 4 config functions above now to src/common/config.cpp, it would avoid having to move them again later.

    I thought about doing this split and was a bit hesitant to include it here, because it is less of a pure move. Is https://github.com/bitcoin/bitcoin/commit/1c9adff530f0b31d2516b471b6ae25f78a08b436 something along the lines of what you meant? Couldn’t think if a less intrusive way to implement this besides declaring a friend class for now.

  16. TheCharlatan force-pushed on Apr 5, 2023
  17. TheCharlatan commented at 9:11 pm on April 5, 2023: contributor

    Updated c02e451882bb1d220b944af3b444d4b529ac351a -> de2546e672d0a2103228d777b35bfa6aab4881ee (splitSystemArgs_1 -> splitSystemArgs_2, compare):

    • Addressed @hebasto’s comment adding a missing copyright header
    • Addressed @hebasto’s comment creating a commit adding args.cpp to the iwyu list and tweaking some of the included headers
    • Addressed @ryanofsky ’s comment correcting a docstring
  18. ryanofsky approved
  19. ryanofsky commented at 4:35 am on April 6, 2023: contributor

    Code review ACK de2546e672d0a2103228d777b35bfa6aab4881ee. Just comment & iwyu change since last review.

    re: #27419 (comment)

    Sorry for the ambiguity, I was suggesting keeping it a pure move and just literally moving those functions to a src/common/config.cpp file without changing them so they don’t have to move again later. If you’re breaking up a big class in c++ you don’t have to move the methods and refactor the class at the same time, you can just move the methods.

    It is a judgement call whether you think a having single class split into 2 cpp files is worse than moving the same code twice. I think moving the same code twice is worse, obviously.

  20. ryanofsky commented at 3:10 pm on April 12, 2023: contributor
    I think this PR could be labeled move-only instead of refactoring, because no code is changing other than #includes.
  21. in src/util/system.cpp:6 in 19b3c124d6 outdated 
    2@@ -3,1011 +3,38 @@
3 // Distributed under the MIT software license, see the accompanying
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 
6+#include <logging.h>
    maflcko commented at 7:23 am on April 13, 2023:
    Any reason to change the order? Unless there is a reason, this shouldn’t be changed, because previously and commonly the first include in the cpp file is the corresponding header file to catch missing includes.
  22. maflcko commented at 7:23 am on April 13, 2023: member

    any reason there are two commits? Seems odd to first add unused includes and then remove them in the second commit.

    It might be better to not add the unused includes in the first place

  23. TheCharlatan force-pushed on Apr 18, 2023
  24. TheCharlatan commented at 5:32 pm on April 18, 2023: contributor

    Updated de2546e672d0a2103228d777b35bfa6aab4881ee -> ec51c252de42e11906f114ac05e476fd88ca2176 (splitSystemArgs_2 -> splitSystemArgs_3, compare):

  25. in src/common/args.cpp:20 in ec51c252de outdated 
    15+#include <util/settings.h>
16+#include <util/strencodings.h>
17+
18+#ifndef WIN32
19+#include <algorithm>
20+#include <cassert>
    ryanofsky commented at 5:40 pm on April 18, 2023:

    In commit “move-only: Extract common/args and common/config.cpp from util/system” (ec51c252de42e11906f114ac05e476fd88ca2176)

    Any reason these header are only included if WIN32 is not defined? I know the ifndef is preexisting, but it seems like it would make more sense to make these includes unconditional.

    TheCharlatan commented at 6:35 pm on April 18, 2023:
    I checked it and its history again and can’t think of a good reason either. Should I amend here, or go for the follow up?
    ryanofsky commented at 8:53 pm on April 18, 2023:

    I checked it and its history again and can’t think of a good reason either. Should I amend here, or go for the follow up?

    Either way seems fine. Would be nice to turn these into simple includes, but I was asking mostly out of curiosity

  26. in src/common/args.h:78 in ec51c252de outdated 
    73+};
74+
75+KeyInfo InterpretKey(std::string key);
76+
77+std::optional<util::SettingsValue> InterpretValue(const KeyInfo& key, const std::string* value,
78+                                                         unsigned int flags, std::string& error);
    ryanofsky commented at 5:52 pm on April 18, 2023:

    In commit “move-only: Extract common/args and common/config.cpp from util/system” (ec51c252de42e11906f114ac05e476fd88ca2176)

    Seems like extra whitespace this line.

    Note for reviewers: This function previously wasn’t exposed as part of the header, but was moved here as part of splitting up args and config file. It can go away with a followup change making config parsing functions more independent.

    ryanofsky commented at 3:36 pm on April 19, 2023:

    In commit “move-only: Extract common/args and common/config.cpp from util/system” (be55f545d53d44fdcf2d8ae802e9eae551d120c6)

    Not important, but there is still some extra whitespace on this line

  27. ryanofsky approved
  28. ryanofsky commented at 5:59 pm on April 18, 2023: contributor
    Code review ACK ec51c252de42e11906f114ac05e476fd88ca2176. Hoping this moveonly change could be reviewed and merged quickly since it conflicts with various PR’s I have open
  29. fanquake added this to the milestone 26.0 on Apr 18, 2023
  30. in ci/test/06_script_b.sh:45 in ec51c252de outdated 
    41@@ -42,6 +42,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
42   ( CI_EXEC run-clang-tidy-15 -quiet "${MAKEJOBS}" ) | grep -C5 "error"
43   export P_CI_DIR="${BASE_BUILD_DIR}/bitcoin-$HOST/"
44   CI_EXEC "python3 ${DIR_IWYU}/include-what-you-use/iwyu_tool.py"\
45+          " src/common/args.cpp"\
    maflcko commented at 6:45 pm on April 18, 2023: 
    0          " src/common/args.cpp"\
1          " src/common/config.cpp"\

    nit (if you retouch)

    TheCharlatan commented at 7:44 pm on April 18, 2023:

    config.cpp still produces the <fstream> false positive. Is that acceptable?

    EDIT: Whoops, forgot to escape the <.

    maflcko commented at 7:32 am on April 19, 2023:
    Yeah, I think it is good to know of false positives and then report them upstream. Otherwise they are less likely to be fixed.
  31. bitcoin deleted a comment on Apr 18, 2023
  32. bitcoin deleted a comment on Apr 18, 2023
  33. in src/common/args.cpp:27 in ec51c252de outdated 
    22+#include <codecvt>    /* for codecvt_utf8_utf16 */
23+#include <shellapi.h> /* for CommandLineToArgvW */
24+#include <shlobj.h>   /* for CSIDL_APPDATA */
25+#endif
26+
27+#include <univalue.h>
    maflcko commented at 8:04 am on April 19, 2023:
    Any reason for a separate section for this header?
  34. in src/node/context.h:10 in ec51c252de outdated 
     6@@ -7,6 +7,8 @@
 7 
 8 #include <kernel/context.h>
 9 
10+#include <util/system.h>
    maflcko commented at 8:15 am on April 19, 2023:
    Can you explain this change?
  35. maflcko added the label DrahtBot Guix build requested on Apr 19, 2023
  36. in src/util/system.h:7 in ec51c252de outdated 
    3@@ -4,8 +4,7 @@
4 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
5 
6 /**
7- * Server/client environment: argument handling, config file parsing,
8- * thread wrappers, startup time
9+ * Server/client environment, startup time
    maflcko commented at 8:24 am on April 19, 2023:
    nit: The comment is incomplete (missing AnyPtr stuff etc), and I don’t think it is a good use of time to make it complete. Also, the file has less than 10 functions and we don’t put those comments in other files, so might as well remove it.
  37. maflcko approved
  38. maflcko commented at 8:25 am on April 19, 2023: member

    review ACK ec51c252de42e11906f114ac05e476fd88ca2176 🦅

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK ec51c252de42e11906f114ac05e476fd88ca2176  🦅
30o6nucYAS/HVSSMijvjryysBuyCio5TbnDcNBbv5vgNSeWWPEXYDIv5BUC7sRVuZCfXkAEtNXghoTUB0D3ZDBg==
  39. move-only: Extract common/args and common/config.cpp from util/system 
    This is an extraction of ArgsManager related functions from util/system
into their own common file.

Config file related functions are moved to common/config.cpp.

The background of this commit is an ongoing effort to decouple the
libbitcoinkernel library from the ArgsManager. The ArgsManager belongs
into the common library, since the kernel library should not depend on
it. See doc/design/libraries.md for more information on this rationale.
    be55f545d5
  40. TheCharlatan force-pushed on Apr 19, 2023
  41. TheCharlatan commented at 10:04 am on April 19, 2023: contributor

    Thank you for the reviews, decided to address the left over nits here:

    Updated ec51c252de42e11906f114ac05e476fd88ca2176 -> be55f545d53d44fdcf2d8ae802e9eae551d120c6 (splitSystemArgs_3 -> splitSystemArgs_4, compare):

  42. TheCharlatan renamed this:
    refactor: Extract common/args from util/system
    move-only: Extract common/args from util/system
    on Apr 19, 2023
  43. DrahtBot commented at 2:19 pm on April 19, 2023: contributor

    Guix builds

    File commit 2fa7344aa9bbce8069ebd6bef5f6a22f0b7c5c56(master) commit e5d840f1fd4af7df25cdf40682e8e901e0e00132(master and this pull)
    SHA256SUMS.part f39c4aab7a75ae88... 8db0e7efbcc324e2...
    *-aarch64-linux-gnu-debug.tar.gz 6a8a9df7860d92b3... 65a0d4d8731dc21e...
    *-aarch64-linux-gnu.tar.gz ef4badcddf1fe9b9... 174e22e15c5d8c1b...
    *-arm-linux-gnueabihf-debug.tar.gz 1f082d00c7cbf8c5... c71b67d19511ca3c...
    *-arm-linux-gnueabihf.tar.gz a0a8a339ada2a975... 35b10d8c61286b31...
    *-powerpc64-linux-gnu-debug.tar.gz 7d41b00495ccc998... 3d12bbf9277da578...
    *-powerpc64-linux-gnu.tar.gz 67bbed17bc022b6d... c5374770cbbd1c3e...
    *-powerpc64le-linux-gnu-debug.tar.gz 7577e6eb6051d000... 9b60c7a535c7e98c...
    *-powerpc64le-linux-gnu.tar.gz ad4d0975d92dae94... 617e3b2e98c704ca...
    *-riscv64-linux-gnu-debug.tar.gz b063704406c70468... 94030fb3791f034e...
    *-riscv64-linux-gnu.tar.gz 83d5ec066fc6b49a... 1668c688c50b80b3...
    *-x86_64-linux-gnu-debug.tar.gz 201a187a4e15341b... 21558c25323b630e...
    *-x86_64-linux-gnu.tar.gz d0d4c3fab8f71c33... 660f27dcc8ff2d1d...
    *.tar.gz 5ce615208ca33fa3... 156682e1e3f80778...
    guix_build.log 1190b264b335453a... cff9cfc1d2f4e039...
    guix_build.log.diff 5f90a092717fef72...
  44. DrahtBot removed the label DrahtBot Guix build requested on Apr 19, 2023
  45. maflcko commented at 2:41 pm on April 19, 2023: member

    re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6 🚲

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6  🚲
3m22irb0PI71mDxsZbJ0k+W9xNfoLWj7bnpCW6Vbsib7wG5l6Im6jM1S1/Vu5XpYquBRBeE5T9aJH0baY8ydGCQ==
  46. DrahtBot requested review from ryanofsky on Apr 19, 2023
  47. maflcko commented at 2:46 pm on April 19, 2023: member
    unrelated: red CI can be ignored
  48. ryanofsky approved
  49. ryanofsky commented at 3:40 pm on April 19, 2023: contributor

    Code review ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6. Just small cleanups since the last review.

    Confirmed move-only except for making InterpretValue function nonstatic, and moving some symbols from util to common namespace.

  50. maflcko commented at 8:44 am on April 21, 2023: member
    Is this rfm with two reviews?
  51. hebasto approved
  52. hebasto commented at 9:57 am on April 21, 2023: member
    ACK be55f545d53d44fdcf2d8ae802e9eae551d120c6, I have reviewed the code and it looks OK, I agree it can be merged.
  53. hebasto commented at 10:02 am on April 21, 2023: member

    Hoping this moveonly change could be reviewed and merged quickly since it conflicts with various PR’s I have open

    So am I (meaning cmake staging branch) :D

  54. fanquake merged this on Apr 21, 2023
  55. fanquake closed this on Apr 21, 2023
  56. sidhujag referenced this in commit 6c03826a4d on Apr 21, 2023
  57. fanquake referenced this in commit 9564f98fee on May 30, 2023
  58. hebasto referenced this in commit 592da16150 on Aug 29, 2023
  59. hebasto referenced this in commit c77e8b5c38 on Aug 29, 2023
  60. hebasto referenced this in commit 8b025633e4 on Aug 29, 2023
  61. hebasto referenced this in commit 14ddf61693 on Aug 31, 2023
  62. bitcoin locked this on Apr 20, 2024


TheCharlatan DrahtBot maflcko hebasto ryanofsky fanquake

Labels
Refactoring

Milestone
26.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-07-03 10:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me