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

1. 
   kiminuo commented at 11:39 am on June 11, 2020: contributor

   Introduction

   C++17 has introduced the filesystem library. cppreference.com describes very well the origin of the library:

   The filesystem library was originally developed as boost.filesystem, was published as the technical specification ISO/IEC TS 18822:2015, and finally merged to ISO C++ as of C++17. The boost implementation is currently available on more compilers and platforms than the C++17 library.

   This draft PR was created to gain feedback and examine what would be necessary to switch from boost::filesystem to std::filesystem in bitcoin codebase.

   Bitcoin codebase contains src/fs.h which is a wrapper for the currently used filesystem library. The first impression is that one may just replace namespace fs = boost::filesystem; with namespace fs = std::filesystem; and all will be good. Unfortunately, there are some differences between the Boost’s filesystem library and the c++17 filesystem library. A nice summary by Davis Herring can be read here.

   Differences between filesystem implementations that affects Bitcoin code

   1. boost::filesystem::system_complete() was renamed to std::filesystem::absolute() [source]

   2. boost::filesystem::absolute() has second const path& base parameter which was dropped in std::filesystem::absolute (see reasoning).

      Each such instance like this one can be fixed by changing:

      fs::path path = fs::absolute(request.params[0].get_str(), GetDataDir());

      to

      fs::path path = fs::absolute(GetDataDir() / request.params[0].get_str()); where / is function call of operator/ function.

      Note: Notably path("foo") / "c:/bar" yields c:/bar. However, that’s how boost::filesystem::absolute(p, base_dir) behaves too.

   3. boost::filesystem::path::imbue() is not present in std::filesystem::path.

   4. boost::filesystem::unique_path() is not present in std::filesystem.

   5. boost::filesystem::equivalent(path1, path2) differs from std::filesystem::equivalent(path1, path2) as the later reports not supported error if one of the files does not exist whereas Boost returns false.

      • See also cppreference.

   Support for C++17 <filesystem>

   • Clang 7+ (see [Phoronix][git commit])
   • GCC 8.1+ ([GCC status page] [git commit])
     • GCC 8.x requires linking with -lstdc++fs.

   Issues & PRs that may be linked with this PR

   #13103, #13787, https://github.com/google/leveldb/pull/760, #6093

   TODO

   • Remove Boost’s filesystem from https://github.com/bitcoin/bitcoin/blob/master/configure.ac (help is welcome)
     • Relevant PR: #7302 & https://github.com/bitcoin/bitcoin/commit/76ac35f36d87078da62f95b4a1167ec296e37363
     • Done in #19667.
   • Am I missing -lstdc++fs?
   • What about #19933?
   • Address this change #19960
   • unicodepath test in src/test/dbwrapper_tests.cpp seems to require some UTF-8 locale.
     • See also https://github.com/bitcoin/bitcoin/blob/ae9ee5bdb18c5babbfa2875ab694df118d6a1c1b/src/util/system.cpp#L1249
     • https://github.com/microsoft/STL/blob/2aa944beba9af1d72537fd5e35d402c8b830a5dc/stl/src/filesystem.cpp#L266 - C++17 in Microsoft C++ standard library
     • https://docs.microsoft.com/en-us/cpp/standard-library/filesystem?view=msvc-160
     • https://docs.microsoft.com/en-us/cpp/standard-library/file-system-navigation?view=msvc-160

   Inspiration

   • https://github.com/ceptontech/filesystem
   • https://github.com/ceztko/u8filesystem
2. fanquake added the label Refactoring on Jun 11, 2020
3. 
   practicalswift commented at 12:54 pm on June 11, 2020: contributor

   Concept ACK: this will be nice to have in the future when we switch to C++17. Thanks for experimenting!
4. 
   MarcoFalke commented at 3:26 pm on June 11, 2020: member

   Would it make sense to split out the two absolute() commits, because they make sense even with C++11?
5. kiminuo force-pushed on Jun 11, 2020
6. 
   kiminuo commented at 8:05 pm on June 11, 2020: contributor

   Would it make sense to split out the two absolute() commits, because they make sense even with C++11?

   I think so. Yes.

7. 
   laanwj commented at 10:50 am on June 12, 2020: member

   Nice work.
8. kiminuo marked this as ready for review on Jun 16, 2020
9. kiminuo force-pushed on Jun 16, 2020
10. 
    DrahtBot commented at 10:48 pm on June 16, 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:

    • #20586 (Fix Windows build with –enable-werror by hebasto)
    • #20487 (draft: Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift)
    • #20017 (rpc: Add RPCContext by promag)
    • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
    • #18077 (net: Add NAT-PMP port forwarding support by hebasto)
    • #17127 (util: Correct permissions for datadir and wallets subdir by hebasto)
    • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

    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.

