Lightweight abstraction of boost::filesystem #9902

laanwj · 2017-03-02T08:40:24Z

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.

theuni · 2017-03-02T20:05:29Z

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 · 2017-03-02T20:39:59Z

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 · 2017-03-03T20:14:39Z

Concept ACK. Added to the Boost migration project.

gmaxwell · 2017-03-06T23:25:45Z

Concept ACK.

laanwj · 2017-03-07T15:50:33Z

Rebased.

TheBlueMatt · 2017-03-08T20:21:52Z

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 · 2017-03-08T20:48:02Z

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 · 2017-03-08T20:52:49Z

@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 · 2017-03-08T21:07:06Z

Not sure this is worth doing anymore.

fanquake · 2017-03-22T23:04:19Z

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

theuni · 2017-03-24T19:07:12Z

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

laanwj · 2017-03-27T08:49:47Z

Rebased

jonasschnelli · 2017-03-27T12:22:14Z

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)

jonasschnelli · 2017-03-27T13:25:00Z

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 · 2017-03-27T13:29:29Z

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.

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

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' ```

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.

Having these inside functions is silly and redundant now.

laanwj · 2017-04-03T10:34:01Z

Rebased for #9424

JeremyRubin · 2017-04-05T19:53:59Z

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 · 2017-04-06T08:10:48Z

utACK f110272

theuni · 2017-04-06T17:56:40Z

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 · 2017-04-06T18:08:31Z

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.

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 · 2017-04-06T21:04:00Z

posthumous Concept ACK

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

286aa99 Remove `namespace fs=fs` (random-zebra) f16a6db torcontrol: Use fs::path instead of std::string for private key path (random-zebra) 7de1ba7 Use fsbridge for fopen and freopen (random-zebra) 5010dac Replace uses of boost::filesystem with fs (random-zebra) 4305cce Replace includes of boost/filesystem.h with fs.h (random-zebra) 1d84a5a Add fs.cpp/h (random-zebra) Pull request description: Based on top of: - [x] #1629 Backports bitcoin#9902 > 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. ACKs for top commit: furszy: Code review ACK 286aa99. furszy: ACK 286aa99. Fuzzbawls: ACK 286aa99 Tree-SHA512: d5409bc67bb1fbb38fd65182ca36f122d81db7ec8a17747c0683602cb71e407b0e09779ca15171902fc0d2906302a993606ff6599381ef9ad3e52dbac1194952

Lightweight abstraction of boost::filesystem This is a refactor backport ahead of replacing most of our Boost dependencies with C++17 code. Cherry-picked from the following upstream PRs: - bitcoin/bitcoin#7667 - Removes merge conflicts. - bitcoin/bitcoin#9902 - bitcoin/bitcoin@2300a5e - bitcoin/bitcoin#10546 - Only the changes to `src/fs.cpp`

286aa991c78da640d9c4afa0766cb231d42bdaa5 Remove `namespace fs=fs` (random-zebra) f16a6db1ac96f5dc8569f8c381340d492dc99c58 torcontrol: Use fs::path instead of std::string for private key path (random-zebra) 7de1ba73db7cf89d1e0fbd882f7cc158ef9ce289 Use fsbridge for fopen and freopen (random-zebra) 5010dac1ef546f08a7662df1da77068d9516a13e Replace uses of boost::filesystem with fs (random-zebra) 4305cce4587eaf7bfb9ce1321a2ce5f6b63449ef Replace includes of boost/filesystem.h with fs.h (random-zebra) 1d84a5ad258f10116e788b7c84fde49568b2f3e9 Add fs.cpp/h (random-zebra) Pull request description: Based on top of: - [x] #1629 Backports bitcoin/bitcoin#9902 > 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. ACKs for top commit: furszy: Code review ACK 286aa99. furszy: ACK 286aa99. Fuzzbawls: ACK 286aa991c78da640d9c4afa0766cb231d42bdaa5 Tree-SHA512: d5409bc67bb1fbb38fd65182ca36f122d81db7ec8a17747c0683602cb71e407b0e09779ca15171902fc0d2906302a993606ff6599381ef9ad3e52dbac1194952 # Conflicts: # src/Makefile.am # src/init.cpp # src/leveldbwrapper.cpp # src/leveldbwrapper.h # src/masternode-budget.cpp # src/masternode-payments.cpp # src/masternodeman.cpp # src/qt/guiutil.cpp # src/qt/vitae.cpp # src/qt/vitae/masternodeswidget.cpp # src/sporkdb.h # src/test/dbwrapper_tests.cpp # src/test/test_vitae.cpp # src/util.cpp # src/util.h # src/vitae-cli.cpp

