Skip to content
This repository has been archived by the owner on Dec 1, 2024. It is now read-only.

Move RocksDB to a Git Submodule #82

Closed
wants to merge 9 commits into from

Conversation

filoozom
Copy link
Contributor

@filoozom filoozom commented Nov 1, 2018

Basically the same as Level/leveldown#522 but for RocksDB.

To circumvent the issue that we need the build_version.cc file (#80), I added a small script in scripts/build-version.js that basically replaces both variables from build_version.cc.in and writes the output to build_version.cc. It has no dependency other than NodeJS and should thus work accross platforms without any issues.

This PR places the upstream commit at https://github.com/facebook/rocksdb/tree/4e0065015d3dab1d94ef7cb2b4b1d1fecfa0e926, which is the exact version we're currently using.

I'll open another PR afterwards to upgrade to an actual tagged release of RocksDB.

I also took the opportunity to rename all LevelDB related stuff to RocksDB to reduce the confusion.

Fixes #80.

Todo

@filoozom
Copy link
Contributor Author

filoozom commented Nov 1, 2018

I force-pushed to make it a bit easier to review. The important commit is this one: 608e26e.
The other one is just rm -r deps/leveldb.

@filoozom
Copy link
Contributor Author

filoozom commented Nov 1, 2018

@ralphtheninja I think there's something wrong with Travis. Is auto cancel on and limit off?

And not quite sure why it never works on the first build on Linux and MacOS. Works on Windows and on my local linux machine, but on Travis I always get:

ar: Release/obj.target/rocksdb/deps/rocksdb/rocksdb/util/build_version.o: No such file or directory.

@ralphtheninja
Copy link
Member

@filoozom

screenshot from 2018-11-01 15-40-33

@filoozom
Copy link
Contributor Author

filoozom commented Nov 1, 2018

@ralphtheninja Wouldn't it be better to disable the concurrent jobs limit and enable auto cancel? Because currently my builds aren't starting because they're waiting for a commit from a few hours ago which I don't need anymore, and I can't cancel them.

@ralphtheninja
Copy link
Member

@ralphtheninja Wouldn't it be better to disable the concurrent jobs limit and enable auto cancel? Because currently my builds aren't starting because they're waiting for a commit from a few hours ago which I don't need anymore, and I can't cancel them.

So change it? :)

@filoozom
Copy link
Contributor Author

filoozom commented Nov 1, 2018

@ralphtheninja Well, I'd need to have write access to the repository for that.

In the meantime I'll just make a few tests on my own Travis instance.

@vweevers
Copy link
Member

vweevers commented Dec 8, 2018

@filoozom What's the status of this? Ready for review?

@filoozom
Copy link
Contributor Author

filoozom commented Dec 8, 2018

@filoozom What's the status of this? Ready for review?

Haven't had a lot of free time recently, but this was ready except for some bug I couldn't solve or even reproduce: on Travis, the first build always fails because of build_version.c.

@vweevers vweevers added the semver-patch Bug fixes that are backward compatible label Jan 1, 2019
@filoozom
Copy link
Contributor Author

filoozom commented Jan 31, 2019

Well, looks like it's finally working! I'm not convinced that it's the best way but I couldn't for the life of me figure out a better one. For future reference:

{
  'action_name': 'build-version',
  'outputs': [
    'rocksdb/util/build_version.cc'
  ]
}

with

{
  'sources': [
    'rocksdb/util/build_version.cc'
  ]
}

doesn't work. It creates the build_version.cc file but doesn't compile it.

In the end I used <(INTERMEDIATE_DIR)/build_version.cc as output because it now picks up the file to compile it, but I needed to add rocksdb/util/ to the included directories as build_version.cc is now in a different folder.


I changed the Travis configuration to allow for more than one concurrent job and auto cancel branch and pull request builds, which makes it significantly faster (should now last ~8 minutes instead of ~40 if all builds can start at the same time), hope it wasn't set like this for some kind of compatibility reasons?

@vweevers
Copy link
Member

vweevers commented Feb 1, 2019

@filoozom What's the benefit of generating build_version.cc? Why do we need it?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@filoozom
Copy link
Contributor Author

filoozom commented Feb 1, 2019

@vweevers Well it's not strictly necessary, but the file doesn't exist initially so I need to copy it anyways, so might as well put the right values in it. It does result in a Git SHA that needs to be updated in rocksdb.gyp though, which might not be ideal. I can't parse the .gitmodules file and can't rely on git being available either so I'm not sure if there's an alternative. It might be useful to print the version for debugging purposes though I feel like.

@vweevers
Copy link
Member

vweevers commented Feb 1, 2019

It does result in a Git SHA that needs to be updated in rocksdb.gyp though, which might not be ideal.

We'd need to remember to update that. Plus, it adds a build step (small, but still). To justify that, there must be a real benefit.

It might be useful to print the version for debugging purposes though I feel like.

Seeing as this is a JS project, we'd have to (also) expose the git hash through a JS interface. I doubt anyone would use it. IMO having the git submodule is an adequate way of finding out the RocksDB version.

@filoozom
Copy link
Contributor Author

filoozom commented Feb 1, 2019

We'd need to remember to update that. Plus, it adds a build step (small, but still). To justify that, there must be a real benefit.

Sure, but I need the build step either way because I need to at least copy build_version.cc.in to build_version.cc. (I think? Or can I just put build_version.cc.in in the sources? Let me give it a shot.)

Seeing as this is a JS project, we'd have to (also) expose the git hash through a JS interface. I doubt anyone would use it. IMO having the git submodule is an adequate way of finding out the RocksDB version.

Yes, that was planned too but we could scratch it and forget about it.

@vweevers vweevers added the stale This issue or pull request is old label Aug 11, 2019
This was referenced Sep 22, 2019
@vweevers
Copy link
Member

Superseded by #141.

@vweevers vweevers closed this Sep 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
semver-patch Bug fixes that are backward compatible stale This issue or pull request is old
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move rocksdb to a git submodule
3 participants