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

Empact · 2022-05-17T06:52:25Z

This is an attempt to significantly improve code organization while making changes as simple as possible to review.

Here I split four files out of util/system:

util/exception, holding PrintExceptionContinue
util/shell, holding ShellEscape, runCommand
util/fs, holding various file, folder, and path-specific functions (building on fs.h and filesystem)
util/args, holding ArgsManager and related functions

The goal was to minimize review burden, so I made an effort to minimize the diffs and only modify the code when it was easy to confirm that the change would not have negative consequences.

That said, sourcing all of the windows-related headers was not straightforward, so I expect I'll need to touch this up to correct any issues there.

Empact · 2022-05-17T06:54:11Z

See prior discussion in #24455 /cc @dongcarl @ryanofsky

dongcarl · 2022-05-17T17:33:35Z

Concept ACK

I think you can fix this CI failure with:

diff --git a/src/Makefile.am b/src/Makefile.am
index 3d6143d1ee..d06e51852e 100644
--- a/src/Makefile.am
+++ b/src/Makefile.am
@@ -923,6 +923,7 @@ libbitcoinkernel_la_SOURCES = \
   util/rbf.cpp \
   util/serfloat.cpp \
   util/settings.cpp \
+  util/shell.cpp \
   util/strencodings.cpp \
   util/string.cpp \
   util/syscall_sandbox.cpp \

Empact · 2022-05-19T05:35:40Z

Thanks @dongcarl - I've got some issues to figure out, particularly on the WIN32 side, and I'll be doing it via CI so it's going to mean some back and forth on this PR. E.g. the change is 5 commits but I'm going to have to play them out one by one to validate them.

DrahtBot · 2022-05-24T13:28:09Z

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
Concept ACK	dongcarl
Stale ACK	TheCharlatan

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:

#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
#26720 (wallet: coin selection, don't return results that exceed the max allowed weight by furszy)
#26715 (Introduce MockableDatabase for wallet unit tests by achow101)
#26690 (wallet: Refactor database cursor into its own object with proper return codes by achow101)
#26654 (util: Show descriptive error messages when FileCommit fails by john-moffett)
#26504 (Add UNREACHABLE macro and drop -Wreturn-type/C4715 warnings suppressions by hebasto)
#26226 (Bump minimum python version to 3.7 by MarcoFalke)
#26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)
#25390 (sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es by vasild)
#24914 (wallet: Load database records in a particular order by achow101)
#24545 (BIP324: Enable v2 P2P encrypted transport by dhruv)
#24479 (Bugfix: util: Correctly handle Number value types in GetArg/GetBoolArg by luke-jr)
#24128 (wallet: BIP 326 sequence based anti-fee-snipe for taproot inputs by MarcoFalke)
#23561 (BIP324: Handshake prerequisites by dhruv)
#22341 (rpc: add getxpub by Sjors)
#19463 (Prune locks by luke-jr)
#16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
#15294 (refactor: Extract RipeMd160 by Empact)
#14485 (Try to use posix_fadvise with CBufferedFile by luke-jr)

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.

error is a low-level function with a sole dependency on LogPrintf, which is defined in logging.h

TheCharlatan · 2023-01-17T22:20:09Z

src/util/shell.cpp

+#ifdef ENABLE_EXTERNAL_SIGNER
+#if defined(__GNUC__)
+// Boost 1.78 requires the following workaround.
+// See: https://github.com/boostorg/process/issues/235
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnarrowing"
+#endif
+#include <boost/process.hpp>
+#include <memory>
+#include <ostream>
+#include <vector>
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
+#endif // ENABLE_EXTERNAL_SIGNER


AFAICT the build failures are coming from including these lines and I don't think they are needed, so I'm guessing this was included to debug a windows problem? I'm keen on moving this PR forward,. Is there some deeper problem here with running this patch on windows, or is it a compile-time issue?

in e5243d3:

and I don't think they are needed,

Yea, there is no Boost usage in this file (and is no-longer any in the util library), so the entire ENABLE_EXTERNAL_SIGNER chunk here should be removed.

fanquake · 2023-01-18T10:44:46Z

src/util/exception.cpp

+#include <typeinfo>
+
+#ifdef WIN32
+#include <compat/compat.h>


In f12cd1d:
Why is compat being included here? if it's for a windows.h include (which isn't in compat anyways), then I'd rather we just include windows.h.

fanquake · 2023-01-18T10:48:04Z

src/util/shell.cpp

