Lightweight abstraction of boost::filesystem #9902

pull laanwj wants to merge 6 commits into bitcoin:master from laanwj:2017_03_fs changing 34 files +233 −207
  1. laanwj commented at 8:40 am on March 2, 2017: member

    Wrap boost::filesystem as the fs namespace, contained in one header. Also add fsbridge for bridging between stdio.h fopen() and the path type.

    This allows:

    • Replacing it when the time comes with std::filesystem (when standardized, and when we start using the appropriate c++ version) with a single change.

    • Using custom filesystem implementations that offer the same interface. I need this in cloudabi, a sandboxed environment which has no concept of a global filesystem namespace, where paths need to be accompanied by a file descriptor which grants access to it.

    It’s less typing, too.

    See individual commits for more information.

  2. laanwj added the label Refactoring on Mar 2, 2017
  3. theuni commented at 8:05 pm on March 2, 2017: member

    Nice! Concept and utACK. Not tested much, but I don’t think behavior should differ at all.

    I wanted to see how compatible this would be to working with the std::filesystem spec, so I tried it out. It’s pretty close. The changes needed are here: https://github.com/theuni/bitcoin/commits/filesystem (I didn’t attempt to build qt, though). The one outlier is boost::filesystem::path::imbue(). I’m not sure what to do about that, so I left it alone.

    When we do our own implementation, it’d be really nice if we could be strictly compatible with std::filesystem, not because we’re necessarily moving to it any time soon, but because it gives us a strict api to follow.

  4. laanwj commented at 8:39 pm on March 2, 2017: member

    The one outlier is boost::filesystem::path::imbue(). I’m not sure what to do about that, so I left it alone.

    For boost that imbue hack is necessary to get paths with international characters working (on Windows, I believe). Hopefully std will have some other (possibly saner!) way to achieve the same.

    When we do our own implementation, it’d be really nice if we could be strictly compatible with std::filesystem, not because we’re necessarily moving to it any time soon, but because it gives us a strict api to follow.

    Yes, std::filesytem makes a more sensible target for that than boost::filesystem. Although I’m not sure it makes much sense to do our own (general, cross-platform) implementation if it’s going to be in the standard library in a few years. Specific implementations for specific sandboxing environments would ofcourse make sense.

  5. fanquake added this to the "In progress" column in a project
  6. fanquake commented at 8:14 pm on March 3, 2017: member
    Concept ACK. Added to the Boost migration project.
  7. gmaxwell commented at 11:25 pm on March 6, 2017: contributor
    Concept ACK.
  8. laanwj force-pushed on Mar 7, 2017
  9. laanwj commented at 3:50 pm on March 7, 2017: member
    Rebased.
  10. laanwj force-pushed on Mar 7, 2017
  11. TheBlueMatt commented at 8:21 pm on March 8, 2017: member
    I’m not really convinced its worth having our own namespace just to mirror boost::filesystem. If we want to start supporting systems that dont have a concept of a filesystem, sure, but going out of our way to effectively just rename boost::filesystem so that its less work in 3/5 years when there is a useable std::filesystem seems like overkill (and more Bitcoin Core-specific things) to me.
  12. laanwj commented at 8:48 pm on March 8, 2017: member

    If we want to start supporting systems that dont have a concept of a filesystem, sure

    That happens to be exactly what I’m doing. At the least this reduces the size of the patch set I need to support that. It’s a minimal and effectively risk-free change. Do you really need to start an argument about this?

  13. TheBlueMatt commented at 8:52 pm on March 8, 2017: member
    @laanwj ahh, ok, I misunderstood your work with cloudabi as testing to see what it would take, instead of a serious effort. I withdraw my complaint.
  14. laanwj commented at 9:07 pm on March 8, 2017: member
    Not sure this is worth doing anymore.
  15. fanquake commented at 11:04 pm on March 22, 2017: member
    Needs a rebase. @laanwj do you want to continue with this? I think it’s worth having in 0.15.0.
  16. theuni commented at 7:07 pm on March 24, 2017: member
    This is very straightforward and ropes off the dependency. I don’t see any downside. +1 for 0.15.
  17. fanquake added this to the milestone 0.15.0 on Mar 25, 2017
  18. laanwj force-pushed on Mar 27, 2017
  19. laanwj commented at 8:49 am on March 27, 2017: member
    Rebased
  20. jonasschnelli commented at 12:22 pm on March 27, 2017: contributor

    The QT tests won’t compile here. I don’t know why the even runcompile on master. LDADD misses $(BOOST_UNIT_TEST_FRAMEWORK_LIB).

    This diff seems to be required:

     0diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
 1index 948e13a..f0aac5e 100644
 2--- a/src/Makefile.qttest.include
 3+++ b/src/Makefile.qttest.include
 4@@ -59,7 +59,7 @@ if ENABLE_ZMQ
 5 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 6 endif
 7 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) \
 8-  $(LIBMEMENV) $(BOOST_LIBS) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) $(QT_LIBS) \
 9+  $(LIBMEMENV) $(BOOST_LIBS) $(BOOST_UNIT_TEST_FRAMEWORK_LIB) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) $(QT_LIBS) \
