refactor: Split ArgsManager out of util/system #24455

pull Empact wants to merge 12 commits into bitcoin:master from Empact:2022-03-util-args-manager changing 115 files +1489 −1379
  1. Empact commented at 5:21 pm on March 1, 2022: member

    By splitting ArgsManager and related functions out of the grab-bag util/system file, this enables more specific includes, ~referencing ArgsManager without pulling in gArgs~, and eliminate a circular dependency between chainparamsbase and utils/system.

    The proposed new modules are:

    • util/args_manager - ArgsManager and related functions
    • util/args - gArgs only - separate to support the goal of limiting the scope of gArgs
    • util/system - a variety of file system and sytem-related functions

    Other notable points:

    • I find it a bit concerning that ArgsManager::ReadConfigFiles calls ClearPathCache and CheckDataDirOption on gArgs rather than the current ArgsManager instance in the status quo.

    The new files are nearly unchanged from their sources, so git diff master --color-moved=dimmed-zebra could be helpful.

  2. dongcarl commented at 5:35 pm on March 1, 2022: member

    Concept ACK!

    In fact I did almost the exact same thing here 😆: https://github.com/dongcarl/bitcoin/commit/81d1857599b51e82cc63ae734b7834085661cf99

    Happy to use your changes though if you’re planning on proactively rebasing over master when merge conflicts arise!

    One thing: the only difference I can spot between your changes and mine is that you didn’t move BITCOIN_CONF_FILENAME and BITCOIN_SETTINGS_FILENAME out of util/system.h, I think doing that makes the split even better? Lmk!

  3. DrahtBot added the label Block storage on Mar 1, 2022
  4. DrahtBot added the label Build system on Mar 1, 2022
  5. DrahtBot added the label Consensus on Mar 1, 2022
  6. DrahtBot added the label GUI on Mar 1, 2022
  7. DrahtBot added the label Mining on Mar 1, 2022
  8. DrahtBot added the label P2P on Mar 1, 2022
  9. DrahtBot added the label Refactoring on Mar 1, 2022
  10. DrahtBot added the label RPC/REST/ZMQ on Mar 1, 2022
  11. DrahtBot added the label TX fees and policy on Mar 1, 2022
  12. DrahtBot added the label Utils/log/libs on Mar 1, 2022
  13. DrahtBot added the label UTXO Db and Indexes on Mar 1, 2022
  14. DrahtBot added the label Validation on Mar 1, 2022
  15. DrahtBot added the label Wallet on Mar 1, 2022
  16. ryanofsky commented at 7:01 pm on March 1, 2022: member

    This is pretty reasonable. I know we don’t generally like to move code around, but this move was probably inevitable.

    One suggestion would be to rename “util/args_manager” to “util/args”. I think “_manager” suffix does not contribute or clarify much, and will make it awkward to add other argument-handling functions and classes to the same file without attaching them to the ArgsManager class. ArgsManager class is already monolithic and it should be possible to break up or extend without putting everything inside.

    Related to this, I don’t necessarily see a need to put the gArgs variable in its own file, but if this is needed to get rid of circular dependencies, or if it just seems like a good idea, I’d suggest putting it in “util/gargs” or “util/args_global” instead of “util/args”

  17. DrahtBot commented at 6:22 am on March 2, 2022: member

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

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #24922 (Isolate the storage abstraction layer from the application/serialization layer by TheQuantumPhysicist)
    • #24915 (lint: Convert lint-circular-dependencies.sh to Python by Smlep)
    • #24914 (wallet: Load database records in a particular order by achow101)
    • #24845 (wallet: createTransaction, return proper error description for “too-long-mempool-chain” + introduce generic Result classes by furszy)
    • #24831 (tidy: add include-what-you-use by fanquake)
    • #24830 (init: Allow -proxy="" setting values by ryanofsky)
    • #24812 (util/check: Add CHECK_NONFATAL identity function and NONFATAL_UNREACHABLE macro by aureleoules)
    • #24764 (Modernize util/strencodings and util/string: string_view and optional by sipa)
    • #24757 (build, ci: add DEBUG_LOCKCONTENTION to –enable-debug and CI by jonatack)
    • #24742 ([POC] build: prune Boost headers in depends by fanquake)
    • #24676 ([WIP] [kernelheaders 1/n] Cleave LevelDB headers from our header tree by dongcarl)
    • #24675 (util: Use ArgsManager::GetPathArg more widely by hebasto)
    • #24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg by luke-jr)
    • #24232 (assumeutxo: add init and completion logic by jamesob)
    • #24230 (indexes: Stop using node internal types and locking cs_main, improve sync logic by ryanofsky)
    • #23561 (BIP324: Handshake prerequisites by dhruv)
    • #23444 (fuzz: Add regression test for wallet crash by MarcoFalke)
    • #23233 (BIP324: Add encrypted p2p transport {de}serializer by dhruv)
    • #22953 (refactor: introduce single-separator split helper (boost::split replacement) by theStack)
    • #20205 (wallet: Properly support a wallet id by achow101)
    • #19792 (rpc: Add dumpcoinstats by fjahr)
    • #17786 (refactor: Nuke policy/fees->mempool circular dependencies by hebasto)
    • #16673 (Relog configuration args on debug.log rotation by LarryRuane)
    • #15936 (Unify bitcoin-qt and bitcoind persistent settings 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.

  18. Empact force-pushed on Mar 2, 2022
  19. fanquake removed the label GUI on Mar 2, 2022
  20. fanquake removed the label Wallet on Mar 2, 2022
  21. fanquake removed the label Build system on Mar 2, 2022
  22. fanquake removed the label TX fees and policy on Mar 2, 2022
  23. fanquake removed the label UTXO Db and Indexes on Mar 2, 2022
  24. fanquake removed the label RPC/REST/ZMQ on Mar 2, 2022
  25. fanquake removed the label P2P on Mar 2, 2022
  26. fanquake removed the label Mining on Mar 2, 2022
  27. fanquake removed the label Validation on Mar 2, 2022
  28. fanquake removed the label Consensus on Mar 2, 2022
  29. fanquake removed the label Block storage on Mar 2, 2022
  30. Empact force-pushed on Mar 2, 2022
  31. DrahtBot added the label Needs rebase on Mar 3, 2022
  32. Empact force-pushed on Mar 3, 2022
  33. Empact force-pushed on Mar 3, 2022
  34. DrahtBot removed the label Needs rebase on Mar 3, 2022
  35. Empact commented at 5:13 pm on March 3, 2022: member
    @ryanofsky thanks, I consolidated args_manager into args - I agree the cost in complexity weighs against the minor benefit of limiting gArgs access.
  36. Empact commented at 5:17 pm on March 3, 2022: member

    In fact I did almost the exact same thing here 😆: https://github.com/dongcarl/bitcoin/commit/81d1857599b51e82cc63ae734b7834085661cf99 @dongcarl Nice! I’m open to rebasing this, and I think it’d be positive to evaluate it independently. I could go either way on the filenames - with this args is the larger file, and system includes a variety of filesystem-related functionality. 🤷

  37. DrahtBot added the label Needs rebase on Mar 7, 2022
  38. dongcarl added this to the "WIP PRs" column in a project
  39. dongcarl moved this from the "WIP PRs" to the "Relevant External PRs" column in a project
  40. kiminuo commented at 3:35 pm on March 28, 2022: contributor

    Concept ACK, nice work

    In commit 066ba007bed96552cfdc6a6ed91199ddc1cace8c (refactor: Split ArgsManager out of util/system), I wonder whether it would be good to introduce a constant for -avoidpartialspends to reduce a risk of a typo.

    Also this PR feels big, would it be reasonable to split it into multiple PRs? I mean having a PR for 066ba007bed96552cfdc6a6ed91199ddc1cace8c is quite easy to review and it can get code review ACKs almost immediately imho. Plus one would avoid the need for many rebases which this big PR risks. I can probably give a hand with it if you need/want.

  41. Empact commented at 7:43 pm on March 30, 2022: member
    @kiminuo thanks, re “this PR feels big” I think you make a good point, I’ll split out a prep work PR. As for the risk of typos, that hasn’t been a practice elsewhere in the codebase, but I have an idea or two of an alternative that could apply across the board.
  42. Empact force-pushed on Apr 8, 2022
  43. Empact marked this as a draft on Apr 8, 2022
  44. Empact force-pushed on Apr 8, 2022
  45. Empact force-pushed on Apr 8, 2022
  46. DrahtBot removed the label Needs rebase on Apr 8, 2022
  47. DrahtBot added the label Needs rebase on Apr 15, 2022
  48. Empact force-pushed on Apr 15, 2022
  49. Empact force-pushed on Apr 15, 2022
  50. DrahtBot removed the label Needs rebase on Apr 15, 2022
  51. refactor: Don't reference gArgs inside of CCoinControl 
    This changes the wallet tests to rely on BasicTestingSetup#m_args
rather than gArgs, which seems more appropriate.
    7af48eb8d2
  52. refactor: Don't reference gArgs inside of CheckDataDirOption 2bab5a4740
  53. refactor: Call ClearPathCache/CheckDataDirOption on the current args, not gArgs 
    In ArgsManager::ReadConfigFiles, we're operating on an ArgsManager - modifying
gArgs is incongruous.
    5f214be3b7
  54. refactor: Don't reference gArgs inside AbsPathForConfigVal & GetConfigFile 94a4ac0474
  55. refactor: Move error from util/system.h to logging.h cfff4b78bf
  56. refactor: Don't reference gArgs in SelectBaseParams 3b8999a21b
  57. Empact force-pushed on Apr 17, 2022
  58. Empact force-pushed on Apr 22, 2022
  59. Empact force-pushed on Apr 22, 2022
  60. refactor: Drop unused logging includes from addrman_impl.h 
    These were introduced in #22950, but they're not used in the header,
rather equivalent includes in addrman.cpp do the work.
    f396b3d3fd
  61. refactor: Remove util/system.h from dbwrapper.h 
    This file is not required for the dbwrapper interfaces provided, but several other files were getting their necessary includes indirectly via this header.

Removing results in more minimal includes throughout.
    45485fcf95
  62. refactor: Remove logging.h include from net.h b1fbcc8ead
  63. refactor: Include logging rather than util/system in banman.cpp 
    This is the more minimal include, and the only used therein.
    fd9bfdab74
  64. Empact force-pushed on Apr 22, 2022
  65. Empact force-pushed on Apr 22, 2022
  66. Empact force-pushed on Apr 22, 2022
  67. Empact force-pushed on Apr 22, 2022
  68. Empact force-pushed on Apr 22, 2022
  69. refactor: Split ArgsManager and gArgs out of util/system e40d7f1963
  70. refactor: Move SetupChainParamsBaseOptions to util/args 
    To eliminate circular chainparamsbase dependencies.
    bfcbe0956c
  71. Empact force-pushed on Apr 23, 2022
  72. Empact marked this as ready for review on Apr 23, 2022
  73. Empact commented at 10:50 pm on April 23, 2022: member
    See #24811 for the setup commits of this PR.
  74. Empact commented at 10:57 pm on April 23, 2022: member
    This PR impacting the need for util/system includes led me to audit those, and the necessity of them. I could perhaps reorder the commits or do a follow up to shrink it down further.
  75. DrahtBot added the label Needs rebase on Apr 24, 2022
  76. DrahtBot commented at 11:55 am on April 24, 2022: member

    🐙 This pull request conflicts with the target branch and needs rebase.

    Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

  77. dongcarl commented at 7:27 pm on May 9, 2022: member

    @Empact I’m wondering if you’d be open to splitting off the last two commits off to their own PR (or just this PR) not based on #24811?

    I think “removing references to gArgs and header tree cleanup” (which is what #24811 is doing) probably stands on its own merit and can be done either before or after the “split util/args and util/system” work here.

    Selfishly, I also want to get the last 2 commits reviewed and merged ASAP since it’ll soon become a blocker for libbitcoinkernel work.

  78. Empact commented at 11:48 pm on May 9, 2022: member

    @Empact I’m wondering if you’d be open to splitting off the last two commits off to their own PR (or just this PR) not based on #24811?

    Sure, I’ll be happy to try that, but I think some of the prior work is necessary to enable the separation. I’ll let you know what I find.

  79. Empact commented at 6:53 am on May 17, 2022: member
    Closing in favor of my next attempt: https://github.com/bitcoin/bitcoin/pull/25152
  80. Empact closed this on May 17, 2022
  81. Empact moved this from the "Relevant External PRs" to the "Done or Closed or Rethinking" column in a project
  82. DrahtBot locked this on May 17, 2023


Empact dongcarl ryanofsky DrahtBot kiminuo

Labels
Refactoring Utils/log/libs Needs rebase

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-10-04 19:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me