Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Lightweight abstraction of boost::filesystem #9902

Merged
merged 6 commits into from Apr 6, 2017

Conversation

9 participants
@laanwj
Copy link
Member

laanwj commented Mar 2, 2017

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 laanwj added the Refactoring label Mar 2, 2017

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Mar 2, 2017

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

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 2, 2017

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 fanquake added this to In progress in Boost → C++11 migration Mar 3, 2017

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Mar 3, 2017

Concept ACK. Added to the Boost migration project.

@gmaxwell

This comment has been minimized.

Copy link
Member

gmaxwell commented Mar 6, 2017

Concept ACK.

@laanwj laanwj force-pushed the laanwj:2017_03_fs branch from 346de63 Mar 7, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 7, 2017

Rebased.

@laanwj laanwj force-pushed the laanwj:2017_03_fs branch Mar 7, 2017

@TheBlueMatt

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 8, 2017

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

This comment has been minimized.

Copy link
Contributor

TheBlueMatt commented Mar 8, 2017

@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

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 8, 2017

Not sure this is worth doing anymore.

@fanquake

This comment has been minimized.

Copy link
Member

fanquake commented Mar 22, 2017

Needs a rebase. @laanwj do you want to continue with this? I think it's worth having in 0.15.0.

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Mar 24, 2017

This is very straightforward and ropes off the dependency. I don't see any downside. +1 for 0.15.

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

@laanwj laanwj force-pushed the laanwj:2017_03_fs branch Mar 27, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 27, 2017

Rebased

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 27, 2017

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:

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 laanwj force-pushed the laanwj:2017_03_fs branch Mar 27, 2017

@jonasschnelli

This comment has been minimized.

Copy link
Member

jonasschnelli commented Mar 27, 2017

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

This comment has been minimized.

Copy link
Member Author

laanwj commented Mar 27, 2017

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.

laanwj added some commits Mar 1, 2017

Replace includes of boost/filesystem.h with fs.h
This is step one in abstracting the use of boost::filesystem.
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'
```
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.
Remove `namespace fs=fs`
Having these inside functions is silly and redundant now.

@laanwj laanwj force-pushed the laanwj:2017_03_fs branch to f110272 Apr 3, 2017

@laanwj

This comment has been minimized.

Copy link
Member Author

laanwj commented Apr 3, 2017

Rebased for #9424

@JeremyRubin

This comment has been minimized.

Copy link
Contributor

JeremyRubin commented Apr 5, 2017

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

This comment has been minimized.

Copy link
Member

sipa commented Apr 6, 2017

utACK f110272

@theuni

This comment has been minimized.

Copy link
Member

theuni commented Apr 6, 2017

utACK f110272. +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

This comment has been minimized.

Copy link
Member Author

laanwj commented Apr 6, 2017

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 laanwj merged commit f110272 into bitcoin:master Apr 6, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Apr 6, 2017

Merge #9902: Lightweight abstraction of boost::filesystem
f110272 Remove `namespace fs=fs` (Wladimir J. van der Laan)
75594bd torcontrol: Use fs::path instead of std::string for private key path (Wladimir J. van der Laan)
2a5f574 Use fsbridge for fopen and freopen (Wladimir J. van der Laan)
bac5c9c Replace uses of boost::filesystem with fs (Wladimir J. van der Laan)
7d5172d Replace includes of boost/filesystem.h with fs.h (Wladimir J. van der Laan)
19e36bb Add fs.cpp/h (Wladimir J. van der Laan)

Tree-SHA512: 2c34f059dfa6850b9323f3389e9090a6b5f839a457a2960d182c2ecfafd9883c956f5928bb796613402d3aad68ebc78259796a7a313f4a6cfa98aaf507a66842
@jtimon

This comment has been minimized.

Copy link
Member

jtimon commented Apr 6, 2017

posthumous Concept ACK

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.