compose+rust: Parse includes via Rust too #1574
Conversation
|
Depends: #1573 |
cgwalters
changed the title
|
This is going to need more work around the treefile context dir array and |
The core bug here is that previously if we had multiple YAML files in include, we ended up overwriting self->treefile_rs for the last one. Handling inheritance worked, but it broke rojig since we generate the specfile Rust side. Let's have first-one-wins semantics for now. I have a bigger fix incoming in coreos#1574
The core bug here is that previously if we had multiple YAML files in include, we ended up overwriting self->treefile_rs for the last one. Handling inheritance worked, but it broke rojig since we generate the specfile Rust side. Let's have first-one-wins semantics for now. I have a bigger fix incoming in #1574 Closes: #1576 Approved by: jlebon
|
☔ The latest upstream changes (presumably 2e56784) made this pull request unmergeable. Please resolve the merge conflicts. |
cgwalters
force-pushed
the
json-parse-via-rust
branch
from
88bd76c to
cc98fae
Compare
cgwalters
changed the title
|
One big reason I'm doing this is that trying to use treefile inheritance aggressively for Fedora CoreOS, I tripped over things like inheriting from a treefile with The big thing this PR doesn't fix is |
The primary reason I'm doing this is that today rpm-ostree's treefile inheritance doesn't quite do what one wants with external files. There's some major work on that in coreos/rpm-ostree#1574 But let's not block on/require it. Furthermore, using the inline `postprocess` value allows us to clearly demarcate the `maipo`-specific hacks in that file.
The primary reason I'm doing this is that today rpm-ostree's treefile inheritance doesn't quite do what one wants with external files. There's some major work on that in coreos/rpm-ostree#1574 But let's not block on/require it. Furthermore, using the inline `postprocess` value allows us to clearly demarcate the `maipo`-specific hacks in that file.
|
☔ The latest upstream changes (presumably 8df07a3) made this pull request unmergeable. Please resolve the merge conflicts. |
cgwalters
force-pushed
the
json-parse-via-rust
branch
from
cc98fae to
17834de
Compare
|
☔ The latest upstream changes (presumably 7595bce) made this pull request unmergeable. Please resolve the merge conflicts. |
cgwalters
force-pushed
the
json-parse-via-rust
branch
from
17834de to
5ad2867
Compare
|
Rebased 🏄♂️ and now passing tests! |
rust/src/treefile.rs
Outdated
| if let Some(ref mut srcv) = src.take() { | ||
| let mut v = dest.take().map_or_else(|| vec![], |v| v); | ||
| v.append(srcv); | ||
| *dest = Some(v) |
There was a problem hiding this comment.
Wait, this is actually slightly changing the semantics now. Before we were prepending fields from includes, but now we're appending, right?
Which I think could matter in some cases? E.g. just from a quick scan:
initramfs_argsis passed directly down todracutand not handled by uspostprocess, i.e. inlined script orderingadd-files... although ahh right, we essentially useNOREPLACEhere so it would error out anyway on the second try to write to the same destination.
That makes it sound like I didn't review this in depth, but I did. :) The high level approach does look good though. |
Since I got it backwards when rewriting it in Rust.
This follows up to coreos#1576 AKA commit 2e56784 - we now process treefile inheritance in Rust code. Previously for elements which reference external files (`postprocess-script` and `add-files`) we'd hardcoded things to only look in the first context dir. Now we open file descriptors in the Rust side for these "externals" as we're parsing, and load them C side. Hence we'll correctly handle a `postprocess-script` from an included config. Other advantages are that the include handling was ugly un-typesafe C code with no unit tests, now it's memory safe Rust with unit tests. The downside here is I ended up spelling out the list of fields again - there's probably a way to unify this via macros but for now I think this is OK.
|
I'm reworking this on top of a test case for inheritance. Also doing so revealed we weren't inheriting the |
cgwalters
force-pushed
the
json-parse-via-rust
branch
from
5ad2867 to
287b122
Compare
|
OK, now with new test case and fixups ⬆️ |
| @@ -24,6 +24,8 @@ pysetjsonmember "postprocess-script" \"$PWD/postprocess.sh\" | |||
| pysetjsonmember "postprocess" '["""#!/bin/bash | |||
| touch /usr/share/postprocess-testing""", | |||
| """#!/bin/bash | |||
| set -xeuo pipefail | |||
There was a problem hiding this comment.
Oh yeah and also this was missing before...I was confused why the test wasn't failing for me with the previous inverted Rust code 😢
|
LGTM! |
|
⚡ Test exempted: pull fully rebased and already tested. |
This follows up to #1576 AKA commit 2e56784 - we now process treefile inheritance in Rust code. Previously for elements which reference external files (`postprocess-script` and `add-files`) we'd hardcoded things to only look in the first context dir. Now we open file descriptors in the Rust side for these "externals" as we're parsing, and load them C side. Hence we'll correctly handle a `postprocess-script` from an included config. Other advantages are that the include handling was ugly un-typesafe C code with no unit tests, now it's memory safe Rust with unit tests. The downside here is I ended up spelling out the list of fields again - there's probably a way to unify this via macros but for now I think this is OK. Closes: #1574 Approved by: jlebon
The core bug here is that previously if we had multiple YAML files
in
include, we ended up overwritingself->treefile_rsfor thelast one. Handling inheritance worked, but it broke
rojigsincewe generate the specfile Rust side.
Fix this by moving more of the parsing into Rust. There are a
lot of advantages to this - the include handling was ugly un-typesafe C code
with no unit tests, now it's memory safe Rust with unit tests.
The downside here is I ended up spelling out the list of fields
again - there's probably a way to unify this via macros but
for now I think this is OK.