Use operator/ in fs::absolute to prepare for C++17 #19466

pull kiminuo wants to merge 4 commits into bitcoin:master from kiminuo:feature/2020-07-08-fs-paths changing 6 files +15 −13

kiminuo commented at 10:52 AM on July 8, 2020: contributor

This PR contains only two commits cherry-picked from #19245 as suggested here. Motivations for the changes are in the commit messages.

Cheers!

Replace `filesystem::absolute(path p, path base)` with `filesystem::absolute(base_dir / p)`.

* This does not change behavior.
* The idea is to replace `boost::filesystem::absolute(base_dir / p)` with `std::filesystem::absolute(base_dir / p)` later on when the migration to C++17 will start.

ce65b5039a

fanquake added the label Refactoring on Jul 8, 2020
DrahtBot commented at 11:26 AM on July 8, 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:
- #19245 ([WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17) by kiminuo)
- #19213 (refactor: Replace RecursiveMutex with Mutex in Get{Data,Blocks}Dir() by hebasto)
- #18689 (rpc: allow dumptxoutset to dump human-readable data by pierreN)
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.
hebasto commented at 11:31 AM on July 8, 2020: member

Concept ACK.
MarcoFalke commented at 1:20 PM on July 8, 2020: member

In the second commit: Why not change those as well: https://github.com/bitcoin/bitcoin/pull/19245/commits/6cf2c8eae6b8c2ea503c9b03cdc5a90ad31d14bd#diff-431ad0fd7e189837a5fcebddb320315e ?

Also, could add commit 6f55de61d8249658de7ee8563fdefc2fafd0d932 or would that be a behaviour change on non-posix machines?
walletutil: Handle empty name. 7bf0bc7e17
Use BOOST_CHECK_EQUAL for paths to get useful debug information when test fails. b100b57d28
system.cpp: Check whethere datadir is not empty. b087859dc3
kiminuo force-pushed on Jul 8, 2020
kiminuo commented at 1:59 PM on July 8, 2020: contributor

In the second commit: Why not change those as well: 6cf2c8e#diff-431ad0fd7e189837a5fcebddb320315e ?

I have added that.

Also, could add commit 6f55de6 or would that be a behaviour change on non-posix machines?

I don't know whether it can break something. When switching system_complete() to absolute() in C++17, it is OK according to https://stackoverflow.com/a/46271698/13716157 so when we wait, it won't break. That's the reason I haven't added the commit.
MarcoFalke commented at 3:17 PM on July 8, 2020: member

According to https://github.com/boostorg/filesystem/commit/7e300b986bb978db54305effceacd94cfc42e6cf it is identical to absolute (except for the kinky Windows behaviour, which I hope we don't rely on)
kiminuo referenced this in commit 92ec7282ce on Jul 8, 2020
kiminuo commented at 4:19 PM on July 8, 2020: contributor
According to boostorg/filesystem@7e300b9 it is identical to absolute (except for the kinky Windows behaviour, which I hope we don't rely on)

Ok, will add that. @MarcoFalke wallet_multiwallet.py test fails on line 101: https://github.com/bitcoin/bitcoin/blob/abdfd2d0e3ebec7dbead89317ee9192189a35809/test/functional/wallet_multiwallet.py#L101:
```
 assert_equal(sorted(map(lambda w: w['name'], self.nodes[0].listwalletdir()['wallets'])), ['', os.path.join('sub', 'w5'), 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])
```
And I wonder why the path corresponding to w6 is missing on the right? What is the reason for that?

Locally I can see in my test output:
```
AssertionError: not(['', 'sub/w5', 'tmp/test_runner_₿_🏃_20200708_160230/wallet_multiwallet_0/extern/w6', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'] == ['', 'sub/w5', 'w', 'w1', 'w2', 'w3', 'w7', 'w7_symlink', 'w8'])
```
so that the left side of the assert contains that w6 path but the right side does not. Right side is the expected result, right?

btw: util.py / def assert_equal(thing1, thing2, *args): can be confusing because it does not state whether thing1 is expected result or actual result. Most unit testing frameworks use that notation.
MarcoFalke commented at 8:07 PM on July 8, 2020: member

w6 is extern, so it should not show up in the wallet dir
in src/rpc/blockchain.cpp:2291 in b087859dc3
```
2287 | @@ -2288,10 +2288,10 @@ UniValue dumptxoutset(const JSONRPCRequest& request)
2288 |          }
2289 |      }.Check(request);
2290 |  
2291 | -    fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir());
2292 | +    fs::path path = fs::absolute(GetDataDir() / request.params[0].get_str());
```
MarcoFalke commented at 8:33 PM on July 8, 2020:

Ah my bad. This is a change in behavior.

Joining "/tmp/" and "/foo" will give:

/foo with the code in current master (correct)

/foo with std lib absolute (your version in #19245)

/tmp//sadf with the current version in this pull
kiminuo commented at 6:56 AM on July 9, 2020:

Yeah.

Even this https://github.com/bitcoin/bitcoin/pull/19466/files#diff-e802a36c28b0140bab62cb5366199656R97 breaks multiwallet test.

So the PR without those changes is essentially empty and it looks to me I can close it.

MarcoFalke commented at 8:55 AM on July 9, 2020:

So the PR without those changes is essentially empty and it looks to me I can close it.

Jup, was my fault for suggesting to split them up
MarcoFalke changes_requested
kiminuo referenced this in commit 59789aa07e on Jul 9, 2020
kiminuo closed this on Jul 9, 2020
kiminuo deleted the branch on Jul 30, 2020
DrahtBot locked this on Feb 15, 2022

Contributors

Labels

Linked (view graph)

#19245 Replace boost::filesystem with std::filesystem

Use operator/ in fs::absolute to prepare for C++17 #19466

Conflicts