refactor: Split ArgsManager out of util/system #24455

Empact · 2022-03-01T17:21:40Z

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.

dongcarl

Concept ACK!

In fact I did almost the exact same thing here 😆: dongcarl@81d1857

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!

ryanofsky · 2022-03-01T19:01:18Z

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"

DrahtBot · 2022-03-02T06:22:13Z

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.

These were introduced in bitcoin#22950, but they're not used in the header, rather equivalent includes in addrman.cpp do the work.

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.

This is the more minimal include, and the only used therein.

To eliminate circular chainparamsbase dependencies.

Empact · 2022-04-23T22:50:00Z

See #24811 for the setup commits of this PR.

Empact · 2022-04-23T22:57:20Z

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.

DrahtBot · 2022-04-24T11:55:22Z

🐙 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".}

dongcarl · 2022-05-09T19:27:58Z

@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.

Empact · 2022-05-09T23:48:27Z

@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.

Empact · 2022-05-17T06:53:21Z

Closing in favor of my next attempt: #25152

dongcarl reviewed Mar 1, 2022

View reviewed changes

DrahtBot added Block storage Build system Consensus GUI Mining P2P Refactoring RPC/REST/ZMQ TX fees and policy Utils/log/libs UTXO Db and Indexes Validation Wallet labels Mar 1, 2022

This was referenced Mar 2, 2022

util: Make ArgsManager::GetPathArg more widely usable #24306

Merged

indexes: Stop using node internal types and locking cs_main, improve sync logic #24230

Draft

Add defaults to vDeployments to avoid uninitialized variables #24032

Merged

Empact force-pushed the 2022-03-util-args-manager branch from da3a3e7 to 6c653cc Compare March 2, 2022 13:52

fanquake removed GUI Wallet Build system TX fees and policy UTXO Db and Indexes RPC/REST/ZMQ P2P Mining Validation Consensus labels Mar 2, 2022

This was referenced Apr 19, 2022

wallet: Load database records in a particular order #24914

Merged

Isolate the storage abstraction layer from the application/serialization layer #24922

Closed

tidy: add include-what-you-use #24831

Merged

Empact force-pushed the 2022-03-util-args-manager branch 2 times, most recently from f12603b to a1b266d Compare April 22, 2022 16:58

Empact added 4 commits April 22, 2022 13:55

refactor: Drop unused logging includes from addrman_impl.h

f396b3d

These were introduced in bitcoin#22950, but they're not used in the header, rather equivalent includes in addrman.cpp do the work.

refactor: Remove util/system.h from dbwrapper.h

45485fc

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.

refactor: Remove logging.h include from net.h

b1fbcc8

refactor: Include logging rather than util/system in banman.cpp

fd9bfda

This is the more minimal include, and the only used therein.

Empact force-pushed the 2022-03-util-args-manager branch 5 times, most recently from f3a7954 to 9f8fe54 Compare April 22, 2022 21:50

Empact added 2 commits April 23, 2022 01:44

refactor: Split ArgsManager and gArgs out of util/system

e40d7f1

refactor: Move SetupChainParamsBaseOptions to util/args

bfcbe09

To eliminate circular chainparamsbase dependencies.

Empact force-pushed the 2022-03-util-args-manager branch from 9f8fe54 to bfcbe09 Compare April 23, 2022 06:44

DrahtBot mentioned this pull request Apr 23, 2022

BIP324: Add encrypted p2p transport {de}serializer #23233

Closed

Empact marked this pull request as ready for review April 23, 2022 22:48

DrahtBot added the Needs rebase label Apr 24, 2022

Empact closed this May 17, 2022

Empact mentioned this pull request May 17, 2022

refactor: Split util/system into exception, shell, and fs-specific files #25152

Closed

Empact mentioned this pull request Oct 12, 2022

refactor: Extract util/exception from util/system #26301

Closed

bitcoin locked and limited conversation to collaborators May 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Split ArgsManager out of util/system #24455

refactor: Split ArgsManager out of util/system #24455

Uh oh!

Empact commented Mar 1, 2022 •

edited

Loading

Uh oh!

dongcarl left a comment •

edited

Loading

Uh oh!

ryanofsky commented Mar 1, 2022

Uh oh!

DrahtBot commented Mar 2, 2022 •

edited

Loading

Uh oh!

Empact commented Apr 23, 2022

Uh oh!

Empact commented Apr 23, 2022

Uh oh!

DrahtBot commented Apr 24, 2022

Uh oh!

dongcarl commented May 9, 2022 •

edited

Loading

Uh oh!

Empact commented May 9, 2022

Uh oh!

Empact commented May 17, 2022

Uh oh!

Uh oh!

refactor: Split ArgsManager out of util/system #24455

refactor: Split ArgsManager out of util/system #24455

Uh oh!

Conversation

Empact commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dongcarl left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Mar 1, 2022

Uh oh!

DrahtBot commented Mar 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

Empact commented Apr 23, 2022

Uh oh!

Empact commented Apr 23, 2022

Uh oh!

DrahtBot commented Apr 24, 2022

Uh oh!

dongcarl commented May 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented May 9, 2022

Uh oh!

Empact commented May 17, 2022

Uh oh!

Uh oh!

Empact commented Mar 1, 2022 •

edited

Loading

dongcarl left a comment •

edited

Loading

DrahtBot commented Mar 2, 2022 •

edited

Loading

dongcarl commented May 9, 2022 •

edited

Loading