Re: [PATCH] Correct sorting of kernel names containing '_'

[Glenn Washburn]

On Tue, 18 Jan 2022 12:56:34 -0500
Robbie Harwood <rharwood@redhat.com> wrote:

> Robbie Harwood <rharwood@redhat.com> writes:
> 
> > sort(1) from GNU coreutils does not treat underscore as part of a
> > version number for `sort -V.  This causes misorderings on x86_64, where
> > e.g. kernel-core-3.17.6-300.11.fc21.x86_64 will incorrectly sort
> > *before* kernel-core-3.17.6-300.fc21.x86_64.
> >
> > To cope with this behavior, replace underscores with dashes in order to
> > approximate their intended meaning as version component separators.
> >
> > Fixes: https://savannah.gnu.org/bugs/?42844
> > Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> > ---
> >  util/grub-mkconfig_lib.in | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/util/grub-mkconfig_lib.in b/util/grub-mkconfig_lib.in
> > index 301d1ac22..23d41475f 100644
> > --- a/util/grub-mkconfig_lib.in
> > +++ b/util/grub-mkconfig_lib.in
> > @@ -243,8 +243,8 @@ version_test_numeric ()
> >  
> >  version_test_gt ()
> >  {
> > -  version_test_gt_a="`echo "$1" | sed -e "s/[^-]*-//"`"
> > -  version_test_gt_b="`echo "$2" | sed -e "s/[^-]*-//"`"
> > +  version_test_gt_a="`echo "$1" | sed -e "s/[^-]*-//" -e "s/_/-/g"`"
> > +  version_test_gt_b="`echo "$2" | sed -e "s/[^-]*-//" -e "s/_/-/g"`"
> >    version_test_gt_cmp=gt
> >    if [ "x$version_test_gt_b" = "x" ] ; then
> >      return 0
> > -- 
> > 2.34.1
> 
> Before yinz bikeshed this into the ground, here are some other
> approaches I considered:

Let the bikeshedding begin! Just kidding :)
This seems like a simple solution. Not being in the weeds on this, can
you comment on the short coming of changing the '_' to '-'? Are there
any known version string formats where this would be worse? At the
moment, it seems like a good incremental solution.

> 
> - Use a C program to invoke librpm.  This has the advantage of being
>   already written - it's what we currently do downstream:
>   
> https://github.com/rhboot/grub2/commit/81e155df39c9fbc97803cf9032f4138f5d9a0310
>   I dislike the approach regardless because it's heavyweight and doesn't
>   generalize to non-RPM distros (especially since e.g. Debian can
>   install a /usr/bin/rpm & co.).  But it is "more correct", so if you
>   want to merge that instead, there's the patch :)

Agreed, not a fan of this approach.

> - Invoke rpmdev-vercmp if available.  Problem here is that it may not be
>   (it's not part of the rpm package itself on Fedora, but rather
>   rpmdev-tools), and again, won't generalize to non-RPM distros.

I like this as a supplement to this patch. I'm thinking first identify
the distro (using /etc/os-release?), if known RPM distro look for
rpmdev-vercmp, else look for dpkg (to use dpkg --compare-versions), and
if neither is found fall back to this approach. This could be extended
to other package manager compare logic as desired.

> - Strip out any dot-delimited section that contains non-numerics.  This
>   has also been written and is the attached solution to
>   https://savannah.gnu.org/bugs/?42844 - but since it hasn't been merged
>   in the six years since posting, I have to assume it's been rejected.
>   It also would not correctly handle the case of
>   e.g. kernel-core-2.6.32-1_test2.x86_64 vs kernel-core-2.6.32-1.x86_64

This patch actually removes the last part of the version string
starting with the first dot segment whose first character is
non-numeric. Regardless, this doesn't seem like a great approach.

> - Strip underscore entirely, either with sed or tr.  This has the same
>   problem as the previous one in my mind - underscore is intended as a
>   separator, and this causes it to not be treated as one.

Agreed.

> - Ignore the sorting problem and keep the status quo as it is in
>   upstream.  Attractive, but my bugzilla mailbox has plenty of angry
>   people in it already :)

I think at a minimum this patch seems like a step in the right
direction. It would be nice to have it accompanied by my suggestions
above, but also recognize its a non-insignificant amount of effort.

Glenn