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

RocksDB version upgrade #140

Closed
lu4 opened this issue Sep 22, 2019 · 8 comments · Fixed by #141
Closed

RocksDB version upgrade #140

lu4 opened this issue Sep 22, 2019 · 8 comments · Fixed by #141

Comments

@lu4
Copy link
Contributor

lu4 commented Sep 22, 2019

Hi, I'm trying to upgrade RocksDB version used in this package:

  1. I've downloaded latest release from RocksDB official github, replaced the contents of /deps/leveldb/leveldb-rocksdb with it.

  2. Updated the leveldb.gyp to this content which can be also found in this diff and the whole fork can be found here.

(I've also temporarily removed some of the code to pinpoint the issue, please ignore those changes)

Now when I install the package locally the compilation phase goes smooth, package installs successfully but when I try to run the code through requiring the package I get runtime error

Error image

Any thoughts on how to deal with this issue?

@vweevers
Copy link
Member

See #82, our plan was to first make RocksDB a git submodule, we need someone to pick up that work.

@lu4
Copy link
Contributor Author

lu4 commented Sep 22, 2019

@vweevers I've came through the thread couple of times trying to figure out what are the action items required to finish that work, it seems to me that most of the work is done. So as it stands to me that there are just two outstanding issues left:

  1. You are trying to remove the build step that copies "build_version.cc.in"
  2. There are some minor conflicts in the code

I also see that my question is tightly related to this build_version.cc.in which I'll try to investigate but if you have a clue I'd be grateful for an advice.

So what's holding from finishing that work?

@vweevers
Copy link
Member

You're right. It's very close to being done! I don't remember the specifics though, other than what's written in that thread.

So what's holding from finishing that work?

Nothing, other than having time. Do you feel up to taking it on? If so, you could start by rebasing that branch and opening a new PR. We can then discuss remaining work there.

@lu4
Copy link
Contributor Author

lu4 commented Sep 22, 2019

So if everyone ok with that build step, I think I'm able to perform the rebase

@vweevers
Copy link
Member

So if everyone ok with that build step

If you meant "is everyone ok with removing that build step" then yes. I believe @filoozom intended to use it for exposing a version number, but we can always add that later, as a semver-minor feature.

@lu4
Copy link
Contributor Author

lu4 commented Sep 22, 2019

@vweevers , I've dived deeper into the code and I would need your guidance then. It turns out that build_version.cc.in is rendered into build_version.cc for a regular RocksDB build, then it is being included in db_impl.cc. However we're using our own GYP based build which does not produce build_version.cc. So you don't like the additional build step approach suggested by @filoozom then it is not clear what do you want to do about build_version.cc file, we can for instance have a separate build_version.cc in the project with version set to something like "leveldown-build" but in order not to guess what's your advice on this one?

Also I see that it would not be enough just to rebase because snappy's bindings has to also be updated or it's the wrong version of snappy being used in @filoozom's branch it doesn't support Node v12 gives me bunch of errors:

errors

So anyway I'm not saying no but it'll take more time than I have right now, I'm not promising anything but I'll try to get back to this work at a later point in time

P.S.

I was also trying to play with original version migration code and was able to get rid of _rocksdb_build_compile_date symbol error. But now I've got new one:

another error

Have you ever bumped into anything similar?

@vweevers
Copy link
Member

I would also have to dive deeper to be able to help. Thanks for your effort so far!

@vweevers
Copy link
Member

I had a look, got it working: #141.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants