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

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.

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

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 \

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.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

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

<!--174a7506f384e20aa4161008e828411d-->

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.

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?

<!--cf906140f33d8803c4a75a2196329ecb-->

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

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.

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.

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