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

Empact commented at 6:52 am on May 17, 2022: member

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 commented at 6:54 am on May 17, 2022: member

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

Empact force-pushed on May 17, 2022

DrahtBot added the label Refactoring on May 17, 2022

Empact force-pushed on May 17, 2022

dongcarl commented at 5:33 pm on May 17, 2022: contributor

Concept ACK

I think you can fix this CI failure with:

 0diff --git a/src/Makefile.am b/src/Makefile.am
 1index 3d6143d1ee..d06e51852e 100644
 2--- a/src/Makefile.am
 3+++ b/src/Makefile.am
 4@@ -923,6 +923,7 @@ libbitcoinkernel_la_SOURCES = \
 5   util/rbf.cpp \
 6   util/serfloat.cpp \
 7   util/settings.cpp \
 8+  util/shell.cpp \
 9   util/strencodings.cpp \
10   util/string.cpp \
11   util/syscall_sandbox.cpp \

Empact force-pushed on May 18, 2022

Empact force-pushed on May 19, 2022

Empact commented at 5:35 am on May 19, 2022: member

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.

Empact force-pushed on May 21, 2022

Empact force-pushed on May 24, 2022

DrahtBot commented at 1:28 pm on May 24, 2022: 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
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.

Empact force-pushed on May 24, 2022

Empact force-pushed on May 25, 2022

Empact force-pushed on Jun 7, 2022

Empact force-pushed on Jun 8, 2022

Empact force-pushed on Jun 9, 2022

DrahtBot added the label Needs rebase on Jun 10, 2022

Empact force-pushed on Jun 16, 2022

DrahtBot removed the label Needs rebase on Jun 16, 2022

Empact force-pushed on Jun 16, 2022

Empact force-pushed on Jun 18, 2022

DrahtBot added the label Needs rebase on Jun 21, 2022

TheCharlatan commented at 11:33 am on September 18, 2022: contributor

Code review ACK 2355b9c

I reproduced much of the code move / split diff manually. The splitting makes sense to me, I had to overthink the split of functions into logging.* and exception.* , but keeping platform specific logging things in util/ (as done for the exceptions) seems sane to me. @Empact is there any platform testing I can do to get this PR moving again?

Empact force-pushed on Oct 10, 2022

DrahtBot removed the label Needs rebase on Oct 10, 2022

Empact force-pushed on Oct 10, 2022

Empact force-pushed on Oct 11, 2022

Empact marked this as ready for review on Oct 12, 2022

Empact marked this as a draft on Oct 12, 2022

DrahtBot added the label Needs rebase on Oct 19, 2022

refactor: Extract util/exception from util/system

This is a minimal extraction of a single public method, but also
the only use of std::exception in util/system.

Include libloaderapi.h for GetModuleFileNameA as that's the
source noted here: https://docs.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea

f12cd1d77b

refactor: Extract util/shell from util/system

This is a minimal extraction of shell-specific functions
from util/system.

e5243d3676

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

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

f1daaf1d89

refactor: Extract util/fs from util/system 7745b29ca4

refactor: Extract util/args from util/system 9ff4bb56ba

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

Empact force-pushed on Jan 10, 2023

DrahtBot removed the label Needs rebase on Jan 10, 2023

in src/util/shell.cpp:26 in b070db4f1f

21+#include <ostream>
22+#include <vector>
23+#if defined(__GNUC__)
24+#pragma GCC diagnostic pop
25+#endif
26+#endif // ENABLE_EXTERNAL_SIGNER

TheCharlatan commented at 10:20 pm on January 17, 2023:

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?

fanquake commented at 10:48 am on January 18, 2023:

in e5243d3676136e4b0a590e76a07c86e62ca7d1f7:

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.

in src/util/exception.cpp:15 in f12cd1d77b outdated

10+
11+#include <iostream>
12+#include <typeinfo>
13+
14+#ifdef WIN32
15+#include <compat/compat.h>

fanquake commented at 10:44 am on January 18, 2023:

In f12cd1d77bc062f1313f98a8aaea8afd96c4f52e: 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.

in src/util/shell.cpp:38 in e5243d3676 outdated

33+#include <cstdlib>
34+#include <stdexcept>
35+#include <string>
36+
37+#ifdef WIN32
38+#include <compat/compat.h>

fanquake commented at 10:49 am on January 18, 2023:

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

in src/util/shell.h:15 in e5243d3676 outdated

10+#include <config/bitcoin-config.h>
11+#endif
12+
13+#include <string>
14+
15+class UniValue;

fanquake commented at 10:51 am on January 18, 2023:

In e5243d3676136e4b0a590e76a07c86e62ca7d1f7: Why is UniValue in here?

in test/lint/lint-circular-dependencies.py:40 in 9ff4bb56ba outdated

36@@ -37,7 +37,7 @@ def main():
37 
38     os.chdir(CODE_DIR)
39     files = subprocess.check_output(
40-        ['git', 'ls-files', '--', '*.h', '*.cpp'],
41+        ["git", "ls-files", "--", "*.h", "*.cpp"],

fanquake commented at 10:54 am on January 18, 2023:

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

DrahtBot commented at 4:26 pm on January 18, 2023: contributor

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

DrahtBot added the label Needs rebase on Jan 18, 2023

TheCharlatan commented at 3:02 pm on March 7, 2023: contributor

I redid this work (fixing more of the includes and splitting out a few more functions) and have a branch over here: https://github.com/TheCharlatan/bitcoin/pull/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.

christ79ma approved

fanquake commented at 7:48 pm on March 7, 2023: member

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.

fanquake referenced this in commit b175bdb9b2 on Mar 14, 2023

fanquake commented at 11:37 am on March 14, 2023: member

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

fanquake closed this on Mar 14, 2023

fanquake referenced this in commit 369d4c03b7 on Apr 3, 2023

bitcoin locked this on Mar 13, 2024

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

Reviews

Conflicts