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

TheCharlatan · 2023-03-14T11:11:00Z

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.

DrahtBot · 2023-03-14T11:11:03Z

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
ACK	hebasto
Stale ACK	ryanofsky

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:

#27302 (init: Error if ignored bitcoin.conf file is found by ryanofsky)
#27294 (refactor: Move chain names to the util library by TheCharlatan)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#27231 (Fix logging RPC and -debugexclude with 0/none values, add test coverage, improve docs by jonatack)
#26832 (compat: move (win) S_* defines into bdb by fanquake)
#26715 (Introduce MockableDatabase for wallet unit tests by achow101)
#26697 (logging: use bitset for categories by LarryRuane)
#26654 (util: Show descriptive error messages when FileCommit fails by john-moffett)
#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)
#24539 (Add a "tx output spender" index by sstone)
#23561 (BIP324: Handshake prerequisites by dhruv)
#19792 (rpc: Add dumpcoinstats by fjahr)

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.

src/util/fs.h

src/flatfile.cpp

src/util/fs.h

src/test/util_tests.cpp

TheCharlatan · 2023-03-14T12:08:50Z

Thanks for the review!

Updated b67efe7 -> ea27809 (splitSystemFs_0 -> splitSystemFs_1, compare) to address review comments.

ryanofsky · 2023-03-14T12:43:39Z

Agree these functions should not be combined into existing src/fs.h since they are doing higher level things than accessing the filesystem. But instead of having two fs.h files in different locations, I would move src/fs.h to src/util/fs.h and move these functions to src/util/fs_misc.h or another place like that.

TheCharlatan · 2023-03-15T09:58:46Z

Updated ea27809 -> c4f2b6d (splitSystemFs_1 -> splitSystemFs_2, compare) to rename util/fs.* to util/fs_helpers.*

Added commit dac4d1d (splitSystemFs_3) as a scripted diff moving src/fs.* to src/util/fs.* as requested by @ryanofsky in #27254 (comment) .

TheCharlatan · 2023-03-15T11:45:00Z

Updated dac4d1d -> b22cf8d (splitSystemFs_3 -> splitSystemFs_4, compare) to fix a broken rename and include an additional header in compat/assumptions.h. Up until this point compat/assumptions.h relied on being included after compat/compat.h. clang-format-diff revealed this by changing the order of the includes. I'm not sure if this inclusion order has to be preserved.

TheCharlatan · 2023-03-15T12:15:15Z

Besides the remaining CI failure, it looks like the lint job does not support clang-format-diff. @MarcoFalke is this the case? I took the formatting line from one of your commits (fac5c37).
EDIT: Ah, I found #26256 . So no formatting in scripted diffs then?

maflcko · 2023-03-15T12:50:56Z

I guess it can be added back? No strong opinion. Though, we might want to look into docker image caching to avoid the apt overhead.

sed -i '11 i #include <cstddef>' src/compat/assumptions.h

Not sure if this should be hidden like this. A separate commit or pull request seems better for this bugfix. Also, could add it to ci iwyu for reviewers to check for other missing includes?

TheCharlatan · 2023-03-20T10:49:23Z

Rebased b22cf8d -> cc662d0 (splitSystemFs_4 -> splitSystemFs_5, compare) to fix conflicts and update the commit message to exclude the scripted diff.

ryanofsky · 2023-03-22T18:12:50Z

src/util/fs_helpers.cpp

