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
Rust Bindings #2730
Comments
I don't know that much about Rust, but I suppose that the bindings generated by AFAIK most Rust binding have one |
Yes, this is the plan. And while doing so, also compare with the C API and maybe find improvements in the C API (at least in the docu). @PhilippGackstatter As discussed: Please also find out how to upload to https://crates.io/ and how to integrate the binding in our CMake system. |
@PhilippGackstatter any progress? We have someone who might also be interested in extending the Rust bindings. |
@markus2330 I started a couple of days ago, but I was mostly reading up on bindgen, cmake and how to integrate it into the project, so there was nothing to show. But I have the first couple of things ready now (see #2826). I'm fully working on the project now. |
To answer the questions from #2826 (please prefer asking questions in issues as PRs have the tendency to get confusing for discussions not directly related to the code):
No, the low-level API is only in kdb.h
Yes, you need to change the docker scripts and maybe also the Jenkinsfile. But you do not need to change all of them, if you build for recent distributions it is enough. It would be nice if you could also make it compile with the native rust compiled bundled in Debian Buster. The Debian Buster docker file is not yet merged #2819
There are some ideas to do this: #730 |
Regeneration on every build seems to be the proper solution, the other bindings also work that way (they require swig to be installed). You can simply install the generated header files to avoid the troubles you are describing. |
So far, I've found some minor opportunities for improvement
Another question: |
Great work, from your questions it is obvious that you already looked deeply in the API.
Maybe you can even use the typesystem to avoid wrong calls? (keyGetBinary is only allowed on binary keys)
The reason was compatibility: the APIs initially only returned -1 and it is not possible to add other error codes without breaking existing programs (which might have Do you want to fix these API problems?
The APIs should not differ in spelling. If we fix it, we should fix it in the C API and all bindings (actually only Java and Go needs to be adapted by hand, the others will be regenerated correctly anyway).
Yes, as you maybe already have noticed keyIs(Direct)Below and keyRel have overlapping functionality. The idea of keyRel was to keep the API small (and thus the library small). But keyRel as-is is not usable and also slow. So we will most likely remove it within 0.9. See doc/todo/FUTURE for other candidates to be removed. |
That's a great idea. I could have
I can do that. Depends on what the priority should be for me. After finishing the safe API for Rust, you said it'd be nice to also have the plugin API in Rust. You can decide what is more important. |
Thank you. It might require too many casts, so let us see if it is a good idea. Also another type might be useful for keys in keysets (where setName is not allowed).
Yes, first finish the Rust API from kdb.h and then we'll see how many more hours we have to spend. |
IMO the Rust binding (and any binding for that matter) should have two versions. One that mirrors the C API as close as possible and another that is more idiomatic for the language which builds on the first version. In the idiomatic version utilising the type system with |
@kodebach I agree. And this seems to be also done with the elektra and elektra-sys crates. |
Yes I think so too. If the safe Rust API has limitations that need to be worked around one can import |
It worked out great for the key implementation. However for KeySets, I've hit a road block. For any methods that returns a Key it has to conform to the common interface that I've created. I've implemented a method So I could unify it to bytes, but then the user has to convert to string himself. Since the only difference of StringKey and BinaryKeys are their I guess the real problem is that KeySet in the current implementation is not explicit about what kind of keys it contains, but the *keys are. But allowing an instance of KeySet to only contain StringKey or BinaryKey is too big of a restriction I suppose. |
From the usability point of view it would make sense to return the Key which has a getter for a String, as this is the most often used variant. Setter for the name should be disabled (if easily possible), as we already know that this key (returned by next) is part of a KeySet. In general, the type system should support the user, not get in its way. So keep it as simple as possible. The most common errors are:
So if the type system can help there, it would be great. 5. will hopefully not possible by design, for 1. and 2. your abstraction should help. The binary/string confusion is actually a rather rare error (because binary keys are very untypical: there are mostly used to hold function pointers). Btw. if you want to write a design decision about safe uses of APIs, please go ahead (doc/decision) |
But not every byte sequence is valid UTF-8 so that wouldn't really be typesafe anymore, would it? AFAIK the macro system in Rust is very powerful, maybe there is a way to write a function that always returns the correct type. For example, in Kotlin there is a technique for maps to encode the value type in the key. The API reference over here is an example. Alternatively a |
But it's still possible, although rare, to have a KeySet with mixed keys. Then always returning a StringKey and calling Before doing that, I suggest making KeySet generic and instantiating it as
I think the only thing I can do is to promote usage of duplicate rather than ref count. It might be worse for performance, but keyDel is invoked automatically by Rust, while ref counting is fully manual. So duplication is certainly easier to get right than ref counting.
Do you mean modifying the keyset while iterating over it?
What would be the content of that?
I think that the "current" design of KeySet that can contain anything and concrete Key types doesn't work together, at least not nicely. But I'll look into macros. |
Neither Elektra's strings or binary value need to be UTF-8. Elektra only decides between string and binary (may contain 0 bytes).
We also need to watch out to put effort in features that are useful. Binary keys are rare.
Yes, but I would see StringKeySet as the normal KeySet.
Yes, but as said this is a minor problem. People who store binary data (like function addresses) will find out how to cast the Key (if there is some docu about it).
It would be only fake safety because KeySets come from KDB (externally) and they might contain binary data in any case. Thus I prefer to have KeySet non-generic. If you want to play around with generics, provide getters and setters which convert KeySets to (generic) data structures. E.g. an Elektra array of integers to
Yes
A summary of what we discuss here and how you designed it.
I agree.
Please do not put priority to it. |
Then we should use |
Right now I'm converting between Rust's |
I agree that it is best to return the languages' most common String type. Non-UTF8 strings should be rare (maybe even more rare than binaries). |
I'm trying to figure out the best way to handle the Rust -> C direction, going from UTF-8 to a C-string. UTF-8 strings are allowed to contain zero bytes, but the only codepoint where that appears is the NUL character, not otherwise. I think it'd be reasonable to state this as a precondition in the binding documentation, that the strings are not allowed to contain the zero byte. If it does anyway, the code panics at that point. The other possibility is to return an error from all the set functions that take a string. But then users have to deal with this
|
Yes, is reasonable.
Yes, it makes sense to panic if a malloc failed. (In the case of Rust as the stdlib would do the same. In C the stdlib does not abort, so C-Elektra also does not abort). |
Now that the Rust bindings are merged into master, I would like to publish them to crates.io. |
Publishing on https://crates.io requires a GitHub account. Ownership of crates can be transferred between accounts, so I could use my own for now. Or someone else could login and send me the API token that is required to publish. |
Thank you for publishing them to crates.io ❇️ I would tie the version directly to Elektra, as they are part of Elektra's repos. But it is not really important: if crates.io usually has some specific version schemata, better stick to what is common there.
I logged in an authorized. I'll send you the API token. |
The major problem with this is, according to semantic versioning, if we have to do a breaking change in the bindings we can't just upgrade the version accordingly. So we could only do breaking changes when elektra does them. |
According to semantic versioning you can do any breaking change as long as the version starts with |
You're right, I didn't think of increasing the major versions later. Right now, the |
Yes, because there is another library (I think from Kerberos) that uses For now |
Per default it is /usr/local/include/elektra but most distributions will use /usr/include/elektra but there is no guarantee. That's why build systems usually have some support to locate header files. Elektra supports cmake and pkg-config. Can you give some context about where you need this?
must be used instead. The relevant PR is #2880 |
Changing to |
It probably won't work for all headers, some rely on Probably the best thing to do (if possible for you) would be using |
So if a user compiles elektra on his machine, then he only needs rust/cargo and can use the bindings. But the other use case, which crates.io should be used for, is if someone installs elektra (and headers) via their package manager. Then the library and headers are available. Now that user includes elektra in their dependencies and cargo will fetch the
I can try to add pkg-config or cmake as a build-dependency and find |
Yes, you can try to call pkg-config in your build script. If pkg-config is not available, you can try hard-coded paths like /usr/include/elektra and /usr/local/include/elektra. (If crates.io does not require pkg-config to be available.) |
You could try this crate |
I added The bindings are now published: elektra and elektra-sys 😃 |
Due to the missing system dependency of libelektra in the docs.rs build environment, the documentation didn't build. Additionally, they will change the build environment on September 30th. I think after #2980 is merged, this issue can be closed. |
Very nice, they reacted super-fast. Will it be a problem that they build it with a quite old libelektra? (I did not check which version but if they include it from the package manager it will be definitely older than 0.9.) |
No problems for the |
Can you then add the links to the docu within our repo? |
It seems like that elektra-sys is nearly unusable anyway. It shows hundreds of symbols which have nothing/little to do with Elektra. Furthermore there is no docu to the individual functions, e.g. https://docs.rs/elektra-sys/0.9.0/elektra_sys/fn.keyString.html |
Yes, I'll add it to the existing PR
I will look into this.
It's typical for -sys crates to not have docu (for example openssl-sys), because they are one-to-one translations of the C equivalent. So one has to look up the C doc directly. I'd also have to hand copy all the docu, which adds another maintenance burden. I can link to https://doc.libelektra.org/api/current/html/index.html on the main docu page though. |
It's fixed in #2980, and will be fixed on docs.rs the next time we publish the crate. |
Yes, good idea! |
Starting roughly in the middle of July, I would like to implement Rust bindings for Elektra.
I think that rust-bindgen should be able to automatically generate (some or all of) the bindings. I still expect there to be quite a bit of manual work to get them working properly. From my current understanding and from @kodebach's comment, this will result in a
elektra-sys
crate.Once it is working I'll add a safe API in Rust, so that it can be used in regular Rust without the need to call unsafe code. This will then be the
elektra
crate.I'll then make sure that they are correct by testing with cargo tests.
The typical way to document crates is with comments in the code. docs.rs will automatically build the documentation and make it publicly available, so I think doing documentation this way makes the most sense.
To publish the crate to crates.io, an account with an API token is needed. As discussed with @markus2330, this account should be part of ElektraInitiative such that it is accessible to future maintainers.
I'll look at the CMake integration at the start of the project, since I'm not familiar with CMake at the moment.
Is there anything else I should add?
The text was updated successfully, but these errors were encountered: