Re: [PATCH v3 2/2] mkconfig: use distro sorts when available

[Glenn Washburn]

On Thu, 20 Jan 2022 14:33:20 -0500
Robbie Harwood <rharwood@redhat.com> wrote:

> For Debian-likes and Fedora-likes, use the distribution's sorting tools
> to determine the latest package before falling back to sort(1).  While
> Fedora's rpmdev-vercmp(1) is likely unavailable, Debian's is built into
> dpkg itself, and hence likely present.
> 
> Refactor to remove unused code and make it easy to add other package
> managers as needed.
> 
> Signed-off-by: Robbie Harwood <rharwood@redhat.com>
> ---
>  util/grub-mkconfig_lib.in | 92 ++++++++++++++++++++++-----------------
>  1 file changed, 51 insertions(+), 41 deletions(-)
> 
> diff --git a/util/grub-mkconfig_lib.in b/util/grub-mkconfig_lib.in
> index 23d41475f..b858cd1a0 100644
> --- a/util/grub-mkconfig_lib.in
> +++ b/util/grub-mkconfig_lib.in
> @@ -200,62 +200,72 @@ grub_file_is_not_garbage ()
>    return 0
>  }
>  
> -version_sort ()
> +# A $SORT function returns 0 if $1 is newer than $2, and 1 otherwise.  Other
> +# package managers can be plugged in here as needed with their own functions.
> +sort_dpkg ()
>  {
> -  case $version_sort_sort_has_v in
> -    yes)
> -      LC_ALL=C sort -V;;
> -    no)
> -      LC_ALL=C sort -n;;
> -    *)
> -      if sort -V </dev/null > /dev/null 2>&1; then
> -        version_sort_sort_has_v=yes
> -     LC_ALL=C sort -V
> -      else
> -        version_sort_sort_has_v=no
> -        LC_ALL=C sort -n
> -      fi;;
> -   esac
> +  left="`echo "$1" | sed -e "s/^[^0-9]*//"`"
> +  right="`echo "$2" | sed -e "s/^[^0-9]*//"`"
> +  dpkg --compare-versions "$left" gt "$right"
>  }
>  
> -version_test_numeric ()
> +sort_rpm ()
>  {
> -  version_test_numeric_a="$1"
> -  version_test_numeric_cmp="$2"
> -  version_test_numeric_b="$3"
> -  if [ "$version_test_numeric_a" = "$version_test_numeric_b" ] ; then
> -    case "$version_test_numeric_cmp" in
> -      ge|eq|le) return 0 ;;
> -      gt|lt) return 1 ;;
> -    esac
> -  fi
> -  if [ "$version_test_numeric_cmp" = "lt" ] ; then
> -    version_test_numeric_c="$version_test_numeric_a"
> -    version_test_numeric_a="$version_test_numeric_b"
> -    version_test_numeric_b="$version_test_numeric_c"
> -  fi
> -  if (echo "$version_test_numeric_a" ; echo "$version_test_numeric_b") | 
> version_sort | head -n 1 | grep -qx "$version_test_numeric_b" ; then
> -    return 0
> -  else
> -    return 1
> +  left="`echo "$1" | sed -e "s/^[^0-9]*//"`"
> +  right="`echo "$2" | sed -e "s/^[^0-9]*//"`"
> +  rpmdev-vercmp "$left" "$right" >/dev/null
> +  if [ $? -eq 12 ]; then
> +    return 0;
>    fi
> +  return 1;
> +}
> +
> +sort_V ()
> +{
> +  left="`echo "$1" | sed -e "s/[^-]*-//" -e "s/_/-/g"`"
> +  right="`echo "$2" | sed -e "s/[^-]*-//" -e "s/_/-/g"`"
> +  printf "$left\n$right\n" | LC_ALL=C sort -V | head -n1 | grep -qx "$right"
> +}
> +
> +sort_n ()
> +{
> +  left="`echo "$1" | sed -e "s/[^-]*-//" -e "s/_/-/g"`"
> +  right="`echo "$2" | sed -e "s/[^-]*-//" -e "s/_/-/g"`"
> +  printf "$left\n$right\n" | LC_ALL=C sort -n | head -n1 | grep -qx "$right"
>  }
>  
>  version_test_gt ()
>  {
> -  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
> +  version_test_gt_a="$1"
> +  version_test_gt_b="$2"
> +
>    if [ "x$version_test_gt_b" = "x" ] ; then
>      return 0
>    fi
>    case "$version_test_gt_a:$version_test_gt_b" in
>      *.old:*.old) ;;
> -    *.old:*) version_test_gt_a="`echo "$version_test_gt_a" | sed -e 
> 's/\.old$//'`" ; version_test_gt_cmp=gt ;;
> -    *:*.old) version_test_gt_b="`echo "$version_test_gt_b" | sed -e 
> 's/\.old$//'`" ; version_test_gt_cmp=ge ;;
> +    *.old:*) version_test_gt_a="`echo "$version_test_gt_a" | sed -e 
> 's/\.old$//'`" ;;
> +    *:*.old) version_test_gt_b="`echo "$version_test_gt_b" | sed -e 
> 's/\.old$//'`" ;;
>    esac
> -  version_test_numeric "$version_test_gt_a" "$version_test_gt_cmp" 
> "$version_test_gt_b"
> -  return "$?"
> +
> +  if [ "$version_test_gt_a" = "$version_test_gt_b" ]; then
> +    return 1;
> +  fi
> +
> +  if [ x"$SORT" = x ]; then
> +    if [ -f /etc/debian_version] && command -v dpkg >/dev/null; then

I think without the space before ']' the test won't be evaluated
correctly.

> +      SORT=sort_dpkg

At first I was wondering why you left out the elif clause that tests for
/etc/redhat-release. But on second thought it does seem to not be
needed because if rpmdev-vercmp exists then we want to use it next
regardless. Even though its not needed, I think the excluded elif clause
makes explicit the intent. If next two elif clauses get swapped, then
we will potentially preferring dpkg over rpmdev-vercmp on redhat based
distros (not good). Perhaps that's an unlikely scenario. I think we
should add a comment here saying something like, "No need to explicitly
check for Redhat-based distros because we want to use rpmdev-vercmp
next if it exists regardless of distro".

My thinking is based on the assumption that the version compare that
rpmdev-vercmp does is different that dpkg and better suited to
Redhat-based distros. Is this a valid assumption? If they are the same
algorithm, then none of this matters. There's also the assumption that
if rpmdev-vercmp, its highly likely that this is on a primarily rpm
based distro.

An even more complete solution would check which package manager
installed the kernel and use that to choose which sorting method to
use. That's probably overkill for now since this is only for doing
the right thing when on non-RPM based distros when both rpmdev-vercmp
and dpkg are installed, and I suspect that's an extremely tiny
minority. And either way we can make changes when/if someone complains.

Glenn

> +    elif command -v rpmdev-vercmp; then
> +      SORT=sort_rpm
> +    elif command -v dpkg >/dev/null; then
> +      SORT=sort_dpkg
> +    elif sort -V </dev/null > /dev/null 2>&1; then
> +      SORT=sort_V
> +    else
> +      SORT=sort_n
> +    fi
> +  fi
> +  $SORT "$version_test_gt_a" "$version_test_gt_b"
>  }
>  
>  version_find_latest ()