Re: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly

[Sinny Kumari <ksinny@gmail.com>]

Hi Dodji,

Thanks for the quick patch!
Overall, it looks good to me. Have minor comments, mentioned inline.

On Wed, Jun 14, 2017 at 3:25 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> Hello Chenxiong,
>
> This patch fixes the Fedabipkgdiff problem that Sinny Kumari filed at
> https://sourceware.org/bugzilla/show_bug.cgi?id=21567.
>
> To understand the problem, consider the build libxcb-1.12-3.fc26, which
> has been tagged for Fedora 27 (i.e, fc27).
>
> When we ask fedabipkgdiff to get the builds of libxcb for Fedora 27,
> it looks for builds which release string ends with 'fc27'.  It thus
> fails to pick libxcb-1.12-3.fc26.
>
> This patch changes this behaviour by making
> Brew.get_package_latest_build to first try to get builds which release
> string match exactly the distro string we are looking at.
>
> But then if no build was found, the member function then tries to get
> builds for which the distro part of the release string is "less than"
> (in a lexicographic way) the distro string we are looking at.
>
> I haven't added any regression test specifically for this because we
> are planning to use the libabigail-selfcheck external tool to perform
> regression testing on tons of Fedora packages:
> https://pagure.io/libabigail-selfcheck.  Fixing this issue is a
> pre-requisite for putting that regression test infrastructure in
> place.
>
> Is it OK to commit this into the master branch?
>
> Thanks.
>
>         * tools/fedabipkgdiff (get_distro_from_string): Define new function.
>         (Brew.get_package_latest_build): Also consider builds which distro
>         property is less than the expected distro string that we were
>         given.
>
>
> Signed-off-by: Dodji Seketeli <dodji@redhat.com>
> ---
>  tools/fedabipkgdiff | 43 ++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 42 insertions(+), 1 deletion(-)
>
> diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
> index a8329d4..42287fe 100755
> --- a/tools/fedabipkgdiff
> +++ b/tools/fedabipkgdiff
> @@ -172,6 +172,26 @@ def is_distro_valid(distro):
>      return re.match(r'^(fc|el)\d{1,2}$', distro) is not None
>
>
> +def get_distro_from_string(str):
> +    """Get the part of a string that designates the Fedora distro version number
> +
> +    For instance, when passed the string '2.3.fc12', this function
> +    returns the string 'fc12'.
> +
> +    :param str the string to consider
> +    :return: The sub-string of the parameter that represents the
> +    Fedora distro version number, or None if the parameter does not
> +    contain such a sub-string.
> +    """
> +
> +    m = re.match(r'(.*)((fc|el)\d{1,2})(.*)', str)
> +    if not m:
> +        return None
> +
> +    distro = m.group(2)
> +    return distro
> +
> +
>  def match_nvr(s):
>      """Determine if a string is a N-V-R"""
>      return re.match(r'^([^/]+)-(.+)-(.+)$', s) is not None
> @@ -776,7 +796,7 @@ class Brew(object):
>
>      @log_call
>      def get_package_latest_build(self, package_name, distro):
> -        """Get latest build from a package
> +        """Get latest build from a package, for a particular distro.
>
>          Example:
>
> @@ -797,6 +817,27 @@ class Brew(object):
>                                   order_by='nvr',
>                                   reverse=True)
>          if not builds:
> +            # So we found no build which distro string exactly matches
> +            # the 'distro' parameter.
> +            #
> +            # Now lets try to get builds which distro string are less
> +            # than the value of the 'distro' parameter.  This is for
> +            # cases when, for instnace, the build of package foo that

Small typo, s/instnace/instance/

> +            # is present in current Fedora 27 is foo-1.fc26.  That
> +            # build originates from Fedora 26 but is being re-used in
> +            # Fedora 27.  So we want this function to pick up that
> +            # foo-1.fc26, even though we want the builds of foo that
> +            # match the distro string fc27.
> +
> +            selector = lambda build: get_distro(build['release']) and \
> +                       get_distro(build['release']) <= distro

You probably wanted to mention get_distro_from_string instead of
get_distro because newly defined function name is get_distro_from_string.

> +
> +            builds = self.listBuilds(packageID=package['id'],
> +                                 selector=selector,
> +                                 order_by='nvr',
> +                                 reverse=True);
> +
> +        if not builds:
>              raise NoCompleteBuilds(
>                  'No complete builds of package {0}'.format(package_name))
>
> --
> 1.8.3.1

Thanks,
Sinny

-- 
http://sinny.io/