+#ifdef ENABLE_EXTERNAL_SIGNER
+#if defined(__GNUC__)
+// Boost 1.78 requires the following workaround.
+// See: https://github.com/boostorg/process/issues/235
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wnarrowing"
+#endif
+#include <boost/process.hpp>
+#include <memory>
+#include <ostream>
+#include <vector>
+#if defined(__GNUC__)
+#pragma GCC diagnostic pop
+#endif
+#endif // ENABLE_EXTERNAL_SIGNER


in e5243d3:

and I don't think they are needed,

Yea, there is no Boost usage in this file (and is no-longer any in the util library), so the entire ENABLE_EXTERNAL_SIGNER chunk here should be removed.

fanquake · 2023-01-18T10:49:47Z

src/util/shell.cpp

+#include <string>
+
+#ifdef WIN32
+#include <compat/compat.h>


In e5243d3:
If this is for windows.h, would rather just include that.

fanquake · 2023-01-18T10:51:31Z

src/util/shell.h

+
+#include <string>
+
+class UniValue;


In e5243d3: Why is UniValue in here?

fanquake · 2023-01-18T10:54:15Z

test/lint/lint-circular-dependencies.py

@@ -37,7 +37,7 @@ def main():

    os.chdir(CODE_DIR)
    files = subprocess.check_output(
-        ['git', 'ls-files', '--', '*.h', '*.cpp'],
+        ["git", "ls-files", "--", "*.h", "*.cpp"],


In 9ff4bb5:
This seems unrelated to this commit, and is not mentioned in the commit message.

DrahtBot · 2023-01-18T16:26:29Z

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

TheCharlatan · 2023-03-07T15:02:32Z

I redid this work (fixing more of the includes and splitting out a few more functions) and have a branch over here: TheCharlatan#10

@Empact I'm still keen on moving this work forward. If you don't have the time right now, I'll attribute the commits to you and try to get my branch merged instead.

fanquake · 2023-03-07T19:48:47Z

I'm still keen on moving this work forward. If you don't have the time right now, I'll attribute the commits to you and try to get my branch merged instead.

Yep, it would be good to start getting some of this merged.

aaced56 refactor: Move error() from util/system.h to logging.h (Ben Woosley) e7333b4 refactor: Extract util/exception from util/system (Ben Woosley) Pull request description: 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". These commits were originally authored by empact and are taken from their parent PR #25152. #### Context There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (#25290, #25487, #25527, #25862, #26177, and #27125). The `ArgsManager` is defined in `system.h`. #### Changes Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving some logging functions out of the `system.*` files. Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed. ACKs for top commit: MarcoFalke: re-ACK aaced56 🐍 Tree-SHA512: cb39f4cb7a77e7dc1887b1cbf340d53decab8880fc00878a2f12dc627fe67245b4aafd4cc31a9eab0fad1e5bb5d0eb4cdb8d501323ca200fa6ab7b201ae34aea

fanquake · 2023-03-14T11:37:18Z

Going to close this for now, given that it's being split out, and there's a thumbs up from the author above.

00e9b97 refactor: Move fs.* to util/fs.* (TheCharlatan) 106b46d Add missing fs.h includes (TheCharlatan) b202b3d Add missing cstddef include in assumptions.h (TheCharlatan) 18fb363 refactor: Extract util/fs_helpers from util/system (Ben Woosley) Pull request description: 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". This commit was originally authored by empact and is taken from its parent PR #25152. #### Context There is an ongoing effort to decouple the `ArgsManager` used for command line parsing user-provided arguments from the libbitcoinkernel library (#25290, #25487, #25527, #25862, #26177, and #27125). The `ArgsManager` is defined in `system.h`. A similar pull request extracting functionality from `system.h` has been merged in #27238. #### Changes Next to providing better code organization, this PR removes some reliance of the tree of libbitcoinkernel header includes on `system.h` (and thus the `ArgsManager` definition) by moving filesystem related functions out of the `system.*` files. There is already a pair of `fs.h` / `fs.cpp` in the top-level `src/` directory. They were not combined with the files introduced here, to keep the patch cleaner and more importantly because they are often included without the utility functions. The new files are therefore named `fs_helpers` and the existing `fs` files are moved into the util directory. Further commits splitting more functionality out of `system.h` are still in #25152 and will be submitted in separate PRs once this PR has been processed. ACKs for top commit: hebasto: ACK 00e9b97 Tree-SHA512: 31422f148d14ba3c843b99b1550a6fd77c77f350905ca324f93d4f97b652246bc58fa9696c64d1201979cf88733e40be02d262739bb7d417cf22bf506fdb7666

Empact mentioned this pull request May 17, 2022

refactor: Split ArgsManager out of util/system #24455

Closed

Empact force-pushed the 2022-05-split-system branch from 017ee15 to 0280e82 Compare May 17, 2022 07:03

DrahtBot added the Refactoring label May 17, 2022

Empact force-pushed the 2022-05-split-system branch 7 times, most recently from 17389de to 44361e9 Compare May 17, 2022 16:38

Empact force-pushed the 2022-05-split-system branch 2 times, most recently from 8792c7a to 1fc8ae3 Compare May 19, 2022 05:33

Empact force-pushed the 2022-05-split-system branch 5 times, most recently from afac05c to 60fd43c Compare May 24, 2022 12:02

This was referenced May 24, 2022

util: Move error message formatting of NonFatalCheckError to cpp #25112

Merged

Re-enable external signer support for Windows #25111

Closed

test: Unit tests for taproot/tapscript coverage in interpreter.cpp #25097

Closed

Empact force-pushed the 2022-05-split-system branch 2 times, most recently from e84125b to 7ead1da Compare May 25, 2022 05:42

This was referenced May 28, 2022

Handle invalid hex encoding in ParseHex #25227

Merged

refactor: Make FEELER_SLEEP_WINDOW type safe (std::chrono) #24974

Merged

Empact force-pushed the 2022-05-split-system branch 2 times, most recently from aa51c7b to c434603 Compare June 7, 2022 18:48

Empact added 4 commits January 10, 2023 12:35

refactor: Move error() from util/system.h to logging.h

f1daaf1

error is a low-level function with a sole dependency on LogPrintf, which is defined in logging.h

refactor: Extract util/fs from util/system

7745b29

refactor: Extract util/args from util/system

9ff4bb5

refactor: Drop now-unnecessary includes of util/system.h

b070db4

Empact force-pushed the 2022-05-split-system branch from 0e2858f to b070db4 Compare January 10, 2023 19:46

DrahtBot removed the Needs rebase label Jan 10, 2023

TheCharlatan reviewed Jan 17, 2023

View reviewed changes

DrahtBot mentioned this pull request Jan 18, 2023

Bump minimum python version to 3.7 #26226

Merged

fanquake reviewed Jan 18, 2023

View reviewed changes

DrahtBot added the Needs rebase label Jan 18, 2023

christ79ma approved these changes Mar 7, 2023

View reviewed changes

TheCharlatan mentioned this pull request Mar 10, 2023

refactor: Split logging utilities from system.h #27238

Merged

TheCharlatan mentioned this pull request Mar 14, 2023

refactor: Extract util/fs from util/system #27254

Merged

fanquake closed this Mar 14, 2023

bitcoin locked and limited conversation to collaborators Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

Empact commented May 17, 2022 •

edited

Loading

Uh oh!

Empact commented May 17, 2022

Uh oh!

dongcarl commented May 17, 2022

Uh oh!

Empact commented May 19, 2022

Uh oh!

DrahtBot commented May 24, 2022 •

edited

Loading

Uh oh!

TheCharlatan Jan 17, 2023

Uh oh!

fanquake Jan 18, 2023

Uh oh!

fanquake Jan 18, 2023

Uh oh!

fanquake Jan 18, 2023

Uh oh!

fanquake Jan 18, 2023

Uh oh!

fanquake Jan 18, 2023

Uh oh!

fanquake Jan 18, 2023

Uh oh!

DrahtBot commented Jan 18, 2023

Uh oh!

TheCharlatan commented Mar 7, 2023

Uh oh!

fanquake commented Mar 7, 2023

Uh oh!

fanquake commented Mar 14, 2023

Uh oh!

Uh oh!


		#include <string>

		class UniValue;

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

Empact commented May 17, 2022

Uh oh!

dongcarl commented May 17, 2022

Uh oh!

Empact commented May 19, 2022

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

TheCharlatan Jan 17, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

fanquake Jan 18, 2023

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jan 18, 2023

Uh oh!

TheCharlatan commented Mar 7, 2023

Uh oh!

fanquake commented Mar 7, 2023

Uh oh!

fanquake commented Mar 14, 2023

Uh oh!

Uh oh!

Empact commented May 17, 2022 •

edited

Loading

DrahtBot commented May 24, 2022 •

edited

Loading