fs: Make compatible with boost 1.78 #24104

pull achow101 wants to merge 1 commits into bitcoin:master from achow101:fix-fs-path-plus changing 3 files +3 −3

achow101 commented at 8:45 PM on January 19, 2022: member

Boost 1.78 removed operator+ in a way that breaks our usage of it in a subclass. A proposed workaround for this is to cast the argument to boost::filesystem::path, and this is backwards compatible with older versions of boost.

Additionally, it appears that fs::canonical no longer removes trailing slashes. This was causing a test to fail. The solution is to explicitly remove the trailing separator in the one place that fs::canonical is used.

Lastly, fs::create_directories now has an error message saying create_directories instead of create_directory. This caused wallet_multiwallet.py to fail. The error message check has been updated to be able accept either string.

Fixes #23846
achow101 cross-referenced this on Jan 19, 2022 from issue No match for fs::path operator+= when compiling with Boost 1.78 by achow101
in src/fs.h:91 in 575ecd0ff5 outdated
```
  87 | @@ -88,7 +88,7 @@ static inline auto quoted(const std::string& s)
  88 |  // Allow safe path append operations.
  89 |  static inline path operator+(path p1, path p2)
  90 |  {
  91 | -    p1 += std::move(p2);
  92 | +    p1 += std::move(static_cast<boost::filesystem::path>(p2));
```
ryanofsky commented at 9:15 PM on January 19, 2022:

In commit "fs: Make compatible with boost 1.78" (575ecd0ff5025bdd2f5cb5e52049aaa202245949)

The static_cast creates an unnecessary temporary here and makes the std::move not have any effect because its argument is already temporary. The right way to write this would be p1 += static_cast<boost::filesystem::path&&>(p2);

You could also consider using the alternate fix from #23846 (comment). I think that fix is a little better since it gets rid of the nonstandard + operator in cases where we don't need it and also avoids more temporaries.

achow101 commented at 9:27 PM on January 19, 2022:

Changed to the correct cast.
ryanofsky approved
ryanofsky commented at 9:20 PM on January 19, 2022: contributor

Code review ACK 575ecd0ff5025bdd2f5cb5e52049aaa202245949. This looks safe but the code is a little nonsensical, because the static_cast creates an unnecessary temporary, and std::move has no effect moving from a temporary that would already be moved from. I suggested two alternatives if you want to update.
fs: Make compatible with boost 1.78 dc5d6b0d47
achow101 force-pushed on Jan 19, 2022
ryanofsky approved
ryanofsky commented at 9:29 PM on January 19, 2022: contributor

Code review ACK dc5d6b0d4793ca978f71f69ef7d6b818794676c2 🪄
vincenzopalazzo approved
vincenzopalazzo commented at 9:59 PM on January 19, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/24104/commits/dc5d6b0d4793ca978f71f69ef7d6b818794676c2
DrahtBot added the label Wallet on Jan 19, 2022
achow101 added the label Utils/log/libs on Jan 20, 2022
fanquake commented at 5:05 AM on January 20, 2022: member

Given that #23846 is becoming more of an issue for devs, and #20744 is still stuck while we make some final changes and fix Guix, I'm going to go-ahead and merge this.
fanquake merged this on Jan 20, 2022
fanquake closed this on Jan 20, 2022
Fabcien referenced this in commit b9928812c9 on Jan 20, 2022
sidhujag referenced this in commit cd1c711b18 on Jan 20, 2022
fanquake referenced this in commit 021c3d892f on Mar 5, 2022
fanquake cross-referenced this on Mar 5, 2022 from issue [22.x] fs: Make compatible with boost 1.78 by fanquake
fanquake referenced this in commit cb13ba6d11 on Mar 7, 2022
reddink cross-referenced this on May 26, 2022 from issue [4.22.x] Backports for 4.22.x by reddink
Fuzzbawls cross-referenced this on Jul 30, 2022 from issue [GA] Run unit/functional tests on native macOS 11 by Fuzzbawls
apoelstra referenced this in commit c08430ab7c on Aug 14, 2022
psgreco referenced this in commit 5629cae32b on Sep 1, 2022
psgreco cross-referenced this on Sep 1, 2022 from issue [backport] Backport fixes from master to elements-22.x for rc5 by psgreco
apoelstra referenced this in commit 433a57a5bd on Sep 6, 2022
apoelstra referenced this in commit f4254f34fb on Sep 20, 2022
Fuzzbawls referenced this in commit 291ef05f53 on Sep 21, 2022
achow101 referenced this in commit a53d6c7a47 on Oct 18, 2022
bitcoin locked this on Jan 20, 2023

Contributors

Labels

Wallet Utils/log/libs

Linked (view graph)

#20744 Use std::filesystem. Remove Boost Filesystem & System #23846 No match for fs::path operator+= when compiling with Boost 1.78 #24476 [22.x] fs: Make compatible with boost 1.78