compose: Support arch-specific packages in YAML (and in JSON again)

Follow up to: https://github.com/projectatomic/rpm-ostree/pull/1459

We now honor arch-specific packages in YAML, and reject unknown
architectures.  I looked a little bit at how to avoid having hardcoded
arch lists, but it doesn't seem worth it right now.

Closes: #1468
Approved by: jlebon

rust/include/rpmostree-rust.h

@@ -20,4 +20,6 @@
 
 #pragma once
 
-int rpmostree_rs_treefile_read (const char *filename, int output_fd, GError **error);
+int rpmostree_rs_treefile_read (const char *filename,
+                                const char *arch,
+                                int output_fd, GError **error);

rust/src/lib.rs

@@ -30,7 +30,6 @@ extern crate serde_yaml;
 use std::ffi::{CStr, OsStr};
 use std::os::unix::ffi::OsStrExt;
 use std::os::unix::io::{FromRawFd, IntoRawFd};
-use std::path::Path;
 use std::{fs, io};
 
 mod glibutils;
@@ -40,22 +39,41 @@ use treefile::treefile_read_impl;
 
 /* Wrapper functions for translating from C to Rust */
 
+/// Convert a C (UTF-8) string to a &str; will panic
+/// if it isn't valid UTF-8.  Note the lifetime of
+/// the return value must be <= the pointer.
+fn str_from_nullable<'a>(s: *const libc::c_char) -> Option<&'a str> {
+    if s.is_null() {
+        None
+    } else {
+        let s = unsafe { CStr::from_ptr(s) };
+        Some(s.to_str().unwrap())
+    }
+}
+
+/// Convert a C "bytestring" to a OsStr; panics if `s` is `NULL`.
+fn bytes_from_nonnull<'a>(s: *const libc::c_char) -> &'a [u8] {
+    assert!(!s.is_null());
+    unsafe { CStr::from_ptr(s) }.to_bytes()
+}
+
 #[no_mangle]
 pub extern "C" fn rpmostree_rs_treefile_read(
     filename: *const libc::c_char,
+    arch: *const libc::c_char,
     fd: libc::c_int,
     error: *mut *mut glib_sys::GError,
 ) -> libc::c_int {
-    // using an O_TMPFILE is an easy way to avoid ownership transfer issues w/ returning allocated
-    // memory across the Rust/C boundary
-
-    // The dance with `file` is to avoid dup()ing the fd unnecessarily
+    // Convert arguments
+    let filename = OsStr::from_bytes(bytes_from_nonnull(filename));
+    let arch = str_from_nullable(arch);
+    // Using an O_TMPFILE is an easy way to avoid ownership transfer issues w/
+    // returning allocated memory across the Rust/C boundary; the dance with
+    // `file` is to avoid dup()ing the fd unnecessarily.
     let file = unsafe { fs::File::from_raw_fd(fd) };
     let r = {
         let output = io::BufWriter::new(&file);
-        let c_str: &CStr = unsafe { CStr::from_ptr(filename) };
-        let filename_path = Path::new(OsStr::from_bytes(c_str.to_bytes()));
-        treefile_read_impl(filename_path, output).to_glib_convention(error)
+        treefile_read_impl(filename.as_ref(), arch, output).to_glib_convention(error)
     };
     file.into_raw_fd(); // Drop ownership of the FD again
     r

rust/src/treefile.rs

@@ -26,7 +26,10 @@ use serde_yaml;
 use std::path::Path;
 use std::{fs, io};
 
-fn treefile_parse_yaml<R: io::Read>(input: R) -> io::Result<TreeComposeConfig> {
+const ARCH_X86_64: &'static str = "x86_64";
+
+/// Parse a YAML treefile definition using architecture `arch`.
+fn treefile_parse_yaml<R: io::Read>(input: R, arch: Option<&str>) -> io::Result<TreeComposeConfig> {
     let mut treefile: TreeComposeConfig = match serde_yaml::from_reader(input) {
         Ok(t) => t,
         Err(e) => {
@@ -37,17 +40,50 @@ fn treefile_parse_yaml<R: io::Read>(input: R) -> io::Result<TreeComposeConfig> {
         }
     };
 
-    // special handling for packages, since we allow whitespaces within items
-    if let Some(pkgs) = treefile.packages {
-        treefile.packages = Some(whitespace_split_packages(&pkgs));
+    // Special handling for packages, since we allow whitespace within items.
+    // We also canonicalize bootstrap_packages to packages here so it's
+    // easier to append the arch packages after.
+    let mut pkgs: Vec<String> = vec![];
+    {
+        if let Some(base_pkgs) = treefile.packages.take() {
+            pkgs.extend_from_slice(&whitespace_split_packages(&base_pkgs));
+        }
+        if let Some(bootstrap_pkgs) = treefile.bootstrap_packages.take() {
+            pkgs.extend_from_slice(&whitespace_split_packages(&bootstrap_pkgs));
+        }
     }
 
+    let arch_pkgs = match arch {
+        Some("aarch64") => treefile.packages_aarch64.take(),
+        Some("armhfp") => treefile.packages_armhfp.take(),
+        Some("ppc64") => treefile.packages_ppc64.take(),
+        Some("ppc64le") => treefile.packages_ppc64le.take(),
+        Some("s390x") => treefile.packages_s390x.take(),
+        Some(ARCH_X86_64) => treefile.packages_x86_64.take(),
+        None => None,
+        Some(x) => panic!("Invalid architecture: {}", x),
+    };
+    if let Some(arch_pkgs) = arch_pkgs {
+        pkgs.extend_from_slice(&whitespace_split_packages(&arch_pkgs));
+    }
+    if pkgs.len() == 0 {
+        return Err(io::Error::new(
+            io::ErrorKind::InvalidInput,
+            format!("Missing 'packages' entry"),
+        ));
+    };
+
+    treefile.packages = Some(pkgs);
     Ok(treefile)
 }
 
-pub fn treefile_read_impl<W: io::Write>(filename: &Path, output: W) -> io::Result<()> {
+pub fn treefile_read_impl<W: io::Write>(
+    filename: &Path,
+    arch: Option<&str>,
+    output: W,
+) -> io::Result<()> {
     let f = io::BufReader::new(fs::File::open(filename)?);
-    let treefile = treefile_parse_yaml(f)?;
+    let treefile = treefile_parse_yaml(f, arch)?;
     serde_json::to_writer_pretty(output, &treefile)?;
     Ok(())
 }
@@ -119,6 +155,28 @@ pub struct TreeComposeConfig {
     // Core content
     #[serde(skip_serializing_if = "Option::is_none")]
     pub packages: Option<Vec<String>>,
+    // Arch-specific packages; TODO replace this with
+    // custom deserialization or so and avoid having
+    // having an architecture list here.
+    #[serde(rename = "packages-aarch64")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub packages_aarch64: Option<Vec<String>>,
+    #[serde(rename = "packages-armhfp")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub packages_armhfp: Option<Vec<String>>,
+    #[serde(rename = "packages-ppc64")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub packages_ppc64: Option<Vec<String>>,
+    #[serde(rename = "packages-ppc64le")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub packages_ppc64le: Option<Vec<String>>,
+    #[serde(rename = "packages-s390x")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub packages_s390x: Option<Vec<String>>,
+    #[serde(rename = "packages-x86_64")]
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub packages_x86_64: Option<Vec<String>>,
+    // Deprecated option
     #[serde(skip_serializing_if = "Option::is_none")]
     pub bootstrap_packages: Option<Vec<String>>,
 
@@ -196,34 +254,61 @@ pub struct TreeComposeConfig {
 mod tests {
     use super::*;
 
-    static VALID_PRELUDE : &str = r###"
+    static VALID_PRELUDE: &str = r###"
 ref: "exampleos/x86_64/blah"
 packages:
  - foo bar
  - baz
+packages-x86_64:
+ - grub2 grub2-tools
+packages-s390x:
+ - zipl
 "###;
 
     #[test]
     fn basic_valid() {
         let input = io::BufReader::new(VALID_PRELUDE.as_bytes());
-        let treefile = treefile_parse_yaml(input).unwrap();
+        let treefile = treefile_parse_yaml(input, Some(ARCH_X86_64)).unwrap();
+        assert!(treefile.treeref == "exampleos/x86_64/blah");
+        assert!(treefile.packages.unwrap().len() == 5);
+    }
+
+    #[test]
+    fn basic_valid_noarch() {
+        let input = io::BufReader::new(VALID_PRELUDE.as_bytes());
+        let treefile = treefile_parse_yaml(input, None).unwrap();
         assert!(treefile.treeref == "exampleos/x86_64/blah");
         assert!(treefile.packages.unwrap().len() == 3);
     }
 
-    #[test]
-    fn basic_invalid() {
+    fn test_invalid(data: &'static str) {
         let mut buf = VALID_PRELUDE.to_string();
-        buf.push_str(r###"install_langs:
-  - "klingon"
-  - "esperanto"
-"###);
+        buf.push_str(data);
         let buf = buf.as_bytes();
         let input = io::BufReader::new(buf);
-        match treefile_parse_yaml(input) {
-            Err(ref e) if e.kind() == io::ErrorKind::InvalidInput => {},
+        match treefile_parse_yaml(input, Some(ARCH_X86_64)) {
+            Err(ref e) if e.kind() == io::ErrorKind::InvalidInput => {}
             Err(ref e) => panic!("Expected invalid treefile, not {}", e.to_string()),
             _ => panic!("Expected invalid treefile"),
         }
     }
+
+    #[test]
+    fn test_invalid_install_langs() {
+        test_invalid(
+            r###"install_langs:
+  - "klingon"
+  - "esperanto"
+"###,
+        );
+    }
+
+    #[test]
+    fn test_invalid_arch() {
+        test_invalid(
+            r###"packages-hal9000:
+  - podbaydoor glowingredeye
+"###,
+        );
+    }
 }

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

@@ -676,7 +676,8 @@ install_packages_in_root (RpmOstreeTreeComposeContext  *self,
 }
 
 static gboolean
-parse_treefile_to_json (const char    *treefile_path,
+parse_treefile_to_json (RpmOstreeTreeComposeContext  *self,
+                        const char    *treefile_path,
                         JsonParser   **out_parser,
                         GError       **error)
 {
@@ -695,7 +696,9 @@ parse_treefile_to_json (const char    *treefile_path,
       if (!glnx_open_anonymous_tmpfile (O_RDWR | O_CLOEXEC, &json_contents, error))
         return FALSE;
 
-      if (!rpmostree_rs_treefile_read (treefile_path, json_contents.fd, error))
+      const char *arch = self ? dnf_context_get_base_arch (rpmostree_context_get_dnf (self->corectx)) : NULL;
+      if (!rpmostree_rs_treefile_read (treefile_path, arch,
+                                       json_contents.fd, error))
         return glnx_prefix_error (error, "Failed to load YAML treefile");
 
       /* or just lseek back to 0 and use json_parser_load_from_data here? */
@@ -754,7 +757,8 @@ process_includes (RpmOstreeTreeComposeContext  *self,
       GList *members;
       GList *iter;
 
-      if (!parse_treefile_to_json (gs_file_get_path_cached (treefile_path),
+      if (!parse_treefile_to_json (self,
+                                   gs_file_get_path_cached (treefile_path),
                                    &parent_parser, error))
         return FALSE;
 
@@ -961,7 +965,7 @@ rpm_ostree_compose_context_new (const char    *treefile_pathstr,
   if (!self->corectx)
     return FALSE;
 
-  if (!parse_treefile_to_json (gs_file_get_path_cached (self->treefile_path),
+  if (!parse_treefile_to_json (self, gs_file_get_path_cached (self->treefile_path),
                                &self->treefile_parser, error))
     return FALSE;
 
@@ -1476,7 +1480,7 @@ rpmostree_compose_builtin_postprocess (int             argc,
   JsonObject *treefile = NULL; /* Owned by parser */
   if (treefile_path)
     {
-      if (!parse_treefile_to_json (treefile_path, &treefile_parser, error))
+      if (!parse_treefile_to_json (NULL, treefile_path, &treefile_parser, error))
         return FALSE;
 
       JsonNode *treefile_rootval = json_parser_get_root (treefile_parser);

tests/composedata/fedora-base.json

@@ -5,8 +5,19 @@
     "repos": ["fedora", "updates"],
 
     "packages": ["kernel", "nss-altfiles", "systemd", "ostree", "selinux-policy-targeted", "chrony",
-                 "tuned", "iputils", "fedora-release-atomichost", "docker", "container-selinux",
-                 "grub2", "grub2-efi", "ostree-grub2", "efibootmgr", "shim"],
+                 "tuned", "iputils", "fedora-release-atomichost", "docker", "container-selinux"],
+
+    "packages-aarch64": ["grub2-efi", "ostree-grub2",
+                         "efibootmgr", "shim"],
+
+    "packages-armhfp": ["extlinux-bootloader"],
+
+    "packages-ppc64": ["grub2", "ostree-grub2"],
+
+    "packages-ppc64le": ["grub2", "ostree-grub2"],
+
+    "packages-x86_64": ["grub2", "grub2-efi", "ostree-grub2",
+                        "efibootmgr", "shim"],
 
     "ignore-removed-users": ["root"],
     "ignore-removed-groups": ["root"],