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.

Sign up for GitHub

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

Jump to bottom

C Api and Rust bindings #44

Merged
merged 19 commits into from
Mar 16, 2018
Merged

C Api and Rust bindings #44

merged 19 commits into from
Mar 16, 2018

Conversation

narekgharibyan
Copy link
Member

  • Keyvi C Api which can be utilized later on to add bindings to other languages (e.g. Go)
  • Keyvi Rust bindings,
  • Rust builds on Travis CI
  • Now style-check doesn't force short functions implemented outside of class to be merged to one line.

@hendrikmuhs This PR is same as cliqz-oss/keyvi#242 with addressed review comments. Also Rust package was renamed from rskeyvi to keyvi.

@narekgharibyan
- version bump for 0.2.6 release
5e97885
@narekgharibyan
- removed external submodules
24319b5
@narekgharibyan
- added missing includes
2cea422
@narekgharibyan
- using size_t instead of int for number_of_results
4eb9e24
@narekgharibyan
- added C Api and keyvi_c library
4ec0879
@narekgharibyan
- added Keyvi Rust bindings, they utilize C Api and keyvi_c library
cb78656
@narekgharibyan
- added tests for Keyvi Rust bindings
e3cea0e
@narekgharibyan
- using strncpy() instead of strcpy()
1c43859
@narekgharibyan
- fixed end state handling in StateTraverser
00f03a3 
- minor refactoring
@narekgharibyan
- added rskeyvi to Travis CI
6e4be66
@narekgharibyan
- using Dictionary::GetFuzzy() for c_api/rust
a37369f
@narekgharibyan
Merge remote-tracking branch 'dev_origin/master' into rust-final
c97b66e
@narekgharibyan
- reverted some minor fixes to prevent style check
9bd6110
@narekgharibyan
- fixed formatting
f060a69 
 - added missing include
 - added clang-format flag AllowShortFunctionsOnASingleLine: Inline
 - added disabled build/include_subdir for cpplint, as we use
   #include "utf8.h"
@narekgharibyan narekgharibyan mentioned this pull request Feb 17, 2018
Merged
@coveralls
Copy link

coveralls commented Feb 17, 2018 •
edited
Loading

Coverage Status

Coverage increased (+0.03%) to 93.517% when pulling 671515c on narekgharibyan:rust-final into 7aeddb3 on KeyviDev:master.

hendrikmuhs
hendrikmuhs reviewed Feb 17, 2018
View reviewed changes
rust/Cargo.toml Outdated
@@ -0,0 +1,14 @@
[package]
name = "keyvi"
version = "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

I get the semantics behind that version number. It would still make sense to have a unified version number.

hendrikmuhs
hendrikmuhs reviewed Feb 17, 2018
View reviewed changes
rust/Cargo.toml Outdated

[build-dependencies]
bindgen = "0.30.0"
cmake = "*"
Copy link
Contributor

Choose a reason for hiding this comment

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

can you say something like > 2.8.12 (that's the minimum right now)?

hendrikmuhs
hendrikmuhs reviewed Feb 17, 2018
View reviewed changes
rust/Cargo.lock Outdated
@@ -0,0 +1,549 @@
[root]
name = "keyvi"
version = "0.0.1"
Copy link
Contributor

Choose a reason for hiding this comment

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

version needs to be set twice (it is set in the ".toml" file below as well)?

hendrikmuhs
hendrikmuhs reviewed Feb 17, 2018
View reviewed changes
rust/Cargo.lock Outdated
"serde_json 1.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
]

[[package]]
Copy link
Contributor

Choose a reason for hiding this comment

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

are these dependencies? Quite a lot, is that correct?

@hendrikmuhs
Copy link
Contributor

Look great!

I wonder a bit about:

  • version number, we should use the same version everywhere, why does it have to be set twice?
  • dependencies, I do not know much about rust, but that looks really strange to me
  • a Readme.md file in the rust folder with some basic getting started documentation would be nice (can be added later)

I would like to merge the index branch first in case of merge trouble. You introduced some style fixes - which are good - but if we merge this first I might have to reformat all files.

@narekgharibyan
- added [dev-dependencies] section
5938df9 
 - changed package version
 - removed Cargo.lock from VCS
@narekgharibyan
Copy link
Member Author

  • version number: Sure, that makes sense! It can be that in the future we will have a bug/fix in bindings part only, which I think will require patch version change and rerelease only for a particular binding. But that should be okay.
  • dependencies: I removed Cargo.lock file from VCS, as it is not needed for libraries, and is required only for executables, also added [dev-dependencies] section for test requirements. Now distribution package is dependent only on 3 packages.
  • Readme.md: Sure, need to add it! Probably along with release on https://crates.io/

Agree, I'll do the review next week, so we can first merge Index and then Rust and make the 0.3 release.

hendrikmuhs
hendrikmuhs approved these changes Mar 15, 2018
View reviewed changes
Copy link
Contributor

@hendrikmuhs hendrikmuhs left a comment

Choose a reason for hiding this comment

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

LGTM

@narekgharibyan
Rust package (#1)
671515c 
*  - fixed rust package build: workaround via symbolic links, so cargo
   will bundle keyvi source folder along with CMakeLists.txt into .crate
   file

*  - building only keyvi_c library, this reduces build time from 61s -> 7s
@narekgharibyan narekgharibyan merged commit 57464f8 into KeyviDev:master Mar 16, 2018
@narekgharibyan narekgharibyan deleted the rust-final branch March 16, 2018 18:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hendrikmuhs hendrikmuhs hendrikmuhs approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@narekgharibyan @coveralls @hendrikmuhs