compose: Support arch-specific packages in YAML (and in JSON again) #1468
Conversation
Follow up to: coreos#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.
cgwalters
force-pushed
the
yaml-treefile-arch-packages
branch
from
63f0f3f to
f5f95fe
Compare
|
I context switched while working on this and realized after it's not done; I have a bigger patch I'm working on to actually plumb it through to the C side now. |
cgwalters
changed the title
rust/src/lib.rs
Outdated
| let arch = unsafe { CStr::from_ptr(arch) }; | ||
| Some(arch.to_string_lossy().into_owned()) | ||
| }; | ||
| let arch = arch.as_ref().map(String::as_str); |
There was a problem hiding this comment.
I found writing what I expected to be an easy "convert a raw nullable string pointer to Option<&str>" to be more awkward than expected. I was going to reach for CStr::to_str().unwrap() but that'd rely on our panic=abort setting, otherwise we'd try to unwind across FFI, which...OK it looks like relatively recently turned into an abort. So we probably can do that.
There was a problem hiding this comment.
OK, I reworked things here and I'm happier ⬇️ - it's worth finding good patterns here as the FFI glue is going to be a risky area of the code.
cgwalters
changed the title
|
Lifted WIP. |
rust/src/lib.rs
Outdated
| fd: libc::c_int, | ||
| error: *mut *mut glib_sys::GError, | ||
| ) -> libc::c_int { | ||
| // Convert arguments | ||
| let filename = Path::new(unsafe { CStr::from_ptr(filename).to_str().unwrap() }); |
There was a problem hiding this comment.
Not that we really want to cater to the anti-UTF-8 rebels, though what was wrong with the previous way of building a Path which supported any path?
There was a problem hiding this comment.
Yeah my feeling is that the OsStr stuff is mostly for tools like rm or du that don't actually need to interpret filenames in general. I'm totally fine erroring out if the user provides us a treefile whose filename isn't valid UTF-8. That said we shouldn't panic. I reworked this some ⬇️
Also holy cow is the *const libc::c_char/CStr/[u8]/OsStr/Path situation confusing...I was able to drop the Path::new as we don't need/want a mutable buffer.
rust/src/treefile.rs
Outdated
| assert!(treefile.treeref == "exampleos/x86_64/blah"); | ||
| assert!(treefile.packages.unwrap().len() == 3); | ||
| eprintln!("{:?}", treefile); |
rust/src/lib.rs
Outdated
| @@ -51,6 +51,12 @@ fn str_from_nullable<'a>(s: *const libc::c_char) -> Option<&'a str> { | |||
| } | |||
| } | |||
|
|
|||
| /// Convert a C "bytestring" to a OsStr; panics if `s` is `NULL`. | |||
rust/src/lib.rs
Outdated
| let file = unsafe { fs::File::from_raw_fd(fd) }; | ||
| let r = { | ||
| let output = io::BufWriter::new(&file); | ||
| treefile_read_impl(filename, arch, output).to_glib_convention(error) | ||
| treefile_read_impl(filename.as_ref(), arch, output).to_glib_convention(error) |
|
💔 Test failed - status-atomicjenkins |
|
Hmm, looks like a registry.fedoraproject.org/fedora flake. |
|
💔 Test failed - status-atomicjenkins |
|
@rh-atomic-bot retry Go go gadget merge! |
|
☀️ Test successful - status-atomicjenkins |
Follow up to: #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.