+{
+    WCHAR pszPath[MAX_PATH] = L"";
+
+    if (SHGetSpecialFolderPathW(nullptr, pszPath, nFolder, fCreate)) {


In commit "refactor: Extract util/fs_helpers from util/system" (59c02e6)

Anybody have advice on easiest way to review changes like this when blocks of code are moving but also changing slightly at the same time?

I find it pretty easy to review blocks of moved code in git with the --color-moved option, but once code is moved and reformatted at the same time (the { at the end of this line was previously on its own line) it becomes very tedious to find the matching blocks of old and new code, and then compare them to make sure the differences are not significant.

If there isn't some more convenient way to review changes like this, I would definitely appreciate it if the code could just be moved and not reformatted.

If there isn't some more convenient way to review changes like this, I would definitely appreciate it if the code could just be moved and not reformatted.

ACK, I will stop using clang-format-diff on all moved code. Thank you for the review!

You could do it in a separate commit, after the move

ryanofsky

Code review ACK 3ceb8dd

src/util/fs_helpers.cpp

This is an extraction of filesystem related functions from util/system into their own utility file. The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager defined in system.h. Moving these functions out of system.h allows including them from a separate source file without including the ArgsManager definitions from system.h.

The inclusion of this header should not depend on the inclusion of other headers that include cstddef themselves.

The inclusion of this header should not depend on the inclusion of other headers that include fs.h themselves.

The fs.* files are already part of the libbitcoin_util library. With the introduction of the fs_helpers.* it makes sense to move fs.* into the util/ directory as well.

TheCharlatan · 2023-03-23T13:49:29Z

Updated 3ceb8dd -> 00e9b97 (splitSystemFs_6 -> splitSystemFs_7, compare):

Addressed my comment by moving the _POSIX_C_SOURCE handling to fs_helpers. These lines were introduced in 288fdc0 for code that is now moved to fs_helpers.
Addressed @hebasto comment by adding another missing include.

Apologies for going another round here.

hebasto

ACK 00e9b97

hebasto · 2023-03-29T13:22:00Z

src/qt/intro.cpp

 #include <chainparams.h>
-#include <fs.h>
 #include <qt/intro.h>
 #include <qt/forms/ui_intro.h>
+#include <util/fs.h>

 #include <qt/guiconstants.h>
 #include <qt/guiutil.h>
 #include <qt/optionsmodel.h>

 #include <interfaces/node.h>
+#include <util/fs_helpers.h>
 #include <util/system.h>
 #include <validation.h>


nit:

--- a/src/qt/intro.cpp +++ b/src/qt/intro.cpp @@ -6,16 +6,16 @@ #include <config/bitcoin-config.h> #endif -#include <chainparams.h> #include <qt/intro.h> #include <qt/forms/ui_intro.h> -#include <util/fs.h> #include <qt/guiconstants.h> #include <qt/guiutil.h> #include <qt/optionsmodel.h> +#include <chainparams.h> #include <interfaces/node.h> +#include <util/fs.h> #include <util/fs_helpers.h> #include <util/system.h> #include <validation.h>

…/system be55f54 move-only: Extract common/args and common/config.cpp from util/system (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin/bitcoin#24303 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". It is part of a series of patches splitting up the `util/system` files. Its preceding pull request is bitcoin/bitcoin#27254. The pull request contains an extraction of ArgsManager related functions from util/system into their own common/ file. The background of this commit is an ongoing effort to decouple the libbitcoinkernel library from the ArgsManager. The ArgsManager belongs into the common library, since the kernel library should not depend on it. See [doc/design/libraries.md](https://github.com/bitcoin/bitcoin/blob/master/doc/design/libraries.md) for more information on this rationale. ACKs for top commit: MarcoFalke: re-ACK be55f54 🚲 ryanofsky: Code review ACK be55f54. Just small cleanups since the last review. hebasto: ACK be55f54, I have reviewed the code and it looks OK, I agree it can be merged. Tree-SHA512: 90eb03334af0155b823030b4f2ecf286d35058d700ee2ddbbaa445be19e31eb0fe982656f35bd14ecee3ad2c3d0db3746855cb8f3777eff7253713e42873e111

3ece0eb build, doc: Adjust comment after PR27254 (Hennadii Stepanov) Pull request description: This is a follow up for bitcoin/bitcoin#27254. ACKs for top commit: TheCharlatan: ACK 3ece0eb Tree-SHA512: 36c627524304f0ea964383488acb9b16d0b553cc9cf3bce7a12aec65a7905b13e4582b7b7ec6d1efa32cd3c623969bc00f805ff31d9e6eec86a614e75796c8ff

3ece0eb build, doc: Adjust comment after PR27254 (Hennadii Stepanov) Pull request description: This is a follow up for bitcoin#27254. ACKs for top commit: TheCharlatan: ACK 3ece0eb Tree-SHA512: 36c627524304f0ea964383488acb9b16d0b553cc9cf3bce7a12aec65a7905b13e4582b7b7ec6d1efa32cd3c623969bc00f805ff31d9e6eec86a614e75796c8ff

…ibrary, interface_ui from validation. 7d3b350 refactor: Move system from util to common library (TheCharlatan) 7eee356 refactor: Split util::AnyPtr into its own file (TheCharlatan) 44de325 refactor: Split util::insert into its own file (TheCharlatan) 9ec5da3 refactor: Move ScheduleBatchPriority to its own file (TheCharlatan) f871c69 kernel: Add warning method to notifications (TheCharlatan) 4452707 kernel: Add progress method to notifications (TheCharlatan) 84d7145 kernel: Add headerTip method to notifications (TheCharlatan) 447761c kernel: Add notification interface (TheCharlatan) Pull request description: This pull request is part of the `libbitcoinkernel` project bitcoin/bitcoin#27587 https://github.com/bitcoin/bitcoin/projects/18 and more specifically its "Step 2: Decouple most non-consensus code from libbitcoinkernel". --- It removes the kernel library's dependency on `util/system` and `interface_ui`. `util/system` contains networking and shell-related code that should not be part of the kernel library. The following pull requests prepared `util/system` for this final step: bitcoin/bitcoin#27419 bitcoin/bitcoin#27254 bitcoin/bitcoin#27238. `interface_ui` defines functions for a more general node interface and has a dependency on `boost/signals2`. After applying the patches from this pull request, the kernel's reliance on boost is down to `boost::multiindex`. The approach implemented here introduces some indirection, which makes the code a bit harder to read. Any suggestions for improving or reworking this pull request to make it more concise, or even reworking it into a more proper interface, are appreciated. ACKs for top commit: MarcoFalke: re-ACK 7d3b350 (no change) 🎋 stickies-v: Code Review ACK 7d3b350 hebasto: re-ACK 7d3b350, only last two commits dropped since my [recent](bitcoin/bitcoin#27636 (review)) review. Tree-SHA512: c8cfc698dc9d78e20191c444708f2d957501229abe95e5806106d1126fb9c5fbcee686fb55645658c0107ce71f10646f37a2fdf7fde16bbf22cbf1ac885dd08d

DrahtBot added the Refactoring label Mar 14, 2023

maflcko reviewed Mar 14, 2023

View reviewed changes

src/util/fs.h Outdated Show resolved Hide resolved

src/flatfile.cpp Outdated Show resolved Hide resolved

fanquake reviewed Mar 14, 2023

View reviewed changes

src/util/fs.h Outdated Show resolved Hide resolved

fanquake reviewed Mar 14, 2023

View reviewed changes

src/test/util_tests.cpp Outdated Show resolved Hide resolved

TheCharlatan force-pushed the splitSystemFs branch from b67efe7 to ea27809 Compare March 14, 2023 12:08

This was referenced Mar 14, 2023

util: Show descriptive error messages when FileCommit fails #26654

Merged

sync: introduce a thread-safe generic container and use it to remove a bunch of "GlobalMutex"es #25390

Closed

wallet: Load database records in a particular order #24914

Merged

TheCharlatan force-pushed the splitSystemFs branch from ea27809 to dac4d1d Compare March 15, 2023 09:57

TheCharlatan force-pushed the splitSystemFs branch from dac4d1d to b22cf8d Compare March 15, 2023 11:44

DrahtBot added the Needs rebase label Mar 15, 2023

TheCharlatan force-pushed the splitSystemFs branch from b22cf8d to cc662d0 Compare March 20, 2023 10:47

DrahtBot removed the Needs rebase label Mar 20, 2023

This was referenced Mar 21, 2023

BIP324: Enable v2 P2P encrypted transport #24545

Closed

BIP324: Handshake prerequisites #23561

Closed

refactor: Move chain names to the util library #27294

Closed

ryanofsky reviewed Mar 22, 2023

View reviewed changes

ryanofsky approved these changes Mar 22, 2023

View reviewed changes

TheCharlatan commented Mar 23, 2023

View reviewed changes

src/util/fs_helpers.cpp Outdated Show resolved Hide resolved

Empact and others added 4 commits March 23, 2023 12:52

Add missing cstddef include in assumptions.h

b202b3d

The inclusion of this header should not depend on the inclusion of other headers that include cstddef themselves.

Add missing fs.h includes

106b46d

The inclusion of this header should not depend on the inclusion of other headers that include fs.h themselves.

refactor: Move fs.* to util/fs.*

00e9b97

The fs.* files are already part of the libbitcoin_util library. With the introduction of the fs_helpers.* it makes sense to move fs.* into the util/ directory as well.

TheCharlatan force-pushed the splitSystemFs branch from 3ceb8dd to 00e9b97 Compare March 23, 2023 13:48

DrahtBot mentioned this pull request Mar 27, 2023

init: Error if ignored bitcoin.conf file is found #27302

Merged

fanquake requested a review from hebasto March 29, 2023 10:48

hebasto approved these changes Mar 29, 2023

View reviewed changes

DrahtBot requested a review from ryanofsky March 29, 2023 13:22

fanquake merged commit 369d4c0 into bitcoin:master Apr 3, 2023

sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Apr 3, 2023

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

1f2e4b6

hebasto mentioned this pull request Apr 3, 2023

build: Add CMake-based build system #25797

Closed

16 tasks

TheCharlatan mentioned this pull request Apr 4, 2023

move-only: Extract common/args from util/system #27419

Merged

hebasto mentioned this pull request Apr 19, 2023

Rebased cmake-staging branch (post PR#13) hebasto/bitcoin#14

Closed

This was referenced May 12, 2023

Chainstate rm kernel UI interface 0 TheCharlatan/bitcoin#12

Closed

kernel: Remove util/system from kernel library, interface_ui from validation. #27636

Merged

hebasto mentioned this pull request May 14, 2023

build, doc: Adjust comment after PR27254 #27656

Merged

ryanofsky mentioned this pull request May 24, 2023

refactor: Replace std::optional<bilingual_str> with util::Result #25977

Merged

bitcoin locked and limited conversation to collaborators Apr 2, 2024

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

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

Uh oh!

Conversation

TheCharlatan commented Mar 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes

Uh oh!

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

Uh oh!

Reviews

Conflicts

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

TheCharlatan commented Mar 14, 2023

Uh oh!

ryanofsky commented Mar 14, 2023

Uh oh!

TheCharlatan commented Mar 15, 2023

Uh oh!

TheCharlatan commented Mar 15, 2023

Uh oh!

TheCharlatan commented Mar 15, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

maflcko commented Mar 15, 2023

Uh oh!

TheCharlatan commented Mar 20, 2023

Uh oh!

ryanofsky Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

TheCharlatan Mar 22, 2023

Choose a reason for hiding this comment

Uh oh!

maflcko Mar 23, 2023

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan commented Mar 23, 2023

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

hebasto Mar 29, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

TheCharlatan commented Mar 14, 2023 •

edited

Loading

DrahtBot commented Mar 14, 2023 •

edited

Loading

TheCharlatan commented Mar 15, 2023 •

edited

Loading