11. DrahtBot added the label Needs rebase on Jun 17, 2020
12. kiminuo force-pushed on Jun 20, 2020
13. DrahtBot removed the label Needs rebase on Jun 20, 2020
14. DrahtBot added the label Needs rebase on Jun 21, 2020
15. laanwj referenced this in commit e3fa3c7d67 on Jun 22, 2020
16. kiminuo force-pushed on Jul 5, 2020
17. DrahtBot removed the label Needs rebase on Jul 5, 2020
18. in src/fs.cpp:40 in 8566218bd4 outdated

    31@@ -31,6 +32,14 @@ FILE *fopen(const fs::path& p, const char *mode)
    32 #endif
    33 }
    34 
    35+fs::path unique_path() {
    36+    // TODO: Do not use Boost in this implementation
    37+    boost::filesystem::path p = boost::filesystem::unique_path();
    38+    fs::path p2{p.string()};
    39+
    40+    return p2;
    

    ---

    ---

    

    MarcoFalke commented at 1:18 pm on July 8, 2020:

    I think all that boost does it pick 64 bits of randomness? Something along the lines of

    0    return g_insecure_rand_ctx_temp_path.rand256().ToString().substr(0, 16);
    

    should be identical.

    ---

    

    kiminuo commented at 9:40 pm on July 12, 2020:

    Yes. I believe so too.

    However, using this leads to a new circular dependency “fs -> random -> logging -> fs”: https://travis-ci.com/github/kiminuo/bitcoin/jobs/360282950#L466 (https://github.com/kiminuo/bitcoin/commit/333ab6ad421f1b25bf66bd2557cd3d0e4b60e0f6)

    unique_path() is used only twice but I don’t know what is preferred solution here. Inline it? Move function somewhere else? Where?

    ---

    For other people who may read this: For used Boost 1.70.0:

    • Source code: https://github.com/boostorg/filesystem/blob/e49775e8bbc4d8dbea5fbf6cf7c29f4d7bbd0c67/src/unique_path.cpp#L131
    • Docs: https://www.boost.org/doc/libs/1_70_0/libs/filesystem/doc/reference.html#unique_path

    ---

    

    MarcoFalke commented at 5:01 am on July 13, 2020:

    I think there are two options:

    • Inline
    • or create a new header, e.g. src/util/fs.h
19. kiminuo referenced this in commit 333ab6ad42 on Jul 12, 2020
20. 
    elichai commented at 9:02 am on July 26, 2020: contributor

    Nice! Please ping me when it’s ready for review :)
21. kiminuo referenced this in commit b9592a96fc on Jul 30, 2020
22. kiminuo referenced this in commit 182d5a88ef on Jul 30, 2020
23. kiminuo referenced this in commit afb0d6988a on Jul 30, 2020
24. kiminuo referenced this in commit 7bc3b41a2a on Jul 30, 2020
25. kiminuo force-pushed on Jul 30, 2020
26. MarcoFalke added this to the milestone 0.22.0 on Aug 2, 2020
27. DrahtBot added the label Needs rebase on Aug 3, 2020
28. kiminuo referenced this in commit 141b1c0646 on Aug 7, 2020
29. kiminuo force-pushed on Aug 7, 2020
30. DrahtBot removed the label Needs rebase on Aug 7, 2020
31. DrahtBot added the label Needs rebase on Aug 31, 2020
32. kiminuo force-pushed on Sep 2, 2020
33. kiminuo referenced this in commit 84adbdf04d on Sep 2, 2020
34. DrahtBot removed the label Needs rebase on Sep 2, 2020
35. DrahtBot added the label Needs rebase on Sep 7, 2020
36. 
    elichai commented at 10:35 am on November 19, 2020: contributor

    @kiminuo Now that #20413 is merged, are you planning on rebasing this and getting it ready for review?
37. 
    kiminuo commented at 10:52 am on November 19, 2020: contributor

    @kiminuo Now that #20413 is merged, are you planning on rebasing this and getting it ready for review?

    I will get to it (hopefully soon ™). However, @MarcoFalke mentioned on IRC briefly that std::fs is not available on some supported OSs even with C++17 enabled. I’m not sure I understood correctly. If true, it would delay this feature months or years.

    If you want to work on this. No problem. I’m quite busy.

