Use cxx-rs for core.rs #2336

cgwalters · 2020-11-25T19:45:51Z

This is much better than bindgen because it's fully safe. It's
much more ergonomic too:

Invoke Rust methods-on-structs just like C++ methods-on-structs
Rust Result<> is translated automatically to exceptions

See https://cxx.rs/context.html for more.

cgwalters · 2020-11-30T15:34:35Z

Looks like CI fails here because Rust is too old in f32. Hopefully we get coreos/coreos-assembler#1881 going soon.

cgwalters · 2020-11-30T15:48:49Z

src/app/rpmostree-dbus-helpers.cxx

+          glnx_autofd int fd = -1;
+          try {
+            fd = rpmostreecxx::download_to_fd(*pkg);
+          } catch (std::exception& e) {


This one is a really interesting topic. GError is closer to Rust's explicit Result than C++ exceptions are. However...for most of the usage of GError in rpm-ostree we are effectively just using it to unwind up the stack - i.e. we don't explicitly catch errors.

The only exceptions¹ I can think of are:

main() (obviously)

GDBus method handlers

And so what I'd probably argue for is that rather than doing try/catch-to-gerror at places like this we just go with the C++ exception flow, and install toplevel try/catch at those places that need it.

(Also in rpm-ostree the usage of g_error_matches() is something like 90% for ENOENT...which totally validates https://github.com/cgwalters/openat-ext/blob/26324991dd1648bab64ce1f2e6471186e6ed21f9/src/lib.rs#L36 )

¹ 😀

Now that main() has a try/catch, I think can drop this one, right?

cgwalters · 2020-12-01T19:23:14Z

OK I looked at this a bit more. Definitely a whole mixed bag of tradeoffs here.

Idiomatic C++ strings and (not to mention Rust String) do totally clash with our pervasive use of C NUL-terminated strings; see e.g. https://www.murrayc.com/permalink/2017/06/20/c-stdstring_view-not-so-useful-when-calling-c-functions/

We are already using CUtf8Buf, so...we could use that more Rust side. Hmm, and perhaps a viable path here is adding support for that crate into cxx-rs as an optional feature (or perhaps just move it into cxx-rs?).

The other thing I haven't tried here but is probably going to be annoying is supporting passing GObjects.

Hmm. So one option I mentioned in our chat is we could fork cxx and add custom stuff. Since some problem domains here are kind of unique to us it's not totally crazy IMO. Particularly since eventually we wouldn't need it.

cgwalters · 2020-12-03T22:25:05Z

OK right, currently our Prow job is using SKIP_INSTALLDEPS=1 so it was missing the hack to pull in cxxbridge-cmd. But actually...to ship we need to vendor that just like we do bindgen.

cgwalters · 2020-12-03T22:25:50Z

And the Jenkins/CoreOS CI is blocked on coreos/coreos-assembler#1881 because the Fedora team isn't backporting newer Rust to 32, and it looks like cxxbridge gained a dep on very recent Rust for some const fn stuff.

cgwalters · 2020-12-06T20:13:05Z

OK cool after filing dtolnay/cxx#544 I figured out how to pass through GObjects, which I think was the biggest blocker to this.

cgwalters · 2020-12-11T14:35:05Z

OK I reworked this to have a better demo; I converted @lucab 's work in core.rs. Note how all the unsafe wrappers drop out (I replaced that section with unit tests 😄 ) and how we can natively invoke methods from the C++ side on the Rust object that throws a Result.

lucab · 2020-12-11T15:12:07Z

rust/src/core.rs

    /// Remove the temporary /etc, and destroy the guard.
-    pub fn redo_usretc(self) -> anyhow::Result<()> {
+    pub fn undo(&self) -> anyhow::Result<()> {


From an out-of-band chat: this seems to be a cxx limitation, not being able to consume the ownership of the object itself.

jlebon · 2020-12-11T15:18:45Z

rust/src/core.rs

    /// Remove the temporary /etc, and destroy the guard.
-    pub fn redo_usretc(self) -> anyhow::Result<()> {
+    pub fn undo(&self) -> anyhow::Result<()> {


Would be cool if we could we make the undo happen in Drop::drop so we don't have to explicitly call it. This matches some other guards we have in C which used autocleanups to "unguard".

We could do that actually! But I explicitly chose not to because there's no way to check for errors in drop(). (This gets into the whole rationale behind things like https://docs.rs/drop_bomb/0.1.5/drop_bomb/ that we use in https://docs.rs/openat-ext/0.1.9/openat_ext/struct.FileWriter.html )

cgwalters · 2020-12-11T15:19:19Z

src/libpriv/rpmostree-core.cxx

@@ -4576,8 +4575,7 @@ rpmostree_context_assemble (RpmOstreeContext      *self,
    }

  /* Undo the /etc move above */
-  if (!ror_tempetc_redo_usretc (etc_guard, error))
-    return FALSE;
+  etc_guard->undo();


I want to emphasize how representative this example will be I think of future code. Notice that we start using C++ method invocation syntax on a struct, and we use C++ exceptions instead of GError.

Personally I think the first is a notable win (on top of how we didn't need to manually create unsafe C wrappers). I'm less happy with C++ exceptions but I think on balance if we can greatly accelerate oxidation it will be worth the short term hit.

cgwalters · 2020-12-11T18:31:50Z

OK this needs #2381

cgwalters · 2020-12-21T14:49:02Z

OK, lifting draft on this finally!

cgwalters · 2020-12-21T18:29:48Z

OK, fixed the thinko I introduced when pulling the utils.rs code out of another branch; now passing CI!

cgwalters · 2020-12-21T20:57:12Z

src/libpriv/gresources.xml

-    <!-- Generated with: fakeroot /bin/sh -c 'cd dracut-urandom && find . -print0 | sort -z | (mknod dev/random c 1 8 && mknod dev/urandom c 1 9 && cpio -o --null -H newc -R 0:0 --reproducible --quiet -D . -O /tmp/dracut-urandom.cpio)' -->
-    <file>dracut-random.cpio.gz</file>
-  </gresource>
-</gresources>


100% less XML!

cgwalters · 2020-12-22T15:42:11Z

src/libpriv/rpmostree-kernel.cxx

  if (lseek (tmpf.fd, 0, SEEK_END) < 0)
    return glnx_throw_errno_prefix (error, "lseek");
-  if (glnx_loop_write (tmpf.fd, random_cpio_data_p, random_cpio_data_len) < 0)
+  if (glnx_loop_write (tmpf.fd, random_cpio_data.data(), random_cpio_data.length()) < 0)


I know the parenthesis spacing is inconsistent. It's a challenge mentally for me at least because the methods being shared with Rust pulls towards Rust style. And in particular for method invocations there's a strong "binding" between the identifier and its arguments. Most C++ projects don't use a space between method name and open paren; there isn't an offical GNU coding style for C++ AFAICS.

Overall this kind of fits in with a theme of having the C/C++ get a bit uglier in some cases in return for making it way easier to bridge to Rust.

Also, perhaps at some point we can investigate using clang-format.

In the previous buildsytem rework we disabled the unit tests because of linking problems. Now I realized that a simple solution is to continue to build one big object, just make it an internal static library and have a tiny "stub main" that delegates to an entrypoint. That's basically what the C unit tests are - an alternative `main()` with some extra code.

We need to cleanly split off "test dependencies" that we install inside the cosa pod from builds (where we won't have `cargo`) from the build time where we use the cosa buildroot image. Prep for using https://cxx.rs

This is much better than bindgen because it's fully safe. It's much more ergonomic too: - Invoke Rust methods-on-structs just like C++ methods-on-structs - Rust `Result<>` is translated automatically to exceptions See https://cxx.rs/context.html for more.

A step towards converting all of utils.

The way gresources work using a constructor function started failing when I was refactoring the build system, and I couldn't figure it out. It's just easier to use Rust for this which has nice toolchain-integrated functionality for this.

cgwalters · 2020-12-22T22:22:16Z

Rebased 🏄 this one, not because it really requires it, but because I have further branches on top of this and they benefit from the rest of the C++ porting.

jlebon

Cool stuff, approach LGTM overall!

jlebon · 2020-12-22T22:28:30Z

rust/src/lib.rs

@@ -10,11 +10,23 @@
 mod ffiutil;
 mod includes;

+#[cxx::bridge(namespace = "rpmostreecxx")]


Shouldn't the namespace name be e.g. rpmostreers to reflect the fact that it's actually calling into Rust code?

Ah but see it's bidirectional, cxx supports Rust calling C++ too and I just submitted #2413 to demo that.
Concretely anything listed in extern "C++" is expected to be in the target namespace too.

That said it could totally use a better name.

ror:: following the ror_ naming for bindgen?

rpmostree:: - Unimaginative but obvious

Some made up name based on...a bridge or something? skytrain:: ?

OTOH we can also change the name later; there's not a lot of code using this right now. It'll just get slightly more expensive to do so as usage increases.

I looked at this a bit more. It's an interesting topic; partly comes down to whether we see the "cxxrs" part as separate from the C/C++ part. And the core question there is, do we stick everything in the C++ side into the same rpmostree namespace? That'd mean that it'd just be e.g. download_to_fd and not rpmostreecxx::download_to_fd() when called.

This also conceptually would lead towards dropping the C function prefixing, e.g. instead of rpmostree_syscore_bump_mtime() it'd be rpmostree::syscore_bump_mtime(). My feeling right now is that'd be way too much churn; the high level goal here is to sprint towards thinning out the C/C++ side into Rust, not to spend time rewriting into "modern C++". But there will likely be some of the latter.

So I think for now let's just keep rpmostreecxx:: (and not stick any C++ code into a new namespace) because it highlights the C++/Rust-bridged code in our C-in-C++ code which just works differently for now.

Right OK, got it. WDYT about cxxrs::? Anyway, yeah we can revisit later.

Also, definitely agreed re. not trying to make too much use of namespaces outside of cxxrs.

jlebon · 2020-12-22T22:31:36Z

src/app/rpmostree-dbus-helpers.cxx

+          glnx_autofd int fd = -1;
+          try {
+            fd = rpmostreecxx::download_to_fd(*pkg);
+          } catch (std::exception& e) {


Now that main() has a try/catch, I think can drop this one, right?

cgwalters · 2020-12-22T22:44:58Z

Now that main() has a try/catch, I think can drop this one, right?

This one is still necessary to print failed; notice in this update though rather than converting to GError we just re-throw;.

cgwalters · 2020-12-23T00:35:03Z

BTW here's a recent blog on cxx in a KDE project.

jlebon · 2020-12-23T16:10:52Z

/lgtm

openshift-ci-robot · 2020-12-23T16:10:57Z

[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:

~~OWNERS~~ [cgwalters,jlebon]

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

jlebon · 2020-12-23T17:15:50Z

ci/install-extra-builddeps.sh

+# Install build dependencies not in the cosa buildroot already
+set -xeuo pipefail
+if ! command -v cxxbridge; then
+    cargo install --root=/usr cxxbridge-cmd


Does this actually need to be in /usr BTW? Running this script locally.

Hmm, I guess we could dispatch on $(id -u)?

Mershl · 2020-12-28T01:35:03Z

Seeing the following errors on a fresh container and master:

src/app/rpmostree-dbus-helpers.cxx: In function ‘gboolean rpmostree_sort_pkgs_strv(const char* const*, GUnixFDList*, GPtrArray**, GVariant**, GError**)’:
src/app/rpmostree-dbus-helpers.cxx:1069:18: error: ‘rpmostreecxx’ has not been declared
 1069 |             fd = rpmostreecxx::download_to_fd (pkg);
      |                  ^~~~~~~~~~~~
src/app/rpmostree-dbus-helpers.cxx:1070:25: error: ‘exception’ in namespace ‘std’ does not name a type
 1070 |           } catch (std::exception& e) {
      |                         ^~~~~~~~~
make[2]: *** [Makefile:3398: src/app/librpmostreeinternals_la-rpmostree-dbus-helpers.lo] Error 1

Don't know the codebase well enough for (1).
Explicitly including <exception> fixes (2).

EDIT: Fixing (1) yields more errors of the same nature. Is master/or me missing a common include?

src/libpriv/rpmostree-core.cxx: In function ‘gboolean rpmostree_context_assemble(RpmOstreeContext*, GCancellable*, GError**)’:
src/libpriv/rpmostree-core.cxx:4326:20: error: ‘rpmostreecxx’ has not been declared
 4326 |   auto etc_guard = rpmostreecxx::prepare_tempetc_guard (tmprootfs_dfd);
      |                    ^~~~~~~~~~~~
src/libpriv/rpmostree-core.cxx:4372:36: error: ‘rpmostreecxx’ has not been declared
 4372 |           auto systemctl_wrapper = rpmostreecxx::get_systemctl_wrapper ();
      |

cgwalters · 2020-12-29T13:59:44Z

In some cases you may need to make clean after doing an update - the buildsystem doesn't correctly reflect the interdependencies between the C/C++ side and Rust (because if it did every Rust change would trigger a C++ build right now).
Or a more precise change is usually: rm -f rpmostree-rust.h rpmostree-cxxrs*.

Mershl · 2020-12-29T14:21:21Z

Thanks for your suggestion @cgwalters. I've ran make clean before which fixed cxxbridge: command not found. Unfortunatly the main branch is still not building for me, with the errors above.

Commands issued in a F33 build container:

git clone https://github.com/coreos/rpm-ostree.git
cd rpm-ostree/
git submodule update --init
env NOCONFIGURE=1 ./autogen.sh
./configure --prefix=/usr --libdir=/usr/lib64 --sysconfdir=/etc
make clean
make

Side suggestion: Let's add make clean as a suggestion before make to https://coreos.github.io/rpm-ostree/HACKING/#raw-build-instructions. Else cxxbridge: command not found will be visible until cleaned.

openshift-ci-robot added the do-not-merge/work-in-progress label Nov 25, 2020

openshift-ci-robot requested review from ashcrow and miabbott November 25, 2020 19:45

cgwalters marked this pull request as draft November 25, 2020 19:45

openshift-ci-robot added the approved label Nov 25, 2020

cgwalters marked this pull request as ready for review November 27, 2020 17:54

cgwalters force-pushed the cxx-rs branch 2 times, most recently from c816c67 to bd0cd00 Compare November 27, 2020 19:56

cgwalters commented Nov 30, 2020

View reviewed changes

cgwalters force-pushed the cxx-rs branch 2 times, most recently from 9afd13f to 73e7405 Compare December 3, 2020 22:22

cgwalters force-pushed the cxx-rs branch from 73e7405 to 59353ad Compare December 5, 2020 17:54

cgwalters mentioned this pull request Dec 5, 2020

Support for custom translation dtolnay/cxx#544

Open

This was referenced Dec 6, 2020

More C++ porting #2351

Merged

Rework some origin APIs to allocate #2331

Closed

libpriv/passwd: move passwd database to Rust #2348

Merged

lucab mentioned this pull request Dec 8, 2020

libpriv/passwd: move RPM layering logic to Rust #2362

Merged

cgwalters force-pushed the cxx-rs branch from 59353ad to 175cc1e Compare December 11, 2020 14:33

lucab reviewed Dec 11, 2020

View reviewed changes

jlebon reviewed Dec 11, 2020

View reviewed changes

cgwalters commented Dec 11, 2020

View reviewed changes

cgwalters force-pushed the cxx-rs branch from 175cc1e to 2d55430 Compare December 11, 2020 19:53

cgwalters changed the title ~~WIP demo of using cxx-rs~~ Use cxx-rs for core.rs Dec 11, 2020

openshift-ci-robot removed the do-not-merge/work-in-progress label Dec 21, 2020

cgwalters force-pushed the cxx-rs branch from 3b4914f to 1da742d Compare December 21, 2020 16:00

cgwalters force-pushed the cxx-rs branch from 1da742d to 4151a5a Compare December 21, 2020 20:53

cgwalters mentioned this pull request Dec 21, 2020

buildsys: Make one big rpmostreeinternals.la, enable unit tests again #2403

Closed

cgwalters commented Dec 21, 2020

View reviewed changes

cgwalters force-pushed the cxx-rs branch from 4151a5a to 35f51b6 Compare December 22, 2020 15:18

cgwalters commented Dec 22, 2020

View reviewed changes

cgwalters force-pushed the cxx-rs branch from 35f51b6 to e2abce9 Compare December 22, 2020 15:54

cgwalters added 5 commits December 22, 2020 22:20

ci: Introduce install-extra-builddeps.sh

1b7efe7

We need to cleanly split off "test dependencies" that we install inside the cosa pod from builds (where we won't have `cargo`) from the build time where we use the cosa buildroot image. Prep for using https://cxx.rs

Use cxx-rs for core.rs

cc6e2e2

This is much better than bindgen because it's fully safe. It's much more ergonomic too: - Invoke Rust methods-on-structs just like C++ methods-on-structs - Rust `Result<>` is translated automatically to exceptions See https://cxx.rs/context.html for more.

Use cxx-rs for utils.rs download_to_fd

b52c83f

A step towards converting all of utils.

Use cxx-rs instead of gresources

56012db

The way gresources work using a constructor function started failing when I was refactoring the build system, and I couldn't figure it out. It's just easier to use Rust for this which has nice toolchain-integrated functionality for this.

cgwalters force-pushed the cxx-rs branch from e2abce9 to 56012db Compare December 22, 2020 22:21

jlebon approved these changes Dec 22, 2020

View reviewed changes

openshift-ci-robot assigned jlebon Dec 23, 2020

openshift-ci-robot added the lgtm label Dec 23, 2020

openshift-merge-robot merged commit 6579ab7 into coreos:master Dec 23, 2020

jlebon reviewed Dec 23, 2020

View reviewed changes

cgwalters added the project/c++ Porting to C++ label Mar 21, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use cxx-rs for core.rs #2336

Use cxx-rs for core.rs #2336

cgwalters commented Nov 25, 2020 •

edited

Loading

cgwalters commented Nov 30, 2020

cgwalters Nov 30, 2020 •

edited

Loading

jlebon Dec 22, 2020

cgwalters commented Dec 1, 2020

cgwalters commented Dec 3, 2020

cgwalters commented Dec 3, 2020

cgwalters commented Dec 6, 2020

cgwalters commented Dec 11, 2020

lucab Dec 11, 2020

jlebon Dec 11, 2020

cgwalters Dec 11, 2020

cgwalters Dec 11, 2020 •

edited

Loading

cgwalters commented Dec 11, 2020

cgwalters commented Dec 21, 2020

cgwalters commented Dec 21, 2020

cgwalters Dec 21, 2020

cgwalters Dec 22, 2020 •

edited

Loading

cgwalters commented Dec 22, 2020

jlebon left a comment

jlebon Dec 22, 2020

cgwalters Dec 22, 2020 •

edited

Loading

cgwalters Dec 22, 2020

cgwalters Dec 23, 2020

jlebon Dec 23, 2020

jlebon Dec 22, 2020

cgwalters commented Dec 22, 2020 •

edited

Loading

cgwalters commented Dec 23, 2020

jlebon commented Dec 23, 2020

openshift-ci-robot commented Dec 23, 2020

jlebon Dec 23, 2020

cgwalters Dec 23, 2020 •

edited

Loading

Mershl commented Dec 28, 2020 •

edited

Loading

cgwalters commented Dec 29, 2020

Mershl commented Dec 29, 2020 •

edited

Loading

Use cxx-rs for core.rs #2336

Use cxx-rs for core.rs #2336

Conversation

cgwalters commented Nov 25, 2020 • edited Loading

cgwalters commented Nov 30, 2020

cgwalters Nov 30, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Dec 1, 2020

cgwalters commented Dec 3, 2020

cgwalters commented Dec 3, 2020

cgwalters commented Dec 6, 2020

cgwalters commented Dec 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters Dec 11, 2020 • edited Loading

Choose a reason for hiding this comment

cgwalters commented Dec 11, 2020

cgwalters commented Dec 21, 2020

cgwalters commented Dec 21, 2020

Choose a reason for hiding this comment

cgwalters Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

cgwalters commented Dec 22, 2020

jlebon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters Dec 22, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Dec 22, 2020 • edited Loading

cgwalters commented Dec 23, 2020

jlebon commented Dec 23, 2020

openshift-ci-robot commented Dec 23, 2020

Choose a reason for hiding this comment

cgwalters Dec 23, 2020 • edited Loading

Choose a reason for hiding this comment

Mershl commented Dec 28, 2020 • edited Loading

cgwalters commented Dec 29, 2020

Mershl commented Dec 29, 2020 • edited Loading

cgwalters commented Nov 25, 2020 •

edited

Loading

cgwalters Nov 30, 2020 •

edited

Loading

cgwalters Dec 11, 2020 •

edited

Loading

cgwalters Dec 22, 2020 •

edited

Loading

cgwalters Dec 22, 2020 •

edited

Loading

cgwalters commented Dec 22, 2020 •

edited

Loading

cgwalters Dec 23, 2020 •

edited

Loading

Mershl commented Dec 28, 2020 •

edited

Loading

Mershl commented Dec 29, 2020 •

edited

Loading