*: add tirocks-sys and xtask #1

BusyJay · 2022-01-22T13:31:45Z

The version of titan and rocksdb submodules are the same as master branch of rust-rocksdb. xtask crate are copied from grpc-rs. Build script are refactored.

Compared to rust-rocksdb, the main differences are:

Use cargo command and bindgen to maintain bindings just like grpc-rs:
```
cargo xtask bindgen
```
Titan and rocksdb are maintained in one crate instead of two. Because compiling titan requires rocksdb, so it's impossible to publish a crate that only includes the source of titan. And compiling crocksdb also requires the source of titan. So it's easier to maintain to just put rocksdb and titan into one crate.
bindgen is completely removed from dependency tree unless we want to regenerate bindings. So compiling tirocks doesn't requires clang and llvm. It's safe for every platform to share the same bindings as all structs are opaque types and libc is used to cover the platform differences.
Released version of snappy-sys is used by default so we can publish tirocks in the future.

Signed-off-by: Jay Lee <[email protected]>

tabokie · 2022-01-23T10:19:11Z

tirocks-sys/Cargo.toml

+links = "tirocks"
+
+[dependencies]
+# native library >=1.0.8


What does this mean?

It means the version of bzip2 package >= 1.0.8.

tabokie · 2022-01-24T03:23:41Z

Cargo.toml

+
+[patch.crates-io]
+# Use cmake to build the binary, so it can be compiled on all supported platform.
+snappy-sys = { git = "https://github.com/busyjay/rust-snappy.git", branch = "static-link" }


Signed-off-by: Jay Lee <[email protected]>

Connor1996

In rust-rocksdb, DBStatisticsTickerType is generated by librocksdb_sys/src/generated.rs. We did some changes in it. It can't simply follow the original c.h.

xtask/src/main.rs

Connor1996 · 2022-01-26T12:42:15Z

tirocks-sys/build.rs

