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

Write plugin in rust #3308

Open
darddan opened this issue Nov 30, 2019 · 10 comments
Labels

Comments

@darddan
Copy link
Contributor

@darddan darddan commented Nov 30, 2019

I've looked at the rust bindings and trying to write a storage plugin. If I'm not overlooking something there's two things stopping me from building it. There's no types for the Plugin class and the from_ptr function in KeySet (elektra) is private.

I've copied elektra-sys to elektra-plugin-sys, changed the wrapper.h to contain only #include "kdbplugin.h" and changed lines 22 and 23 in build.rs to:

        .whitelist_function("(elektraPlugin).*")
        .whitelist_var("(ELEKTRA_PLUGIN|Plugin).*")

With those I was able to create a macro which would generate the needed functions for a plugin (handling the unsafe elektra-plugin-sys code). By using that macro, creating a storage plugin in rust can look like this:

generate_storage_plugin!(
    elektraPluginSymbol: "exampleplugin",
    getFunction: example_plugin_get, // name of function declared below
    setFunction: example_plugin_set, // name of function declared below
    author: "Author Name <author@email.com>",
    license: "MIT",
    needs: "anything", // Optional field, delete if not needed
    recommends: "anything", // Optional field, delete if not needed
    status: "experimental unfinished",
    metadata: "anything", // Optional field, delete if not needed
    version: "1",
    description: r###"
    Reads and writes the example plugin format
    You can add the content of README.md here
    "###,
);

// Type signature needs to be same as here.
// KeySet and StringKey are from the safe bindings `elektra` crate
fn example_plugin_get(_: &mut KeySet, _parent: &StringKey) -> ElektraReturnCode {
    // Parse code here
    ElektraReturnCode::Success
}

// Type signature needs to be same as here.
fn example_plugin_set(_: &KeySet, _parent: &StringKey) -> ElektraReturnCode {
    // Serialize code here
    ElektraReturnCode::NoUpdate
}

Would this be of use?

@darddan darddan added the lang/rust label Nov 30, 2019
@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 30, 2019

Great job, looks nice! But this might not be enough, you also need some ways to set error/warnings, access the handle, ... Maybe @PhilippGackstatter knows more about that.

I think we should focus on making the PR for kconfig now.

@darddan

This comment has been minimized.

Copy link
Contributor Author

@darddan darddan commented Nov 30, 2019

I'm working on the kconfig task, but it takes so long with the CI and the docker images so I've tried this in the meantime

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Nov 30, 2019

Very good idea to do this in the meantime 💯

https://www.explainxkcd.com/wiki/index.php/303:_Compiling

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Nov 30, 2019

Seems like a good idea to me, to use a macro for generating a plugin.

The Rust bindings are only for the low-level API and do not touch the Plugins in any way, that's why there's no Plugin struct.

As I haven't looked at the plugin API yet, I can't give much guidance at the moment, but if you want any help feel free to ask.

KeySet::from_ptr can be made public, since the from_ptr functions on keys are also public.

(Slightly Off-topic)
I'm a bit reluctant to publish a new version of the crate though, since that would put the crate and Elektra versions out of sync (e.g. 0.9.2 crate vs 0.9.1 Elektra), which makes it less obvious that they're compatible. But waiting for the next Elektra release might be too long, so we may have to live with that.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Dec 1, 2019

You could use some subversion number like 0.9.1-1 (this we use for Debian packages and Docker images) for changes that are only relevant to the binding (like making a from_ptr public or updating docu).

If we have some problem that blocks KDE/LCDproc/GSettings progress, @mpranj might also decide to release earlier. But the subversions for bindings will not harm anyway, as it gives you flexibility to fix little things.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Dec 1, 2019

You could use some subversion number

