public inbox for libabigail@sourceware.org
 help / color / mirror / Atom feed
* Re: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly
  2017-01-01  0:00     ` Chenxiong Qi
@ 2017-01-01  0:00       ` Dodji Seketeli
  0 siblings, 0 replies; 6+ messages in thread
From: Dodji Seketeli @ 2017-01-01  0:00 UTC (permalink / raw)
  To: Chenxiong Qi; +Cc: Sinny Kumari, Abigail Project Mailing List

Chenxiong Qi <cqi@redhat.com> a écrit:

>> +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)
>
> Or just use koji.parse_NVR?

str is not an NVR.  What I want is the part of R that designates the
*distro* version number.  Not the version of the package, but the
version of the *distro*.

Alas, koji.parse_NVR expects a full NVR string and breaks it down into
an N, a V and an R.  Again, I already have the R, and I want some
sub-part of it.

Or what am I missing?

>
>> +    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 instance, the build of package foo that
>> +            # 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_from_string(build['release']) and \
>> +                       get_distro_from_string(build['release']) <= distro
>
> Suggest to merge this selector with above selector together, that
> saves another request to Koji by calling listBuilds API.

I think it's more complicated than that.

Here is what I want.

If there is a build that matches *exactly* the distro version number, I want
self.listBuilds to return *just* that one build.  And this is what the
initial selector (the one you refer to as 'above') does.  But note that
the initial selector has to run on *all* the builds returned by
self.listBuilds, because maybe the build we want is the last one.  OK?

And then, if there is no build that is matched by the first selector,
then (and only then) I want listBuilds to return the set of all the
builds that are lexicographically less than the distro version number.
This set can be built progressively as listBuilds applies the selector
to the builds it has.

So I don't think the two selectors can be merged in a "simple and
readable" way.  And as for the performance impact (two requests instead
of one), I'd rather first measure the performance impact of this before
going into early and potentially useless optimization that would lead to
an unreadable code.

But if you think I am wrong (which I can very well be), please feel free
to come up with a better way.  I am not a Python fan, I have written too
much python with this already :-)

Cheers,

-- 
		Dodji

^ permalink raw reply	[flat|nested] 6+ messages in thread

* [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly
@ 2017-01-01  0:00 Dodji Seketeli
  2017-01-01  0:00 ` Sinny Kumari
  0 siblings, 1 reply; 6+ messages in thread
From: Dodji Seketeli @ 2017-01-01  0:00 UTC (permalink / raw)
  To: libabigail; +Cc: cqi

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
+            # 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
+
+            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
		Dodji

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly
  2017-01-01  0:00   ` Dodji Seketeli
@ 2017-01-01  0:00     ` Sinny Kumari
  2017-01-01  0:00     ` Chenxiong Qi
  1 sibling, 0 replies; 6+ messages in thread
From: Sinny Kumari @ 2017-01-01  0:00 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Abigail Project Mailing List, Chenxiong Qi

On Wed, Jun 14, 2017 at 7:54 PM, Dodji Seketeli <dodji@redhat.com> wrote:
> Sinny Kumari <ksinny@gmail.com> a écrit:
>
> [...]
>
>>>          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/
>
> Good catch.  I have fixed this in the patch below.
>
>>
>>> +            # 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.
>
> And you are right again.  Oops.  I am fixing this too in the patch
> below.
>
> So here is the update patch.

+1 from me

> Thanks!
>
> From 08d297dd35e6bde7638787a246c181af3c534d72 Mon Sep 17 00:00:00 2001
> From: Dodji Seketeli <dodji@redhat.com>
> Date: Wed, 14 Jun 2017 11:08:19 +0200
> Subject: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too
>  tightly
>
> The build libxcb-1.12-3.fc26 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.
>
>         * 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..20cfd19 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 instance, the build of package foo that
> +            # 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_from_string(build['release']) and \
> +                       get_distro_from_string(build['release']) <= distro
> +
> +            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
>                 Dodji



-- 
http://sinny.io/

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly
  2017-01-01  0:00 ` Sinny Kumari
@ 2017-01-01  0:00   ` Dodji Seketeli
  2017-01-01  0:00     ` Sinny Kumari
  2017-01-01  0:00     ` Chenxiong Qi
  0 siblings, 2 replies; 6+ messages in thread
From: Dodji Seketeli @ 2017-01-01  0:00 UTC (permalink / raw)
  To: Sinny Kumari; +Cc: Abigail Project Mailing List, Chenxiong Qi

Sinny Kumari <ksinny@gmail.com> a écrit:

[...]

>>          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/

Good catch.  I have fixed this in the patch below.

>
>> +            # 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.

And you are right again.  Oops.  I am fixing this too in the patch
below.

So here is the update patch.

Thanks!

From 08d297dd35e6bde7638787a246c181af3c534d72 Mon Sep 17 00:00:00 2001
From: Dodji Seketeli <dodji@redhat.com>
Date: Wed, 14 Jun 2017 11:08:19 +0200
Subject: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too
 tightly

The build libxcb-1.12-3.fc26 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.

	* 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..20cfd19 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 instance, the build of package foo that
+            # 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_from_string(build['release']) and \
+                       get_distro_from_string(build['release']) <= distro
+
+            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
		Dodji

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly
  2017-01-01  0:00   ` Dodji Seketeli
  2017-01-01  0:00     ` Sinny Kumari
@ 2017-01-01  0:00     ` Chenxiong Qi
  2017-01-01  0:00       ` Dodji Seketeli
  1 sibling, 1 reply; 6+ messages in thread
From: Chenxiong Qi @ 2017-01-01  0:00 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Sinny Kumari, Abigail Project Mailing List

> +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)

Or just use koji.parse_NVR?

> +    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 instance, the build of package foo that
> +            # 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_from_string(build['release']) and \
> +                       get_distro_from_string(build['release']) <= distro

Suggest to merge this selector with above selector together, that
saves another request to Koji by calling listBuilds API.

> +
> +            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
>                 Dodji

LGTM with above two suggestions.

-- 
--
Regards,
Chenxiong Qi

^ permalink raw reply	[flat|nested] 6+ messages in thread

* Re: [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly
  2017-01-01  0:00 [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly Dodji Seketeli
@ 2017-01-01  0:00 ` Sinny Kumari
  2017-01-01  0:00   ` Dodji Seketeli
  0 siblings, 1 reply; 6+ messages in thread
From: Sinny Kumari @ 2017-01-01  0:00 UTC (permalink / raw)
  To: Dodji Seketeli; +Cc: Abigail Project Mailing List, Chenxiong Qi

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/

^ permalink raw reply	[flat|nested] 6+ messages in thread

end of thread, other threads:[~2017-06-27  9:39 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-01-01  0:00 [PATCH] Bug 21567 - Fedabipkgdiff matches build distro names too tightly Dodji Seketeli
2017-01-01  0:00 ` Sinny Kumari
2017-01-01  0:00   ` Dodji Seketeli
2017-01-01  0:00     ` Sinny Kumari
2017-01-01  0:00     ` Chenxiong Qi
2017-01-01  0:00       ` Dodji Seketeli

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).