Support greater than/less than versions for packages in treefile. #2151

kelvinfan001 · 2020-06-26T18:03:09Z

Add a helper function for whitespace_split_packages() so that it now splits a String by whitespace only if it is not wrapped between single quotes.
This should allow RHCOS to use syntax like podman > 1.4 in the treefile.

cgwalters

Overall looks like a great start, thanks so much for taking this on!

rust/src/treefile.rs

cgwalters · 2020-06-26T18:12:58Z

rust/src/treefile.rs

+    let mut to_whitespace_split: Vec<String> = vec![];
+    let mut start_index = 0;
+    let mut looping_over_quoted_pkg = false;
+    for (i, c) in element.chars().enumerate() {


I'm OK if we try to handle the fully general case but...I would also be totally fine if we just required that users only include a single package on a line if using version qualifiers.

IOW,

packages: - kernel grub2 - "'selinux-policy > 1.5'" - "'podman > 2.0'"

but not:

packages: - kernel grub2 - "'selinux-policy > 1.5' 'podman > 2.0'"

If if we make that a requirement, we can simply check:

if element.starts_with('\'') { ... }

which would simplify all of this code a lot.

It would indeed. I consulted with Jonathan about this and he recommended solving for the general case. I originally had a hacky solution using Rust's library that would shorten the code by a lot, but it felt a bit unintuitive and difficult to understand what was going on.

I consulted with Jonathan about this and he recommended solving for the general case

There might've a misunderstanding. It's 100% fine with me if you want to go the easy route (I initially suggested it myself in #2138 (comment)). That said, if you feel like tackling the generic case, that's totally cool with me too! :)

Ah, I see. I'm okay with either way, as well :)
I'll leave it as is for now, but let me know if I should change it!

If we're keeping the code as is I'd need to think hard about whether it's correct; it looks plausible - the unit tests would help a lot too particularly if we have failure conditions like missing quotes, etc.

If we only handle the simple case I'd be much more confident it's correct.

cgwalters · 2020-06-26T18:14:52Z

tests/compose/test-misc-tweaks.sh


 echo gpgcheck=0 >> yumrepo.repo
 ln "$PWD/yumrepo.repo" config/yumrepo.repo
 # the top-level manifest doesn't have any packages, so just set it
-treefile_append "packages" '["foobar"]'
+treefile_append "packages" $'["\'foobar >= 0.5\' quuz \'corge < 1.0.0\'"]'


The quoting required here is... 🤯
It has to get through bash, then Python, then YAML, then rpm-ostree...

tests/compose/test-misc-tweaks.sh

zonggen

Some feedback from my side:

Can we assume the correctness of the package lists? e.g. can I assume there's no incorrect use of single quotes such as 'foo bar baz, foo bar baz' and foo' 'bar baz in the package lists. The last case foo' 'bar baz in particular will result in an empty string in the quoted_pkgs. It might be helpful to check for this case and trim the empty string in quoted_pkgs.
It might be helpful to wrap the return value of both functions (whitespace_split_packages and split_whitespace_unless_quoted) in a Result<>, and report/return error if necessary.
For commit messages it will be more clear to add the origin of the change at the start, e.g. treefile: Support greater than/less than versions for packages in treefile and no period at the end of subject line

zonggen · 2020-06-29T21:10:56Z

Overall looks really good, and nice work on learning and working on Rust code 😄 .

kelvinfan001 · 2020-06-30T18:25:21Z

Thanks, @zonggen, for the helpful comments! I guess I have the same question as Allen: @cgwalters, can we assume that there's no incorrect usage of quotes in the treefile? If not, I'm happy to implement checks for whether there are missing/improper quotes and return errors if necessary in the functions.

jlebon · 2020-06-30T20:41:25Z

IMO, bad quoting should be hard errors because it likely implies someone mistyped something.

kelvinfan001 · 2020-06-30T21:13:06Z

IMO, bad quoting should be hard errors because it likely implies someone mistyped something.

sorry I didn't quite get what you meant by incorrect quoting should be hard errors. Would you suggest that I modify the helper functions such that they return errors when things are misquoted? Also, I'm not sure if I did the unit tests correctly; I added a couple tests that show that nothing breaks when a user misquotes some packages, and show that it just leads to weird "packages" like ' ', or '<=', etc.

cgwalters · 2020-07-01T12:34:54Z

rust/src/treefile.rs

@@ -576,10 +576,42 @@ impl TreefileExternals {
 /// array elements.
 fn whitespace_split_packages(pkgs: &[String]) -> Vec<String> {
    pkgs.iter()
-        .flat_map(|pkg| pkg.split_whitespace().map(String::from))
+        .flat_map(|element| split_whitespace_unless_quoted(element).map(String::from))


BTW you can drop the .map(String::from) now because the function is already returning String and not &str.

cgwalters · 2020-07-01T12:36:38Z

rust/src/treefile.rs

+// and returns split Strings in an Iterator, with quoted packages first.
+fn split_whitespace_unless_quoted(element: &String) -> impl Iterator<Item = String> {
+    let mut quoted_pkgs: Vec<String> = vec![];
+    let mut to_whitespace_split: Vec<String> = vec![];


You can likely drop the : Vec<String> and let type inference handle it.

cgwalters · 2020-07-01T12:39:07Z

rust/src/treefile.rs

+            start_index = i + 1;
+        }
+        if i == element.len() - 1 {
+            to_whitespace_split.push(String::from(&element[start_index..]));


And related to the above...I think you can also drop String::from here and build up instead a Vec<&str> - i.e. pointers into the original element. This saves intermediate allocations.

Performance doesn't matter here really - I'm mostly saying this because a powerful thing about Rust is you can fearlessly build up pointers into other allocated data, not duplicating and not worry about double frees/invalid pointers etc.

Oh that's a great point! Thanks :) It's cool how split_whitespace_unless_quoted() can actually just work on the borrowed String and not have to allocate anything at all if we change the return type in this function and keep the .map(String::from) in the outer function.

cgwalters · 2020-07-01T17:21:55Z

rust/src/treefile.rs

+        }
+    }
+    let mut ret: Vec<String> = vec![];
+    ret.extend(quoted_pkgs);


Let's move this to the top - i.e. directly push into ret rather than having a separate vector.

Right, this would make a lot more sense.

cgwalters · 2020-07-01T17:22:50Z

rust/src/treefile.rs

+        if i == element.len() - 1 {
+            to_whitespace_split.push(String::from(&element[start_index..]));
+        }
+    }


And then here something like:

if looping_over_quoted_pkg { bail!("Missing terminating quote: {}", element); }

cgwalters · 2020-07-01T17:42:17Z

rust/src/treefile.rs

+    fn test_split_whitespace_unless_quoted() {
+        // test single quoted package
+        let single_quoted_pkg = String::from("'foobar >= 1.0'");
+        let pkgs: Vec<String> = split_whitespace_unless_quoted(&single_quoted_pkg).collect();


Also it would make sense for split_whitespace_unless_quoted to take an &str and not a &String - that way the test cases like this can just pass a static pointer rather than an allocated string.

cgwalters · 2020-07-01T17:44:03Z

rust/src/treefile.rs

@@ -1226,6 +1259,66 @@ etc-group-members:
                == "table"
        );
    }
+
+    #[test]
+    fn test_split_whitespace_unless_quoted() {


If we change split_whitespace_unless_quoted to be -> Result<impl Iterator<Item = String>>, then I'd change this function to be -> Result<()> too.

cgwalters · 2020-07-01T17:44:31Z

rust/src/treefile.rs

+
+        // test multiple quoted packages
+        let mult_quoted_pkg = String::from("'foobar >= 1.0' 'quuz < 0.5' 'corge > 2'");
+        let pkgs: Vec<String> = split_whitespace_unless_quoted(&mult_quoted_pkg).collect();


Then once it returns Result<> this call would be:
let pkgs: Vec<String> = split_whitespace_unless_quoted(&mult_quoted_pkg)?.collect();
(Notice the added ?)

cgwalters · 2020-07-01T17:45:26Z

rust/src/treefile.rs

+        let missing_quotes = String::from("foobar >= 1.0 quuz");
+        let pkgs: Vec<String> = split_whitespace_unless_quoted(&missing_quotes).collect();
+        assert_ne!("foobar >= 1.0", pkgs[0]);
+        assert_eq!(">=", pkgs[1]);


Then these tests would be:
assert!(split_whitespace_unless_quoted(&missing_quotes).is_err())

cgwalters · 2020-07-01T19:39:04Z

/approve

jlebon · 2020-07-02T14:48:56Z

sorry I didn't quite get what you meant by incorrect quoting should be hard errors. Would you suggest that I modify the helper functions such that they return errors when things are misquoted?

Yeah, essentially rpm-ostree compose tree should error out if a treefile package entry is something like foo'bar. Though... hmm I guess we could simply rely on the fact that no package matching that name will be found. But a "found quoting issues" type error would be clearer.

cgwalters · 2020-07-02T22:31:18Z

rust/src/treefile.rs

-        let single_quoted_pkg = String::from("'foobar >= 1.0'");
-        let pkgs: Vec<String> = split_whitespace_unless_quoted(&single_quoted_pkg).collect();
+        let single_quoted_pkg = "'foobar >= 1.0'";
+        let pkgs: Vec<&str> = split_whitespace_unless_quoted(&single_quoted_pkg)?.collect();


This is completely fine as is, but you can also just use:
let pkgs: Vec<_> = ...
Here the .collect() call needs to know that we're collecting into a Vec<>, but type inference can take over for there and know it must be &str because that's what split_whitespace_unless_quoted() returns.

This is totally fine as is though!

Thanks :) Hopefully I'll get the hang of type inference and make better use of it soon. Still in "playing it safe" mode.

Absolutely! I'm writing this feedback not because there's any real problem with the code, but because it's a useful way to help learn!

cgwalters · 2020-07-02T22:33:15Z

I think we'll need to wait until CoreOS CI is up to merge, but LGTM!

kelvinfan001 · 2020-07-02T22:40:45Z

I think we'll need to wait until CoreOS CI is up to merge, but LGTM!

@cgwalters Thanks so much for the detailed feedback! Learned a lot in the process. I'll merge when CI is up :)

jlebon

LGTM as well overall, nice work! Just some comments, but none of them are blockers. :)

