Lightweight abstraction of boost::filesystem #9902

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.

laanwj added the label Refactoring on Mar 2, 2017

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.

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.

fanquake added this to the "In progress" column in a project

fanquake commented at 8:14 PM on March 3, 2017: member

Concept ACK. Added to the Boost migration project.

gmaxwell commented at 11:25 PM on March 6, 2017: contributor

Concept ACK.

laanwj force-pushed on Mar 7, 2017

laanwj commented at 3:50 PM on March 7, 2017: member

Rebased.

laanwj force-pushed on Mar 7, 2017

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.

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?

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.

laanwj commented at 9:07 PM on March 8, 2017: member

Not sure this is worth doing anymore.

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.

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.

fanquake added this to the milestone 0.15.0 on Mar 25, 2017

laanwj force-pushed on Mar 27, 2017

laanwj commented at 8:49 AM on March 27, 2017: member

Rebased

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 ~~run~~compile on master. LDADD misses $(BOOST_UNIT_TEST_FRAMEWORK_LIB).~~

~~This diff seems to be required:~~

diff --git a/src/Makefile.qttest.include b/src/Makefile.qttest.include
index 948e13a..f0aac5e 100644
--- a/src/Makefile.qttest.include
+++ b/src/Makefile.qttest.include
@@ -59,7 +59,7 @@ if ENABLE_ZMQ
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_ZMQ) $(ZMQ_LIBS)
 endif
 qt_test_test_bitcoin_qt_LDADD += $(LIBBITCOIN_CLI) $(LIBBITCOIN_COMMON) $(LIBBITCOIN_UTIL) $(LIBBITCOIN_CONSENSUS) $(LIBBITCOIN_CRYPTO) $(LIBUNIVALUE) $(LIBLEVELDB) \
-  $(LIBMEMENV) $(BOOST_LIBS) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) $(QT_LIBS) \
+  $(LIBMEMENV) $(BOOST_LIBS) $(BOOST_UNIT_TEST_FRAMEWORK_LIB) $(QT_DBUS_LIBS) $(QT_TEST_LIBS) $(QT_LIBS) \
   $(QR_LIBS) $(PROTOBUF_LIBS) $(BDB_LIBS) $(SSL_LIBS) $(CRYPTO_LIBS) $(MINIUPNPC_LIBS) $(LIBSECP256K1) \
   $(EVENT_PTHREADS_LIBS) $(EVENT_LIBS)
 qt_test_test_bitcoin_qt_LDFLAGS = $(RELDFLAGS) $(AM_LDFLAGS) $(QT_LDFLAGS) $(LIBTOOL_APP_LDFLAGS)

laanwj force-pushed on Mar 27, 2017

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

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.

Add fs.cpp/h 19e36bbef6

Replace includes of boost/filesystem.h with fs.h

This is step one in abstracting the use of boost::filesystem.

7d5172d354

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

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

torcontrol: Use fs::path instead of std::string for private key path 75594bd7f2

Remove `namespace fs=fs`

Having these inside functions is silly and redundant now.

f110272dc9

laanwj force-pushed on Apr 3, 2017

laanwj commented at 10:34 AM on April 3, 2017: member

Rebased for #9424

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.

sipa commented at 8:10 AM on April 6, 2017: member

utACK f110272dc90cd870bfff48c9a61e091e67dbb2e9

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.

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.

laanwj merged this on Apr 6, 2017

laanwj closed this on Apr 6, 2017

laanwj referenced this in commit 8c28670e92 on Apr 6, 2017

jtimon commented at 9:04 PM on April 6, 2017: contributor

posthumous Concept ACK

fanquake moved this from the "In progress" to the "Done" column in a project

PastaPastaPasta referenced this in commit 5e41474a9a on May 20, 2019

PastaPastaPasta referenced this in commit 506b85e169 on May 20, 2019

PastaPastaPasta referenced this in commit 3cec796990 on May 21, 2019

PastaPastaPasta referenced this in commit f4d62f60be on May 22, 2019

PastaPastaPasta referenced this in commit b45cdb0dbf on May 22, 2019

PastaPastaPasta referenced this in commit 2ad7bcbc97 on May 22, 2019

PastaPastaPasta referenced this in commit 115e3c3347 on May 22, 2019

PastaPastaPasta referenced this in commit b4636b8cfa on May 23, 2019

UdjinM6 referenced this in commit c046cdb432 on May 23, 2019

PastaPastaPasta referenced this in commit b2ad60e9af on May 23, 2019

PastaPastaPasta referenced this in commit a54ff70ff4 on May 27, 2019

barrystyle referenced this in commit 63727cea4f on Jan 22, 2020

random-zebra referenced this in commit a9ac1cc51f on Jun 2, 2020

zkbot referenced this in commit 40d5f0aa57 on Oct 26, 2020

wqking referenced this in commit 3ee219d069 on Dec 29, 2020

MarcoFalke locked this on Sep 8, 2021