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

Calling MQF from Rust #3

Merged
merged 18 commits into from Nov 7, 2019

Conversation

@luizirber
Copy link
Member

luizirber commented Nov 3, 2019

This uses bindgen to generate Rust bindings from the gqf.h header. Slowly adding more functions, but this initial PoC is based on the "simple counting test" in tests/CountingTests.cpp

TODO

@luizirber luizirber force-pushed the rust_ffi branch from e213de2 to 96d51f0 Nov 4, 2019
@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 4, 2019

Question for @shokrof and @mr-eyes: is there a way to tell CMake to build only the static lib, and skip binaries and tests?

@luizirber luizirber force-pushed the rust_ffi branch from 27b29a8 to 4f08dff Nov 4, 2019
@@ -109,7 +109,7 @@ extern "C" {

void qf_destroy(QF *qf);

void qf_copy(QF *dest, QF *src);
void qf_copy(QF *dest, const QF *src);

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 4, 2019

Author Member

@shokrof how do you feel about making src into a const QF *? This makes my life easier (because of mutability and ownership rules in Rust), and should still be valid wherever qf_copy was called before, right?

@shokrof

This comment has been minimized.

Copy link
Collaborator

shokrof commented Nov 4, 2019

I think we need to implement a c wrapper for rust to catch exceptions and communicate the messages in a "Rusty" way. I use exceptions to signal when mqf is full and If you are trying to access out of the boundaries items.

@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 4, 2019

I think we need to implement a c wrapper for rust to catch exceptions and communicate the messages in a "Rusty" way. I use exceptions to signal when mqf is full and If you are trying to access out of the boundaries items.

A mix of https://github.com/luizirber/ukhs/blob/master/bbhash-sys/ffi.cpp and rust-lang/rust-bindgen#1208 should be enough. Ideally it would be better to have exceptions never cross thru a C call (wrap in a try/catch block, and return an error code?), but I can use this ideas from the links to wrap all functions that do throw exceptions and expose them to Rust.

@mr-eyes

This comment has been minimized.

Copy link
Member

mr-eyes commented Nov 4, 2019

Question for @shokrof and @mr-eyes: is there a way to tell CMake to build only the static lib, and skip binaries and tests?

@luizirber Here you are #4

Build with cmake $DIR -DSUPRESS_BIN=ON -DSUPRESS_TESTS=ON

@mr-eyes

This comment has been minimized.

Copy link
Member

mr-eyes commented on build.rs in e05a9b3 Nov 4, 2019

@luizirber I think you will just need to add:

.define("SUPRESS_BIN", "ON")
.define("SUPRESS_TESTS", "ON")

I hope it works.

luizirber added 5 commits Nov 4, 2019
CMake options to suppress building binaries & tests
@luizirber luizirber changed the title [WIP] Calling MQF from Rust Calling MQF from Rust Nov 7, 2019
luizirber added 2 commits Nov 7, 2019
@luizirber luizirber force-pushed the rust_ffi branch from 40702d1 to dee6e9d Nov 7, 2019
fn main() {
let dst = Config::new(".")
.define("BUILD_STATIC_LIBS", "ON")
.build_target("MQF")

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 7, 2019

Author Member

@mr-eyes I ended up reverting the CMake changes, this was enough to avoid all the other targets (including binaries and tests)

"debug" => "_debug",
_ => "",
};
println!("cargo:rustc-link-lib=static=stxxl{}", mode);

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 7, 2019

Author Member

@shokrof I can't skip linking stxxl here, since libMQF.a brings the on-disk MQF and if it is not included there are many symbols missing when I try to run the tests. Not sure how to (or even if it is desirable) to modularize what gets added to the static/dynamic library,

This comment has been minimized.

Copy link
@shokrof

shokrof Nov 7, 2019

Collaborator

Do you mean the c++ test?

This comment has been minimized.

Copy link
@luizirber

luizirber Nov 7, 2019

Author Member

No, the Rust tests. The code builds, but when I try to link the static library to the Rust code (to run the tests) there are missing symbols.

This comment has been minimized.

Copy link
@shokrof

shokrof Nov 7, 2019

Collaborator

I can create a new target for you that only has MQF if it will make your life easier

@luizirber luizirber referenced this pull request Nov 7, 2019
0 of 5 tasks complete
@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 7, 2019

Currently there is enough functionality exposed to have a working SBT with internal MQF nodes in dib-lab/sourmash#772, so how do people feel about:

  1. merging this PR
  2. punting remaining TODOs to issues
  3. add more API/methods over time (in new PRs)

Oh, and also releasing it to crates.io (the Rust package repository)?

@drtamermansour

This comment has been minimized.

Copy link
Member

drtamermansour commented Nov 7, 2019

@luizirber @shokrof This is really going above my head :)
Does this affect the compilation of the MQF or increase the dependencies? Is it better to make the merge to the master or keep it in another branch?

@shokrof

This comment has been minimized.

Copy link
Collaborator

shokrof commented Nov 7, 2019

@luizirber Do we need to add a special target on the cmake to do the rust bindings?

@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 7, 2019

@luizirber @shokrof This is really going above my head :)

Oops, sorry! I suggested merging because it is already a minimum-viable wrapper, and it can be improved over time (instead of trying to make everything perfect now).

Punting to issues and making new PRs is something we do in sourmash, to avoid having huge PRs to review and merge.

Publishing to crates.io is preferred to installing from git in the Rust world, and it is actually a blocker for me updating the sourmash crate, because crates published in crates.io can't have dependencies external to crates.io.

Does this help a bit? =]

Does this affect the compilation of the MQF or increase the dependencies?
There are no changes for the C/C++ code (what is already in this repo).

Is it better to make the merge to the master or keep it in another branch?
I would rather have this merged in master, because keeping feature branches up to date is a bit annoying. Because the Rust wrapper is being checked by CI it also helps catch changes at the C/C++ level that break the wrapper, which is something that will not happen (automatically) if this stays in a feature branch.

@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 7, 2019

@luizirber Do we need to add a special target on the cmake to do the rust bindings?

No, it's all taken care by Cargo (the Rust package manager). You would need a cmake target if you want to create a Rust static library, but that doesn't make much sense for this project. In sourmash I generate a Rust static lib because that's what the Python side sees (including generating a C header to link against the right functions in the Rust static lib, which is something not being used in the project).

@shokrof

This comment has been minimized.

Copy link
Collaborator

shokrof commented Nov 7, 2019

if we have a new version, will Cargo update itself automatically or you have to run some commands?

@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 7, 2019

if we have a new version, will Cargo update itself automatically or you have to run some commands?

You need to bump the version in Cargo.toml, and run cargo publish to update the package registry. I can set up a github action or travis job to do that (if you want)

@shokrof

This comment has been minimized.

Copy link
Collaborator

shokrof commented Nov 7, 2019

it is ok. I got it

@shokrof shokrof merged commit fc5f9d1 into master Nov 7, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@luizirber luizirber deleted the rust_ffi branch Nov 7, 2019
@luizirber

This comment has been minimized.

Copy link
Member Author

luizirber commented Nov 8, 2019

Yay! But it was merged before I added the cargo metadata, please see #5 =]

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.