Skip to content

Use cmake#159

Merged
BusyJay merged 11 commits intomasterfrom
use-cmake
Nov 30, 2017
Merged

Use cmake#159
BusyJay merged 11 commits intomasterfrom
use-cmake

Conversation

@BusyJay
Copy link
Copy Markdown
Member

@BusyJay BusyJay commented Nov 21, 2017

This pr changes the way compiling and linking to rocksdb.

In the past, rocksdb is linked dynamically by default. When link to rocksdb statically, shell script is used to download and build all the native libraries. This method is straightforward and worked quite well.

However, things change now. There are several patches and bug fixes that are not merged into upstream yet (and some are probably not merged at all), if we are stick to the old way, there will be conflict between our fork and official release.

Besides, dynamic link is hard to use and maintain. Many issues in TiKV are related to linking to rocksdb dynamically. And shell script is not platform-independent.

So this pr makes rocksdb as a subtree of the project, so that it doesn't have to be downloaded again and again very time clean is run. And then use cmake to skip the details of compilation. Dynamic link is disabled now, only the subtree version of rocksdb is supported.

@siddontang
Copy link
Copy Markdown

do we need to use submodule?

@BusyJay
Copy link
Copy Markdown
Member Author

BusyJay commented Nov 21, 2017

No, use subtree is OK. When using submodule, more than 50 Mib data needs to be downloaded. However, when using subtree, less than 6 Mib data is required.

@siddontang
Copy link
Copy Markdown

@BusyJay

Do we have a better way to update RocksDB from our forked repository later?

@BusyJay
Copy link
Copy Markdown
Member Author

BusyJay commented Nov 28, 2017

Ping, do you have any concern about it?

@zhangjinpeng87
Copy link
Copy Markdown
Member

@BusyJay Can you give an explanation how to use it?

@siddontang
Copy link
Copy Markdown

can we prune the RocksDB source, E.g, only keep the source file and build scripts.

@BusyJay BusyJay mentioned this pull request Nov 29, 2017
@BusyJay BusyJay changed the title [WIP] Use cmake Use cmake Nov 29, 2017
@BusyJay
Copy link
Copy Markdown
Member Author

BusyJay commented Nov 29, 2017

PTAL

@siddontang
Copy link
Copy Markdown

LGTM


[dependencies.lz4-sys]
git = "https://github.com/busyjay/lz4-rs.git"
branch = "adjust-build"
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this branch mean?

Copy link
Copy Markdown
Member Author

@BusyJay BusyJay Nov 30, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Enable cmake support. /cc bozaro/lz4-rs#23

Copy link
Copy Markdown
Member

@zhangjinpeng87 zhangjinpeng87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
@huachaohuang PTAL

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's this for?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bzip2 is compiled with stdio disabled, so there has to be an external hook to handle error, which is defined in bzip2_sys already.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why comment out this two lines?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because when iterator is not valid anymore, prev should not be called, otherwise assertion in prev will fail.

huachaohuang
huachaohuang previously approved these changes Nov 30, 2017
@BusyJay
Copy link
Copy Markdown
Member Author

BusyJay commented Nov 30, 2017

Windows support is not complete yet, there are still some issues on windows platform. But I think it's fine to merge it first, and deal with the issues later.

@BusyJay BusyJay merged commit fbee9af into master Nov 30, 2017
@BusyJay BusyJay deleted the use-cmake branch November 30, 2017 08:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants