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

Upgrade to LevelDB 1.21 or 1.22 #400

Open
rvagg opened this issue Oct 18, 2017 · 7 comments
Open

Upgrade to LevelDB 1.21 or 1.22 #400

rvagg opened this issue Oct 18, 2017 · 7 comments
Labels
help wanted Extra attention is needed

Comments

@rvagg
Copy link
Member

rvagg commented Oct 18, 2017

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
Copy link
Member

peakji commented Oct 18, 2017

google/crc32c looks great, benchmarks: google/leveldb@5c39524

@pirxpilot
Copy link

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
Copy link
Member

I guess now we have to add cmake-js to prebuildify? Like prebuild/prebuildify#7

@vweevers
Copy link
Member

vweevers commented Oct 6, 2019

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

  • Diff our deps/leveldb/leveldb-1.20 against upstream 1.20, generate .patch files (generate .patch files for patched leveldb #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.

@vweevers vweevers changed the title Heads-up on build changes in LevelDB 1.21 Upgrade to LevelDB 1.21 or 1.22 Oct 6, 2019
@vweevers vweevers added the help wanted Extra attention is needed label Oct 6, 2019
@rvagg
Copy link
Member Author

rvagg commented Oct 7, 2019

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
Copy link
Member

vweevers commented Oct 7, 2019

@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) (cmake-js/cmake-js#186). That's fixable of course, so perhaps flaky isn't fair.

@braydonf
Copy link
Contributor

braydonf commented Oct 9, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

5 participants