38. kiminuo referenced this in commit fd46064ff6 on Nov 19, 2020
39. kiminuo referenced this in commit 2b5b4a65bc on Nov 20, 2020
40. kiminuo force-pushed on Nov 20, 2020
41. DrahtBot removed the label Needs rebase on Nov 20, 2020
42. kiminuo referenced this in commit 6c01a2c8e0 on Nov 24, 2020
43. kiminuo force-pushed on Nov 24, 2020
44. DrahtBot added the label Needs rebase on Nov 24, 2020
45. kiminuo force-pushed on Nov 26, 2020
46. kiminuo referenced this in commit 9d6d916f0f on Nov 26, 2020
47. kiminuo force-pushed on Nov 26, 2020
48. DrahtBot removed the label Needs rebase on Nov 26, 2020
49. DrahtBot added the label Needs rebase on Nov 30, 2020
50. kiminuo referenced this in commit 4ba6d440eb on Nov 30, 2020
51. kiminuo force-pushed on Nov 30, 2020
52. DrahtBot removed the label Needs rebase on Nov 30, 2020
53. kiminuo referenced this in commit 7ce843cb4b on Dec 1, 2020
54. kiminuo force-pushed on Dec 1, 2020
55. kiminuo referenced this in commit 58a9971b35 on Dec 3, 2020
56. kiminuo force-pushed on Dec 3, 2020
57. kiminuo referenced this in commit 087ccc5863 on Dec 3, 2020
58. kiminuo force-pushed on Dec 3, 2020
59. DrahtBot added the label Needs rebase on Dec 12, 2020
60. MarcoFalke added the label Waiting for other on Dec 15, 2020
61. MarcoFalke removed this from the milestone 22.0 on Dec 15, 2020
62. kiminuo referenced this in commit fda8dc9939 on Dec 15, 2020
63. kiminuo force-pushed on Dec 15, 2020
64. DrahtBot removed the label Needs rebase on Dec 15, 2020
65. Modify fs.h to switch from boost::filesystem to std::filesystem

    (cherry picked from commit b526d92bf6b3deecb8bb2a9f6fcc50476a28ea80)
    (cherry picked from commit 29cd1a22327469dcd625d24c9a317be23144887f)

    00dfc62408
66. Replace `fs::system_complete(path)` with `fs::absolute(path)`

    Resources:
    
    * https://stackoverflow.com/a/46271698
    * https://www.bfilipek.com/2019/05/boost-to-stdfs.html#differences-found-systemcomplete
    
    (cherry picked from commit 88bb4e1bdfc43a449ea9d62d9336de9e162babfd)
    (cherry picked from commit fc0d5a4c1c7153167afbca028d543ea65701658c)

    6ee22b607f
67. Replace boost::filesystem::absolute(path p, path base) with `std::filesystem::absolute(base_dir / p)`

    Resources:
    
    * http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2017/p0492r2.html#US78
    * https://stackoverflow.com/questions/40899267/how-similar-are-boost-filesystem-and-the-c-standard-filesystem-library
    
    (cherry picked from commit 65970378ab8a4b3e9b906d455afc32422635dab2)
    (cherry picked from commit 465645d3a3d87c19d9c0f6b5b8c7795fd99ece6e)
    
    # Conflicts:
    #	src/rpc/blockchain.cpp
    #	src/wallet/walletutil.cpp

    9ed2009a42
68. Remove `fs::path::imbue()` call as it no longer exist in `std::filesystem`

    (cherry picked from commit eb89f20e1a45f1c58ec7396b376b65c7ae50bf28)
    (cherry picked from commit b24d14961cd93a3229e4983ee31f56277b612ed5)

    e929c64bd4
69. Replace `fs::unique_path()` with `fsbridge::unique_path()` for now to re-implement it later

    Resources:
    
    * https://stackoverflow.com/questions/43316527/what-is-the-c17-equivalent-to-boostfilesystemunique-path
    
    (cherry picked from commit fdeb20204a23b71a268c88b9d52d8678ea5b140d)
    (cherry picked from commit 1a09157f3ca66f2931a55cf94a25cfa9944079a4)

    92f4300362
70. Remove `BOOST_FILESYSTEM_C_STR` as we want to use the overload with `fs::path`

    See (1) to understand what the macro `BOOST_FILESYSTEM_C_STR` does.
    
    Resources:
    
    (1) https://www.boost.org/doc/libs/1_70_0/boost/filesystem/fstream.hpp
    (2) https://stackoverflow.com/questions/821873/how-to-open-an-stdfstream-ofstream-or-ifstream-with-a-unicode-filename/#54842261
    
    (cherry picked from commit 7543252fe53f992f93d6af802a8f5f30d40cce15)
    (cherry picked from commit a02bba11c22108ecc4a88dbc106c2b7693b4838a)

    38d8432c1b