jlebon · 2020-07-03T16:57:47Z

rust/src/treefile.rs

+        }
+    }
+    for item in to_whitespace_split.iter() {
+        ret.extend(item.split_whitespace());


Couldn't we just get rid of to_whitespace_split and just do the ret.extend() directly in the loop above as we encounter it? This also has the property of maintaining the order of packages (it doesn't matter much to rpm-ostree/libdnf really, but it'd make writing the unit tests below slightly easier).

This would make a lot more sense, thanks!

jlebon · 2020-07-03T17:01:19Z

rust/src/treefile.rs

@@ -1226,6 +1263,65 @@ etc-group-members:
                == "table"
        );
    }
+
+    #[test]
+    fn test_split_whitespace_unless_quoted() -> Result<()> {


Nice, love all the unit tests! :)

jlebon · 2020-07-03T17:02:53Z

tests/compose/test-misc-tweaks.sh


 echo gpgcheck=0 >> yumrepo.repo
 ln "$PWD/yumrepo.repo" config/yumrepo.repo
 # the top-level manifest doesn't have any packages, so just set it
-treefile_append "packages" '["foobar"]'
+treefile_append "packages" $'["\'foobar >= 0.5\' quuz \'corge < 1.0.0\'"]'


Hmm, I think a more convincing example is building e.g. two corge packages, one at 1.0 and one at 2.0, then have the request be for cargo < 2.0 and verify that corge-1.0 is the one that got pulled in.

