Upgrade to LevelDB 1.21 or 1.22

Level / leveldown

Pure C++ Node.js LevelDB binding. An abstract-leveldown compliant store.

MIT License

775 stars 176 forks source link

Upgrade to LevelDB 1.21 or 1.22 #400

Open rvagg opened 7 years ago

rvagg commented 7 years ago

This just in on the LevelDB mailing list that I thought might be interesting for folks here.

From: Victor Costan

Hi everyone!

For planning purposes, please know that the upcoming leveldb release (1.21) will introduce significant changes to the build process. As always, the API will remain backwards-compatible. The build system will be switched to CMake. Configuration macros will change to reflect CMake conventions. (example: HAVE_SNAPPY instead of SNAPPY) Building the library will require C++11 support. A new optional dependency will be added, google/crc32c. Some of the changes above are still under development, while others have already landed on master. I hope this information will help you plan adopting the next release.

Probably just going to mean some GYP config hackery to adapt to the changes they introduce to make it CMake-idiomatic. The C++11 support is easy because Node core is fully C++11 since at least Node 4 and V8 keeps on pushing hard on that front (a little awkwardly in some cases). The google/crc32c dependency will be interesting, we may want to pull that in because I guess it'll add some nice extra error-checking capabilities.

peakji commented 7 years ago

google/crc32c looks great, benchmarks: https://github.com/google/leveldb/commit/5c39524f3639e6bf6ab49215152d24273e662986

pirxpilot commented 6 years ago

Somewhat related: I attempted switching to cmake-js to build latest leveldb (master) in my fork. I seems to work OK for MacOS and Linux, I did not try that on other platforms and I am sure I am missing some things that are obvious to people more familiar with the project.

For what it's worth cmake builds seem to be easy: snappy and crc32c already use cmake, which makes our CMakeLists.txt look... uncomplicated (at least compared to what I see in .gyp files). Latest prebuild (and prebuild-install) works with cmake-js as well.

In any case, feel free to pick up any useful stuff.

vweevers commented 5 years ago

I guess now we have to add cmake-js to prebuildify? Like https://github.com/prebuild/prebuildify/pull/7

vweevers commented 5 years ago

A rough description of tasks (feel free to edit):

Diff our deps/leveldb/leveldb-1.20 against upstream 1.20, generate .patch files (#459)
Update to 1.22
Possibly get rid of deps/leveldb/port-libuv, because LevelDB got Windows support
Apply patches (if still needed)
Either update leveldb.gyp or switch to cmake-js. I prefer the former atm, because cmake-js is flaky on Windows and we'd have to change and retest our whole prebuild setup.

rvagg commented 5 years ago

Got any details on the cmake-js flaky problems? We've been discussing the future of gyp and node-gyp and cmake-js came up as something that we might encourage as an option but I have no experience with it. Gyp still has a future, we're taking proper ownership of it in the nodejs org and have a plan for a gradual migration go a gyp.js that will help deal with the massive gyp investment in the ecosystem. So I for leveldown, sticking with gyp is a safe choice if cmake-js bears any risk.

vweevers commented 5 years ago

@rvagg I have no experience as a user of cmake-js, but as a maintainer of prebuild which integrates with cmake-js, I know cmake-js is unable to find MSVS since 5.2.0 (july-ish) (https://github.com/cmake-js/cmake-js/issues/186). That's fixable of course, so perhaps flaky isn't fair.

braydonf commented 5 years ago

In addition of the change from SNAPPY to HAVE_SNAPPY, there is also LEVELDB_IS_BIG_ENDIAN, HAVE_FDATASYNC, HAVE_FULLFSYNC and HAVE_CRC32C.

The patches to LevelDB are likely not necessary anymore, as it's related to LEVELDB_PLATFORM_UV, which can likely be LEVELDB_PLATFORM_WINDOWS now, and defined(OS_IOS) may not be necessary.

Edit: These can be defined in port/port_config.h.