That would be ideal. But crates use semver and according to that 0.9.1 > 0.9.1-1, because the -1 is a pre-release. Maybe it would be better then to do a "pre-release" of 0.9.2 by using 0.9.2-pre.1, 0.9.2-pre.2, ... The pre is debatable, but I haven't seen something like 0.9.2-1 on crates.io.

Then 0.9.2-pre.1is actually greater than 0.9.1. And once 0.9.2 is released it supersedes all the pre-releases.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Dec 2, 2019

Ahh, ok, seems like Docker, Debian and semver handle this differently. Then any other char ~ for which 0.9.1 < 0.9.1~1, I am sure you will find one.

I would not do the pre-releases as long as we did not update Elektra's version. It would be confusing if you get from system/elektra/version/constants something different as the binding has. At the moment @mpranj changes the version in CMakeLists.txt you can start doing pre-releases if you would like to make them.

@PhilippGackstatter

This comment has been minimized.

Copy link
Contributor

@PhilippGackstatter PhilippGackstatter commented Dec 3, 2019

Semver only allows - and +. Versions with - for pre-releases are always smaller than the version without the pre-release. And + is just a build number that doesn't affect the ordering.

I agree that 0.9.2-pre.1 doesn't make the compatibility to 0.9.1 obvious.

Unfortunately I believe the only options are pre-releases and clearly stating the compatible version at the top of the readme, or detaching the version from Elektra's entirely.

@markus2330

This comment has been minimized.

Copy link
Contributor

@markus2330 markus2330 commented Dec 3, 2019

And + is just a build number that doesn't affect the ordering.

There is a difference between ordering and precedence.

Somehow a tool needs to decide which version to pick if several versions (only differing in the build number) are available. So ordering somehow needs to be defined.

I would say if the Cargo implementation does the correct thing (0.9.1 < 0.9.1+1 < 0.9.1+2 ordering), we should simply use the build number for fixes in docu.

At least the semver implementation seems to not ignore the build number (for Cargo I did not check):

See also:
maxhauser/semver#9
maxhauser/semver#15

The only problem with this "abuse" of build number is that someone who already has 0.9.1+1 installed will not get 0.9.1+2. But this should not matter for fixing docu.

KeySet::from_ptr can be made public, since the from_ptr functions on keys are also public.

But for making something public (or also fixing a bug) it makes a difference, as this is a change relevant to semantic versioning precedence. People might want the upgrade even if they already have 0.9.1. So here the build number stops working.

I'm a bit reluctant to publish a new version of the crate though, since that would put the crate and Elektra versions out of sync (e.g. 0.9.2 crate vs 0.9.1 Elektra), which makes it less obvious that they're compatible. But waiting for the next Elektra release might be too long, so we may have to live with that.

I now started a PR with an proposal how to solve it: #3319

It basically says that you can go out of sync with Elektra's patch version. But this is maybe not the best idea as long as we are in the 0. version schema where the patch level could be anything (including major changes).

I agree that 0.9.2-pre.1 doesn't make the compatibility to 0.9.1 obvious.

Yes. If someone needs the KeySet::from_ptr releasing Elektra 0.9.2 just for that makes sense. Releases only take a lot of time if there are huge changes (and release notes). But this is of course to @mpranj to decide. Maybe kconfig, gsettings or LCDproc also need some little patch and there is some synergy?

@mpranj

This comment has been minimized.

Copy link
Member

@mpranj mpranj commented Dec 3, 2019

I agree that having the versions in sync makes the compatibility intuitively obvious, but I would leave the decision up to the maintainer of the binding.

releasing Elektra 0.9.2

I don't have anything against doing releases more often than our somewhat-quarterly release cycle. For me it would depend on the changes. Currently we don't have so much in the changelog that another release makes sense to me. Maybe we could schedule it in ~two weeks if needed. That would give us some time to work on more things for the release.

If there is a real need to do 0.9.2 immediately, ping me and we can release it earlier.

@markus2330 markus2330 mentioned this issue Dec 3, 2019
0 of 16 tasks complete
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.