Re: [Patch] selecting latest build in a stable way

Re: [Patch] selecting latest build in a stable way

[Chenxiong Qi]

On 06/08/2016 09:19 AM, Chenxiong Qi wrote:
> Hi,
>
> This patch aims to sort builds of a package by creation_event_id rather
> than nvr to get the latest build. This is a stable way and a better choice.
>

Well, while I'm on my way home, I just realized that getting the latest 
build by sorting creation_event_id is not a good choice. For example, a 
packager built package foo-2.0-1.fc25, and then he built an old version 
such as foo-1.0-2.fc25, obviously, the latest build should be the former 
one, which has a smaller creation_event_id than latter's. So, let me 
update patch with another way. Sorry for this.

-- 
Regards,
Chenxiong Qi

Re: [Patch] selecting latest build in a stable way

[Dodji Seketeli]

Hello Chenxiong,

Chenxiong Qi <qcxhome@yahoo.com> a écrit:

[...]

> When getting latest builds, builds are sorted by nvr rather than
> creation_event_id. This is the right choice as mentioned in my
> previous mail.

OK.

[...]

> Thank you for pointing out this mistake for me.

No problem :-)

I still believe you should update the cover letter and the ChangeLog of
the patch because this ...

> Generally, koji provides event for each creation of resources, for
> example build, rpm, task. So, selecting latest build by attribute nvr
> is not a stable way, instead, creation_event_id is the right choice.

 ... doesn't reflect what it does anymore.

And this ...

> 	* tools/fedabipkgdiff: (Brew.get_package_latest_build): order
> 	builds by creation_event_id.

... doesn't reflect it anymore either.

Also, it seems the patch doesn't apply to master since I committed your
other patch that makes fedabipkgdiff take development packages into
account when comparing ABIs.  So it would be nice if you could update it
to make it apply.  Thanks.

Cheers,

-- 
		Dodji

Re: [Patch] selecting latest build in a stable way

[Chenxiong Qi]