71. Remove workaround for opening UTF-8 paths on Windows

    (cherry picked from commit 6df4d92034b4302870162e108f48cefd4e21bd1a)
    (cherry picked from commit f71054b2a407a089e1915b992a536c8731356922)

    4aa0452ceb
72. Remove `boost/filesystem/fstream.hpp` from `EXPECTED_BOOST_INCLUDES` in `lint-includes.sh`

    (cherry picked from commit 3358071a54c9d803f91b4daca00cf963b4390863)
    (cherry picked from commit 8d1464d4513bf705a7ae904c96a768c9b08ce027)

    536ca52047
73. Replace `boost::system::error_code` with `std::error_code`

    (cherry picked from commit 6d375ab584355a78ff2f2e347063b990b1573ee5)
    (cherry picked from commit 593e7df400e0d71e1e355668ee747491fb60749b)
    
    # Conflicts:
    #	src/wallet/walletutil.cpp

    8038c17224
74. Fix db test "getwalletenv_file"

    (cherry picked from commit 2e19c4cbc29e9778c8d037961f156a5f1b7280d9)
    (cherry picked from commit 1043687a555101c0b3550dde48bfd29c8e82c28f)

    926a704f3d
75. Fix init_tests

    (cherry picked from commit db44346266d71b716f136fe6d8febf819168ce27)
    (cherry picked from commit 62e7fb353efbbc125b1a96ef78e0331149b144ca)

    ab400705d0
76. User u8path rather -- TODO: Verify!!

    (cherry picked from commit 018574609b16ae45f2448f9437267ac68c1e70d7)
    (cherry picked from commit 2d8823f1bdb530088e44c2cdca040ad146ce0752)

    857f04e8e6
77. db.cpp - change fs::copy_option::ovewrite_if_exists -> fs::copy_options::ovewraite_existing

    (cherry picked from commit 5af0554ce526374af076e266ff46e29dff3bd5cc)

    fc5329b591
78. Do not use Boost implementation of `fs::path::unique_path`.

    Suggestion here: https://github.com/bitcoin/bitcoin/pull/19245#discussion_r451537286

    c7abfc150d
79. Remove AX_BOOST_FILESYSTEM test. f274f092fb
80. Remove suppressions for boost::filesystem from valgrind.supp file. d780856702
81. depends: Remove Boost filesystem from config libraries. 0cf54c500f
82. Do not require to install `libboost-filesystem-dev` in tests and in documentation.

    # Conflicts:
    #	build_msvc/README.md
    #	build_msvc/vcpkg-packages.txt
    #	ci/test/00_setup_env_native_asan.sh
    #	ci/test/00_setup_env_native_valgrind.sh

    0915eac95e
83. Modify fs::absolute call in ArgsManager::GetSettingsPath. 2f52c5679b
84. To split into multiple commits 0349794528
85. Fix tests. 834afbb3fc
86. Fix util_tests. 5ee8d971b0
87. why? 4187d5ae12
88. Test. 9dd1c910d3
89. Test. 7b071c5491
90. workaround: dbwrapper_tests 13f1cb6db0
91. Test with 20.04 b96e01722d
92. kiminuo force-pushed on Dec 16, 2020
93. Test: Use Clang 10. cf948ffac6
94. sipa added the label Up for grabs on Dec 16, 2020
95. 
    MarcoFalke commented at 6:34 am on December 17, 2020: member

    Hi, catching up on the IRC log.

    I just wanted to say that this is blocked on #20460 and there is nothing you can do about this in this pull. Fixing #20460 will need build system and gitian/guix, as well as CI changes. This would be too much to do here.

96. MarcoFalke removed the label Up for grabs on Dec 17, 2020
97. fanquake renamed this:
    [WIP DONOTMERGE] Replace boost::filesystem with std::filesystem (in c++17)
    Replace boost::filesystem with std::filesystem
    on Dec 17, 2020
98. fanquake marked this as a draft on Dec 17, 2020
99. 
    fanquake commented at 2:30 pm on December 22, 2020: member

    Thanks for your work here so far @kiminuo. I’ve addressed the build system requirements, and combined this changes into #20744 (still attributed to you). However due to runtime and compiler requirements it’s unlikely we’ll be able to start using them just yet. I’m going to close this PR in favour of #20744 for now.
100. fanquake closed this on Dec 22, 2020

     ---

101. MarcoFalke referenced this in commit 3ace3a17c9 on Feb 3, 2022
102. DrahtBot locked this on Feb 15, 2022
103. fanquake removed the label Waiting for other on Jan 12, 2023

Contributors
kiminuo practicalswift MarcoFalke laanwj DrahtBot elichai fanquake

Labels
Refactoring