+#[cfg(feature = "update-bindings")]
+fn bindgen_rocksdb(file_path: &Path) {
+    println!("cargo:rerun-if-env-changed=TEST_BIND");
+    let gen_tests = env::var("TEST_BIND").map_or(false, |s| s == "1");


should it be removed?

No, it will be used in tests.

Signed-off-by: Jay Lee <[email protected]>

BusyJay · 2022-01-26T15:30:06Z

@tabokie @Connor1996

I add the missing ticker and histogram types by including them into whitelist. So they are generated by bindgen automatically instead of custom scripts.

I also define all the properties names in c.h, so they can also be maintained by bindgen.

tirocks-sys/bindings/bindings.rs

Signed-off-by: Jay Lee <[email protected]>

tirocks-sys/bindings/bindings.rs

Connor1996 · 2022-01-28T07:36:20Z

tirocks-sys/crocksdb/crocksdb/c.h

@@ -0,0 +1,2718 @@
+/*  Copyright (c) 2011-present, Facebook, Inc.  All rights reserved.


Wish we can use C API defined in rocksdb and Titan instead of maintaining it here.

Agree. Maybe we need to have rocksdb headers, titan headers and custom headers maintained by ourselves. This can be done later.

Connor1996 · 2022-01-28T07:40:02Z

xtask/src/main.rs

+}
+
+fn clang_format() {
+    exec(cmd("clang-format").args(&[


How about calling it in make format

There is no Makefile. The point of using xtask is to use cargo flow completely.

If I have some changes, then I need to call cargo fmt and cargo xtask clang-format separately. I don't think it's friendly.

Added a new command cargo xtask format.

Connor1996 · 2022-01-28T07:42:03Z

README.md

+## Build
+
+```
+$ cargo xtask submodule # if you just cloned the repository


I suggest provide a make build

It's a one time task. And update submodule every time may not be a good idea if I want to checkout submodule to a different version.

So we need to provide a Makfile. update_rocksdb and update_titan are needed by the auto-update bot.

What is auto-update bot? All functionality can be implemented as a new command of xtask.

update the rocksdb/titan submodule when any rocksdb/titan's PRs get merged, and then file a PR to update rust-rocksdb dependency in tikv repo

Connor1996 · 2022-01-28T08:13:58Z

tirocks-sys/build.rs

+    figure_link_lib(&dst, "rocksdb");
+
+    if cfg!(target_os = "windows") {
+        build.define("OS_WIN", None);


Have a question: as the rocksdb has already been built above, do these defines make any sense?

Yes, it will be used in the headers included in c.h.

Connor1996 · 2022-01-28T08:29:06Z

tirocks-sys/build.rs

+    let mut builder = bindgen::Builder::default();
+
+    if env::var("CARGO_CFG_TARGET_OS").map_or(false, |s| s == "windows") {
+        builder = builder.clang_arg("-D _WIN32_WINNT=0x600");


what's it for. Please add a comment.

It's copied from grpc-rs. Some file may require to define minimal supported windows version at compile time.

Connor1996 · 2022-01-28T08:33:53Z

tirocks-sys/build.rs

+        .whitelist_type(r"\brocksdb::Histograms")
+        .whitelist_type(r"\brocksdb::titandb::TickerType")
+        .whitelist_type(r"\brocksdb::titandb::HistogramType")
+        .blacklist_type(r"\b__.*")


what is it for

To filter symbols like __uint32.

Connor1996 · 2022-01-28T08:36:33Z

tirocks-sys/build.rs

+        .whitelist_type(r"\brocksdb::titandb::HistogramType")
+        .blacklist_type(r"\b__.*")
+        .derive_copy(false)
+        .opaque_type(r"\bctitandb_.*")


why titan's type are opaque type

Actually all structs are opaque types as the current way they are defined. But this is a stale change, I will remove it.

Connor1996 · 2022-01-28T08:39:28Z

tirocks-sys/bindings/bindings.rs

+#[doc = "  4. Add the enum conversions from Java and C++ to portal.h's toJavaTickerType"]
+#[doc = " and toCppTickers"]
+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+pub enum rocksdb_Tickers {


can we have a way to add a prefix c for these enums to make them consistent with other types

or cancel the prefix c in the crocksdb.h

No, we can't. rust-lang/rust-bindgen#1098. I think the c prefix is fine as it can tell what types are defined by ourselves and what types are defined by rocksdb itself.

Connor1996 · 2022-01-28T09:04:00Z

tirocks-sys/src/lib.rs

@@ -0,0 +1,32 @@
+// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.


Suggested change

// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.

// Copyright 2022 TiKV Project Authors. Licensed under Apache-2.0.

Connor1996

rest LGTM

Signed-off-by: Jay Lee <[email protected]>

Connor1996

LGTM

BusyJay added 7 commits January 21, 2022 19:38

tirocks-sys: add submodules

6cb28b8

Signed-off-by: Jay Lee <[email protected]>

librocks-sys: add build script

ac97f19

Signed-off-by: Jay Lee <[email protected]>

use same bindings

f3db315

Signed-off-by: Jay Lee <[email protected]>

link bzip2

ed9b74f

Signed-off-by: Jay Lee <[email protected]>

add comment about patch

e6ee497

Signed-off-by: Jay Lee <[email protected]>

update README

1b86c6c

Signed-off-by: Jay Lee <[email protected]>

update feature name

b21b3e0

Signed-off-by: Jay Lee <[email protected]>

BusyJay requested review from Connor1996 and tabokie January 22, 2022 13:31

tabokie approved these changes Jan 24, 2022

View reviewed changes

BusyJay added 2 commits January 24, 2022 15:15

address comment

1347180

Signed-off-by: Jay Lee <[email protected]>

change lint to format

36905ed

Signed-off-by: Jay Lee <[email protected]>

Connor1996 reviewed Jan 26, 2022

View reviewed changes

BusyJay added 2 commits January 26, 2022 22:47

add missing bindings

42df49c

Signed-off-by: Jay Lee <[email protected]>

introduce crocksdb_slice_t

c9fef5a

Signed-off-by: Jay Lee <[email protected]>

Connor1996 reviewed Jan 27, 2022

View reviewed changes

tirocks-sys/bindings/bindings.rs Show resolved Hide resolved

add missing titan tickers and histograms

bbf2215

Signed-off-by: Jay Lee <[email protected]>

Connor1996 reviewed Jan 28, 2022

View reviewed changes

tirocks-sys/bindings/bindings.rs Show resolved Hide resolved

Connor1996 reviewed Jan 28, 2022

View reviewed changes

BusyJay added 2 commits January 28, 2022 17:06

address comment

4a39467

Signed-off-by: Jay Lee <[email protected]>

support switching submodule branch and url

6ba3762

Signed-off-by: Jay Lee <[email protected]>

Connor1996 approved these changes Jan 28, 2022

View reviewed changes

BusyJay merged commit a15458c into main Jan 29, 2022

BusyJay deleted the add-native-lib branch January 29, 2022 16:55

BusyJay mentioned this pull request Feb 28, 2022

Update grpc c core to 1.44.0 tikv/grpc-rs#558

Merged

		@@ -0,0 +1,2718 @@
		/* Copyright (c) 2011-present, Facebook, Inc. All rights reserved.

		@@ -0,0 +1,32 @@
		// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.

	// Copyright 2019 TiKV Project Authors. Licensed under Apache-2.0.
	// Copyright 2022 TiKV Project Authors. Licensed under Apache-2.0.

*: add tirocks-sys and xtask #1

*: add tirocks-sys and xtask #1

Uh oh!

Conversation

BusyJay commented Jan 22, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay commented Jan 26, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BusyJay Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connor1996 Jan 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

Uh oh!

Connor1996 left a comment

Choose a reason for hiding this comment

BusyJay commented Jan 26, 2022 •

edited

Loading

BusyJay Jan 28, 2022 •

edited

Loading

Connor1996 Jan 28, 2022 •

edited

Loading