Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix building librocksdb-sys with system libraries #393

Merged
merged 4 commits into from Apr 16, 2020

Conversation

basvandijk
Copy link
Contributor

@basvandijk basvandijk commented Mar 6, 2020

This PR:

  • Uses the correct names for the zlib and bzip2 libraries. The zlib library is called libz.a and the bzip2 library is called libbz2.a.

  • Only fails on empty sub-module directories when the librocksdb-sys is configured to build them.

  • Parameterize the rocksdb include directory.

@basvandijk basvandijk changed the title Use correct names for the zlib and bzip2 libraries Fix building librocksdb-sys with system libraries Mar 15, 2020
Copy link
Contributor

@vitvakatu vitvakatu left a comment

Thank you for your contribution!

@@ -26,9 +26,16 @@ fn fail_on_empty_directory(name: &str) {
}
}

fn rocksdb_include_dir() -> std::string::String {
Copy link
Contributor

@vitvakatu vitvakatu Mar 24, 2020

Choose a reason for hiding this comment

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

Why is it needed? Do you have a specific use-case.

Copy link
Contributor Author

@basvandijk basvandijk Apr 3, 2020

Choose a reason for hiding this comment

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

I would like to build the rocksdb crate with a system-provided rocksdb library. So in my build recipe I'm setting:

ROCKSDB_INCLUDE_DIR = "${rocksdb}/include";

where rocksdb is the path to the rocksdb on my system.

Copy link
Member

@aleksuss aleksuss Apr 3, 2020

Choose a reason for hiding this comment

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

By the way. A system-provided library places its headers with paths like /usr/include in linux case or /usr/local/include in macos case. A compiler uses these paths by default AFAIK 🤔 This var could be useful when you use sources from other different places.

Copy link
Contributor Author

@basvandijk basvandijk Apr 12, 2020

Choose a reason for hiding this comment

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

Indeed. In my case I set:

ROCKSDB_INCLUDE_DIR=/nix/store/m9xx9jq9b6vz42zy8xdzj03grskdlm89-rocksdb-6.6.4/include

since I use the Nix build system.

Copy link
Member

@aleksuss aleksuss Apr 16, 2020

Choose a reason for hiding this comment

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

Yes. I think it could be useful in some cases.

@aleksuss aleksuss merged commit 8a7996e into rust-rocksdb:master Apr 16, 2020
1 check passed
rleungx pushed a commit to rleungx/rust-rocksdb that referenced this issue Jun 17, 2020
* Fix various unsafe declarations

As reported by clippy. These are all cases where a pointer argument is
dereferenced by a declared safe function. The functions are changed to be unsafe
functions. Though some of these functions are declared pub, they all seem to be
related to internal binding matters and appear to be written correctly, just
declared incorrectly, so there shouldn't be any external impact for callers
that aren't using the crocksdb API directly.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Fix some clippy style lints

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Fix mutability of crocksdb_load_latest_options errptr

This value can be mutated on the C side. Caught by clippy.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Cleanup some unit-return functions

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Disable a clippy lint

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* make a transmute more typesafe

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Elide a lifetime

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Remove unnecessary unsafe blocks

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Use repr(C) structs of c_void for opaque types.

These represent opaque RocksDB types. They are used behind pointers, but are
also wrapped in other types in the higher-level bindings.

These use the strategy for opaque C types described in the nomicon [1]:
but with the exception that they contain c_void instead of [u8; 0], thus
making them uninstantiable sized types instead of ZSTs.

The c_void documentation publicly recommends using the ZST pattern from the
nomicon, but in private documentation [2] warns about UB from dereferencing
pointers to uninhabited types, which these bindings do.

Additionally, these bindings wrap some these types directly (not through
pointers) and it's impossible to repr(transparent) a ZST, without which the
unsafe casts within are dubious.

[1]: https://doc.rust-lang.org/nomicon/ffi.html#representing-opaque-structs
[2]: https://doc.rust-lang.org/nightly/src/core/ffi.rs.html#28

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Apply repr(transparent) to various unsafe-cast wrappers

These are wrappers of opaque FFI types, and pointers are being unsafely cast
between the two. repr(transparent) is probably needed to make this well-defined.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Replace mem::transmute with pointer casts

Slightly more typesafe, and more explicit about what's being cast.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Replace more mem::transmute with casts

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Remove a no-op mem::transmute

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Remove is_power_of_two

std contains a branchless bitmagic version of this function

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Add clippy makefile target

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Add clippy to CI

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Fix types in memcpy

This code is copying from c_void pointers when it should be copying
from byte pointers.

Fortunately c_void and u8 have the same size, but I don't know if that is
guaranteed.

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Update src/rocksdb.rs

Co-Authored-By: kennytm <kennytm@gmail.com>
Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Install clippy on travis

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Adjust clippy lints for CI toolchain

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* Make a typesafe cast

Signed-off-by: Brian Anderson <andersrb@gmail.com>

* rustfmt

Signed-off-by: Brian Anderson <andersrb@gmail.com>
Kixunil pushed a commit to Kixunil/rust-rocksdb that referenced this issue Nov 2, 2020
aleksuss pushed a commit that referenced this issue Nov 4, 2020
Co-authored-by: Bas van Dijk <v.dijk.bas@gmail.com>
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.

None yet

3 participants