jlebon · 2020-07-03T19:45:29Z

/approve

(Let's wait to /lgtm until CI is fully back up because Prow on its own doesn't run the full tests.)

jlebon · 2020-07-06T19:22:19Z

/lgtm

Add a helper function for whitespace_split_packages() so that it now splits a String by whitespace only if it is not wrapped between single quotes. This should allow RHCOS to use syntax like podman > 1.4 in the treefile. Also add new unit tests and tweak existing compose tests to test this functionality.

cgwalters · 2020-07-08T17:05:45Z

(Rebased this to retrigger the latest CI)
/lgtm

openshift-ci-robot · 2020-07-08T17:05:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cgwalters, jlebon, kelvinfan001

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

Thin release. Mostly for a bunch of RHCOS-relevant fixes and enhancements: 752f4f0 (coreos#2178), 4d836dd (coreos#2151) and f608eb0 (coreos#2158).

Thin release. Mostly for a bunch of RHCOS-relevant fixes and enhancements: 752f4f0 (#2178), 4d836dd (#2151) and f608eb0 (#2158).

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

openshift-ci-robot requested review from jlebon and miabbott June 26, 2020 18:03

cgwalters reviewed Jun 26, 2020

View reviewed changes

kelvinfan001 changed the title ~~WIP: Support greater than/less than versions for packages in treefile.~~ Support greater than/less than versions for packages in treefile. Jun 29, 2020

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

zonggen reviewed Jun 29, 2020

View reviewed changes

kelvinfan001 force-pushed the greater-than-less-than-packages-treefile branch from aa38a1d to d5e653f Compare June 30, 2020 20:56

cgwalters reviewed Jul 1, 2020

View reviewed changes

openshift-ci-robot added the approved label Jul 1, 2020

cgwalters approved these changes Jul 2, 2020

View reviewed changes

kelvinfan001 force-pushed the greater-than-less-than-packages-treefile branch from d82a5df to 3bf8e02 Compare July 2, 2020 22:35

jlebon approved these changes Jul 3, 2020

View reviewed changes

kelvinfan001 force-pushed the greater-than-less-than-packages-treefile branch from 89c09e3 to b9fc51b Compare July 3, 2020 19:41

openshift-ci-robot assigned jlebon Jul 6, 2020

openshift-ci-robot added the lgtm label Jul 6, 2020

cgwalters force-pushed the greater-than-less-than-packages-treefile branch from b9fc51b to 408a423 Compare July 8, 2020 17:05

openshift-ci-robot removed the lgtm label Jul 8, 2020

openshift-ci-robot assigned cgwalters Jul 8, 2020

openshift-ci-robot added the lgtm label Jul 8, 2020

openshift-merge-robot merged commit 4d836dd into coreos:master Jul 8, 2020

jlebon added a commit to jlebon/rpm-ostree that referenced this pull request Jul 29, 2020

Release 2020.4

bc8b2fa

Thin release. Mostly for a bunch of RHCOS-relevant fixes and enhancements: 752f4f0 (coreos#2178), 4d836dd (coreos#2151) and f608eb0 (coreos#2158).

jlebon mentioned this pull request Jul 29, 2020

Release 2020.4 #2180

Merged

openshift-merge-robot pushed a commit that referenced this pull request Jul 29, 2020

Release 2020.4

bcf241d

Thin release. Mostly for a bunch of RHCOS-relevant fixes and enhancements: 752f4f0 (#2178), 4d836dd (#2151) and f608eb0 (#2158).

miabbott mentioned this pull request Aug 12, 2020

RFE: support greater than/less than versions for packages in treefile #2138

Closed

Support greater than/less than versions for packages in treefile. #2151

Support greater than/less than versions for packages in treefile. #2151

Conversation

kelvinfan001 commented Jun 26, 2020

cgwalters left a 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

zonggen left a comment • edited Loading

Choose a reason for hiding this comment

zonggen commented Jun 29, 2020

kelvinfan001 commented Jun 30, 2020 • edited Loading

jlebon commented Jun 30, 2020

kelvinfan001 commented Jun 30, 2020

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

cgwalters commented Jul 1, 2020

jlebon commented Jul 2, 2020

Choose a reason for hiding this comment

kelvinfan001 Jul 2, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cgwalters commented Jul 2, 2020

kelvinfan001 commented Jul 2, 2020

jlebon left a 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 Jul 3, 2020

jlebon commented Jul 6, 2020

cgwalters commented Jul 8, 2020

openshift-ci-robot commented Jul 8, 2020

zonggen left a comment •

edited

Loading

kelvinfan001 commented Jun 30, 2020 •

edited

Loading

kelvinfan001 Jul 2, 2020 •

edited

Loading