Use std::filesystem. Remove Boost Filesystem & System#20744
Conversation
2733fe3 to
ac8a446
Compare
maflcko
left a comment
There was a problem hiding this comment.
Concept ACK for 0.23 (earliest), given that this again requires a bunch of aggressive bumps.
ac8a446 to
8d148b9
Compare
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
mjdietzx
left a comment
There was a problem hiding this comment.
Nice
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
I rebased on your branch and tested it out, seems to work: mjdietzx@55bafb9 - not sure if you want to pull it into this PR, or once this get merged I'll re-open the PR
| if (!BaseParams().DataDir().empty()) { | ||
| path /= BaseParams().DataDir(); | ||
| } |
There was a problem hiding this comment.
similar to "Why not just GetWalletDir() / name even if name is empty?", is another if branch really needed here?
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (f449c26)
re: #20744 (comment)
similar to "Why not just GetWalletDir() / name even if name is empty?", is another
ifbranch really needed here?
See #20744 (comment), this avoids adding an empty path component and trailing slash if the base datadir is empty
There was a problem hiding this comment.
@fanquake I believe this one should be:
path = fsbridge::AbsPathJoin(path, BaseParams().DataDir());There was a problem hiding this comment.
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:
fs::path AbsPathJoin(fs::path p1, fs::path p2)
{
assert(p1.is_absolute());
return boost::filesystem::absolute(p2, p1);
}Then I think this PR wouldn't need to touch any meaningful code except one line in AbsPathJoin:
fs::path AbsPathJoin(fs::path p1, fs::path p2)
{
assert(p1.is_absolute());
return p2.empty() ? p1 : p1 / p2;
}|
@ryanofsky It is maybe worth reading "Differences between filesystem implementations that affects Bitcoin code" in #19245. |
Thanks, I saw it but don't think it takes into account the "appends path::preferred_separator" behavior described https://en.cppreference.com/w/cpp/filesystem/path/append which can result in trailing slashes and is different from boost::absolute behavior https://www.boost.org/doc/libs/1_66_0/libs/filesystem/doc/reference.html#absolute. 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. |
|
Concept ACK |
|
Tested 0726932 on Ubuntu Bionic 18.04. The third commit b87f9c5 compiles ok, but the last commit fails: |
|
Concept ACK 0726932 🎀 Show signatureSignature: |
|
This only happens with system libs and not depends? |
CI builds with depends, right? |
|
Yes |
| #ifndef WIN32 | ||
| // Windows does not consider "datadir/.//" to be a valid directory path. | ||
| args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.//"); | ||
| args.ClearPathCache(); | ||
| BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase()); | ||
| #endif |
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (41d7166)
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:
args.ForceSetArg("-datadir", fs::PathToString(dd_norm) + "/.//");
args.ClearPathCache();
#ifndef WIN32
BOOST_CHECK_EQUAL(dd_norm, args.GetDataDirBase());
#else
// Windows does not consider "datadir/.//" to be a valid directory path, so
// the check to see if the directory exists returns false on windows, and
// GetDataDirBase() returns nothing. When this test was originally written
// GetDataDirBase() function stripped trailing "/" slashes, so the test was
// the same on all platforms. But as of #20744, trailing slashes are no
// longer stripped (because it would have complicated the GetDataDirBase
// implementation), so the test is now platform-specific.
BOOST_CHECK(args.GetDataDirBase().empty());
#endifI 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 (review))
There was a problem hiding this comment.
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
There was a problem hiding this comment.
re: #20744 (comment)
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.
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (41d7166)
I think this change might be able to be reverted actually, because since this test was disabled a StripRedundantLastElementsOfPath call was added to GetDataDir. It looks like the call was added in the Dec 23 push #20744 (comment) https://github.com/bitcoin/bitcoin/compare/f53be20eade794e55470473f03a8d05d57691c87..2c990b05fae7e1bfef2d3f59044c24274812c559#diff-19427b0dd1a791adc728c82e88f267751ba4f1c751e19262cac03cccd2822216R438 without an explanation
| { | ||
| auto result = path; | ||
| while (fs::PathToString(result.filename()) == ".") { | ||
| while (result.filename().empty() || fs::PathToString(result.filename()) == ".") { |
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (41d7166)
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.
This change was originally suggested #20744 (comment), and a test program for this behavior is https://godbolt.org/z/voadYc8M1)
There was a problem hiding this comment.
(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.)
There was a problem hiding this comment.
(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.
|
Compiled and run on Windows 11:
Seems to work fine 🎉 |
| path /= fs::PathFromString(BaseParams().DataDir()); | ||
| path /= "blocks"; | ||
| fs::create_directories(path); | ||
| path = StripRedundantLastElementsOfPath(path); |
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (41d7166)
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 (comment)
|
@hebasto I tried to compile on Ubuntu 18.04.6 LTS with your instructions and I can see the same result. |
|
I think this is ready for merge. Any developers (hopefully none) on Ubuntu Bionic can:
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. |
Guix builds: |
|
ACK 0726932 Agree with @MarcoFalke's comment. |
ryanofsky
left a comment
There was a problem hiding this comment.
Partial code review ACK for commit "refactor: replace boost::filesystem with std::filesystem" (41d7166). 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.
|
|
||
| try { | ||
| if (fs::equivalent(pathSrc, pathDest)) { | ||
| if (fs::exists(pathDest) && fs::equivalent(pathSrc, pathDest)) { |
There was a problem hiding this comment.
In commit "build: remove boost::filesystem usage" (b87f9c5)
Note: this change was needed because fs::equivalent was throwing an exception if the path didn't exist. Earlier discussion was #20744 (comment)
| std::error_code error; | ||
| // The canonical path cleans the path, preventing >1 Berkeley environment instances for the same directory | ||
| fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error).remove_trailing_separator(); | ||
| fs::path canonical_wallet_dir = fs::canonical(wallet_dir, error); |
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (41d7166)
Note: std canonical function already strips trailing slashes, so this should not be a change in behavior
| } | ||
|
|
||
| #ifndef WIN32 | ||
| // Windows does not consider "datadir/wallets//" to be a valid directory path. |
There was a problem hiding this comment.
In commit "refactor: replace boost::filesystem with std::filesystem" (41d7166)
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.
There was a problem hiding this comment.
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.
|
This broke OSS-Fuzz: https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=44466#c1 |
This PR replaces our Boost Filesystem usage with
std::filesystemand 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
-latomicto facilitate linking with-lstdc++fswhen 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.Guix build: