This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
This PR doesn't change behavior aside from adding an assert and fixing a test bug.
20 | @@ -21,6 +21,15 @@ namespace fs = boost::filesystem; 21 | namespace fsbridge { 22 | FILE *fopen(const fs::path& p, const char *mode); 23 | 24 | + /** 25 | + * Helper function asserting that \p base is an absolute path and returning the result of boost::filesystem::absolute(p, base) then. 26 | + * 27 | + * @param[in] base Absolute path. 28 | + * @param[in] p Path to combine with \p base. 29 | + * @return \p p if \p p is an absolute path, othwerwise \p base joined with \p p is returned.
* [@return](/bitcoin-bitcoin/contributor/return/) \p p if \p p is an absolute path, otherwise \p base joined with \p p is returned.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
Reviewers, 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.
26 | + * 27 | + * @param[in] base Absolute path. 28 | + * @param[in] p Path to combine with \p base. 29 | + * @return \p p if \p p is an absolute path, otherwise \p base joined with \p p is returned. 30 | + */ 31 | + fs::path AbsPathJoin(const fs::path& base, const fs::path& p);
In commit "Introduce fsbridge::AbsPathJoin(base, p)." (c0da7456833fac6d5613d97e09266cf54f13594f)
I think it would be good to add "refactor:" to the PR title to be clear that there is no intended behavior change here. New code is exactly equivalent to previous code, it is just going through a helper function and adding an assert.
naming nit, suggest s/p/path/
and the following Doxygen documentation seems more clear to me
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] base Absolute path.
- * [@param](/bitcoin-bitcoin/contributor/param/)[in] p Path to combine with \p base.
- * [@return](/bitcoin-bitcoin/contributor/return/) \p p if \p p is an absolute path, otherwise \p base joined with \p p is returned.
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] base Absolute path
+ * [@param](/bitcoin-bitcoin/contributor/param/)[in] path Path to combine with base
+ * [@returns](/bitcoin-bitcoin/contributor/returns/) path if it is an absolute path, otherwise base joined with path
Yes, thanks.
20 | @@ -21,6 +21,15 @@ namespace fs = boost::filesystem; 21 | namespace fsbridge { 22 | FILE *fopen(const fs::path& p, const char *mode); 23 | 24 | + /** 25 | + * Helper function asserting that \p base is an absolute path and returning the result of boost::filesystem::absolute(p, base) then.
In commit "Introduce fsbridge::AbsPathJoin(base, p)." (c0da7456833fac6d5613d97e09266cf54f13594f)
Would be good to drop "and returning the result of boost::filesystem...". Return value is already described below and the point of this function is to avoid the need for people to understand the extremely complicated boost function, which doesn't make any assumptions, by replacing with it a more straightforward join assuming at least one absolute path.
3779 | @@ -3780,7 +3780,7 @@ 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 = fsbridge::AbsPathJoin(GetWalletDir(), name);
const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), name);
I understand that one is supposed to apply the commit locally without using Github's "Commit suggestion" as that would lead to a new CI run, right?
@kiminuo Yes, I don't recall seeing someone use the GitHub web "commit suggestion" interface here other than for just giving feedback, but you may be right. It's probably best (and more secure) to use git locally and not become dependent on GitHub-specific workflows.
104 | @@ -105,7 +105,7 @@ static void WalletShowInfo(CWallet* wallet_instance) 105 | 106 | bool ExecuteWalletToolFunc(const ArgsManager& args, const std::string& command, const std::string& name) 107 | { 108 | - fs::path path = fs::absolute(name, GetWalletDir()); 109 | + fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name);
const fs::path path = fsbridge::AbsPathJoin(GetWalletDir(), name);
2398 | @@ -2399,10 +2399,10 @@ static RPCHelpMan dumptxoutset() 2399 | }, 2400 | [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue 2401 | { 2402 | - fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir()); 2403 | + fs::path path = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str());
const fs::path path = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str());
Thanks
2398 | @@ -2399,10 +2399,10 @@ static RPCHelpMan dumptxoutset() 2399 | }, 2400 | [&](const RPCHelpMan& self, const JSONRPCRequest& request) -> UniValue 2401 | { 2402 | - fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir()); 2403 | + fs::path path = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str()); 2404 | // Write to a temporary path and then move into `path` on completion 2405 | // to avoid confusion due to an interruption. 2406 | - fs::path temppath = fs::absolute(request.params[0].get_str() + ".incomplete", GetDataDir()); 2407 | + fs::path temppath = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str() + ".incomplete");
const fs::path temppath = fsbridge::AbsPathJoin(GetDataDir(), request.params[0].get_str() + ".incomplete");
Thanks
ACK c0da745683
The PR title should probably be prefixed with "refactor:" -> Edit: I see you just did it :)
A few suggestions below, happy to re-review if you take them.
51 | @@ -52,6 +52,18 @@ BOOST_AUTO_TEST_CASE(fsbridge_fstream) 52 | file >> input_buffer; 53 | BOOST_CHECK_EQUAL(input_buffer, "bitcoin"); 54 | } 55 | + { 56 | + // Join an absolute path and a relative path.
In commit "Introduce fsbridge::AbsPathJoin(base, p)." (c0da7456833fac6d5613d97e09266cf54f13594f)
It would be good to add another test joining an absolute path with the empty "" path and ensuring the absolute path is returned unchanged (no trailing slash). This is the difference between boost and std join functions and cause of bugs in #20744
Conditional code review ACK c0da7456833fac6d5613d97e09266cf54f13594f if appveyor CreateWallet test is fixed:
https://ci.appveyor.com/project/DrahtBot/bitcoin/builds/37264180#L1102
C:\projects\bitcoin\src\wallet\test\wallet_tests.cpp(696): Entering test case "CreateWallet"
Assertion failed: base.is_absolute(), file C:\projects\bitcoin\src\fs.cpp, line 36
Command exited with code -1073740791
I think it probably needs some extra test setup (maybe a call to GetDataDir()) to avoid the failure.
I'm able to reproduce appveyor failure on linux with
make -C src test/test_bitcoin && src/test/test_bitcoin -l test_suite -t init_tests -t wallet_tests/CreateWallet --random=2
To reproduce, it may be necessary to vary the random seed to get CreateWallet test to run after the init_tests as happens on appveyor.
The problem seems to be that init_tests force set a -walletdir that GetWalletDir rejects and turns into an empty path, which gets rejected by the assert because it is not absolute.
Probably a minimal fix is possible in init_tests. Backtrace at assert fail looks like
[#0](/bitcoin-bitcoin/0/) __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:51
[#1](/bitcoin-bitcoin/1/) 0x00007ffff59b38b1 in __GI_abort () at abort.c:79
[#2](/bitcoin-bitcoin/2/) 0x00007ffff59a342a in __assert_fail_base (fmt=0x7ffff5b2aa38 "%s%s%s:%u: %s%sAssertion `%s' failed.\n%n",
assertion=assertion@entry=0x5555566fc54f "base.is_absolute()", file=file@entry=0x5555566fc548 "fs.cpp", line=line@entry=36,
function=function@entry=0x5555566fc600 <fsbridge::AbsPathJoin(boost::filesystem::path const&, boost::filesystem::path const&)::__PRETTY_FUNCTION__> "boost::filesystem::path fsbridge::AbsPathJoin(const boost::filesystem::path&, const boost::filesystem::path&)") at assert.c:92
[#3](/bitcoin-bitcoin/3/) 0x00007ffff59a34a2 in __GI___assert_fail (assertion=0x5555566fc54f "base.is_absolute()", file=0x5555566fc548 "fs.cpp", line=36,
function=0x5555566fc600 <fsbridge::AbsPathJoin(boost::filesystem::path const&, boost::filesystem::path const&)::__PRETTY_FUNCTION__> "boost::filesystem::path fsbridge::AbsPathJoin(const boost::filesystem::path&, const boost::filesystem::path&)") at assert.c:101
[#4](/bitcoin-bitcoin/4/) 0x000055555620de2f in fsbridge::AbsPathJoin (base=..., p=...) at fs.cpp:36
[#5](/bitcoin-bitcoin/5/) 0x00005555563d5298 in MakeWalletDatabase (name="", options=..., status=@0x7fffffffb57c: 21845, error_string=...) at wallet/wallet.cpp:3783
[#6](/bitcoin-bitcoin/6/) 0x0000555555cd0d59 in wallet_tests::TestLoadWallet (chain=...) at wallet/test/wallet_tests.cpp:46
[#7](/bitcoin-bitcoin/7/) 0x0000555555ce03f2 in wallet_tests::CreateWallet::test_method (this=0x7fffffffc9c0) at wallet/test/wallet_tests.cpp:699
[#8](/bitcoin-bitcoin/8/) 0x0000555555cdf585 in wallet_tests::CreateWallet_invoker () at wallet/test/wallet_tests.cpp:696
Following fix seems to work:
--- a/src/wallet/test/init_test_fixture.cpp
+++ b/src/wallet/test/init_test_fixture.cpp
@@ -3,6 +3,7 @@
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <fs.h>
+#include <univalue.h>
#include <util/check.h>
#include <util/system.h>
@@ -37,6 +38,9 @@ InitWalletDirTestingSetup::InitWalletDirTestingSetup(const std::string& chainNam
InitWalletDirTestingSetup::~InitWalletDirTestingSetup()
{
+ gArgs.LockSettings([&](util::Settings& settings) {
+ settings.forced_settings.erase("walletdir");
+ });
fs::current_path(m_cwd);
}
Would suggest commit message like:
test: Clear forced -walletdir setting after wallet init_tests
Leaving this value set interfered with the CreateWallet test if it happened to execute later in the test ordering. Specifically it would cause CreateWallet test to write data to the current directory instead of temporary test directory.
re: #20932#issue-554809577
This PR is based on @ryanofsky suggestion here #20744 (review) and it should help make #20744 diff smaller in the long run.
Would suggest writing a self-contained PR description that doesn't require reading a comment in another issue to understand. Something like:
refactor: Replace fs::absolute calls with AbsPathJoin calls
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
This PR doesn't change behavior aside from adding an assert and fixing a test bug.
67 | + { 68 | + // Joining with empty path behaves differently in various filesystem implementations. 69 | + fs::path p = fsbridge::AbsPathJoin(tmpfolder, ""); 70 | + BOOST_CHECK(p.is_absolute()); 71 | + // "tmpfolder" does not contain trailing slash. No trailing slash should be added by the tested method. 72 | + BOOST_CHECK(p.string().back() != '/');
In commit "Replace fs::absolute calls with AbsPathJoin calls" (215c78465059175505218566f2ec38d52d3be827)
I don't think the comment and test are quite right. Joining behavior is well defined for boost and std path types and does not depend on the filesystem. The differences are between the boost library behavior and the standard library behavior are design choices, not OS differences. Would suggest replacing with:
{
// Ensure joining with empty paths does not add trailing path components.
BOOST_CHECK_EQUAL(tmpfile1, fsbridge::AbsPathJoin(tmpfile1, ""));
BOOST_CHECK_EQUAL(tmpfile1, fsbridge::AbsPathJoin(tmpfile1, {}));
}
This is the empty path behavior that we are relying on AbsPathJoin and boost::filesystem::absolute functions for that the C++17 standard library doesn't directly provide in https://en.cppreference.com/w/cpp/filesystem/path/append
Yes, it's bad wording on my side. Thanks for the suggestion.
Code review ACK 215c78465059175505218566f2ec38d52d3be827. Thanks for updates!
Concept ACK
Leaving this value set interfered with the CreateWallet test if it happened to execute later in the test ordering. Specifically it would cause CreateWallet test to write data to the current directory instead of temporary test directory.
20 | @@ -21,6 +21,15 @@ namespace fs = boost::filesystem; 21 | namespace fsbridge { 22 | FILE *fopen(const fs::path& p, const char *mode); 23 | 24 | + /** 25 | + * Helper function for joining paths 26 | + * 27 | + * @param[in] base Absolute path 28 | + * @param[in] path Path to combine with base 29 | + * @returns path if it is an absolute path, otherwise base joined with path
In commit "Replace fs::absolute calls with AbsPathJoin calls" (215c78465059175505218566f2ec38d52d3be827)
Maybe expand this line a little to say "@returns path unchanged if it is an absolute path, otherwise returns base joined with path. Returns base unchanged if path is empty."
If desired, could also add preconditions and postconditions (base path must be absolute, returned path will always be absolute).
The information about paths being unchanged could be useful to callers and would be hard to discover without digging into the implementation and library documentation. It also explains how this join is different than other possible joins.
20 | @@ -21,6 +21,15 @@ namespace fs = boost::filesystem; 21 | namespace fsbridge { 22 | FILE *fopen(const fs::path& p, const char *mode); 23 | 24 | + /** 25 | + * Helper function for joining paths 26 | + * 27 | + * @param[in] base Absolute path
* [@param](/bitcoin-bitcoin/contributor/param/)[in] base Base path (must be absolute)
better yet, see the Doxygen section in doc/developer-notes.md and add something like
* [@pre](/bitcoin-bitcoin/contributor/pre/) Base must be absolute
* [@post](/bitcoin-bitcoin/contributor/post/) Returned path will always be absolute
20 | @@ -21,6 +21,15 @@ namespace fs = boost::filesystem; 21 | namespace fsbridge { 22 | FILE *fopen(const fs::path& p, const char *mode); 23 | 24 | + /** 25 | + * Helper function for joining paths
* Helper function for joining two paths to return an absolute path
ACK d867d7a64c5c modulo two comments based on ryanofsky's excellent feedback in #20932 (review) "If desired, could also add preconditions and postconditions (base path must be absolute, returned path will always be absolute)."
This adds better test coverage and will make it easier in #20744 to remove our dependency on the two-argument boost::filesystem::absolute() function which does not have a direct equivalent in C++17.
Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916. Just comment and test tweaks since previous review.
Final change is very simple and easy to review, and I think makes code more understandable and improves tests. Thanks for implementing this!
Code review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 only doxygen improvements since my last review per git diff d867d7a da9caa1
Thanks for updating.
review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯
<details><summary>Show signature and timestamp</summary>
Signature:
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
review ACK da9caa1cedd69702aea44cb44b2fd0a2d6d56916 📯
-----BEGIN PGP SIGNATURE-----
iQGzBAEBCgAdFiEE+rVPoUahrI9sLGYTzit1aX5ppUgFAlwqrYAACgkQzit1aX5p
pUgRdQv8DzNpEk8/k2bFwErjQdb2bW93pR3nNfNIarBpkEE+QHpmP7vSlMXz+04q
JlOLmdAPEVv2jWA+d2lEdvoMapyCEBbUvii7jSgxBN/Qb6ONyXddjRTjDEVXmJZW
UCjgJ9hfo59NxuA49LcAVPMVatdkA7PtgKkYZ78sG14BlJ+15csFYA8R+FzMW+hh
kD/j8tYTQQQ1KN11Vmyw08rLcUU/XFavbCf/0++FlDGKbZOROYVtfhqxybyn4YzN
Ibt/AJq+dGIOBB3LY9cN0Muwb8sxVZQrM4GWxNUwHzkH1VHZL7vdVGGWJBx+rU2c
5mICi+9n60wmeRUahHH5hAT2+hr5XDgBo1i5mZUpZYmTZXp+kO2a83LI/oNGpkK5
MTWy4K+5koCzgLPQNDRGRiVM/aB81F0l4LsaY9/q5ANDQg5LP6YWwyHnuzbLujHs
pHTn2rUe+DnLmEM94a6p05Ht0NsfyWusw/NpTkAYl9ZkvNjqixRmIbkwIm7sYC65
UrqATWdd
=3DWR
-----END PGP SIGNATURE-----
Timestamp of file with hash 635e482e0a7be99ddac7857318934ace812fbbbfd5d8237d0445be8d5d77dff1 -
</details>