fanquake
commented at 2:27 pm on December 22, 2020:
member
This PR replaces our Boost Filesystem usage with std::filesystem and includes dropping Boost System as a dependency. It includes a squashed down version of the changes from #19245.
A macro has been added, modelling how we check for -latomic to facilitate linking with -lstdc++fs when required. This is ~GCC < 9.1 & ~Clang < 9.0, however not always. i.e you could be using Clang 7 on top of a GCC 9 installation (i.e Ubuntu Focal) and use <filesystem> without passing any additional arguments. I’ve tested this with GCC 8 on Bionic, Clang 7 on Focal & with Apple Clang 12.0.0 on macOS.
fanquake added the label
Refactoring
on Dec 22, 2020
fanquake added the label
Build system
on Dec 22, 2020
MarcoFalke closed this
on Dec 22, 2020
MarcoFalke reopened this
on Dec 22, 2020
MarcoFalke added the label
Waiting for other
on Dec 22, 2020
in
configure.ac:79
in
2733fe31baoutdated
75@@ -76,6 +76,9 @@ AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory])
76 dnl Check if -latomic is required for <std::atomic>
77 CHECK_ATOMIC
7879+dnl check if -lstdc++fs is required for <std::filesystem>
nit: std::filesystem is part of C++17, maybe not worth mentioning explicitly.
ryanofsky
commented at 5:07 pm on February 5, 2021:
In commit “build: remove boost::filesystem” (9f37a39b11b240f75b97fb565e6d8cf44972ec49)
This change seems like it belongs in previous commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7) instead of this one.
fanquake force-pushed
on Dec 22, 2020
MarcoFalke removed the label
Waiting for other
on Dec 22, 2020
in
ci/test/00_setup_env_native_nowallet.sh:10
in
ac8a44674boutdated
6@@ -7,8 +7,8 @@
7 export LC_ALL=C.UTF-8
8 9 export CONTAINER_NAME=ci_native_nowallet
10-export DOCKER_NAME_TAG=ubuntu:18.04 # Use bionic to have one config run the tests in python3.6, see doc/dependencies.md
11-export PACKAGES="python3-zmq clang-5.0 llvm-5.0" # Use clang-5 to test C++17 compatibility, see doc/dependencies.md
12+export DOCKER_NAME_TAG=ubuntu:20.04
MarcoFalke
commented at 2:54 pm on December 22, 2020:
Would be nice to not change this. clang-7 is also in bionic:
MarcoFalke
commented at 12:36 pm on February 9, 2021:
Still not addressed?
fanquake
commented at 12:38 pm on February 9, 2021:
That’s why it hasn’t been marked as resolved.
fanquake
commented at 12:53 pm on February 9, 2021:
This has now been dropped.
in
ci/test/00_setup_env_win64.sh:10
in
ac8a44674boutdated
6@@ -7,7 +7,7 @@
7 export LC_ALL=C.UTF-8
8 9 export CONTAINER_NAME=ci_win64
10-export DOCKER_NAME_TAG=ubuntu:18.04 # Check that bionic can cross-compile to win64 (bionic is used in the gitian build as well)
11+export DOCKER_NAME_TAG=ubuntu:20.04
MarcoFalke
commented at 2:55 pm on December 22, 2020:
the comment should stay to say that this is the version used in gitian as well. I presume this pull is blocked (waiting on) gitian getting bumped to focal? Thus the label “waiting for other”.
MarcoFalke
commented at 2:58 pm on December 22, 2020:
Oh, and this has to be bumped in the cirrus yaml as well (Sorry, confusing, but I didn’t find a way around this redundancy)
fanquake
commented at 3:07 pm on December 22, 2020:
Oh, and this has to be bumped in the cirrus yaml as well
Thanks. Did not realise that was required.
fanquake
commented at 12:33 pm on February 9, 2021:
Gitian has now been bumped.
MarcoFalke
commented at 2:56 pm on December 22, 2020:
member
Concept ACK for 0.23 (earliest), given that this again requires a bunch of aggressive bumps.
in
src/rpc/blockchain.cpp:2399
in
ac8a44674boutdated
2392@@ -2393,10 +2393,10 @@ static RPCHelpMan dumptxoutset()
2393 },
2394 [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue
2395 {
2396- fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir());
2397+ fs::path path = fs::absolute(GetDataDir() / request.params[0].get_str());
2398 // Write to a temporary path and then move into `path` on completion
2399 // to avoid confusion due to an interruption.
2400- fs::path temppath = fs::absolute(request.params[0].get_str() + ".incomplete", GetDataDir());
2401+ fs::path temppath = fs::absolute(GetDataDir() / request.params[0].get_str() / ".incomplete");
ryanofsky
commented at 4:36 pm on February 5, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7)
Would be more reassuring that behavior was not changing if tests could stay the same. Would be useful to know the reason for changes to this test if they are can’t be reverted.
fanquake
commented at 12:44 pm on February 9, 2021:
Why this change? Does it break with + “/”?
Yes, at ef8c98b1a54280f279f5319f386c44026893bf1f this currently fails with the trailing slash:
0error: in "util_tests/util_datadir": check dd_norm== GetDataDir() has failed ["/var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_common_Bitcoin Core/873b1e644e770a9d5cdfc8f89323a344256001e7cedc14e18f2ae7c4a8ea8f94" !="/var/folders/z2/cn877pxd3czdfh47mfkmbwgm0000gn/T/test_common_Bitcoin Core/873b1e644e770a9d5cdfc8f89323a344256001e7cedc14e18f2ae7c4a8ea8f94/"]
ryanofsky
commented at 11:41 pm on February 11, 2021:
Yes, at ef8c98b this currently fails with the trailing slash:
Thanks, I think this can be marked resolved, but I it would be good to add a warning in the commit description to alert reviewers and people trying to track down regressions in the future (in case there are bugs), like: “Warning: Replacing fs::system_complete calls with fs::absolute calls in this commit may cause minor changes in behavior because fs::absolute no longer strips trailing slashes, but these changes are believed to be safe.”
If we really wanted to be paranoid here, we could add a wrapper function emulating the old behavior, but I think the new behavior actually makes more sense (avoids gratuitous changes to strings), and is not likely to cause any serious problems.
fanquake
commented at 1:49 am on February 17, 2021:
I’ve added to the commit message, so will mark this as resolved.
fanquake force-pushed
on Dec 22, 2020
in
src/wallet/wallettool.cpp:108
in
8d148b93bboutdated
Why not just GetWalletDir() / name even if name is empty?
See #20744 (comment), this avoids adding an empty path component and trailing slash if name is empty. This code should be simpler when this is rebased after #20932
DrahtBot
commented at 8:43 pm on December 22, 2020:
member
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Conflicts
Reviewers, this pull request conflicts with the following ones:
#23931 (create bitcoin.conf on first run with template by prayank23)
#22472 (fuzz: Add environment option to keep /tmp/ clean by agroce)
#22380 (build: add and use C_STANDARD and CXX_STANDARD in depends by fanquake)
#17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
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.
in
src/wallet/wallet.cpp:3783
in
8d148b93bboutdated
3779@@ -3780,11 +3780,11 @@ std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, cons
3780 // 2. Path to an existing directory.
3781 // 3. Path to a symlink to a directory.
3782 // 4. For backwards compatibility, the name of a data file in -walletdir.
3783- const fs::path& wallet_path = fs::absolute(name, GetWalletDir());
3784+ const fs::path& wallet_path = fs::absolute((name == "") ? GetWalletDir() : (GetWalletDir() / name));
mjdietzx
commented at 3:33 pm on December 23, 2020:
same: “Why not just GetWalletDir() / name even if name is empty?”
ryanofsky
commented at 8:03 pm on January 21, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (f449c26f30bb8e223d4d7833c579d555a0005daa)
same: “Why not just GetWalletDir() / name even if name is empty?”
See #20744 (comment), this avoids adding an empty path component and trailing slash if name is empty. This code should be simpler when this is rebased after #20932
in
src/util/system.cpp:793
in
8d148b93bboutdated
742- if (fNetSpecific)
743- path /= BaseParams().DataDir();
744+ if (fNetSpecific) {
745+ if (!BaseParams().DataDir().empty()) {
746+ path /= BaseParams().DataDir();
747+ }
mjdietzx
commented at 3:40 pm on December 23, 2020:
similar to “Why not just GetWalletDir() / name even if name is empty?”, is another if branch really needed here?
ryanofsky
commented at 8:05 pm on January 21, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (f449c26f30bb8e223d4d7833c579d555a0005daa)
mjdietzx
commented at 4:00 pm on December 23, 2020:
contributor
Nice‼️ Concept Ack
I didn’t have any problems building and running some tests locally on my machine. Also, there’s a minor TODO that kind of got lost in the mix here. I had a PR for it when we were on boost #20265
Issue should be resolved when this is rebased after #20932.
fanquake
commented at 1:17 am on February 10, 2021:
This has been rebased post #20932, so marking this as resolved.
ryanofsky
commented at 5:53 pm on December 23, 2020:
member
This seems good, but it would be nice to move code changes to their own PR separate from the build changes, so they can get better review and so the final code is simpler. (It would be nice if final code wasn’t checking for empty strings multiple places and wasn’t calling fs::absolute on paths already known to be absolute.)
I think an initial PR simplifying this one would just replace fs::system_complete(x) with fs::absolute(x) and replace fs::absolute(x, y) with AbsPathJoin(y, x) everywhere. AbsPathJoin would be a util function defined like:
Also I think it is clearer and better to avoid calling fs::absolute on paths that we already know are absolute. Ideally we would go further and never call fs::absolute or fs::system_complete after daemonization. Behavior of code shouldn’t change depending on the process working directory, and in most cases it makes sense for usability to interpret paths relative to the data directory or wallet directory, not the process working directory.
practicalswift
commented at 11:05 pm on December 27, 2020:
contributor
Concept ACK
DrahtBot added the label
Needs rebase
on Jan 7, 2021
DrahtBot
commented at 11:46 am on January 13, 2021:
member
🕵️ @laanwj@harding have been requested to review this pull request as specified in the REVIEWERS file.
kiminuo referenced this in commit
72205add2a
on Jan 15, 2021
kiminuo referenced this in commit
8fbb7a8151
on Jan 15, 2021
kiminuo referenced this in commit
215c784650
on Jan 15, 2021
kiminuo referenced this in commit
d867d7a64c
on Jan 15, 2021
kiminuo referenced this in commit
da9caa1ced
on Jan 15, 2021
MarcoFalke referenced this in commit
45952dab9d
on Jan 21, 2021
ryanofsky
commented at 8:22 pm on January 21, 2021:
member
Now that #20932 is merged, the messier path joining code and bugs here can disappear when this is rebased. The call sites where AbsPathJoin is used should no longer have to change. And the AbsPathJoin implementation can simply be changed to return path.empty() ? base : base / path instead of fs::absolute(path, base).
kiminuo
commented at 10:23 pm on January 21, 2021:
contributor
I have attempted to do the rebase with the fsbridge::AbsPathJoin modifications:
0test 2021-01-22T13:56:39.722000Z TestFramework.node0 (DEBUG): Node stopped
1 test 2021-01-22T13:56:39.723000Z TestFramework.node1 (DEBUG): Node stopped
2 test 2021-01-22T13:56:39.723000Z TestFramework (WARNING): Not cleaning up dir /home/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193
3 test 2021-01-22T13:56:39.723000Z TestFramework (ERROR): Test failed. Test logging available at /home/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193/test_framework.log
4 test 2021-01-22T13:56:39.746000Z TestFramework (ERROR):
5 test 2021-01-22T13:56:39.754000Z TestFramework (ERROR): Hint: Call /home/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/combine_logs.py '/home/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193' to consolidate all logs
6 test 2021-01-22T13:56:39.754000Z TestFramework (ERROR):
7 test 2021-01-22T13:56:39.754000Z TestFramework (ERROR): If this failure happened unexpectedly or intermittently, please file a bug and provide a link or upload of the combined log. 8 test 2021-01-22T13:56:39.754000Z TestFramework (ERROR): https://github.com/bitcoin/bitcoin/issues
9 test 2021-01-22T13:56:39.755000Z TestFramework (ERROR):
101112TEST | STATUS | DURATION
1314wallet_hd.py |✖ Failed |45 s
1516ALL |✖ Failed |45 s (accumulated)
and then docker attach <ID> and combined logs per instructions:
0node1 2021-01-22T13:56:25.176678Z [httpworker.2] error copying wallet.dat to /home/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193/node1/hd.bak - filesystem error: in equivalent: Operation not supported [/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193/node1/regtest/wallets/wallet.dat] [/work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193/node1/hd.bak]
1 test 2021-01-22T13:56:38.577000Z TestFramework (ERROR): JSONRPC error
2 Traceback (most recent call last):
3 File "/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/test_framework.py", line 126, in main
4 self.run_test()
5 File "/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/wallet_hd.py", line 46, in run_test
6 self.nodes[1].backupwallet(os.path.join(self.nodes[1].datadir, "hd.bak"))
7 File "/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/coverage.py", line 47, in __call__
8 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
9 File "/work/bitcoin/bitcoin/ci/scratch/build/bitcoin-x86_64-pc-linux-gnu/test/functional/test_framework/authproxy.py", line 146, in __call__
10 raise JSONRPCException(response['error'], status)
11 test_framework.authproxy.JSONRPCException: Error: Wallet backup failed! (-4)
and now the files tested on equivalence:
File /work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193/node1/regtest/wallets doe/wallet.dat` does exist.
File /work/bitcoin/bitcoin/ci/scratch/test_runner/test_runner_₿_🏃_20210122_135553/wallet_hd_193/node1/hd.bak does NOT exist.
And now why do we get that filesystem error: in equivalent: Operation not supported?
I have put together two samples for fs::equivalent:
0// Run with: g++ --std=c++17 equivalent.cpp17.cpp && ./a.out
1 2#include<string> 3#include<iostream> 4#include<filesystem> 5namespace fs = std::filesystem;
6 7intmain()
8{
9 fs::path pathSrc ="/home/user/a.log";
10 fs::path pathDest ="/home/user/b.log";
11bool success = fs::equivalent(pathSrc, pathDest);
1213 std::cout <<"Result is: "<< (int)success <<"\n";
1415// # Case "Both files do not exist"
16//
17// Exception is thrown:
18//
19// > terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
20// > what(): filesystem error: cannot check file equivalence: No such file or directory [/home/user/a.log] [/home/user/b.log]
21// > Aborted (core dumped)
2223// # Case "pathSrc exists, pathDest does not"
24//
25// No exception. Std output is:
26// > Result is: 0
2728// # Case "Both pathSrc and pathDest exist but are different."
29//
30// No exception. Std output is:
31// > Result is: 0
3233// # Case "Both pathSrc and pathDest exist and b.log is symlink for a.log." (`ln -s a.log b.log`)
34//
35// No exception. Std output is:
36// > Result is: 0
37}
and Boost variant:
0// Run with: g++ equivalent.boost.cpp -lboost_system -lboost_filesystem && ./a.out
1 2#include<string> 3#include<iostream> 4#include<boost/filesystem.hpp> 5#include<boost/filesystem/fstream.hpp> 6namespace fs = boost::filesystem;
7 8intmain()
9{
10 fs::path pathSrc ="/home/user/a.log";
11 fs::path pathDest ="/home/user/b.log";
12bool success = fs::equivalent(pathSrc, pathDest);
1314 std::cout <<"Result is: "<< (int)success <<"\n";
1516// # Case "Both files do not exist"
17//
18// Exception is thrown:
19//
20// terminate called after throwing an instance of 'boost::filesystem::filesystem_error'
21// what(): boost::filesystem::equivalent: Operation not permitted: "/home/user/a.log", "/home/user/b.log"
22// Aborted (core dumped)
2324// # Case "pathSrc exists, pathDest does not"
25//
26// No exception. Std output is:
27// > Result is: 0
2829// # Case "Both pathSrc and pathDest exist but are different."
30//
31// No exception. Std output is:
32// > Result is: 0
3334// # Case "Both pathSrc and pathDest exist and b.log is symlink for a.log." (`ln -s a.log b.log`)
35//
36// No exception. Std output is:
37// > Result is: 0
38}
So these samples shows that fs::equivalent behaves similarly in both implementations when compiled using g++.
And now finally, when I run the sample in Docker:
0clang++ -stdlib=libc++ --std=c++17 equivalent.cpp17.cpp && ./a.out # /home/user/a.log EXISTS!
12// returns:
3// > terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in equivalent: Operation not supported [/home/user/a.log] [/home/user/b.log]
4// > Aborted (core dumped)
56clang++ --std=c++17 equivalent.cpp17.cpp && ./a.out
7// returns:
8// > Result is: 0
So it looks like -stdlib=libc++ is the culprit or am I missing something? Can anybody verify?
remyers referenced this in commit
2211d05bfd
on Jan 26, 2021
in
src/test/script_tests.cpp:1732
in
7810d151f5outdated
ryanofsky
commented at 4:40 pm on February 5, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7)
The change doesn’t seem right and would be good to revert or explain. This is the non-windows section and HOME environment variable and pathRet should both be byte strings with no need to assume any character encoding
ryanofsky
commented at 4:47 pm on February 5, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7)
If using fs::u8path for -datadir below, probably makes sense to add it here for -blocksdir to be consistent.
These one-off usages of fs::u8path seem dodgy in general though. Given that c++17 path class doesn’t support an imbue method (https://stackoverflow.com/questions/42765168/change-narrow-string-encoding-or-missing-stdfilesystempathimbue), I think the best thing would be to copy approach in the stackoverflow answer and override the windows fs::path constructor to use fs::u8path automatically. It should be easy to copy the solution there since we already have our own fs namespace.
in
src/util/system.cpp:1324
in
7810d151f5outdated
1270- // fs::path, which is then used to explicitly imbue the path.
1271- std::locale loc = fs::path::imbue(std::locale::classic());
1272-#ifndef WIN32
1273- fs::path::imbue(loc);
1274-#else
1275- fs::path::imbue(std::locale(loc, new std::codecvt_utf8_utf16<wchar_t>()));
ryanofsky
commented at 5:02 pm on February 5, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7)
ryanofsky
commented at 5:07 pm on February 5, 2021:
In commit “build: remove boost::filesystem” (9f37a39b11b240f75b97fb565e6d8cf44972ec49)
This change seems like it belongs in previous commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7) instead of this one.
in
configure.ac:87
in
7810d151f5outdated
76@@ -77,6 +77,9 @@ AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory])
77 dnl Check if -latomic is required for <std::atomic>
78 CHECK_ATOMIC
7980+dnl check if -lstdc++fs is required for std::filesystem
81+CHECK_FILESYSTEM
ryanofsky
commented at 5:09 pm on February 5, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (7810d151f5219e634b431c5393771616c759aef7)
It would be nice to split this commit up into two commits to separate the build system changes from the c++ code changes. The build system changes can be a first commit and the c++ code changes can be a second commit.
in
depends/hosts/darwin.mk:1
in
db29710bc1outdated
ryanofsky
commented at 5:17 pm on February 5, 2021:
In commit “build: set OSX_MIN_VERSION to 10.15” (db29710bc12f97d9de78fb177322a8ac41f7c87d)
Would be good to consolidate all the config, makefile, and documentation changes adding build support for std::filesystem into a single atomic commit. Right now they are scattered across three commits and everything seems very mixed up:
7810d151f5219e634b431c5393771616c759aef7 refactor: replace boost::filesystem with std::filesystem (2/5)
Switch c++ code from boost::filesystem to std::filesystem
Remove boost::filesystem
ryanofsky approved
ryanofsky
commented at 5:27 pm on February 5, 2021:
member
Code review ACKf0667f1d4289b65ac92c70db30e2123179f611e1. No major issues and nothing that couldn’t be fixed later, but:
It seems like there might be a macos backupwallet failure: https://cirrus-ci.com/task/5475027611025408?command=ci#L3029. It’s unclear what would cause it, since I wouldn’t expect there to be path handling differences between linux and macos.
Windows character set conversion doesn’t seem as robust as it could be (see review comments)
Commit organization doesn’t seem as clear as it could be (see review comments)
ryanofsky
commented at 5:36 pm on February 5, 2021:
member
Oh, it looks like kiminuo tracked down the wallet_hd backupwallet failure earlier (https://github.com/bitcoin/bitcoin/pull/20744#issuecomment-765467226) to a problem with certain platforms throwing when calling fs::equivalent on non-existent paths. Simple fix might be to define a wrapper function for the affected platforms catching the specific exception or checking for existence before calling fs::equivalent
DrahtBot added the label
Needs rebase
on Feb 8, 2021
fanquake force-pushed
on Feb 9, 2021
fanquake
commented at 12:41 pm on February 9, 2021:
member
@ryanofsky thanks for the comments. I’ve reordered and split the commits up more cleanly now. I’ve marked some comments as resolved, however some are still outstanding, and I’m planning on addressing shortly. I’ll also be going back over the discussion in #19245, as i think that contains answers to some queries.
fanquake force-pushed
on Feb 9, 2021
fanquake force-pushed
on Feb 9, 2021
DrahtBot removed the label
Needs rebase
on Feb 9, 2021
laanwj referenced this in commit
d202054675
on Feb 9, 2021
DrahtBot added the label
Needs rebase
on Feb 9, 2021
fanquake force-pushed
on Feb 10, 2021
fanquake removed the label
Needs rebase
on Feb 10, 2021
MarcoFalke referenced this in commit
f61c3a1090
on Feb 10, 2021
sidhujag referenced this in commit
92f9cd5b8f
on Feb 11, 2021
sidhujag referenced this in commit
05c420bf56
on Feb 11, 2021
fanquake force-pushed
on Feb 11, 2021
ryanofsky
commented at 0:33 am on February 12, 2021:
member
Thanks for updates, commits are simpler now! It looks what’s left to be done are debugging test failures and fixing windows encoding:
Windows failure “C:/projects/bitcoin/src/test/dbwrapper_tests.cpp(412): error: in “dbwrapper_tests/unicodepath”: check fs::exists(lockPath) has failed” from https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37725701#L335 looks like a unicode error caused by dropping imbue (see below).
Windows walletinit_verify_walletdir_no_trailing2 failure in https://cirrus-ci.com/task/5243515519107072?command=ci#L5082 looks like a broken test that was never testing what the author intended to test. I think the test and “trailing2” map entry could be deleted entirely, ideally in an preceding commit or PR. Also the test setup uses an awkward m_walletdir_path_cases map that seems like it is pointless and just obfuscates test behavior. Maybe that could be removed too.
Right now this PR is dropping path imbue, and trying to use ad-hoc u8path calls as a workaround. Earlier comments: #20744 (review), #20744 (review). Manual u8path calls are error-prone and we don’t actually want them on POSIX. (POSIX path strings should be passed straight through to the fs::path constructor without assuming a particular encoding). On windows, we do want u8path calls whenever converting any string to a path. Unlike POSIX, windows natively uses wide strings for arguments, environment variables and paths, so to work correctly on windows we either need to use wide strings internally, or do wide <-> narrow conversions at the borders of windows API interactions. We’ve chosen the latter approach, and can stick with it here by overriding the path(std::string) constructor as described https://stackoverflow.com/questions/42765168/change-narrow-string-encoding-or-missing-stdfilesystempathimbue
DrahtBot added the label
Needs rebase
on Feb 12, 2021
fanquake force-pushed
on Feb 13, 2021
DrahtBot removed the label
Needs rebase
on Feb 13, 2021
fanquake referenced this in commit
1e4b1973a0
on Feb 15, 2021
fanquake referenced this in commit
1b960f0780
on Feb 15, 2021
fanquake referenced this in commit
ffe3d65aaa
on Feb 15, 2021
fanquake referenced this in commit
7bf04e358a
on Feb 17, 2021
fanquake force-pushed
on Feb 17, 2021
laanwj referenced this in commit
04336086d3
on Feb 17, 2021
sidhujag referenced this in commit
5d5698c0ae
on Feb 17, 2021
DrahtBot added the label
Needs rebase
on Feb 19, 2021
fanquake force-pushed
on Feb 21, 2021
DrahtBot removed the label
Needs rebase
on Feb 21, 2021
DrahtBot added the label
Needs rebase
on Feb 23, 2021
fanquake force-pushed
on Mar 11, 2021
DrahtBot removed the label
Needs rebase
on Mar 11, 2021
kiminuo
commented at 2:01 pm on March 30, 2021:
contributor
Interestingly, the command fs::remove_all fails even if the folder is empty (I modified it just for fun). It happens always.
MarcoFalke
commented at 5:49 pm on March 30, 2021:
member
Does it help if the folder name only contains 128 bit of randomness (as opposed to the 512 bits)?
kiminuo
commented at 7:48 pm on March 30, 2021:
contributor
Does it help if the folder name only contains 128 bit of randomness (as opposed to the 512 bits)?
The error is the same even for /tmp/test_common_Bitcoin_Core/35c where:
test_common_Bitcoin Core -> test_common_Bitcoin_Core and
35c is a very short folder name.
It does not matter whether I try to remove_all on /tmp/test_common_Bitcoin_Core/35c path or /tmp/test_common_Bitcoin_Core/35c/ path (see the trailing slash).
So this does not seem to be the issue.
I have also tried to copy /tmp/test_common_Bitcoin_Core/35c to /home/user/bitcoin/bitcoin/ci/scratch/out/arm-linux-gnueabihf and delete that folder and it does not work too so it’s not about /tmp.
At this point, it looks like it does not work at all for directories (full or empty). However, deleting /home/user/bitcoin/bitcoin/ci/scratch/out/arm-linux-gnueabihf/35c/test.txt (i.e. a file) finished without an error when deleted with fs::remove_all(path).
The remaining errors seem unicode related and probably fixed by implementing the suggested imbue workaround.
kiminuo
commented at 9:00 am on April 1, 2021:
contributor
@ryanofsky I have narrowed the failing ARM issue down to:
0auto iterator = fs::directory_iterator(m_path_root); // where m_path_root = /tmp/test_common_Bitcoin Core/29248e247aad6a68595f5e097d311e923c00300291f160ee2254ed8b5b4668c4
which throws:
0/tmp/test_common_Bitcoin Core/29248e247aad6a68595f5e097d311e923c00300291f160ee2254ed8b5b4668c4
1terminate called after throwing an instance of 'std::filesystem::__cxx11::filesystem_error'
2 what(): filesystem error: directory iterator cannot open directory: Value too large for defined data type [/tmp/test_common_Bitcoin Core/29248e247aad6a68595f5e097d311e923c00300291f160ee2254ed8b5b4668c4]
Interestingly
0auto status = fs::status(m_path_root);
does not throw.
MarcoFalke
commented at 9:43 am on April 1, 2021:
member
The gcc bug is fixed on all branches, but is seems that gcc for arm on debian buster didn’t receive the patch? This should be a relative edge-case platform and I think we can just bump the ci config from buster to bullseye to fix the problem. (It does fix it for me locally).
kiminuo
commented at 10:50 am on April 1, 2021:
contributor
The gcc bug is fixed on all branches, but is seems that gcc for arm on debian buster didn’t receive the patch?
Might be so.
This should be a relative edge-case platform and I think we can just bump the ci config from buster to bullseye to fix the problem. (It does fix it for me locally).
Bullseye is not stable yet. Maybe gcc-9 instead of gcc-8 on buster would suffice here to be less aggressive in version bumps? Anyway, I’m kind of worried that this would mean to delay this PR by a Bitcoin Core version or two.
MarcoFalke added this to the milestone 23.0
on Apr 1, 2021
MarcoFalke
commented at 11:05 am on April 1, 2021:
member
Assigned the 23.0 milestone, because rushing this into 0.22 might be too aggressive. See also #20460 (comment)
In a year from now (when this is going to be releases), debian:buster may have bumped the gcc-8 to include the fix, or bullseye may have been released as stable. So I think re-implementing std::fs features in our code base is not needed.
laanwj
commented at 11:49 am on April 1, 2021:
member
So I think re-implementing std::fs features in our code base is not needed.
Agree on the general idea. It is slightly disappointing but there is no rush. If time will solve the issues automatically there is no need to spend work and reviewer time on doing some in-between thing.
MarcoFalke
commented at 11:55 am on April 1, 2021:
member
Branch-off is in only 3 months, so I think it is fine to drink some tea in the meantime and wait a bit
fanquake force-pushed
on Apr 1, 2021
kiminuo
commented at 12:12 pm on April 2, 2021:
contributor
This is probably useful for the Win64 test failure:
kiminuo
commented at 7:18 pm on June 24, 2021:
contributor
Regarding Android CI error: Currently, Android NDK 21 is used in the master branch, but Android NDK 22 is necessary for std::filesystem support.
Qt works with Android NDK 21 but not with NDK 22 out of the box - it did not work for me and I’m not sure how to fix it, build gurus here will understand it much better.
hebasto
commented at 11:54 pm on June 24, 2021:
member
Qt works with Android NDK 21 but not with NDK 22 out of the box - it did not work for me and I’m not sure how to fix it, build gurus here will understand it much better.
kiminuo
commented at 9:34 am on June 25, 2021:
contributor
Qt works with Android NDK 21 but not with NDK 22 out of the box - it did not work for me and I’m not sure how to fix it, build gurus here will understand it much better.
DrahtBot added the label
Needs rebase
on Jun 28, 2021
fanquake referenced this in commit
7fc9a45f47
on Jul 21, 2021
fanquake force-pushed
on Jul 21, 2021
fanquake
commented at 7:50 am on July 21, 2021:
member
Now that we’ve branch of for 22.x, we can actually think about merging this. I’ve rebased on master (which should fix the Android build issues) and included some more changes from @kiminuo. That should mean that what’s left to finish up here is the Windows changes, which I’d like to get to shortly.
DrahtBot removed the label
Needs rebase
on Jul 21, 2021
hebasto
commented at 8:25 am on July 21, 2021:
member
That should mean that what’s left to finish up here is the Windows changes, which I’d like to get to shortly.
As this pr discussion is pretty long, here are Windows related comments in one place:
DrahtBot removed the label
DrahtBot Guix build requested
on Jul 24, 2021
MarcoFalke
commented at 5:43 pm on July 24, 2021:
member
0+ env CONFIG_SITE=/bitcoin/depends/x86_64-w64-mingw32/share/config.site ./configure --prefix=/--disable-ccache --disable-maintainer-mode --disable-dependency-tracking --enable-reduce-exports --disable-bench --disable-gui-tests --disable-fuzz-binary 'CFLAGS=-O2 -g -fno-ident''CXXFLAGS=-O2 -g -fno-ident' LDFLAGS=-Wl,--no-insert-timestamp
1configure: loading site script /bitcoin/depends/x86_64-w64-mingw32/share/config.site
2checking for x86_64-w64-mingw32-pkg-config.../root/.guix-profile/bin/pkg-config --static 3checking pkg-config is at least version 0.9.0... yes
4checking build system type... x86_64-pc-linux-gnu
5checking host system type... x86_64-w64-mingw32
6checking for a BSD-compatible install.../root/.guix-profile/bin/install -c
7checking whether build environment is sane... yes
8checking for x86_64-w64-mingw32-strip... x86_64-w64-mingw32-strip
9checking for a thread-safe mkdir -p.../root/.guix-profile/bin/mkdir -p
10checking for gawk... gawk
11checking whether make sets $(MAKE)... yes
12checking whether make supports nested variables... yes
13checking whether to enable maintainer-specific portions of Makefiles... no
14checking whether make supports nested variables... (cached) yes
15checking whether the C++ compiler works... yes
16checking for C++ compiler default output file name... a.exe
17checking for suffix of executables....exe
18checking whether we are cross compiling... yes
19checking for suffix of object files... o
20checking whether we are using the GNU C++ compiler... yes
21checking whether x86_64-w64-mingw32-g++ accepts -g... yes
22checking whether make supports the include directive... yes (GNU style)
23checking dependency style of x86_64-w64-mingw32-g++... none
24checking whether x86_64-w64-mingw32-g++ supports C++17 features with -std=c++17... yes
25checking whether std::atomic can be used without link library... yes
26checking whether std::filesystem can be used without link library... no
27checking whether std::filesystem needs -lstdc++fs... no
28configure: error: in`/distsrc-base/distsrc-4bd690d8c420-x86_64-w64-mingw32':29configure: error: cannot figure out how to use std::filesystem
30See `config.log' for more details
DrahtBot added the label
Needs rebase
on Aug 20, 2021
MarcoFalke
commented at 9:02 am on August 27, 2021:
member
Note that progress is still being made here. Someone else is currently looking at the Windows portion of the changes.
fanquake renamed this:
[POC] Use std::filesystem. Remove Boost Filesystem & System
Use std::filesystem. Remove Boost Filesystem & System
on Aug 31, 2021
fanquake removed the label
Needs rebase
on Aug 31, 2021
dongcarl
commented at 8:18 pm on September 1, 2021:
member
Concept ACK
DrahtBot added the label
Needs rebase
on Sep 2, 2021
fanquake force-pushed
on Sep 2, 2021
DrahtBot removed the label
Needs rebase
on Sep 2, 2021
DrahtBot added the label
Needs rebase
on Sep 7, 2021
fanquake force-pushed
on Sep 8, 2021
DrahtBot removed the label
Needs rebase
on Sep 8, 2021
hebasto
commented at 4:34 pm on September 8, 2021:
member
In the commit “build: set OSX_MIN_VERSION to 10.15” (edeed577501693e0b8e8f0c8c5d1ccdc47587557) the following change is also required:
Some observations, maybe it helps somebody to fix the issue:
Setting locale
Addingstd::setlocale(LC_ALL, "en_US.UTF-8"); makes the test pass.
Original test
0BOOST_AUTO_TEST_CASE(unicodepath)
1{
2// Attempt to create a database with a UTF8 character in the path.
3// On Windows this test will fail if the directory is created using
4// the ANSI CreateDirectoryA call and the code page isn't UTF8.
5// It will succeed if created with CreateDirectoryW.
6 fs::path ph = m_args.GetDataDirBase() /"test_runner_₿_🏃_20191128_104644";
7 CDBWrapper dbw(ph, (1<<20));
8 9 fs::path lockPath = ph /"LOCK";
10 BOOST_CHECK(fs::exists(lockPath));
11}
My Visual Studio debugger shows that path fs::path ph = m_args.GetDataDirBase() / "test_runner_₿_🏃_20191128_104644"; is garbled once created. Likely "test_runner_₿_🏃_20191128_104644" is intepreted using some some local codepage.
Modified test with euro sign
(No pun intended :))
0BOOST_AUTO_TEST_CASE(unicodepath)
1{
2// Attempt to create a database with a UTF8 character in the path.
3// On Windows this test will fail if the directory is created using
4// the ANSI CreateDirectoryA call and the code page isn't UTF8.
5// It will succeed if created with CreateDirectoryW.
6// fs::path ph = m_args.GetDataDirBase() / "test_runner_₿_🏃_20191128_104644"; // REMOVED
7 fs::path ph = m_args.GetDataDirBase() /L"test_runner_€_20191128_104644"; // ADDED
8 CDBWrapper dbw(ph, (1<<20));
910 fs::path lockPath = ph /"LOCK";
11 BOOST_CHECK(fs::exists(lockPath));
12}
This test leads on my computer to creating of two folders in C:\Users\kimi\AppData\Local\Temp\test_common_Bitcoin Core\efd602083fe69783d5e6eff1220a12770bd2168d338ae233e260bae6a10c6068:
test_runner_�_20191128_104644
test_runner_€_20191128_104644
€ was used because it can be represented by two bytes.
Note that using L"test_runner_₿_🏃_20191128_104644" leads to class std::system_error: No mapping for the Unicode character exists in the target multi-byte code page. error which seems to make sense to me.
Interconversions between these [pathname] representations are mediated, as needed, by the use of one or more codecvt facets. If no specific locale object is specified, these facets are obtained from the global locale.
hebasto
commented at 12:36 pm on September 9, 2021:
member
Recently added to Windows CI task functional tests are broken with this branch.
fanquake force-pushed
on Sep 9, 2021
hebasto
commented at 2:40 pm on September 9, 2021:
member
in
src/test/dbwrapper_tests.cpp:408
in
56f9476619outdated
403@@ -404,6 +404,7 @@ BOOST_AUTO_TEST_CASE(unicodepath)
404 // On Windows this test will fail if the directory is created using
405 // the ANSI CreateDirectoryA call and the code page isn't UTF8.
406 // It will succeed if created with CreateDirectoryW.
407+ std::setlocale(LC_ALL, "en_US.UTF-8");
MarcoFalke
commented at 2:42 pm on September 9, 2021:
might need
0#include<clocale>
hebasto
commented at 2:44 pm on September 9, 2021:
fanquake
commented at 3:20 am on September 10, 2021:
added & added
ryanofsky
commented at 9:49 pm on October 18, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (6316193878d66bb35dbf96f8ec2047591e3bdb46)
These changes are not going to have any useful effect on dbwrapper after #22937, so should be reverted now. leveldb is expecting a UTF8 string on windows, that’s what #22937 already passes.
Suggested change:
0diff --git a/src/test/dbwrapper_tests.cpp b/src/test/dbwrapper_tests.cpp
1index a6ac3a33533..b6f5938892c 100644
2--- a/src/test/dbwrapper_tests.cpp
3+++ b/src/test/dbwrapper_tests.cpp
4@@ -6,7 +6,6 @@
5 #include <test/util/setup_common.h>
6 #include <uint256.h>
7 8-#include <clocale>
9 #include <memory>
1011 #include <boost/test/unit_test.hpp>
12@@ -405,7 +404,6 @@ BOOST_AUTO_TEST_CASE(unicodepath)
13 // On Windows this test will fail if the directory is created using
14 // the ANSI CreateDirectoryA call and the code page isn't UTF8.
15 // It will succeed if created with CreateDirectoryW.
16- std::setlocale(LC_ALL, "en_US.UTF-8");
17 fs::path ph = m_args.GetDataDirBase() / "test_runner_₿_🏃_20191128_104644";
18 CDBWrapper dbw(ph, (1 << 20));
1920diff --git a/test/lint/lint-locale-dependence.sh b/test/lint/lint-locale-dependence.sh
21index 1e7d04ad26e..b119cffec80 100755
22--- a/test/lint/lint-locale-dependence.sh
23+++ b/test/lint/lint-locale-dependence.sh
24@@ -42,7 +42,6 @@ export LC_ALL=C
25 # TODO: Reduce KNOWN_VIOLATIONS by replacing uses of locale dependent snprintf with strprintf.
26 KNOWN_VIOLATIONS=(
27 "src/dbwrapper.cpp:.*vsnprintf"
28- "src/test/dbwrapper_tests.cpp:.*setlocale"
29 "src/test/dbwrapper_tests.cpp:.*snprintf"
30 "src/test/fuzz/locale.cpp"
31 "src/test/fuzz/string.cpp"
ryanofsky
commented at 9:04 pm on September 9, 2021:
member
I didn’t really get an chance to test and finish test this yet, but here is my attempt at improving the encoding situation on windows using the path(std::string) constructor override I mentioned earlier: 099e0207808687e3af2148e7c31353677b866d72 (branch)
I also see there are a lot of path::string() calls that should probably be replaced by path::u8string() calls for logging / rpc / display purposes, but this would be another followup
fanquake force-pushed
on Sep 10, 2021
fanquake
commented at 3:23 am on September 10, 2021:
member
I’m seeing functional test failures on macOS again due to differences in paths /var/ vs /private/var, where the former is a symlink to the later. I have a recollection of seeing this previously, and thought we’d fixed/worked around it.
I didn’t really get an chance to test and finish test this yet, but here is my attempt at improving the encoding situation on windows using the path(std::string) constructor override I mentioned earlier: 099e020 (branch)
Thanks. I’ll take a look at testing / including this shortly.
One other thing we’ll likely need to do here is move Guix to using a newer version of mingw-w64. iirc we’re currently using 7, but I’m pretty sure we’ll required 8, which is available in Guix. cc @dongcarl.
ryanofsky
commented at 4:45 am on September 10, 2021:
member
I had second thoughts about the path(std::string) constructor approach I was trying above. I think it adds to much magic. Better not to make a custom path class that differs subtly from the standard path class on windows, just to be able to do magic implicit conversions, when explicit conversions would be clearer anyway. So starting over.
Windows encoding bugfix, attempt 2
Summary
The implicit path <=> std::string conversions done by the std::filesystem::path(std::string) constructor and std::filesystem::path::string() method generally work well on linux, but are unsafe and broken for our purposes on Windows. Solution is to disable these methods and start using safer, more explicit path <=> std::string conversion functions instead.
Details
On POSIX, paths natively are 8-bit strings that do not specify any particular encoding, so they can be straightforwardly converted to and from std::string objects with no transformations. On Windows, paths natively are unicode strings that have to be encoded or decoded in some way to be represented in std::string objects. Ideally, the way we would convert paths to std::string is by encoding them as UTF-8 (technically a “wobbly” variation of UTF-8 to handle some edge cases). This is what the previous boost path class did after imbueing it with a locale. But with the new std::filesystem::path class, imbueing a locale is not possible. So the standard path(std::string) constructor and path::string() methods will no longer use UTF-8. Instead they will use different encodings depending on the local system’s current code page which makes these functions unsafe and useless for our purposes. In Windows versions since May 2019 it is possible to change the build in a way that would make these functions always use UTF-8. But if we want to keep providing the same behavior and level of compatibility that we had with boost we need to stop using calling these functions and start doing explicit conversions. So I think the simplest and best way forward is just to prevent the bad string functions from being called, and to update the calling code to use explicit conversions. I implemented this here: 3e88c9f6b4e84ab5da78f4862266b5ff13bb8fc0 (tag)
It’s kind of a big commit, but the majority of it is just introducing and calling new PathToString and PathFromString functions and I can easily split it out into a standalone PR that does not depend on anything else here.
kiminuo
commented at 6:10 am on September 10, 2021:
contributor
@ryanofsky The unicodepath test passes for me on Windows 10 with your patch (3e88c9f (tag)). Nice work.
ryanofsky referenced this in commit
1e07139279
on Sep 10, 2021
ryanofsky referenced this in commit
ddbfc51002
on Sep 10, 2021
ryanofsky referenced this in commit
a7f5053ca3
on Sep 10, 2021
ryanofsky referenced this in commit
d312e529ce
on Sep 10, 2021
ryanofsky
commented at 7:43 pm on September 13, 2021:
member
I posted a version of this PR rebased on top of #22937 here: e03ede894b86c793a33e84eade230c35d35457df, (tag)
hebasto
commented at 7:47 pm on September 13, 2021:
member
I posted a version of this PR rebased on top of #22937 here: e03ede8, (tag)
Testing…
hebasto
commented at 0:49 am on September 15, 2021:
member
I posted a version of this PR rebased on top of #22937 here: e03ede8, (tag)
The version with green CI for the Windows MSVC build is here:
MarcoFalke
commented at 6:24 am on September 27, 2021:
Wouldn’t it make more sense to use EXEEXT to avoid issues when the tests are run in wine or so?
fanquake referenced this in commit
67eae69f3f
on Sep 28, 2021
fanquake force-pushed
on Sep 28, 2021
fanquake
commented at 6:12 am on September 28, 2021:
member
Rebased after the merge of #23060. All prerequisites that were part of this PR have now been merged. The next step to moving to std::filesystem should be #22937.
in
test/lint/lint-includes.sh:58
in
a073a2f6fdoutdated
hebasto
commented at 12:41 pm on September 28, 2021:
Removing of boost/filesystem/fstream.hpp should be done in the “refactor: replace boost::filesystem with std::filesystem” commit (5fb92bab5f8c6c07bb668d7a7f2203f389a4956d) to let it pass test/lint/lint-includes.sh.
fanquake force-pushed
on Sep 29, 2021
fanquake
commented at 2:39 am on September 29, 2021:
member
I’ve modified the std::filesystem macro to try using -lc++fs if -lstdc++fs doesn’t work. Should fix the issues in the nowallet CI, which uses Clang 7 and libc++.
Removing of boost/filesystem/fstream.hpp should be done in the
Done.
hebasto
commented at 10:25 pm on September 29, 2021:
member
That should mean that what’s left to finish up here is the Windows changes, which I’d like to get to shortly.
Any update about cross-compiling for Windows?
dongcarl
commented at 9:24 pm on October 5, 2021:
member
One other thing we’ll likely need to do here is move Guix to using a newer version of mingw-w64. iirc we’re currently using 7, but I’m pretty sure we’ll required 8, which is available in Guix. cc @dongcarl.
I’m not sure upgrading to mingw-w64 v8 will affect anything, could you link me to the specific failure that makes you think this is required?
fanquake
commented at 10:23 am on October 7, 2021:
member
Any update about cross-compiling for Windows?
I’m not sure upgrading to mingw-w64 v8 will affect anything, could you link me to the specific failure that makes you think this is required?
While the GCC release notes claim that, (incomplete) support for std::filesystem only landed in GCC-9:
Runtime Library (libstdc++)
Incomplete support for the C++17 Filesystem library and the Filesystem TS on Windows.
it’s not clear that’s actually the case, as you can use <filesystem> with a GCC 8.3 backed mingw-w64 6.0.0 on Debian Buster (with -lstdc++fs).
fanquake force-pushed
on Oct 11, 2021
DrahtBot added the label
Needs rebase
on Oct 12, 2021
fanquake force-pushed
on Oct 13, 2021
fanquake force-pushed
on Oct 13, 2021
DrahtBot removed the label
Needs rebase
on Oct 13, 2021
laanwj referenced this in commit
1884ce2f4c
on Oct 15, 2021
DrahtBot added the label
Needs rebase
on Oct 15, 2021
fanquake force-pushed
on Oct 16, 2021
fanquake
commented at 7:32 am on October 16, 2021:
member
I’ve rebased to account for #22937 added a commit for Guix to pass --enable-libstdcxx-filesystem-ts=yes to our mingw GCC build. However libstdc++ is failing to build it’s filesystem component. @dongcarl could you take a look? The full GCC build output is here: https://gist.github.com/fanquake/c040358952075b054cda7786e3e98563.
fanquake force-pushed
on Oct 16, 2021
DrahtBot removed the label
Needs rebase
on Oct 16, 2021
in
src/fs.h:25
in
6316193878outdated
37- * method, which worked well in the boost::filesystem implementation, but have
38- * unsafe and unpredictable behavior on Windows in the std::filesystem
39- * implementation (see implementation note in \ref PathToString for details).
40+ * Path class wrapper to blocks calls to the fs::path(std::string) implicit
41+ * constructor and the fs::path::string() method, which worked well in the
42+ * boost::filesystem implementation, but have unsafe and unpredictable behavior
ryanofsky
commented at 5:38 pm on October 16, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (6316193878d66bb35dbf96f8ec2047591e3bdb46)
I think it would be good to simplify
which worked well in the boost::filesystem implementation but have unsafe and unpredictable behavior on Windows in the std::filesystem implementation
to just
which have unsafe and unpredictable behavior on Windows
because the boost comparison is no longer relevant after this PR.
fanquake
commented at 4:13 am on October 21, 2021:
0/usr/bin/x86_64-w64-mingw32-ld: libbitcoin_util.a(libbitcoin_util_a-system.o):system.cpp:(.text$_ZN5boost7process6detail18basic_execute_implIcJRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENS1_7windows8pipe_outILi1ELin1EEENSC_ILi2ELin1EEENSB_7pipe_inEEEENS0_5childEDpOT0_[_ZN5boost7process6detail18basic_execute_implIcJRKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEENS1_7windows8pipe_outILi1ELin1EEENSC_ILi2ELin1EEENSB_7pipe_inEEEENS0_5childEDpOT0_]+0x362): undefined reference to `boost::filesystem::path::operator/=(boost::filesystem::path const&)'
in
src/wallet/db.cpp:32
in
6316193878outdated
28@@ -29,15 +29,14 @@ std::vector<fs::path> ListDatabases(const fs::path& wallet_dir)
2930 try {
31 // Get wallet path relative to walletdir by removing walletdir from the wallet path.
32- // This can be replaced by boost::filesystem::lexically_relative once boost is bumped to 1.60.
ryanofsky
commented at 10:14 pm on October 18, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (6316193878d66bb35dbf96f8ec2047591e3bdb46)
This suggestion is still relevant. It would be better to implement or update it than to drop it. It can be implemented with:
ryanofsky
commented at 10:42 pm on October 18, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (6316193878d66bb35dbf96f8ec2047591e3bdb46)
I think this line should just be deleted. It looks like this StripRedundantLastElementsOfPath call was added by mistake in #21244, and never did anything, because the last path element above is always “blocks”. Keeping this just adds unnecessary complexity and creates a minor change behavior (it resolves symlinks now)
MarcoFalke
commented at 8:40 am on October 19, 2021:
Isn’t it a better user experience? At least for -blocksdir?
This is a minor point, but I don’t think changing the path in general is a better experience, and I’m not sure in which cases it could be a better experience. If I specify -blocksdir=/var/lib/bitcoin/blocks I think like to see that same path when I’m grepping through log files, or checking the result of some RPC, not some different transformed path if the system happens to be using symlinks to manage storage.
Either way, I think dropping this line to preserve behavior makes sense for this commit. Marco was right this StripRedundantLastElementsOfPath call was was added in #20080, and in even in that PR it should not have any effect because the last path element was "blocks" not ".":
ryanofsky
commented at 12:59 pm on October 19, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (6316193878d66bb35dbf96f8ec2047591e3bdb46)
To preserve behavior, I think it’d be best to keep this function instead of replacing it with a call to canonical which adds new transformations like normalizing path elements in the middle of the path and resolving symlinks. Just a small tweak here is required to port from boost:
0diff --git a/src/util/system.cpp b/src/util/system.cpp
1index 0e516f10949..99688de0483 100644
2--- a/src/util/system.cpp
3+++ b/src/util/system.cpp
4@@ -243,7 +243,7 @@ namespace {
5 fs::path StripRedundantLastElementsOfPath(const fs::path& path)
6 {
7 auto result = path;
8- while (fs::PathToString(result.filename()) == ".") {
9+ while (result.filename().empty() || fs::PathToString(result.filename()) == ".") {
10 result = result.parent_path();
11 }
12
fanquake
commented at 4:14 am on October 21, 2021:
We’re now preserving StripRedundantLastElementsOfPath and I’ve included this change.
ryanofsky
commented at 1:01 pm on October 19, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (6316193878d66bb35dbf96f8ec2047591e3bdb46)
Mentioned above, but I think it’d be best to keep this line unchanged to preserve behavior, keeping the call to StripRedundantLastElementsOfPath instead of switching to canonical
fanquake
commented at 4:15 am on October 21, 2021:
Reverted the change here.
in
src/wallet/test/init_test_fixture.cpp:31
in
6316193878outdated
In c++17, path("wallets") / path("/") results in path("/") more similar to a python-style path join because the second path is an absolute path so it just becomes the new path.
Without these changes the two trailing tests try to use the root directory / as the wallets directory and fail.
Changing / to + makes the tests work the same as prevously
ryanofsky
commented at 2:10 pm on October 19, 2021:
member
Finally finished going through commit 6316193878d66bb35dbf96f8ec2047591e3bdb46 (in branch e4d50b8f0367e9ac3bb6189c9299cdda9c9d7c7e) and I think it will look good with all the suggested changes. After these changes, I’m still not sure if would be 100% refactoring because of possible system_complete and absolute differences mentioned in the commit description about extra slashes, but the the only place these could possibly be show up is in the middle regtest/testnet GetDataDir paths, so they would be harmless and the commit message already mentions this. And aside from that there are no other places I would see behavior changing at all.
fanquake force-pushed
on Oct 21, 2021
fanquake
commented at 8:00 am on October 21, 2021:
member
Forgot to push?
Was working on the Guix changes. I’ve now cherry-picked patches from GCC 9, which should mirror what Debian did with it’s mingw-w64 and GCC 8, such that the Guix Windows cross compile should work when using std::filesystem. Compiling GCC 8.4.0 (with --enable-libstdcxx-filesystem-ts=yes) and mingw-w64 works, however I’m running into issues when compiling Core, and will investigate further. Note that this branch currently cross-compiles for Windows using mingw-w64 outside of Guix. I’ve pushed up the current changes in any case.
Current Guix issue when cross-compiling for Windows:
0 CXX libbitcoin_server_a-net_processing.o
1 CXX libbitcoin_server_a-noui.o
2 CXX libbitcoin_server_a-pow.o
3In file included from ./fs.h:12,
4 from ./addrdb.h:9,
5 from ./banman.h:8,
6 from init.cpp:13:
7/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream: In instantiation of 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const _Path&, std::ios_base::openmode) [with _Path = std::filesystem::__cxx11::path; _Require = std::filesystem::__cxx11::path; _CharT = char; _Traits = std::char_traits<char>; std::ios_base::openmode = std::_Ios_Openmode]':
8init.cpp:121:81: required from here
9/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:839:38: error: no matching functionfor call to 'std::basic_ofstream<char>::basic_ofstream(const value_type*, std::ios_base::openmode&)'10 : basic_ofstream(__s.c_str(), __mode)11 ^
12/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:845:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(std::basic_ofstream<_CharT, _Traits>&&) [with _CharT = char; _Traits = std::char_traits<char>]'13 basic_ofstream(basic_ofstream&& __rhs)14 ^~~~~~~~~~~~~~
15/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:845:7: note: candidate expects 1 argument, 2 provided
16/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:837:2: note: candidate: 'template<class _Path, class _Require> std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const _Path&, std::ios_base::openmode)'17 basic_ofstream(const _Path& __s,
18 ^~~~~~~~~~~~~~
19/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:837:2: note: template argument deduction/substitution failed:
20/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:836:32: error: request for member 'make_preferred' in 'std::declval<const wchar_t*&>()', which is of non-class type 'const wchar_t*'21 template<typename _Path, typename _Require= _If_fs_path<_Path>>
22 ^~~~~~~~
23/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:820:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const string&, std::ios_base::openmode) [with _CharT = char; _Traits = std::char_traits<char>; std::__cxx11::string = std::__cxx11::basic_string<char>; std::ios_base::openmode = std::_Ios_Openmode]'24 basic_ofstream(const std::string& __s,
25 ^~~~~~~~~~~~~~
26/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:820:7: note: no known conversion for argument 1 from 'const value_type*'{aka 'const wchar_t*'} to 'const string&'{aka 'const std::__cxx11::basic_string<char>&'}27/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:785:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const char*, std::ios_base::openmode) [with _CharT = char; _Traits = std::char_traits<char>; std::ios_base::openmode = std::_Ios_Openmode]'28 basic_ofstream(const char* __s,
29 ^~~~~~~~~~~~~~
30/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:785:7: note: no known conversion for argument 1 from 'const value_type*'{aka 'const wchar_t*'} to 'const char*'31/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:774:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream() [with _CharT = char; _Traits = std::char_traits<char>]'32 basic_ofstream(): __ostream_type(), _M_filebuf()33 ^~~~~~~~~~~~~~
34/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:774:7: note: candidate expects 0 arguments, 2 provided
35/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:839:38: error: no matching functionfor call to 'std::basic_ofstream<char>::basic_ofstream(const value_type*, std::ios_base::openmode&)'36 : basic_ofstream(__s.c_str(), __mode)37 ^
38/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:845:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(std::basic_ofstream<_CharT, _Traits>&&) [with _CharT = char; _Traits = std::char_traits<char>]'39 basic_ofstream(basic_ofstream&& __rhs)40 ^~~~~~~~~~~~~~
41/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:845:7: note: candidate expects 1 argument, 2 provided
42/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:837:2: note: candidate: 'template<class _Path, class _Require> std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const _Path&, std::ios_base::openmode)'43 basic_ofstream(const _Path& __s,
44 ^~~~~~~~~~~~~~
45/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:837:2: note: template argument deduction/substitution failed:
46/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:836:32: error: request for member 'make_preferred' in 'std::declval<const wchar_t*&>()', which is of non-class type 'const wchar_t*'47 template<typename _Path, typename _Require= _If_fs_path<_Path>>
48 ^~~~~~~~
49/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:820:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const string&, std::ios_base::openmode) [with _CharT = char; _Traits = std::char_traits<char>; std::__cxx11::string = std::__cxx11::basic_string<char>; std::ios_base::openmode = std::_Ios_Openmode]'50 basic_ofstream(const std::string& __s,
51 ^~~~~~~~~~~~~~
52/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:820:7: note: no known conversion for argument 1 from 'const value_type*'{aka 'const wchar_t*'} to 'const string&'{aka 'const std::__cxx11::basic_string<char>&'}53/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:785:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream(const char*, std::ios_base::openmode) [with _CharT = char; _Traits = std::char_traits<char>; std::ios_base::openmode = std::_Ios_Openmode]'54 basic_ofstream(const char* __s,
55 ^~~~~~~~~~~~~~
56/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:785:7: note: no known conversion for argument 1 from 'const value_type*'{aka 'const wchar_t*'} to 'const char*'57/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:774:7: note: candidate: 'std::basic_ofstream<_CharT, _Traits>::basic_ofstream() [with _CharT = char; _Traits = std::char_traits<char>]'58 basic_ofstream(): __ostream_type(), _M_filebuf()59 ^~~~~~~~~~~~~~
60/gnu/store/pjmb3jry271q98pfrdr44sx4w0cy6mw5-gcc-cross-x86_64-w64-mingw32-8.4.0/include/c++/fstream:774:7: note: candidate expects 0 arguments, 2 provided
61 CXX libbitcoin_server_a-rest.o
62 CXX libbitcoin_server_a-shutdown.o
63 CXX libbitcoin_server_a-signet.o
64make[2]: *** [Makefile:9217: libbitcoin_server_a-init.o] Error 1
hebasto
commented at 9:26 am on October 21, 2021:
member
0 node1 2021-10-21T08:30:38.456944Z [httpworker.0][wallet/wallet.h:792][WalletLogPrintf][default wallet] Rescan completed in 0ms
1 node1 2021-10-21T08:30:38.458646Z [http][httpserver.cpp:238][http_request_cb] Received a POST request for / from 127.0.0.1:57124
2 node1 2021-10-21T08:30:38.458768Z [httpworker.2][rpc/request.cpp:174][parse] ThreadRPCServer method=backupwallet user=__cookie__
3 node1 2021-10-21T08:30:38.460693Z [httpworker.2][util/syscall_sandbox.cpp:487][SyscallSandboxDebugSignalHandler] ERROR: The syscall "sendfile"(syscall number 40) is not allowed by the syscall sandbox in thread "httpworker.2". Please report.
Yes it’s happening during filesystem operations. I’ll add an exception to the commit where we swap to std::filesystem.
MarcoFalke
commented at 10:30 am on October 21, 2021:
member
Any idea why the windows unit test failed?
fanquake
commented at 10:32 am on October 21, 2021:
member
Any idea why the windows unit test failed?
0test/util_tests.cpp(50): Entering test case"util_datadir"12021-10-21T09:51:37.572779Z [test/util/setup_common.cpp:65][Seed] Seed: Setting random seed for current tests to RANDOM_CTX_SEED=72b0882141d4b40dac04b3bff3080a44a6998b9418d1227d677d5e2ab298171c
22021-10-21T09:51:37.572779Z [init/common.cpp:165][LogPackageVersion] Bitcoin Core version v22.99.0-ef048d3a36c4 (release build)32021-10-21T09:51:37.572779Z [init.cpp:904][AppInitParameterInteraction] Assuming ancestors of block 00000000000000000008a89e854d57e5667df88f1cdef6fde2fbca1de5b639ad have valid signatures.
42021-10-21T09:51:37.572779Z [init.cpp:917][AppInitParameterInteraction] Setting nMinimumChainWork=00000000000000000000000000000000000000001fa4663bbbe19f82de910280
52021-10-21T09:51:37.573779Z [script/sigcache.cpp:101][InitSignatureCache] Using 16 MiB out of 32/2 requested for signature cache, able to store 524288 elements
62021-10-21T09:51:37.574779Z [validation.cpp:1274][InitScriptExecutionCache] Using 16 MiB out of 32/2 requested for script execution cache, able to store 524288 elements
7test/util_tests.cpp(72): error: in "util_tests/util_datadir": check dd_norm== args.GetDataDirBase() has failed ["C:\\users\\root\\Temp\\test_common_Bitcoin Core\\04ef3851bd5ebef826abb443ef652d2eee95c19cbcefc481267df04b8f0df460" !=""]8test/util_tests.cpp(50): Leaving test case"util_datadir"; testing time: 57090us
hebasto
commented at 10:35 am on October 21, 2021:
member
ryanofsky
commented at 12:44 pm on October 21, 2021:
In commit “refactor: replace boost::filesystem with std::filesystem” (b70c84348ac7a8e427a1183f894c73e52c734529)
These static_casts seem to be added because even though GetPidFile returns fs::path and fs::path inherits from std::filesystem::path, the basic_ofstream constructor has an _If_fs_path requirement that needs an exact std::filesystem::path match:
It might be nice to drop the need for these static_casts in fsbridge with something like
0- typedef std::ofstream ofstream;
1+ class ofstream : public std::ofstream
2+ {
3+ using std::ofstream::ofstream;
4+ ofstream(const fs::path& p) : ofstream(static_cast<const std::filesystem::path&>(p)) {}
5+ };
Another alternative would just be to pass path.c_str() instead of path when constructing ofstream objects. This should be safe because c_str returns a wide string on windows.
I was looking into this again, thinking passing path.native() might be a third alternative, but it doesn’t work because there’s no constructor https://en.cppreference.com/w/cpp/io/basic_ifstream/basic_ifstream accepting the wstring on windows like the one accepting the wide character pointer returned by path.c_str().
ryanofsky
commented at 1:15 pm on October 21, 2021:
member
Reviewing b70c84348ac7a8e427a1183f894c73e52c734529 (in branch f53be20eade794e55470473f03a8d05d57691c87)
janus referenced this in commit
a9092f47aa
on Nov 11, 2021
pravblockc referenced this in commit
2da267b8c3
on Nov 18, 2021
fanquake referenced this in commit
681b25e3cd
on Nov 25, 2021
sidhujag referenced this in commit
67658eec7d
on Nov 25, 2021
fanquake force-pushed
on Dec 23, 2021
fanquake
commented at 5:04 am on December 23, 2021:
member
I’ve rebased this on master and #23778 (dropping 9604eda1abe6ffc02c97e3434c3c2c4ce1250794 which was the “backport std::filesystem to GCC” commit). I’ve also addressed a number of the comments, including sticking to using fsbridge and removing the use of the static_casts in favour of passing path.c_str(), as well as included some suggestions to fix test failures. There are still a couple failures in the Windows unit tests that need to be addressed.
DrahtBot removed the label
Needs rebase
on Dec 23, 2021
fanquake force-pushed
on Dec 23, 2021
fanquake
commented at 9:48 am on December 23, 2021:
member
Addressed:
The syscall “sendfile” (syscall number 40) is not allowed by the syscall sandbox in thread “httpworker.0”.
fanquake force-pushed
on Dec 23, 2021
fanquake referenced this in commit
bb0c4978db
on Dec 24, 2021
fanquake referenced this in commit
6bd020e2f4
on Dec 24, 2021
fanquake referenced this in commit
2da5bc557d
on Dec 24, 2021
fanquake referenced this in commit
5668ad4339
on Dec 24, 2021
fanquake referenced this in commit
711ce2e533
on Jan 7, 2022
fanquake force-pushed
on Jan 7, 2022
sidhujag referenced this in commit
870a4d39b0
on Jan 7, 2022
Is the c_str() actually needed? On my system (g++ 11.1) this also compiles when I write file.open(filepath_tmp);. This calls operator string_type() const { return _M_pathname; } from std::fileystem::path.
Or maybe add operator string_type() const = delete; to explicitly delete that conversion in the path wrapper
ryanofsky
commented at 6:05 pm on January 7, 2022:
MarcoFalke
commented at 3:47 pm on January 25, 2022:
Wouldn’t it be better to somehow do the c_str call hidden inside the fsbridge namespace? Seems a bit odd to force devs to write ugly and unintuitive code just because mingw can’t even.
ryanofsky
commented at 3:08 pm on January 26, 2022:
Wouldn’t it be better to somehow do the c_str call hidden inside the fsbridge namespace? Seems a bit odd to force devs to write ugly and unintuitive code just because mingw can’t even.
I also like the idea of hiding this in the fsbridge namespace (where there is no need to use c_str() at all), which is why I originally suggested fixing this with:
0- typedef std::ofstream ofstream;
1+ class ofstream : public std::ofstream
2+ {
3+ using std::ofstream::ofstream;
4+ ofstream(const fs::path& p) : ofstream(static_cast<const std::filesystem::path&>(p)) {}
5+ };
But @hebasto preferred the second alternative I suggested, which was calling to c_str() so we went with that approach instead. I still do like not calling c_str() better, but don’t have a strong opinion.
ryanofsky
commented at 4:50 pm on January 26, 2022:
I still do like not calling c_str() better,
What reasons for?
Mainly just for the reason that marco wrote #20744 (comment) that the .c_str() calls look ugly and and unsafe even if they are safe. It could be confusing that path.c_str() is safe when path.string().c_str() is unsafe, and it’s nice to just be able to pass plain path. Also just passing the path object directly avoids temporary objects. No strong preference though, and no problem with either approach.
Would it be useful to summarize what one should do and what shouldn’t with the new filesystem API with regard to paths? Or is it somewhere? I mean I know ryanofsky wrote many useful comments in this PR and in others. Maybe we can have a few bullet points saying “this is OK, this is not” so that there are actually more people who can review PRs with FS changes. Just an idea. Feel free to ignore.
ryanofsky
commented at 6:32 pm on January 26, 2022:
Would it be useful to summarize what one should do and what shouldn’t with the new filesystem API with regard to paths? Or is it somewhere? I mean I know ryanofsky wrote many useful comments in this PR and in others. Maybe we can have a few bullet points saying “this is OK, this is not” so that there are actually more people who can review PRs with FS changes. Just an idea. Feel free to ignore.
I think main recommendation is to use fs::path object wherever possible and avoid representing paths as strings. Beyond that there is a note in developer notes:
Use fs::path::u8string() and fs::u8path() functions when converting path
to JSON strings, not fs::PathToString and fs::PathFromString
Rationale: JSON strings are Unicode strings, not byte strings, and
RFC8259 requires JSON to be encoded as UTF-8.
And a detailed comment on the PathToString function saying when it should be used.
You could write more documentation, but I think current documentation is at least not bad.
martinus approved
martinus
commented at 5:54 pm on January 7, 2022:
contributor
tested ACK, fixes the boost 1.78 issue #23846 for me. All tests work for me, and bitcoind is syncing. Could only do light code review as I don’t know too much about the details
fanquake force-pushed
on Jan 10, 2022
MarcoFalke
commented at 9:29 am on January 10, 2022:
member
0 test 2022-01-10T08:47:25.172000Z TestFramework (INFO): Restoring wallets on node 3 using backup files
1 test 2022-01-10T08:47:25.172000Z TestFramework (ERROR): Assertion failed
2 Traceback (most recent call last):
3 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\util.py", line 133, in try_rpc
4 fun(*args, **kwds)
5 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\coverage.py", line 49, in __call__
6 return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
7 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\authproxy.py", line 144, in __call__
8 raise JSONRPCException(response['error'], status)
9 test_framework.authproxy.JSONRPCException: copy_file: The system cannot find the path specified.:
10"C:\Users\ContainerAdministrator\AppData\Local\Temp\test_runner_₿_🏃_20220110_084607\wallet_backup_228\node0\invalid_wallet_file.bak",
11"C:\Users\ContainerAdministrator\AppData\Local\Temp\test_runner_?_??_20220110_084607\wallet_backup_228\node3\regtest\wallets\res0\wallet.dat" (-1)
12 During handling of the above exception, another exception occurred:
13 Traceback (most recent call last):
14 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\test_framework.py", line 132, in main
15 self.run_test()
16 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\wallet_backup.py", line 195, in run_test
17 self.restore_invalid_wallet()
18 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\wallet_backup.py", line 120, in restore_invalid_wallet
19 assert_raises_rpc_error(-18, error_message, node.restorewallet, wallet_name, invalid_wallet_file)
20 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\util.py", line 124, in assert_raises_rpc_error
21 assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
22 File "C:\Users\ContainerAdministrator\AppData\Local\Temp\cirrus-ci-build\test\functional\test_framework\util.py", line 137, in try_rpc
23 raise AssertionError("Unexpected JSONRPC error code %i" % e.error["code"])
24 AssertionError: Unexpected JSONRPC error code -1
fanquake
commented at 9:44 am on January 10, 2022:
member
@sipsorcery@kiminuo would either of you be interested in taking a look at the native windows failures / windows related changes in general?
hebasto
commented at 11:18 am on January 10, 2022:
member
FWIW, both wallet_backup.py --legacy-wallet and wallet_backup.py --descriptors passed locally on Windows 10 Pro 21H1 (build 19043.1415):
0C:\Users\hebasto\bitcoin>test\functional\wallet_backup.py --legacy-wallet
12022-01-10T11:11:21.779000Z TestFramework (INFO): Initializing test directory C:\Users\hebasto\AppData\Local\Temp\bitcoin_func_test_k024z371
22022-01-10T11:11:29.404000Z TestFramework (INFO): Generating initial blockchain
32022-01-10T11:12:00.353000Z TestFramework (INFO): Creating transactions
42022-01-10T11:12:21.280000Z TestFramework (INFO): Backing up
52022-01-10T11:12:21.982000Z TestFramework (INFO): More transactions
62022-01-10T11:13:09.299000Z TestFramework (INFO): Restoring wallets on node 3 using backup files
72022-01-10T11:13:13.435000Z TestFramework (INFO): Restoring using dumped wallet
82022-01-10T11:13:45.515000Z TestFramework (INFO): Stopping nodes
92022-01-10T11:13:46.544000Z TestFramework (INFO): Cleaning up C:\Users\hebasto\AppData\Local\Temp\bitcoin_func_test_k024z371 on exit
102022-01-10T11:13:46.544000Z TestFramework (INFO): Tests successful
1112C:\Users\hebasto\bitcoin>test\functional\wallet_backup.py --descriptors
132022-01-10T11:15:03.093000Z TestFramework (INFO): Initializing test directory C:\Users\hebasto\AppData\Local\Temp\bitcoin_func_test_8dguzjzh
142022-01-10T11:15:06.702000Z TestFramework (INFO): Generating initial blockchain
152022-01-10T11:15:12.310000Z TestFramework (INFO): Creating transactions
162022-01-10T11:15:31.376000Z TestFramework (INFO): Backing up
172022-01-10T11:15:31.482000Z TestFramework (INFO): More transactions
182022-01-10T11:15:48.732000Z TestFramework (INFO): Restoring wallets on node 3 using backup files
192022-01-10T11:15:50.200000Z TestFramework (INFO): Stopping nodes
202022-01-10T11:15:50.440000Z TestFramework (INFO): Cleaning up C:\Users\hebasto\AppData\Local\Temp\bitcoin_func_test_8dguzjzh on exit
212022-01-10T11:15:50.440000Z TestFramework (INFO): Tests successful
and
0C:\Users\hebasto\bitcoin>test\functional\test_runner.py --filter="wallet_backup"
1Temporary test directory at C:\Users\hebasto\AppData\Local\Temp/test_runner_₿_🏃_20220110_132744
2Running Unit Tests for Test Framework Modules
3..........
4----------------------------------------------------------------------
5Ran 10 tests in 0.676s
6 7OK
8Remaining jobs: [wallet_backup.py --legacy-wallet, wallet_backup.py --descriptors]
91/2 - wallet_backup.py --descriptors passed, Duration: 46 s
10Remaining jobs: [wallet_backup.py --legacy-wallet]
112/2 - wallet_backup.py --legacy-wallet passed, Duration: 82 s
1213TEST | STATUS | DURATION
1415wallet_backup.py --descriptors | ✓ Passed | 46 s
16wallet_backup.py --legacy-wallet | ✓ Passed | 82 s
1718ALL | ✓ Passed | 128 s (accumulated)
19Runtime: 82 s
kiminuo
commented at 11:18 am on January 10, 2022:
contributor
@fanquake I’ll try my best to have a look in a day or two.
Great job so far :)
hebasto
commented at 7:46 pm on January 10, 2022:
member
fanquake referenced this in commit
fa74718414
on Jan 11, 2022
fanquake force-pushed
on Jan 11, 2022
in
src/fs.h:25
in
9a1ace7029outdated
37- * method, which worked well in the boost::filesystem implementation, but have
38- * unsafe and unpredictable behavior on Windows in the std::filesystem
39- * implementation (see implementation note in \ref PathToString for details).
40+ * Path class wrapper to blocks calls to the fs::path(std::string) implicit
41+ * constructor and the fs::path::string() method, which have unsafe and
42+ * unpredictable behavior on Windows(see implementation note in
in
src/wallet/test/init_tests.cpp:76
in
9a1ace7029outdated
71@@ -72,6 +72,8 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
72 BOOST_CHECK_EQUAL(walletdir, expected_path);
73 }
7475+#ifndef WIN32
76+// Windows does not consider "datadir///" to be a valid directory path.
0// Windows does not consider "datadir/wallets//" to be a valid directory path.
in
configure.ac:86
in
9a1ace7029outdated
82@@ -83,6 +83,9 @@ AX_CXX_COMPILE_STDCXX([17], [noext], [mandatory])
83 dnl Check if -latomic is required for <std::atomic>
84 CHECK_ATOMIC
8586+dnl check if -lstdc++fs is required for std::filesystem
6fa8c2300ba1d05fd3afeacc3da31204cd05c189
nit: CHECK_FILESYSTEM macro actually can test -lc++fs flag as well. Therefore, the comment should mention both flags or none of them.
in
build-aux/m4/l_filesystem.m4:23
in
9a1ace7029outdated
11+
12+AC_DEFUN([CHECK_FILESYSTEM], [
13+
14+ AC_LANG_PUSH(C++)
15+
16+ AC_MSG_CHECKING([whether std::filesystem can be used without link library])
6fa8c2300ba1d05fd3afeacc3da31204cd05c189
nit: Could a comment be added with examples of currently supported tools which require -lstdc++fs or -lc++fs? This can help in the future to determine whether this macro could be dropped.
hebasto
commented at 9:05 am on January 11, 2022:
member
Also a reminder about #20744#pullrequestreview-848269313
Maybe you’d want to address some comments from #23522:
Maybe you’d want to address some comments from #23522:
All should now be addressed. I resolved in that PR.
Fixed up current review comments as well.
Fabcien referenced this in commit
feb2a01c35
on Jan 11, 2022
sipsorcery
commented at 5:25 pm on January 11, 2022:
member
@sipsorcery@kiminuo would either of you be interested in taking a look at the native windows failures / windows related changes in general?
My tests so far point to the Windows test failure being a pythin unicode issue.
Running the test using wallet_backup.py always passes:
0c:\Dev\github\sipsorcery_bitcoin>test\functional\wallet_backup.py --descriptors
12022-01-10T21:02:34.014000Z TestFramework (INFO): Initializing test directory C:\Users\aaron\AppData\Local\Temp\bitcoin_func_test_jze1co0z
Running the test using test_runner.py always fails:
0c:\Dev\github\sipsorcery_bitcoin>test\functional\test_runner.py --filter="wallet_backup.py --descriptors"
1Temporary test directory at C:\Users\aaron\AppData\Local\Temp/test_runner_₿_🏃_20220110_213157
In the latter, failing case there are unicode characters in the path. The python exception message states it can’t encode ‘\u20bf’, but as seen in the log message above my console has no issue with it: ₿.
I’ll keep digging.
Update:
The Python character encoding errors can be fixed with set PYTHONIOENCODING=UTF-8, e.g:
0c:\Dev\github\sipsorcery_bitcoin>set PYTHONIOENCODING=UTF-8
1c:\Dev\github\sipsorcery_bitcoin>test\functional\test_runner.py --filter="wallet_backup.py --descriptors"
2Temporary test directory at C:\Users\aaron\AppData\Local\Temp/test_runner_₿_🏃_20220111_185504
3Running Unit Tests for Test Framework Modules
4..........
5----------------------------------------------------------------------
6Ran 10 tests in 0.899s
7 8OK
9Remaining jobs: [wallet_backup.py --descriptors]
101/1 - wallet_backup.py --descriptors passed, Duration: 58 s
1112TEST | STATUS | DURATION
1314wallet_backup.py --descriptors | ✓ Passed | 58 s
1516ALL | ✓ Passed | 58 s (accumulated)
17Runtime: 58 s
There are still a bunch of RPC and other errors that occur when doing a full test_runner.py but I don’t think they are introduced by this PR. I’m going to compare against master now.
hebasto
commented at 6:11 pm on January 11, 2022:
member
sidhujag referenced this in commit
7f43055fac
on Jan 12, 2022
hebasto referenced this in commit
e0ae5418cb
on Jan 12, 2022
sipsorcery
commented at 6:40 pm on January 12, 2022:
member
@sipsorcery@kiminuo would either of you be interested in taking a look at the native windows failures / windows related changes in general?
@fanquake what are the windows failures with this PR? I’d assumed it was to do with the python functional tests but I get the same failures before and after this PR.
Relevant results of test\functional\test_runner.py:
0wallet_watchonly.py --usecli --legacy-wallet |✓ Passed |5 s
1feature_backwards_compatibility.py --descriptors |○ Skipped |1 s
2feature_backwards_compatibility.py --legacy-wallet |○ Skipped |1 s
3feature_bind_extra.py |○ Skipped |0 s
4feature_init.py |○ Skipped |3 s
5feature_syscall_sandbox.py |○ Skipped |0 s
6feature_taproot.py --previous_release |○ Skipped |0 s
7feature_txindex_compatibility.py |○ Skipped |0 s
8mempool_compatibility.py |○ Skipped |0 s
9rpc_bind.py --ipv4 |○ Skipped |1 s
10rpc_bind.py --ipv6 |○ Skipped |0 s
11rpc_bind.py --nonloopback |○ Skipped |0 s
12wallet_upgradewallet.py --legacy-wallet |○ Skipped |0 s
13feature_config_args.py |✖ Failed |13 s
14interface_bitcoin_cli.py --descriptors |✖ Failed |6 s
15interface_bitcoin_cli.py --legacy-wallet |✖ Failed |6 s
16interface_rpc.py |✖ Failed |34 s
17tool_wallet.py --descriptors |✖ Failed |19 s
18tool_wallet.py --legacy-wallet |✖ Failed |15 s
19wallet_multiwallet.py --descriptors |✖ Failed |3 s
20wallet_multiwallet.py --legacy-wallet |✖ Failed |3 s
21wallet_multiwallet.py --usecli |✖ Failed |3 s
2223ALL |✖ Failed |5971 s (accumulated)
MarcoFalke
commented at 6:41 pm on January 12, 2022:
member
I think the CI issues are fixed (telling from the green ci?)
MarcoFalke
commented at 6:42 pm on January 12, 2022:
member
achow101
commented at 8:49 pm on January 18, 2022:
member
On windows, I am hitting the assert at src/util/system.cpp:258
fanquake referenced this in commit
63fc2f5cce
on Jan 20, 2022
fanquake force-pushed
on Jan 20, 2022
MarcoFalke
commented at 12:46 pm on January 20, 2022:
member
You forgot to revert dc5d6b0d4793ca978f71f69ef7d6b818794676c2
fanquake force-pushed
on Jan 20, 2022
hebasto
commented at 9:35 pm on January 20, 2022:
member
I’ve restarted “ARM64 Android APK [focal]” CI build due to unrelated to this PR network issues.
fanquake force-pushed
on Jan 25, 2022
MarcoFalke added the label
DrahtBot Guix build requested
on Jan 25, 2022
hebasto approved
hebasto
commented at 10:14 am on January 25, 2022:
member
ACK45e546c877a30a006ccb24aa465a9f6c1a96554a
MarcoFalke
commented at 9:45 am on January 26, 2022:
member
Seems fine to merge, but I am not yet convinced this is all correct:
There is a fsbridge::ofstream constructor that takes a std::string. Why is this safe, considering that usually this was passed through PathFromString? See also #23857 (comment)
There are a bunch of calls to c_str(), which are ugly and not exactly promoting safe code. If they are scattered around in the codebase, reviewers won’t be able to see fs::path{std::string{}.c_str()} calls, which are less safe than PathFromString? See also https://github.com/bitcoin/bitcoin/pull/20744/files#r791853189
fanquake referenced this in commit
9ec56a84c3
on Jan 26, 2022
fanquake referenced this in commit
c6cc016679
on Jan 26, 2022
fanquake referenced this in commit
21f781ad79
on Jan 26, 2022
fanquake referenced this in commit
486261dfcb
on Jan 26, 2022
ryanofsky
commented at 3:20 pm on January 26, 2022:
member
Seems fine to merge, but I am not yet convinced this is all correct:
There is a fsbridge::ofstream constructor that takes a std::string. Why is this safe, considering that usually this was passed through PathFromString? See also [fs: consistently use fsbridge:: for ifstream / ofstream #23857 (comment)](/bitcoin-bitcoin/23857/#issuecomment-1021995207)
It is not safe to call the fstream std::string constructors even with PathToString! On windows, this will still do a locale conversion. Briefly skimming the code, I don’t see this happening anywhere though.
There are a bunch of calls to c_str(), which are ugly and not exactly promoting safe code. If they are scattered around in the codebase, reviewers won’t be able to see fs::path{std::string{}.c_str()} calls, which are less safe than PathFromString? See also https://github.com/bitcoin/bitcoin/pull/20744/files#r791853189
To be clear, the reason why path.c_str() is safe is because it is equivalent to path.native().c_str() which returns const wchar_t* on windows not path.string().c_str() which returns const char*. Using path.native() is good, using path.string() is bad, using something.c_str() is neutral.
MarcoFalke
commented at 5:44 pm on January 26, 2022:
member
It is not safe to call the fstream std::string constructors even with PathToString! On windows, this will still do a locale conversion. Briefly skimming the code, I don’t see this happening anywhere though.
I see. There isn’t necessarily a bug here if command line arguments are utf-8, but the code is not ideal. More ideally benchmark::Argsoutput_csv and output_json fields would be fs::path not std::string members, so any encoding/decoding would happen when the command line argument is parsed, not when the path is accessed.
fanquake referenced this in commit
d87a37a4ab
on Jan 27, 2022
DrahtBot added the label
Needs rebase
on Jan 27, 2022
fanquake force-pushed
on Jan 28, 2022
fanquake
commented at 3:49 am on January 28, 2022:
member
I’ve rebased, removed some non-essential changes, (there seemed to be issues with mingw and braced initialization), and given the discussion I have dropped the usage of .c_str() in favour of @ryanofsky’s suggested change.
fanquake force-pushed
on Jan 28, 2022
DrahtBot removed the label
Needs rebase
on Jan 28, 2022
in
src/fs.h:23
in
6dd6f6f4dfoutdated
35- * functions not present in boost. It also blocks calls to the
36- * fs::path(std::string) implicit constructor and the fs::path::string()
37- * method, which worked well in the boost::filesystem implementation, but have
38- * unsafe and unpredictable behavior on Windows in the std::filesystem
39- * implementation (see implementation note in \ref PathToString for details).
40+ * Path class wrapper to blocks calls to the fs::path(std::string) implicit
0rpc/request.cpp: In function ‘bool GenerateAuthCookie(std::string*)’:
1rpc/request.cpp:88:27: error: no matching function for call to ‘fsbridge::ofstream::open(fs::path&)’
2 88 | file.open(filepath_tmp);
3 | ^
prusnak
commented at 11:18 am on January 28, 2022:
contributor
Ah, sorry. I missed that error. There are lots of others related to Qt 5.15.2.
sidhujag referenced this in commit
f9de29fcb9
on Jan 28, 2022
prusnak
commented at 5:56 pm on January 31, 2022:
contributor
Just for the record:
While trying to reproduce this error I hit another error which is fixed by #24131 (I uninstalled g++-mingw-w64-x86-64 and kept only g++-mingw-w64-x86-64-posix installed on my Debian 11 Buster system).
prusnak
commented at 6:14 pm on January 31, 2022:
contributor
I reproduced the issue locally.
According to reference, ofstream::open(fs::path&) has been add in C++17.
I can see x86_64-w64-mingw32-g++-posix -std=c++17 ... in the build log, so the standard is passed correctly. Maybe mingw does not implement everything from the standard?
prusnak
commented at 6:44 pm on January 31, 2022:
contributor
The following changes fix the build on Mingw for me. I have not tested the resulting binaries!
Not sure if calling c_str() is correct or desired, but at least the changes indicate where are the places which upset the compiler.
Another and probably more clean option is to overload the ifstream::open/ofstream::open functions to also accept fs::path as the first argument.
prusnak
commented at 8:43 pm on January 31, 2022:
contributor
Not sure if calling c_str() is correct or desired, but at least the changes indicate where are the places which upset the compiler.
Reading more about how things are implemented in fs.h, it seems that we are actively avoiding the default conversions of std::filesystem::path to std::string, by using our own custom fs::path. Therefore the correct thing would be to call fs::PathToString(foo) instead of foo.c_str() in all places I indicated in my patch above.
Updated patch (unfortunately, I don’t have the capacity to try whether it compiles right now):
ryanofsky
commented at 8:53 pm on January 31, 2022:
member
Reading more about how things are implemented in fs.c, it seems that we are actively avoiding the default conversions of std::filesystem::path to std::string, by using our own custom fs::path. Therefore the correct thing would be to call fs::PathToString(foo) instead of foo.c_str() in all places I indicated in my patch above.
To be clear we are actively avoiding converting paths to narrow strings, because on windows it will result in possibly incorrect encodings using the current windows code page. Calling fs::PathToString would also be incorrect and does not avoid the problem, because it would convert the wide string to a narrow string as utf8, but then narrow string would be decoded back to a wide string using the current windows code page again (which would be incorrect if the code page is not also UTF-8). Calling path.c_str() is correct because it is equivalent to path.native().c_str() and avoids encoding / decoding whatsoever.
ryanofsky
commented at 9:04 pm on January 31, 2022:
member
0wallet/db.cpp: In function ‘bool wallet::IsSQLiteFile(const fs::path&)’:
1wallet/db.cpp:113:51: error: no matching function for call to ‘std::basic_ifstream<char>::basic_ifstream(const fs::path&, const openmode&)’2113| fsbridge::ifstream file(path, std::ios::binary);
You could either define an fsbridge::ifstream class similar to the fsbridge::ofstream class defining an fs::path constructor:
hebasto
commented at 9:36 pm on February 2, 2022:
member
I’ve done some research, and suggesting a–seemingly more elegant–solution: s/fsbridge::{i,o}fstream/std::{i,o}fstream/g, and apply the following patch:
0--- a/src/fs.h
1+++ b/src/fs.h
2@@ -51,6 +51,11 @@ public:
3 4 // Disallow std::string conversion method to avoid locale-dependent encoding on windows.
5 std::string string() const = delete;
6+
7+ // Required for path overloads in <fstream>.
8+ // See https://gcc.gnu.org/git/?p=gcc.git;a=commit;h=96e0367ead5d8dcac3bec2865582e76e2fbab190
9+ path& make_preferred() { std::filesystem::path::make_preferred(); return *this; }
10+ path filename() const { return std::filesystem::path::filename(); }
11 };
1213 // Disallow implicit std::string conversion for absolute to avoid
14@@ -174,20 +179,6 @@ namespace fsbridge {
15 };
1617 std::string get_filesystem_error_message(const fs::filesystem_error& e);
18-
19- class ifstream : public std::ifstream
20- {
21- using std::ifstream::ifstream;
22- public:
23- ifstream(const fs::path& p) : ifstream(static_cast<const std::filesystem::path&>(p)) {}
24- };
25-
26- class ofstream : public std::ofstream
27- {
28- using std::ofstream::ofstream;
29- public:
30- ofstream(const fs::path& p) : ofstream(static_cast<const std::filesystem::path&>(p)) {}
31- };
32 };
3334 // Disallow path operator<< formatting in tinyformat to avoid locale-dependent
Guix builds for all (working) HOSTS will be added to the PR description shortly.
It seems they lack a build for aarch64-linux-gnu arch.
fanquake
commented at 10:02 am on February 3, 2022:
member
It seems they lack a build for aarch64-linux-gnu arch.
Added.
build: add support for std::filesystem
Add a macro to check if linking with -lstdc++fs or -lc++fs is required.
ffc89d1f21
refactor: replace boost::filesystem with std::filesystem
Warning: Replacing fs::system_complete calls with fs::absolute calls
in this commit may cause minor changes in behaviour because fs::absolute
no longer strips trailing slashes; however these changes are believed to
be safe.
Co-authored-by: Russell Yanofsky <russ@yanofsky.org>
Co-authored-by: Hennadii Stepanov <32963518+hebasto@users.noreply.github.com>
41d7166c8a
build: remove boost::filesystem usageb87f9c5edf
build: remove Boost::system usage07269321f3
in
build-aux/m4/l_filesystem.m4:6
in
db2b9123bboutdated
0@@ -0,0 +1,47 @@
1+dnl Copyright (c) 2022 The Bitcoin Core developers
2+dnl Distributed under the MIT software license, see the accompanying
3+dnl file COPYING or http://www.opensource.org/licenses/mit-license.php.
4+
5+# GCC 9.1 and earlier requires -lstdc++fs
6+# Clang 9.0 (libc++) and earlier requires -lc++fs
hebasto
commented at 10:18 am on February 3, 2022:
def4c1704d27754398375880d9a0ba8f31a0464d
This comment makes me think that using CXX='clang++-9 -stdlib=libc++' ends with linking -lc++fs. That is not the case. Tested on tested on Linux Mint 20.2 (x86_64) with clang-8 and clang-9.
0# Prior to LLVM 9.0, libc++ provides the implementation of the filesystem library in a separate static library,
1# and using of <filesystem> requires to link -lc++fs.
or something similar with better wording?
hebasto
commented at 10:30 am on February 3, 2022:
I’ve done this, as it seems everyone is in favour of keeping changes constrained to fs.* rather than using c_str() over the codebase.
Definitely.
hebasto
commented at 12:59 pm on February 3, 2022:
member
Tested 07269321f38e46e9e02f16d7cd135ea90692638d on macOS Catalina 10.15.7 (19H1713): being compiled with Homebrew’s packages and run flawlessly.
hebasto
commented at 1:14 pm on February 3, 2022:
member
Tested 07269321f38e46e9e02f16d7cd135ea90692638d on Ubuntu Bionic 18.04.
The third commit b87f9c5edf3895df9650131fcf8551c3ad1d7301 compiles ok, but the last commit fails:
0$./autogen.sh
1$./configure --without-gui --without-bdb CC=gcc-8 CXX=g++-8 2$ make clean
3$ make
4... 5libbitcoin_util.a(libbitcoin_util_a-system.o): In function `boost::system::error_category::std_category::equivalent(std::error_code const&, int) const': 6/usr/include/boost/system/error_code.hpp:686: undefined reference to `boost::system::generic_category()' 7/usr/include/boost/system/error_code.hpp:689: undefined reference to `boost::system::generic_category()' 8/usr/include/boost/system/error_code.hpp:701: undefined reference to `boost::system::generic_category()' 9libbitcoin_util.a(libbitcoin_util_a-system.o): In function `boost::system::error_category::std_category::equivalent(int, std::error_condition const&) const':10/usr/include/boost/system/error_code.hpp:656: undefined reference to `boost::system::generic_category()'11/usr/include/boost/system/error_code.hpp:659: undefined reference to `boost::system::generic_category()'12libbitcoin_util.a(libbitcoin_util_a-system.o):/usr/include/boost/system/error_code.hpp:206: more undefined references to `boost::system::generic_category()' follow13libbitcoin_util.a(libbitcoin_util_a-system.o): In function `__static_initialization_and_destruction_0':14/usr/include/boost/system/error_code.hpp:210: undefined reference to `boost::system::system_category()'15libbitcoin_util.a(libbitcoin_util_a-system.o): In function `_GLOBAL__sub_I_BITCOIN_CONF_FILENAME':16/usr/include/boost/asio/error.hpp:230: undefined reference to `boost::system::system_category()'17collect2: error: ld returned 1 exit status
18Makefile:5708: recipe for target 'bitcoind' failed
19make[2]: *** [bitcoind] Error 120...
MarcoFalke
commented at 1:29 pm on February 3, 2022:
member
MarcoFalke
commented at 1:47 pm on February 3, 2022:
member
Yes
in
src/test/util_tests.cpp:79
in
41d7166c8aoutdated
70@@ -70,9 +71,12 @@ BOOST_AUTO_TEST_CASE(util_datadir)
71 args.ClearPathCache();
72 BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
7374+#ifndef WIN32
75+ // Windows does not consider "datadir/.//" to be a valid directory path.
76 args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.//");
77 args.ClearPathCache();
78 BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
79+#endif
ryanofsky
commented at 2:00 pm on February 3, 2022:
In commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247)
The way this is written, it’s confusing and vague about windows test behavior. Would change this to actually check windows behavior instead of skipping it, and also add more detail to the comment:
0 args.ForceSetArg("-datadir", fs::PathToString(dd_norm) +"/.//");
1 args.ClearPathCache();
2#ifndef WIN32
3 BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
4#else
5// Windows does not consider "datadir/.//" to be a valid directory path, so
6// the check to see if the directory exists returns false on windows, and
7// GetDataDirBase() returns nothing. When this test was originally written
8// GetDataDirBase() function stripped trailing "/" slashes, so the test was
9// the same on all platforms. But as of [#20744](/bitcoin-bitcoin/20744/), trailing slashes are no
10// longer stripped (because it would have complicated the GetDataDirBase
11// implementation), so the test is now platform-specific.
12 BOOST_CHECK(args.GetDataDirBase().empty());
13#endif
I think trying to clarify things here is worth it, because this test is already very confusing (every time I look at it my brain hurts), and most developers are not using windows, so problems that only happen on windows are especially painful to deal with.
(This is also closer to originally suggested fix from #20744#pullrequestreview-785640725)
MarcoFalke
commented at 2:07 pm on February 3, 2022:
I think the goal was to not allow the unit test to do random crap to the root dir /, outside their assigned m_path_root
ryanofsky
commented at 2:51 pm on February 3, 2022:
I think the goal was to not allow the unit test to do random crap to the root dir /, outside their assigned m_path_root
I don’t think that’s what’s happening here. In any case:
It is good to replace a vague comment with a specific comment.
The point of the test is to ensure bitcoin has sane behavior if user uses extra trailing “./” characters in a custom datadir on linux. If we care about this behavior on linux, I don’t think think we should say anything goes on windows and not check behavior there. Windows is a 30+ year old operating system with stable path semantics. If path semantics somehow do change, it is a good thing if the test fails, and the test has an understandable comment, we decide can what to do about it.
ryanofsky
commented at 2:59 pm on February 3, 2022:
In commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247)
248@@ -243,7 +249,7 @@ namespace {
249 fs::path StripRedundantLastElementsOfPath(const fs::path& path)
250 {
251 auto result = path;
252- while (fs::PathToString(result.filename()) == ".") {
253+ while (result.filename().empty() || fs::PathToString(result.filename()) == ".") {
ryanofsky
commented at 2:16 pm on February 3, 2022:
In commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247)
Note: Just to explain this change, it’s needed because std path class, unlike boost path class treats paths with trailing slashes as having a final "" component, instead of a final "." component.
(Unfortunately, links like #20744 (review) do not work for me. There are probably too much comments in this PR or it’s resolved. I don’t know.)
ryanofsky
commented at 2:54 pm on February 3, 2022:
(Unfortunately, links like #20744 (comment) do not work for me. There are probably too much comments in this PR or it’s resolved. I don’t know.)
Github struggles with long discusions threads, and these links often they stop working and then start working again. They’re the same link used in github notification emails so not special.
kiminuo
commented at 2:18 pm on February 3, 2022:
contributor
ryanofsky
commented at 2:20 pm on February 3, 2022:
In commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247)
Note: Just to explain this change, this line is deleted because the path variable is set 2 lines above with literal “blocks” last element, so this line would never do anything, even before this PR. Earlier discussion was #20744 (review)
kiminuo
commented at 2:22 pm on February 3, 2022:
contributor
@hebasto I tried to compile on Ubuntu 18.04.6 LTS with your instructions and I can see the same result.
MarcoFalke
commented at 2:37 pm on February 3, 2022:
member
I think this is ready for merge. Any developers (hopefully none) on Ubuntu Bionic can:
Upgrade to a more recent Ubuntu
Revert the last commit locally
Build from depends
Install a more recent boost
Merging now gives more time for testing in master and we still have about 3 weeks to decide how to fix the build on Ubuntu Bionic, with the easiest being a clean revert of the last commit.
Any other minor fixups (like in the tests) can also land in the next 3 weeks.
hebasto
commented at 2:40 pm on February 3, 2022:
member
ryanofsky
commented at 3:04 pm on February 3, 2022:
In commit “build: remove boost::filesystem usage” (b87f9c5edf3895df9650131fcf8551c3ad1d7301)
Note: this change was needed because fs::equivalent was throwing an exception if the path didn’t exist. Earlier discussion was #20744 (comment)
in
src/wallet/load.cpp:34
in
41d7166c8aoutdated
28@@ -27,9 +29,9 @@ bool VerifyWallets(WalletContext& context)
2930 if (args.IsArgSet("-walletdir")) {
31 fs::path wallet_dir = fs::PathFromString(args.GetArg("-walletdir", ""));
32- boost::system::error_code error;
33+ std::error_code error;
34 // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory
35- fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error).remove_trailing_separator();
36+ fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error);
ryanofsky
commented at 3:13 pm on February 3, 2022:
In commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247)
Note: std canonical function already strips trailing slashes, so this should not be a change in behavior
in
src/wallet/test/init_tests.cpp:77
in
41d7166c8aoutdated
72@@ -73,6 +73,8 @@ BOOST_AUTO_TEST_CASE(walletinit_verify_walletdir_no_trailing)
73 BOOST_CHECK_EQUAL(walletdir, expected_path);
74 }
7576+#ifndef WIN32
77+// Windows does not consider "datadir/wallets//" to be a valid directory path.
ryanofsky
commented at 3:24 pm on February 3, 2022:
In commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247)
What’s the behavior if you use this path on windows? I don’t think we want bitcoins’s behavior undefined and untested in this case. It would be better for users and also clearer in the test code to check what the resulting behavior is on windows and decide if it is good or bad.
Or best thing might be to add a StripRedundantLastElementsOfPath call in the -walletdir= parsing code like the one that was added to GetDataDir in this PR, so this test could go back to be enabled and working the same across platforms.
MarcoFalke
commented at 4:04 pm on February 3, 2022:
I was assuming that this modifies gArgs, but given that the args is local, it should be fine, assuming that the argsmanager doesn’t write to disk by itself.
ryanofsky
commented at 3:35 pm on February 3, 2022:
member
Partial code review ACK for commit “refactor: replace boost::filesystem with std::filesystem” (41d7166c8a598b604aad6c6b1d88ad46e23be247). I looked at this commit in detail, just not the other commits.
Overall I think this is good, but there is some dodginess going on with trailing / characters in paths, that is showing up in comments here about tests on windows, but could indicate unintended changes in behavior that cause bugs on other platforms. Windows complains more about extra / characters in paths but that doesn’t mean extra / characters don’t break things on other platforms, too. For example if there are string comparisons that no longer return true because of extra slashes.
ryanofsky referenced this in commit
8287489e7c
on Feb 3, 2022
ryanofsky referenced this in commit
3278e64a22
on Feb 3, 2022
sidhujag referenced this in commit
5ebae3655f
on Feb 3, 2022
rebroad referenced this in commit
b5656d9925
on Feb 3, 2022
rebroad referenced this in commit
5bc3ee2cd6
on Feb 3, 2022
hebasto
commented at 9:12 pm on February 3, 2022:
member
This is a metadata mirror of the GitHub repository
bitcoin/bitcoin.
This site is not affiliated with GitHub.
Content is generated from a GitHub metadata backup.
generated: 2024-11-16 21:12 UTC
This site is hosted by @0xB10C More mirrored repositories can be found on mirror.b10c.me