[-- Attachment #1: Type: text/plain, Size: 814 bytes --]



On 06/08/2016 08:12 PM, Chenxiong Qi wrote:
>
>
> On 06/08/2016 09:19 AM, Chenxiong Qi wrote:
>> Hi,
>>
>> This patch aims to sort builds of a package by creation_event_id rather
>> than nvr to get the latest build. This is a stable way and a better
>> choice.
>>
>
> Well, while I'm on my way home, I just realized that getting the latest
> build by sorting creation_event_id is not a good choice. For example, a
> packager built package foo-2.0-1.fc25, and then he built an old version
> such as foo-1.0-2.fc25, obviously, the latest build should be the former
> one, which has a smaller creation_event_id than latter's. So, let me
> update patch with another way. Sorry for this.
>

It's done. rpm.labelCompare is used this time to compare two NVRs while 
sorting builds by sorted.

-- 
Regards,
Chenxiong Qi

[-- Attachment #2: 0001-selecting-latest-build-in-a-stable-way.patch --]
[-- Type: application/octetstream, Size: 7935 bytes --]

[Patch] selecting latest build in a stable way

[Chenxiong Qi]

[-- Attachment #1: Type: text/plain, Size: 181 bytes --]

Hi,

This patch aims to sort builds of a package by creation_event_id rather 
than nvr to get the latest build. This is a stable way and a better choice.

-- 
Regards,
Chenxiong Qi

[-- Attachment #2: 0001-selecting-latest-build-in-a-stable-way.patch --]
[-- Type: application/octetstream, Size: 5673 bytes --]

Re: [Patch] selecting latest build in a stable way

[Dodji Seketeli]

Hello,

Chenxiong Qi <qcxhome@yahoo.com> a écrit:

[...]

> Well, while I'm on my way home, I just realized that getting the
> latest build by sorting creation_event_id is not a good choice.

OK, I understand that.

So I understand when you say this in preamble of your latest patch:

    It's done. rpm.labelCompare is used this time to compare two NVRs
    while sorting builds by sorted.

But then, your last patch still says in its cover letter:

    Generally, koji provides event for each creation of resources, for
    example build, rpm, task. So, selecting latest build by attribute
    nvr is not a stable way, instead, creation_event_id is the right
    choice.

So I believe there is a contradiction in your intent, at least.  What do
you want to do?  Sort by creation_event_id or sort by NVR using the
rpm.labelCompare function?

Furthermore, looking at the code of the patch itself I see this:

diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff

[...]

 def log_call(func):
     """A decorator that logs a method invocation
 
@@ -433,8 +454,10 @@ class Brew(object):
         if order_by is not None:
             # FIXME: is it possible to sort builds by using opts parameter of
             # listBuilds
+            cmp_func = cmp_nvr if order_by == 'nvr' else None
             builds = sorted(builds,
                             key=lambda item: item[order_by],
+                            cmp=cmp_func,
                             reverse=reverse)

So here, in the Brew.listBuilds member function, you are using the new
cmp_nvr function to sort by NVR *if* Brew.listBuilds is called with its
order_by parameter set to the  'nvr' argument.

But then, Brew.get_package_latest_build is later calling Brew.listBuilds
with its order_by parameter set to 'creation_event_id' here:

         if topone:
             builds = builds[0:1]
@@ -535,7 +558,7 @@ class Brew(object):
 
         builds = self.listBuilds(packageID=package['id'],
                                  selector=selector,
-                                 order_by='nvr',
+                                 order_by='creation_event_id',
                                  reverse=True)
         if not builds:
             raise NoCompleteBuilds(

So my understanding is that this patch is still ordering by
'creation_event_id' even if you said it was not a good idea.

What am I missing?

Cheers,

-- 
		Dodji

Re: [Patch] selecting latest build in a stable way

[Chenxiong Qi]

[-- Attachment #1: Type: text/plain, Size: 1329 bytes --]



On 06/10/2016 03:44 PM, Dodji Seketeli wrote:
> Hello Chenxiong,
>
> Chenxiong Qi <qcxhome@yahoo.com> a écrit:
>
> [...]
>
>> When getting latest builds, builds are sorted by nvr rather than
>> creation_event_id. This is the right choice as mentioned in my
>> previous mail.
>
> OK.
>
> [...]
>
>> Thank you for pointing out this mistake for me.
>
> No problem :-)
>
> I still believe you should update the cover letter and the ChangeLog of
> the patch because this ...
>
>> Generally, koji provides event for each creation of resources, for
>> example build, rpm, task. So, selecting latest build by attribute nvr
>> is not a stable way, instead, creation_event_id is the right choice.
>
>  ... doesn't reflect what it does anymore.
>
> And this ...
>
>> 	* tools/fedabipkgdiff: (Brew.get_package_latest_build): order
>> 	builds by creation_event_id.
>
> ... doesn't reflect it anymore either.
>
> Also, it seems the patch doesn't apply to master since I committed your
> other patch that makes fedabipkgdiff take development packages into
> account when comparing ABIs.  So it would be nice if you could update it
> to make it apply.  Thanks.
>
> Cheers,
>

Hi Dodji,

Both cover letter and changelog are updated. This patch is also rebased 
on the master branch. Please review again. Thanks :)

-- 
Regards,
Chenxiong Qi

[-- Attachment #2: 0001-selecting-latest-build-in-a-stable-way.patch --]
[-- Type: application/octetstream, Size: 6009 bytes --]

Re: [Patch] selecting latest build in a stable way

[Dodji Seketeli]

Hello Chenxiong,

Chenxiong Qi <qcxhome@yahoo.com> a écrit:

> Both cover letter and changelog are updated. This patch is also
> rebased on the master branch.

Thanks!  It's appreciated!

I committed it to master.  I just did a slight wording amendment to the
cover letter and I fixed the ChangeLog to make it mention the file
tools/fedabipkgdiff and also mention the change in Brew.listBuilds.

I hope this is OK.

> While sorting a package's builds by nvr to select the latest build,
> comparing nvrs as string type is not a stable way, for example
> foo-0.10-1.fc25 and foo-0.2-1.fc25, latter one will be selected as the
> latest build, but the former one is the right one. So, after research on
> this, I find rpm.labelCompare is a better choice to compare two NVRs to
> get the expected result.
>
>  Meanwhile, why I choose rpm.labelCompare finally is because the latest
>  build in fedabipkgdiff means a build with the latest version.release
>  within a specific Fedora dist such as fc23, fc25.
>
> 	* configure.ac: add new dependency.
> 	* tests/runtestfedabipkgdiff.py.in: (builds): add new builds
> 	for running tests to test selecting latest build from a package.
> 	(packages): add new package gnutls.
> 	(GetPackageLatestBuildTest.{test_get_latest_one,
> 	test_cannot_find_a_latest_build_with_invalid_distro}): use new
> 	builds of package gnutls to run tests.
> 	(cmp_nvr): new method used to compare nvrs by Python built-in
> 	function sorted.

So, the new commit subject and log are now:

    Fix package NVR comparison in fedabipkgdiff
    
    When sorting two packages by their {Name Value Release} triplet to
    select the latest one, just doing a string comparison of the NVRs is
    wrong.  Take for example the packages foo-0.10-1.fc25 and
    foo-0.2-1.fc25.  A basic string comparison will result in the string
    "foo-0.10-1.fc25" being less than "foo-0.2-1.fc25", and thus
    foo-0.2-1.fc25 will be selected as the latest package.  And that is
    wrong, because the latest one is obviously foo-0.10-1.fc25.
    
    So, after some research on this, I figured rpm.labelCompare is a
    better choice to appropriately compare two NVRs.
    
    Another reason why I chose rpm.labelCompare is because the latest
    build in fedabipkgdiff means a build with the latest version.release
    within a specific Fedora distribution such as fc23 or fc25.
    
    	* configure.ac: Add new dependency.
    	* tests/runtestfedabipkgdiff.py.in (builds): Add new builds for
    	running tests to test selecting latest build from a package.
    	(packages): Add new package gnutls.
    	(GetPackageLatestBuildTest.{test_get_latest_one,
    	test_cannot_find_a_latest_build_with_invalid_distro}): Use new
    	builds of package gnutls to run tests.
    	* tools/fedabipkgdiff (cmp_nvr): New function used to compare nvrs
    	by Python built-in function sorted.
    	(Brew.listBuilds): Use the new cmp_nvr function.


Thanks again!

Cheers,

-- 
		Dodji

Re: [Patch] selecting latest build in a stable way

[Chenxiong Qi]

[-- Attachment #1: Type: text/plain, Size: 2937 bytes --]



On 06/09/2016 05:55 PM, Dodji Seketeli wrote:
> Hello,
>
> Chenxiong Qi <qcxhome@yahoo.com> a écrit:
>
> [...]
>
>> Well, while I'm on my way home, I just realized that getting the
>> latest build by sorting creation_event_id is not a good choice.
>
> OK, I understand that.
>
> So I understand when you say this in preamble of your latest patch:
>
>     It's done. rpm.labelCompare is used this time to compare two NVRs
>     while sorting builds by sorted.
>
> But then, your last patch still says in its cover letter:
>
>     Generally, koji provides event for each creation of resources, for
>     example build, rpm, task. So, selecting latest build by attribute
>     nvr is not a stable way, instead, creation_event_id is the right
>     choice.
>
> So I believe there is a contradiction in your intent, at least.  What do
> you want to do?  Sort by creation_event_id or sort by NVR using the
> rpm.labelCompare function?
>
> Furthermore, looking at the code of the patch itself I see this:
>
> diff --git a/tools/fedabipkgdiff b/tools/fedabipkgdiff
>
> [...]
>
>  def log_call(func):
>      """A decorator that logs a method invocation
>
> @@ -433,8 +454,10 @@ class Brew(object):
>          if order_by is not None:
>              # FIXME: is it possible to sort builds by using opts parameter of
>              # listBuilds
> +            cmp_func = cmp_nvr if order_by == 'nvr' else None
>              builds = sorted(builds,
>                              key=lambda item: item[order_by],
> +                            cmp=cmp_func,
>                              reverse=reverse)
>
> So here, in the Brew.listBuilds member function, you are using the new
> cmp_nvr function to sort by NVR *if* Brew.listBuilds is called with its
> order_by parameter set to the  'nvr' argument.
>
> But then, Brew.get_package_latest_build is later calling Brew.listBuilds
> with its order_by parameter set to 'creation_event_id' here:
>
>          if topone:
>              builds = builds[0:1]
> @@ -535,7 +558,7 @@ class Brew(object):
>
>          builds = self.listBuilds(packageID=package['id'],
>                                   selector=selector,
> -                                 order_by='nvr',
> +                                 order_by='creation_event_id',

Sorry for making you confused. When getting latest builds, builds are 
sorted by nvr rather than creation_event_id. This is the right choice as 
mentioned in my previous mail. So, it should not change nvr to 
creation_event_id of parameter order_by. An updated patch is attached. 
Please review again.

Thank you for pointing out this mistake for me.

>                                   reverse=True)
>          if not builds:
>              raise NoCompleteBuilds(
>
> So my understanding is that this patch is still ordering by
> 'creation_event_id' even if you said it was not a good idea.
>
> What am I missing?


>
> Cheers,
>

-- 
Regards,
Chenxiong Qi

[-- Attachment #2: 0001-selecting-latest-build-in-a-stable-way.patch --]
[-- Type: application/octetstream, Size: 7556 bytes --]