refactor: Extract util/fs from util/system

TheCharlatan commented at 11:11 am on March 14, 2023: contributor

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 (https://github.com/bitcoin/bitcoin/pull/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 commented at 11:11 am on March 14, 2023: 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
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.

DrahtBot added the label Refactoring on Mar 14, 2023

in src/util/fs.h:14 in b67efe778d outdated

 9+#include <fs.h>
10+
11+#include <iosfwd>
12+#include <limits>
13+#include <stdint.h>
14+#include <stdio.h>

maflcko commented at 11:25 am on March 14, 2023:

0#include <cstdint>
1#include <cstdio>

nit

hebasto commented at 1:02 pm on March 29, 2023:

Considering switching s/stdio.h/cstdio/, does it make sense to s/FILE/std::FILE/ as well? Maybe as a follow up?

in src/flatfile.cpp:13 in b67efe778d outdated

13 
14-FlatFileSeq::FlatFileSeq(fs::path dir, const char* prefix, size_t chunk_size) :
15-    m_dir(std::move(dir)),
16-    m_prefix(prefix),
17-    m_chunk_size(chunk_size)
18+FlatFileSeq::FlatFileSeq(fs::path dir, const char* prefix, size_t chunk_size) : m_dir(std::move(dir)),

maflcko commented at 11:26 am on March 14, 2023:

unrelated change?

TheCharlatan commented at 11:32 am on March 14, 2023:

This is from clang format diff. Should I drop these kind of changes in the future? There are also some other not strictly related include order fixes.

fanquake commented at 11:34 am on March 14, 2023:

I think include fixing is fine. However theres no need to include unrelated clang-format changes.

maflcko commented at 11:34 am on March 14, 2023:

You’ll need to set -U0 for the context to avoid unrelated changes, no?

TheCharlatan commented at 11:44 am on March 14, 2023:

I’m formatting each commit with git diff -U0 HEAD~1.. | ./contrib/devtools/clang-format-diff.py -p1 -i -v. I think I must have included this unrelated change somewhere. Will fix.

in src/util/fs.h:63 in b67efe778d outdated

58+
59+#ifdef WIN32
60+fs::path GetSpecialFolderPath(int nFolder, bool fCreate = true);
61+#endif
62+
63+#endif // BITCOIN_UTIL_FS_H

fanquake commented at 11:30 am on March 14, 2023:

nit: missing newline

in src/test/util_tests.cpp:5 in b67efe778d outdated

1@@ -2,7 +2,7 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5-#include <util/system.h>
6+#include <util/fs.h>

fanquake commented at 11:33 am on March 14, 2023:

Pretty sure this can just be merged into the includes below.

TheCharlatan force-pushed on Mar 14, 2023

TheCharlatan commented at 12:08 pm on March 14, 2023: contributor

Thanks for the review!

Updated b67efe778d4e4ad359fb8dccea7f92e17d1b1f95 -> ea278094e9937bf96157161941d38a0c08a0aa4e (splitSystemFs_0 -> splitSystemFs_1, compare) to address review comments.

ryanofsky commented at 12:43 pm on March 14, 2023: contributor

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 force-pushed on Mar 15, 2023

TheCharlatan commented at 9:58 am on March 15, 2023: contributor

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

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

TheCharlatan force-pushed on Mar 15, 2023

TheCharlatan commented at 11:45 am on March 15, 2023: contributor

Updated dac4d1dc3d75d510efe6c84c2cb6d7d7dc96e2c6 -> b22cf8d563a469e29c9942b4fd9f93351d8b9766 (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 commented at 12:15 pm on March 15, 2023: contributor

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 (fac5c373006a9e4bcbb56843bb85f1aca4d87599). EDIT: Ah, I found #26256 . So no formatting in scripted diffs then?

maflcko commented at 12:50 pm on March 15, 2023: member

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?

DrahtBot added the label Needs rebase on Mar 15, 2023

TheCharlatan force-pushed on Mar 20, 2023

TheCharlatan commented at 10:49 am on March 20, 2023: contributor

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

DrahtBot removed the label Needs rebase on Mar 20, 2023

hebasto commented at 1:43 pm on March 20, 2023: member

Concept ACK.

in ci/test/06_script_b.sh:73 in cc662d0938 outdated

69@@ -70,6 +70,7 @@ if [ "${RUN_TIDY}" = "true" ]; then
70           " src/util/check.cpp"\
71           " src/util/error.cpp"\
72           " src/util/exception.cpp"\
73+          " src/util/fs.cpp"\

hebasto commented at 1:55 pm on March 20, 2023:

3665c1bdf7b9d47ccab39916c13a8cd9dfbf0eea

There is no src/util/fs.cpp file at this commit. It is renamed in the last commit only.

in src/Makefile.am:157 in cc662d0938 outdated

153@@ -154,7 +154,7 @@ BITCOIN_CORE_H = \
154   deploymentstatus.h \
155   external_signer.h \
156   flatfile.h \
157-  fs.h \
158+  util/fs.h \

hebasto commented at 1:56 pm on March 20, 2023:

Sort files after renaming?

TheCharlatan force-pushed on Mar 20, 2023

TheCharlatan commented at 2:41 pm on March 20, 2023: contributor

Thank you for the review @hebasto,

Updated cc662d09386e50eb92a7ccbaa1edff62fd8cdd44 -> 3ceb8dde48fc12f4f16372f661a4cccf7104e194 (splitSystemFs_5 -> splitSystemFs_6, compare) to fix sorting in the Makefile and remove fs.cpp, which was still supposed to refer to fs_helpers.cpp, from the iwyu list in the CI script. I did not re-include fs_helpers.cpp in the list, because iwyu produced false positives when including <fstream>.

hebasto approved

hebasto commented at 6:58 pm on March 20, 2023: member

ACK 3ceb8dde48fc12f4f16372f661a4cccf7104e194

The commit “Add missing fs.h include…” could address another IWYU suggestion:

0--- a/src/node/chainstate.cpp
1+++ b/src/node/chainstate.cpp
2@@ -16,6 +16,7 @@
3 #include <tinyformat.h>
4 #include <txdb.h>
5 #include <uint256.h>
6+#include "util/fs.h"
7 #include <util/time.h>
8 #include <util/translation.h>
9 #include <validation.h>

in src/util/fs_helpers.cpp:252 in 59c02e68f7 outdated

236+#ifdef WIN32
237+fs::path GetSpecialFolderPath(int nFolder, bool fCreate)
238+{
239+    WCHAR pszPath[MAX_PATH] = L"";
240+
241+    if (SHGetSpecialFolderPathW(nullptr, pszPath, nFolder, fCreate)) {

ryanofsky commented at 6:12 pm on March 22, 2023:

In commit “refactor: Extract util/fs_helpers from util/system” (59c02e68f75f7b727162d2ea36bf3ed99fabf035)

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.

TheCharlatan commented at 7:48 pm on March 22, 2023:

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!

maflcko commented at 8:41 am on March 23, 2023:

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

ryanofsky approved

ryanofsky commented at 6:19 pm on March 22, 2023: contributor

Code review ACK 3ceb8dde48fc12f4f16372f661a4cccf7104e194

in src/util/fs_helpers.cpp:27 in 3ceb8dde48 outdated

22+#include <memory>
23+#include <string>
24+#include <system_error>
25+#include <utility>
26+
27+#ifdef WIN32

TheCharlatan commented at 11:41 am on March 23, 2023:

The _POSIX_C_SOURCE should be moved here too.

refactor: Extract util/fs_helpers from util/system

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.

18fb36367a

Add missing cstddef include in assumptions.h

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

b202b3dd63

Add missing fs.h includes

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

106b46d9d2

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

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.

00e9b97f37

TheCharlatan force-pushed on Mar 23, 2023

TheCharlatan commented at 1:49 pm on March 23, 2023: contributor

Updated 3ceb8dde48fc12f4f16372f661a4cccf7104e194 -> 00e9b97f37e0bdf4c647236838c10b68b7ad5be3 (splitSystemFs_6 -> splitSystemFs_7, compare):

Addressed my comment by moving the _POSIX_C_SOURCE handling to fs_helpers. These lines were introduced in https://github.com/bitcoin/bitcoin/commit/288fdc092aff9d7e0cea159196b2e96044a786c7 for code that is now moved to fs_helpers.
Addressed @hebasto comment by adding another missing include.

Apologies for going another round here.

fanquake requested review from hebasto on Mar 29, 2023

in src/qt/intro.cpp:21 in 00e9b97f37

17 #include <qt/optionsmodel.h>
18 
19 #include <interfaces/node.h>
20+#include <util/fs_helpers.h>
21 #include <util/system.h>
22 #include <validation.h>

hebasto commented at 1:22 pm on March 29, 2023:

nit:

 0--- a/src/qt/intro.cpp
 1+++ b/src/qt/intro.cpp
 2@@ -6,16 +6,16 @@
 3 #include <config/bitcoin-config.h>
 4 #endif
 5 
 6-#include <chainparams.h>
 7 #include <qt/intro.h>
 8 #include <qt/forms/ui_intro.h>
 9-#include <util/fs.h>
10 
11 #include <qt/guiconstants.h>
12 #include <qt/guiutil.h>
13 #include <qt/optionsmodel.h>
14 
15+#include <chainparams.h>
16 #include <interfaces/node.h>
17+#include <util/fs.h>
18 #include <util/fs_helpers.h>
19 #include <util/system.h>
20 #include <validation.h>

hebasto approved

hebasto commented at 1:22 pm on March 29, 2023: member

ACK 00e9b97f37e0bdf4c647236838c10b68b7ad5be3

DrahtBot requested review from ryanofsky on Mar 29, 2023

fanquake merged this on Apr 3, 2023

fanquake closed this on Apr 3, 2023

sidhujag referenced this in commit 1f2e4b6647 on Apr 3, 2023

fanquake referenced this in commit 669af32632 on Apr 21, 2023

fanquake referenced this in commit b2c85bd82f on May 15, 2023

sidhujag referenced this in commit 707bbb439e on May 15, 2023

fanquake referenced this in commit 9564f98fee on May 30, 2023

bitcoin locked this on Apr 2, 2024

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

Context

Changes

Reviews

Conflicts