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

Add more client Rust bindings, port apply-live builtin #2629

Merged
merged 4 commits into from
Mar 4, 2021

Conversation

cgwalters
Copy link
Member

@cgwalters cgwalters commented Mar 2, 2021

app: Rename dbus-helpers to clientlib

Originally it was dbus helpers, but it grew into "shared code for client",
so let's name the file better.


app: Factor out a helper function to register as a client

Prep for exposing just this functionality via cxxrs so we
can more easily write CLI entrypoints in Rust.


Add basic Rust CLI bindings for DBus

This stubs out sufficient infrastructure for us to register
as a client and call the Moo API.

A glaring problem here is the lack of extensive glib::Variant
bindings; that's covered in the next gtk-rs release.

My real goal was to try porting the rpmostree-builtin-apply-live.cxx
code entirely to Rust, but there's more to do to expose the
transaction helper APIs we have.


Add more client Rust bindings, port apply-live builtin

This adds sufficient infrastructure to fully port the
rpmostree-builtin-applylive.cxx client code to Rust.
We just keep a stub entrypoint for now until we port
the rest of rpmostree-builtin-ex.cxx, at which point
a lot of C++ files go away.

The "finish" bits move from the daemon-oriented live.rs
into a new rust/src/builtins directory. I'd like
to try to more cleanly split up the Rust sources along
core(shared)/client/daemon directories in the future.


@openshift-ci-robot
Copy link
Collaborator

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@cgwalters
Copy link
Member Author

OK right and I forgot I'd already experimented with better GVariant bindings; there are some in gtk-rs git from gtk-rs/glib#651 but not in a release yet, and even then the version of gtk-rs is exposed by higher level bindings like ostree-rs that we need to also update, etc.

@cgwalters cgwalters marked this pull request as ready for review March 2, 2021 15:32
@cgwalters cgwalters changed the title WIP: demonstrate Rust CLI speaking DBus Add basic Rust CLI bindings for DBus Mar 2, 2021
@cgwalters cgwalters changed the title Add basic Rust CLI bindings for DBus Add more client Rust bindings, port apply-live builtin Mar 2, 2021
@cgwalters
Copy link
Member Author

OK pushed another commit here that fully pushes over the apply-live builtin bits to Rust - there's a lot of back and forth calling with C++ since we have nontrivial bits around connecting and printing transaction progress, etc.

But this shows how we can start writing new CLI entrypoints in Rust to start (with awesome stuff like structopt etc.) and hopefully slowly port over more of the existing C/C++ entrypoints.

@cgwalters cgwalters force-pushed the client-rs branch 2 times, most recently from 34dfb3f to c876588 Compare March 3, 2021 13:17
@cgwalters cgwalters closed this Mar 3, 2021
@cgwalters cgwalters reopened this Mar 3, 2021
Copy link
Member

@jlebon jlebon left a comment

Choose a reason for hiding this comment

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

Cool stuff!

)?;
// Today the daemon mode basically requires running inside a booted
// deployment. Technically, in the future our architecture could
// support
Copy link
Member

Choose a reason for hiding this comment

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

This sentence looks

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree that

rust/src/builtins/apply_live.rs Outdated Show resolved Hide resolved
src/app/rpmostree-clientlib.cxx Outdated Show resolved Hide resolved
Comment on lines 54 to 59
let params = unsafe {
let args = args.to_glib_none().0;
let r = glib_sys::g_variant_new_tuple(&args as *const *mut _, 1);
glib_sys::g_variant_ref_sink(r);
from_glib_full(r)
};
Copy link
Member

Choose a reason for hiding this comment

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

Might be worth a wrapper function for this bit.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK yeah, definitely. I added a variant_utils.rs.

@cgwalters cgwalters force-pushed the client-rs branch 2 times, most recently from 276dcf1 to 3234b74 Compare March 3, 2021 21:47
Originally it was dbus helpers, but it grew into "shared code for client",
so let's name the file better.
Prep for exposing just this functionality via cxxrs so we
can more easily write CLI entrypoints in Rust.
This stubs out sufficient infrastructure for us to register
as a client and call the Moo API.

A glaring problem here is the lack of extensive `glib::Variant`
bindings; that's covered in the next gtk-rs release.

My real goal was to try porting the `rpmostree-builtin-apply-live.cxx`
code entirely to Rust, but there's more to do to expose the
transaction helper APIs we have.
@cgwalters
Copy link
Member Author

Rebased 🏄

This adds sufficient infrastructure to fully port the
`rpmostree-builtin-applylive.cxx` client code to Rust.
We just keep a stub entrypoint for now until we port
the rest of `rpmostree-builtin-ex.cxx`, at which point
a lot of C++ files go away.

The "finish" bits move from the daemon-oriented `live.rs`
into a new `rust/src/builtins` directory.  I'd like
to try to more cleanly split up the Rust sources along
core(shared)/client/daemon directories in the future.
@jlebon
Copy link
Member

jlebon commented Mar 4, 2021

/lgtm

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 13804d8 into coreos:master Mar 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants