Move rocksdb to a git submodule

[filoozom]

So, I'm working on moving all dependencies to Git Submodules (as discussed in Level/leveldown#522), but I'm not quite able to figure out what the exact version of RocksDB is.

So, I know it's somewhere in ^5.3.0, but no upstream tag (v5.3.3 to v5.3.6) exactly matches the code in the deps/leveldb/leveldb-rocksdb folder.

I guess I could try to find out the exact commit by digging into this a bit deeper (without 08fd9a3 I suppose), but would it be fine to upgrade to the latest 5.3 version (v5.3.6) instead if tests pass?

[filoozom]

It was actually a bit after v5.3.6: https://github.com/facebook/rocksdb/tree/4e0065015d3dab1d94ef7cb2b4b1d1fecfa0e926.

[filoozom]

Now, I've never really used node-gyp before, but it looks like this should be executed before compiling: https://github.com/facebook/rocksdb/blob/4e0065015d3dab1d94ef7cb2b4b1d1fecfa0e926/Makefile#L263-L269. More specifically:

date := $(shell date +%F)
git_sha := $(shell git rev-parse HEAD 2>/dev/null)
gen_build_version = sed -e s/@@GIT_SHA@@/$(git_sha)/ -e s/@@GIT_DATE_TIME@@/$(date)/ util/build_version.cc.in

Does anyone know how to do this? Without forgetting that it also needs to work on Windows.

[ralphtheninja]

Does anyone know how to do this? Without forgetting that it also needs to work on Windows.

Umm, not really. But if it needs to be run before compiling and given it should work on windows. How does rocksdb itself solve it for windows?

[vweevers]

What's the benefit of defining these?

How does rocksdb itself solve it for windows?

With CMake it seems: https://github.com/facebook/rocksdb/blob/50895e5f0dd293602b7ccb5d284e6143715a27e5/CMakeLists.txt#L124-L138

[filoozom]

Yes, with make (#80 (comment)) and cmake (#80 (comment)). Actually I'm thinking about compiling rocksdb as a shared lib directly with make and/or cmake and linking that in our bindings.gyp, instead of trying to replicate the Makefile in node-gyp.

Do you think that would make sense? On linux I think it wouldn't be a problem as we already have make and all dependencies, but for Windows we'd need to install cmake somehow. I've been having a look at cmake-js since I don't think that windows-build-tools ships it.

[vweevers]

BTW, what does the use of submodules mean for npm-installed github dependencies? E.g. npm i Level/rocksdb. Does that work?

[ralphtheninja]

@vweevers It seems to work well. I tried with npm i Level/leveldown#master though. Interestingly, I made a diff:

lms@nuc ~/tmp/node_modules/leveldown/deps/snappy/snappy                                        
$ diff . ~/src/level/leveldown/deps/snappy/snappy                                              
Only in /home/lms/src/level/leveldown/deps/snappy/snappy: AUTHORS                              
Common subdirectories: ./cmake and /home/lms/src/level/leveldown/deps/snappy/snappy/cmake      
Only in /home/lms/src/level/leveldown/deps/snappy/snappy: NEWS                                 
Common subdirectories: ./testdata and /home/lms/src/level/leveldown/deps/snappy/snappy/testdata

At first I couldn't figure out why AUTHORS and NEWS wasn't there but they seem to be filtered out due to .npmignore inside leveldown:

$ cat .npmignore
*.tar.gz
build/
build-pre-gyp/
test-data.tar
test-data.db.tar
deps/leveldb/leveldb-basho/
deps/leveldb/leveldb-hyper/
deps/leveldb/leveldb-rocksdb/
deps/snappy/snappy-*/testdata/
leakydb
bench/
test/
deps/leveldb/leveldb-*/doc/
README
INSTALL
NEWS
AUTHORS
.nyc_output/

Also, just noticed when typing this that deps/snappy/snappy-*/testdata/ above should probably be deps/snappy/snappy/testdata/ now.

[vweevers]

@ralphtheninja why does it work though? leveldown has no postinstall step to initialize submodules; so is it a builtin behavior of npm?

[ralphtheninja]

@ralphtheninja why does it work though? leveldown has no postinstall step to initialize submodules; so is it a builtin behavior of npm?

Good question. It must be imo.

[vweevers]

Yup, that's the case for npm 3.10, which logs:

npm info git [ 'submodule', '-q', 'update', '--init', '--recursive' ]

npm 5.3 and 6.4 don't log this (or any other line containing submodule) but do seem to work.

[vweevers]

OK, it's officially supported, see npm/npm#1876.

It's not supported by yarn, however: yarnpkg/yarn#1488

[vweevers]

Done in #141.