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

[Dodji Seketeli <dodji@redhat.com>]

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