🦀 Add support for YAML treefiles #1377

jlebon · 2018-05-30T21:47:23Z

Let's modernize and start supporting YAML treefiles. I'll dare make the
sweeping generalization that most people would prefer reading and
writing YAML over JSON.

This takes bits from coreos-assembler[1] that know how to serialize a
YAML file and spit it back out as a JSON and makes it into a shared lib
that we can link against. We could use this eventually for JSON inputs
as well to force a validation check before composing.

If we go this route, we could then turn on --enable-rust in FAHC for
now and drop the duplicate code in coreos-assembler.

[1] https://github.com/cgwalters/coreos-assembler

jlebon · 2018-05-30T21:49:54Z

One cool side-effect of the way this is implemented here is that we get implicit support for include: keys. Actually, a YAML could include: a JSON and vice-versa, which might be a nice property for interactions in build systems.

I did have to add a few skip_serializing_if for it to work.

cgwalters · 2018-05-31T14:10:43Z

rust/src/treefile.rs

+
+    match treefile_read_impl(filename_path, output_file) {
+        Err(e) => {
+            // just write it out to stderr for now


A whole world I haven't looked at is the glib-rs bits. If we were to do this more seriously we'd probably need to pull that in.

I spent some time trying to figure out how to use this, but it seems like these bindings are more meant for Rust to interact with glib rather than Rust pretending to be glib. E.g. there's an Error::wrap() method for wrapping GErrors received from glib, but I don't know how to create an Error::new() and pass it back as an unowned GError to rpm-ostree.

@jlebon I think you are looking for https://docs.rs/glib-sys/0.6.0/glib_sys/struct.GError.html, which can be directly built by consumers (I am not familiar with glib, so I don't know who is supposed to own message). That said I think you don't want to pull all of glib-rs in here (as it will likely create linkage issues with its build.rs), but only some ctypes definitions for exchange at FFI border.

@lucab Thanks for the pointer. I finally got this working by just directly using the glib-sys binding for g_error_new_literal. This works around the main hurdle of somehow having to pass ownership of an allocated string on the Rust side to C to be freed by free, which AFAICT is tricky business. OTOH, g_error_new_literal just does an strdup.

I made this into a trait to make it extra easy to use! :)

cgwalters · 2018-05-31T14:10:53Z

rust/src/treefile.rs

+    match treefile_read_impl(filename_path, output_file) {
+        Err(e) => {
+            // just write it out to stderr for now
+            let _ = writeln!(io::stderr(), "treefile error: {}", e);


eprintln! ?

cgwalters · 2018-05-31T14:12:28Z

src/app/rpmostree-compose-builtin-tree.c

@@ -89,6 +97,9 @@ static GOptionEntry install_option_entries[] = {
  { "touch-if-changed", 0, 0, G_OPTION_ARG_STRING, &opt_touch_if_changed, "Update the modification time on FILE if a new commit was created", "FILE" },
  { "workdir", 0, 0, G_OPTION_ARG_STRING, &opt_workdir, "Working directory", "WORKDIR" },
  { "workdir-tmpfs", 0, G_OPTION_FLAG_HIDDEN, G_OPTION_ARG_NONE, &opt_workdir_tmpfs, "Use tmpfs for working state", NULL },
+#ifdef HAVE_RUST
+  { "yaml-manifest", 0, 0, G_OPTION_ARG_NONE, &opt_yaml_manifest, "Assume the manifest is in YAML format", NULL },


I think I'd vote we do this automatically if the file ends in .yaml, and require that extension just for simplicity. If we wanted something a bit more robust we could try GLib's MIME sniffing.

(I see you did this below...kind of uncertain about the need for an option, but eh.)

cgwalters · 2018-05-31T14:14:11Z

src/app/rpmostree-compose-builtin-tree.c

+  g_auto(GLnxTmpfile) json_contents = { 0, };
+  if (opt_yaml_manifest ||
+      g_str_has_suffix (treefile_path, ".yaml") ||
+      g_str_has_suffix (treefile_path, ".yml"))


Let's detect this outside the conditional, and throw a clear error #ifndef HAVE_RUST.

cgwalters · 2018-05-31T14:15:44Z

src/app/rpmostree-compose-builtin-tree.c

+        return glnx_throw (error, "Failed to load YAML treefile %s", treefile_path);
+
+      /* or just lseek back to 0 and use json_parser_load_from_data here? */
+      treefile_path = fdpath = g_strdup_printf ("/proc/self/fd/%d", json_contents.fd);


That won't correctly reference external files like the passwd and treecompose-post.sh bits. This is why the current assembler code does the tmpdir/copy dance.

(Shades of Dockerfile+docker build+"contextdir" here)

It still works because as far as the rest of the code is concerned, the treefile path is still the JSON one. We're only passing the /proc/self path to json-glib when parsing.

cgwalters · 2018-05-31T14:16:49Z

rust/Cargo.lock

@@ -0,0 +1,204 @@
+[[package]]


I think opinion is split on this somewhat, but I personally don't like shipping Cargo.lock in git. Do pinning when we hit breakage (and ideally submit patches), rather than the constant maintenance overhead of bumping it.

lucab · 2018-06-01T12:39:02Z

rust/src/treefile.rs

+}
+
+#[derive(Serialize, Deserialize, Debug)]
+pub struct TreeComposeConfig {


If this is a human format (as YAML suggests) and newly introduced, I'd suggest to version this with a "kind+value" tag as it will make future breaking changes easier to handle.

Ooh that's beautiful. Indeed that'd be useful here. That said the plan is to deprecate this specific functionality entirely in favor of sysusers.

lucab · 2018-06-01T12:45:40Z

rust/src/treefile.rs

+fn treefile_read_impl(filename: &Path, output: fs::File) -> io::Result<()> {
+    let f = fs::File::open(filename)?;
+
+    let mut treefile: TreeComposeConfig = match serde_yaml::from_reader(f) {


You'll likely want to wrap f in a std::io::BufReader for serde_yaml.

jlebon · 2018-06-01T21:28:20Z

This still needs some sanity test!

jlebon · 2018-06-04T13:58:51Z

Now with proper error handling and a test! ⬆️ (And a pass of rustfmt).

cgwalters · 2018-06-04T14:45:36Z

I pushed a fixup to do the same thing as ostreedev/ostree@1f83259 - out of curiosity how are you installing cargo? I'm using the Fedora packages.

jlebon · 2018-06-04T14:55:09Z

Ahh thanks. Odd I wasn't seeing this locally.
I switched to using rustup recently to play with nightly more easily, though I was using stable for this.

cgwalters · 2018-06-04T15:36:12Z

OK what do you think about this:

From d0d95645e220ca56e381278c44facbb6ad9009f3 Mon Sep 17 00:00:00 2001
From: Colin Walters <walters@verbum.org>
Date: Mon, 4 Jun 2018 11:32:16 -0400
Subject: [PATCH] wip

---
 rust/src/treefile.rs | 53 +++++++++++++++++++++-----------------------
 1 file changed, 25 insertions(+), 28 deletions(-)

diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs
index 24fdfd48..9080be98 100644
--- a/rust/src/treefile.rs
+++ b/rust/src/treefile.rs
@@ -12,15 +12,11 @@ extern crate serde;
 extern crate serde_json;
 extern crate serde_yaml;
 
-use std::ffi::CStr;
-use std::ffi::CString;
-use std::ffi::OsStr;
-use std::fmt::Display;
-use std::fs;
-use std::io;
+use std::ffi::{CStr, CString, OsStr};
 use std::os::unix::ffi::OsStrExt;
 use std::os::unix::io::FromRawFd;
 use std::path::Path;
+use std::{fs, io, ptr};
 
 #[no_mangle]
 pub extern "C" fn treefile_read(
@@ -36,10 +32,7 @@ pub extern "C" fn treefile_read(
     let c_str: &CStr = unsafe { CStr::from_ptr(filename) };
     let filename_path = Path::new(OsStr::from_bytes(c_str.to_bytes()));
 
-    match treefile_read_impl(filename_path, output_file) {
-        Err(e) => error.propagate(e),
-        _ => 1,
-    }
+    treefile_read_impl(filename_path, output_file).map_gerr(error)
 }
 
 fn treefile_read_impl(filename: &Path, output: fs::File) -> io::Result<()> {
@@ -75,28 +68,32 @@ fn whitespace_split_packages(pkgs: &Vec<String>) -> Vec<String> {
     return ret;
 }
 
-// This is like the Rust -> C version of `g_propagate_error`.
-// https://developer.gnome.org/glib/stable/glib-Error-Reporting.html#g-propagate-error
-trait ErrorPropagate {
-    fn propagate<E: Display>(&self, e: E) -> libc::c_int;
+// With this, you can just add .map_gerr(error) at the end of functions that
+// return a Result.
+trait RustToGError {
+    fn map_gerr(self: Self, error: *mut *mut glib_sys::GError) -> libc::c_int;
 }
 
-impl ErrorPropagate for *mut *mut glib_sys::GError {
-    fn propagate<E: Display>(&self, e: E) -> libc::c_int {
-        if !(*self).is_null() {
-            let c_msg = CString::new(format!("{}", e)).unwrap().into_raw();
-            unsafe {
-                // XXX: should check for overwriting
-                **self = glib_sys::g_error_new_literal(
-                    gio_sys::g_io_error_quark(),
-                    gio_sys::G_IO_ERROR_FAILED,
-                    c_msg,
-                );
-                // recapture and let fall out
-                let _ = CString::from_raw(c_msg);
+impl<T, E> RustToGError for Result<T, E>
+where
+    E: std::error::Error,
+{
+    fn map_gerr(self: Result<T, E>, error: *mut *mut glib_sys::GError) -> libc::c_int {
+        match &self {
+            &Ok(_) => 1,
+            &Err(ref e) => {
+                unsafe {
+                    assert!(*error == ptr::null_mut());
+                    let c_msg = CString::new(e.description()).unwrap().into_raw();
+                    *error = glib_sys::g_error_new_literal(
+                        gio_sys::g_io_error_quark(),
+                        gio_sys::G_IO_ERROR_FAILED,
+                        c_msg,
+                    )
+                };
+                0
             }
         }
-        0 // for convenience as return/final expressions
     }
 }
 
-- 
2.17.0

jlebon · 2018-06-04T15:51:32Z

Yeah, that looks nice to me!
Though I think you still need the CString::from_raw() bit so we don't leak it.

cgwalters · 2018-06-04T16:59:56Z

Isn't this simpler? Reference

diff --git a/rust/src/treefile.rs b/rust/src/treefile.rs
index 9080be98..35878692 100644
--- a/rust/src/treefile.rs
+++ b/rust/src/treefile.rs
@@ -84,12 +84,12 @@ where
             &Err(ref e) => {
                 unsafe {
                     assert!(*error == ptr::null_mut());
-                    let c_msg = CString::new(e.description()).unwrap().into_raw();
+                    let c_msg = CString::new(e.description()).unwrap();
                     *error = glib_sys::g_error_new_literal(
                         gio_sys::g_io_error_quark(),
                         gio_sys::G_IO_ERROR_FAILED,
-                        c_msg,
-                    )
+                        c_msg.as_ptr(),
+                    );
                 };
                 0
             }

Let's modernize and start supporting YAML treefiles. I'll dare make the sweeping generalization that most people would prefer reading and writing YAML over JSON. This takes bits from coreos-assembler[1] that know how to serialize a YAML file and spit it back out as a JSON and makes it into a shared lib that we can link against. We could use this eventually for JSON inputs as well to force a validation check before composing. If we go this route, we could then turn on `--enable-rust` in FAHC for now and drop the duplicate code in coreos-assembler. [1] https://github.com/cgwalters/coreos-assembler

cgwalters · 2018-06-04T17:39:40Z

OK I squashed in that, and changed the commit message to drop the WIP. I also split out a glibutils.rs module, though I'm sure something like this exists in the ecosystem, we'll have to do some learning here.

jlebon · 2018-06-04T18:26:08Z

Oh yeah, of course. I think I was so focused on trying to use CString.into_raw() to return a dynamic string to rpm-ostree during experimentations that I forgot to back out of it when I later switched to g_error_new_literal().

I like the new approach. My only bikeshed would be s/to_gerr/to_gerror/ but that's minor.

@lucab mentioned earlier some concerns with double linking, but AFAICT playing with ldd, this isn't an issue.

Anyway, I'll let you do the r+!

cgwalters · 2018-06-04T18:29:25Z

My only bikeshed would be s/to_gerr/to_gerror/

I thought about that actually on the drive to lunch. If I had to spell it out, it's really to_gerror_convention - we're not necessarily making a GError instance right? I don't mind spelling it out, at least in Emacs dabbrev-expand is OK for that type of stuff.

jlebon · 2018-06-04T19:15:43Z

WDYT of ⬆️? (I.e. to_glib_convention).

cgwalters · 2018-06-04T19:18:50Z

Oh man I was literally writing the same thing at the same time!

My git diff - want to squash this into yours?

diff --git a/rust/src/glibutils.rs b/rust/src/glibutils.rs
index 1926696c..0bc9f447 100644
--- a/rust/src/glibutils.rs
+++ b/rust/src/glibutils.rs
@@ -27,12 +27,15 @@ use std;
 use std::ffi::CString;
 use std::ptr;
 
-// With this, you can just add .map_gerr(error) at the end of functions that
-// return a Result.
+// Consume a Result into the "GError convention":
+// https://developer.gnome.org/glib/stable/glib-Error-Reporting.html
+// To use, just add .to_gerror_convention(error) at the end of functions that
+// return a Result (using the std Error).
 pub trait ToGErrorConventions {
     fn to_gerror_convention(self: Self, error: *mut *mut glib_sys::GError) -> libc::c_int;
 }
 
+// TODO: Add a variant for io::Result?  Or try upstreaming this into the glib crate?
 impl<T, E> ToGErrorConventions for Result<T, E>
 where
     E: std::error::Error,

cgwalters · 2018-06-04T19:19:26Z

(I don't have a strong opinion on glib vs gerror - best resolved by upstreaming it IMO and we can bikeshed then)

jlebon · 2018-06-04T19:27:22Z

Done! ⬆️

cgwalters · 2018-06-04T19:32:43Z

Looks like C7 is having rpm-md issues, but let's try this...

@rh-atomic-bot r+ af1ddd7

rh-atomic-bot · 2018-06-04T19:32:48Z

⌛ Testing commit af1ddd7 with merge 90f54ea...

Let's modernize and start supporting YAML treefiles. I'll dare make the sweeping generalization that most people would prefer reading and writing YAML over JSON. This takes bits from coreos-assembler[1] that know how to serialize a YAML file and spit it back out as a JSON and makes it into a shared lib that we can link against. We could use this eventually for JSON inputs as well to force a validation check before composing. If we go this route, we could then turn on `--enable-rust` in FAHC for now and drop the duplicate code in coreos-assembler. [1] https://github.com/cgwalters/coreos-assembler Closes: #1377 Approved by: cgwalters

rh-atomic-bot · 2018-06-04T20:23:19Z

💔 Test failed - status-atomicjenkins

jlebon · 2018-06-05T13:08:31Z

@rh-atomic-bot retry

rh-atomic-bot · 2018-06-05T13:08:37Z

⌛ Testing commit af1ddd7 with merge 479406e...

rh-atomic-bot · 2018-06-05T14:00:49Z

☀️ Test successful - status-atomicjenkins
Approved by: cgwalters
Pushing 479406e to master...

rpm-ostree now includes some Rust code. See coreos/rpm-ostree#1377 and coreos/rpm-ostree#1391

We'd like to use YAML for future rpm-ostree work; among other things it supports comments. See https://pagure.io/fedora-atomic/pull-request/125 and the original coreos/rpm-ostree#1377 Signed-off-by: Colin Walters <walters@verbum.org>

cgwalters reviewed May 31, 2018

View reviewed changes

lucab reviewed Jun 1, 2018

View reviewed changes

jlebon force-pushed the pr/yaml-rust branch from b5316da to 4529fba Compare June 1, 2018 21:21

cgwalters force-pushed the pr/yaml-rust branch from 62188b1 to 5041c97 Compare June 4, 2018 17:39

jlebon changed the title ~~RFC: WIP: Add support for YAML treefiles~~ Add support for YAML treefiles Jun 4, 2018

fixup! Add support for YAML treefiles

af1ddd7

jlebon force-pushed the pr/yaml-rust branch from f324e23 to af1ddd7 Compare June 4, 2018 19:25

jlebon changed the title ~~Add support for YAML treefiles~~ 🦀 Add support for YAML treefiles Jun 4, 2018

cgwalters added the reviewed/approved label Jun 4, 2018

rh-atomic-bot closed this in 479406e Jun 5, 2018

This was referenced Jun 5, 2018

Support source generation projectatomic/rpmdistro-gitoverlay#48

Open

main: introduce buffering for file I/O coreos/coreos-assembler#3

Merged

LorbusChris pushed a commit to LorbusChris/rpm-ostree-spec that referenced this pull request Oct 22, 2018

Enable Rust

a707bea

rpm-ostree now includes some Rust code. See coreos/rpm-ostree#1377 and coreos/rpm-ostree#1391

jlebon mentioned this pull request Oct 29, 2020

rust: Enable lto by default #2294

Merged

🦀 Add support for YAML treefiles #1377

🦀 Add support for YAML treefiles #1377

Conversation

jlebon commented May 30, 2018

jlebon commented May 30, 2018

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

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

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

jlebon commented Jun 1, 2018

jlebon commented Jun 4, 2018 • edited Loading

cgwalters commented Jun 4, 2018

jlebon commented Jun 4, 2018

cgwalters commented Jun 4, 2018 • edited by jlebon Loading

jlebon commented Jun 4, 2018

cgwalters commented Jun 4, 2018 • edited Loading

cgwalters commented Jun 4, 2018 • edited Loading

jlebon commented Jun 4, 2018

cgwalters commented Jun 4, 2018

jlebon commented Jun 4, 2018 • edited Loading

cgwalters commented Jun 4, 2018

cgwalters commented Jun 4, 2018

jlebon commented Jun 4, 2018

cgwalters commented Jun 4, 2018

rh-atomic-bot commented Jun 4, 2018

rh-atomic-bot commented Jun 4, 2018

jlebon commented Jun 5, 2018

rh-atomic-bot commented Jun 5, 2018

rh-atomic-bot commented Jun 5, 2018

jlebon commented Jun 4, 2018 •

edited

Loading

cgwalters commented Jun 4, 2018 •

edited by jlebon

Loading

cgwalters commented Jun 4, 2018 •

edited

Loading

cgwalters commented Jun 4, 2018 •

edited

Loading

jlebon commented Jun 4, 2018 •

edited

Loading