Lightweight abstraction of boost::filesystem#9902
Conversation
|
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. |
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.
Yes, |
|
Concept ACK. Added to the Boost migration project. |
|
Concept ACK. |
|
Rebased. |
|
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. |
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. |
|
@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. |
|
Not sure this is worth doing anymore. |
|
Needs a rebase. @laanwj do you want to continue with this? I think it's worth having in 0.15.0. |
|
This is very straightforward and ropes off the dependency. I don't see any downside. +1 for 0.15. |
|
Rebased |
|
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) |
|
The LDADD patch above was incorrect. There was an accidental Tested ACK 462119817ae41313c3285d57e6adf67f4595b81a |
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.
|
Rebased for #9424 |
|
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. |
|
utACK f110272 |
|
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. |
|
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 |
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
|
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
9 participants
Wrap
boost::filesystemas thefsnamespace, contained in one header.Also add
fsbridgefor bridging betweenstdio.hfopen()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.