10   $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
11   $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
12 qt_test_test_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)
  21. laanwj force-pushed on Mar 27, 2017
  22. jonasschnelli commented at 1:25 pm on March 27, 2017: contributor

    The LDADD patch above was incorrect. There was an accidental include <boost/test/unit_test.hpp> in this PR. All good.

    Tested ACK 462119817ae41313c3285d57e6adf67f4595b81a

  23. laanwj commented at 1:29 pm on March 27, 2017: member

    The LDADD patch above was incorrect. There was an accidental include <boost/test/unit_test.hpp> in this PR.

    Yes, thanks for noticing the issue. I didn’t have the problem locally for some reason.

  24. Add fs.cpp/h 19e36bbef6
  25. Replace includes of boost/filesystem.h with fs.h 
    This is step one in abstracting the use of boost::filesystem.
    7d5172d354
  26. Replace uses of boost::filesystem with fs 
    Step two in abstracting away boost::filesystem.

To repeat this, simply run:
```
git ls-files \*.cpp \*.h | xargs sed -i 's/boost::filesystem/fs/g'
```
    bac5c9cf64
  27. Use fsbridge for fopen and freopen 
    Abstracts away how a path is opened to a `FILE*`.

Reduces the number of places where path is converted to a string
for anything else but printing.
    2a5f574762
  28. torcontrol: Use fs::path instead of std::string for private key path 75594bd7f2
  29. Remove `namespace fs=fs` 
    Having these inside functions is silly and redundant now.
    f110272dc9
  30. laanwj force-pushed on Apr 3, 2017
  31. laanwj commented at 10:34 am on April 3, 2017: member
    Rebased for #9424
  32. JeremyRubin commented at 7:53 pm on April 5, 2017: contributor

    utACK f110272.

    Because only a few calls are actually used, it might be good to explicitly wrap those so you can pull the boost #includes out of the fs.h header and into fs.cpp.

  33. sipa commented at 8:10 am on April 6, 2017: member
    utACK f110272dc90cd870bfff48c9a61e091e67dbb2e9
  34. theuni commented at 5:56 pm on April 6, 2017: member
    utACK f110272dc90cd870bfff48c9a61e091e67dbb2e9. +1 for @JeremyRubin’s wrapper idea as well, though not for this PR. That has the side-effect of creating a whitelist of allowed boost::filesystem functions.
  35. laanwj commented at 6:08 pm on April 6, 2017: member
    I started out with the wrapper idea, but it grew large and hard to review quite quickly. We use more than one’d expect on first sight. I think I stopped at boost::filesystem::[io]fstream. But we absolutely could do that in a later pull.
  36. laanwj merged this on Apr 6, 2017
  37. laanwj closed this on Apr 6, 2017
  38. laanwj referenced this in commit 8c28670e92 on Apr 6, 2017
  39. jtimon commented at 9:04 pm on April 6, 2017: contributor
    posthumous Concept ACK
  40. fanquake moved this from the "In progress" to the "Done" column in a project
  41. PastaPastaPasta referenced this in commit 5e41474a9a on May 20, 2019
  42. PastaPastaPasta referenced this in commit 506b85e169 on May 20, 2019
  43. PastaPastaPasta referenced this in commit 3cec796990 on May 21, 2019
  44. PastaPastaPasta referenced this in commit f4d62f60be on May 22, 2019
  45. PastaPastaPasta referenced this in commit b45cdb0dbf on May 22, 2019
  46. PastaPastaPasta referenced this in commit 2ad7bcbc97 on May 22, 2019
  47. PastaPastaPasta referenced this in commit 115e3c3347 on May 22, 2019
  48. PastaPastaPasta referenced this in commit b4636b8cfa on May 23, 2019
  49. UdjinM6 referenced this in commit c046cdb432 on May 23, 2019
  50. PastaPastaPasta referenced this in commit b2ad60e9af on May 23, 2019
  51. PastaPastaPasta referenced this in commit a54ff70ff4 on May 27, 2019
  52. barrystyle referenced this in commit 63727cea4f on Jan 22, 2020
  53. random-zebra referenced this in commit a9ac1cc51f on Jun 2, 2020
  54. zkbot referenced this in commit 40d5f0aa57 on Oct 26, 2020
  55. wqking referenced this in commit 3ee219d069 on Dec 29, 2020
  56. MarcoFalke locked this on Sep 8, 2021


laanwj theuni fanquake gmaxwell TheBlueMatt jonasschnelli JeremyRubin sipa jtimon

Labels
Refactoring

Milestone
0.15.0

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-10-04 22:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me