laanwj added the Refactoring label Mar 2, 2017

laanwj force-pushed the 2017_03_fs branch from 346de63 to 68dc186 Compare March 7, 2017 15:50

laanwj force-pushed the 2017_03_fs branch from 68dc186 to b83177b Compare March 7, 2017 16:08

fanquake added this to the 0.15.0 milestone Mar 25, 2017

laanwj force-pushed the 2017_03_fs branch from b83177b to fddabe7 Compare March 27, 2017 08:49

laanwj force-pushed the 2017_03_fs branch from fddabe7 to 4621198 Compare March 27, 2017 13:05

laanwj added 6 commits April 3, 2017 12:32

Add fs.cpp/h

19e36bb

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

7d5172d

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

Replace uses of boost::filesystem with fs

bac5c9c

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

2a5f574

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.

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

75594bd

Remove namespace fs=fs

f110272

Having these inside functions is silly and redundant now.

laanwj force-pushed the 2017_03_fs branch from 4621198 to f110272 Compare April 3, 2017 10:33

laanwj merged commit f110272 into bitcoin:master Apr 6, 2017

theuni mentioned this pull request Apr 11, 2017

devtools/net: add a verifier for scriptable changes. Use it to make CNode::id private. #10189

Merged

sickpig mentioned this pull request Oct 1, 2017

dbcache phase 3 enhancements BitcoinUnlimited/BitcoinUnlimited#785

Merged

str4d mentioned this pull request Jan 29, 2020

util: CBufferedFile fixes zcash/zcash#4265

Merged

random-zebra mentioned this pull request May 19, 2020

[Refactoring] Lightweight abstraction of boost::filesystem PIVX-Project/PIVX#1630

Merged

1 task

str4d mentioned this pull request Oct 22, 2020

Lightweight abstraction of boost::filesystem zcash/zcash#4805

Merged

lyricidal mentioned this pull request Aug 31, 2021

[Upstream] [Refactoring] Lightweight abstraction of boost::filesystem PRCYCoin/PRCYCoin#104

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Lightweight abstraction of boost::filesystem #9902

Lightweight abstraction of boost::filesystem #9902

Uh oh!

Conversation

laanwj commented Mar 2, 2017

Uh oh!

theuni commented Mar 2, 2017

Uh oh!

laanwj commented Mar 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Mar 3, 2017

Uh oh!

gmaxwell commented Mar 6, 2017

Uh oh!

laanwj commented Mar 7, 2017

Uh oh!

TheBlueMatt commented Mar 8, 2017

Uh oh!

laanwj commented Mar 8, 2017

Uh oh!

TheBlueMatt commented Mar 8, 2017

Uh oh!

laanwj commented Mar 8, 2017

Uh oh!

fanquake commented Mar 22, 2017

Uh oh!

theuni commented Mar 24, 2017

Uh oh!

laanwj commented Mar 27, 2017

Uh oh!

jonasschnelli commented Mar 27, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jonasschnelli commented Mar 27, 2017

Uh oh!

laanwj commented Mar 27, 2017

Uh oh!

laanwj commented Apr 3, 2017

Uh oh!

JeremyRubin commented Apr 5, 2017

Uh oh!

sipa commented Apr 6, 2017

Uh oh!

theuni commented Apr 6, 2017

Uh oh!

laanwj commented Apr 6, 2017

Uh oh!

jtimon commented Apr 6, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

laanwj commented Mar 2, 2017 •

edited

Loading

jonasschnelli commented Mar 27, 2017 •

edited

Loading