Linux Archive

Linux Archive (http://www.linux-archive.org/)
-   ArchLinux Pacman Development (http://www.linux-archive.org/archlinux-pacman-development/)
-   -   vercmp discussion (was: String freeze for 3.2 release) (http://www.linux-archive.org/archlinux-pacman-development/126443-vercmp-discussion-string-freeze-3-2-release.html)

Xavier 07-17-2008 12:20 PM

vercmp discussion (was: String freeze for 3.2 release)
 
On Wed, Jul 16, 2008 at 10:17 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
>
>> nicer, but harder ;-)
>> See 84283672853350a84d2a71b72dc06e180cad1587, search for 'type
>> mismatch'.
>>
> Hm. Wrong. With the new vercmp: 1.1 > 1.b (but in ASCII 'b' > '1'),
> strange. Our problem is at the end of string: '' vs. alpha, as I see,
> this case was handled after comment /* see if we ran out of segments on
> one string */
>

It indeed looks like we just need to handle the case where it runs out
of segments on one string.
But we have to handle two cases : run out of segments with the
-release number or without it.
So in both cases, I handle it differently if the last remaining
segment starts with a letter or not.

I am not 100% sure that it will work correctly in every single cases,
but the logic seems alright, there is no regression on all existing
vercmp test, and the 4 new tests you posted in an older thread now
pass fine.
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier 07-17-2008 12:22 PM

vercmp discussion (was: String freeze for 3.2 release)
 
I made a typo in one of the comment inside the patch : above casse ->
above case .

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

"Dan McGee" 07-17-2008 03:37 PM

vercmp discussion (was: String freeze for 3.2 release)
 
On Thu, Jul 17, 2008 at 10:23 AM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
>> It indeed looks like we just need to handle the case where it runs out
>> of segments on one string.
>> But we have to handle two cases : run out of segments with the
>> -release number or without it.
>> So in both cases, I handle it differently if the last remaining
>> segment starts with a letter or not.
>>
>> I am not 100% sure that it will work correctly in every single cases,
>> but the logic seems alright, there is no regression on all existing
>> vercmp test, and the 4 new tests you posted in an older thread now
>> pass fine.
>
> I did a quick read on the new vercmp code. This is not an easy-to-read
> code (and IMHO it is a bit overkill: strdup, free? - this is not
> related to your patch), but it seems OK to me. Overall it is much better
> than pre-patch state.

Here is the deal- this patch was meant to unify our code with the
upstream RPM code as much as possible. The one thing I shied away from
was using alloca() and using strdup/free instead.

> Some little observations:
> 1. This vercmp the code doesn't follow alpm code style (not related
> to your patch, again):
> while (*one == '0') one++;
> if(!(*one && *two)) break;
> I like this one-liners better than {} 3 liner version of these.
> Is this allowed in our patches, too?
In short, no. The idea here was to stick with the RPM upstream code so
someone can do another diff at a later time to rectify the changes.

> 2. A very little notice:
> Now 1.5b < 1.5, but 1.5.b > 1.5
> In the "block terminology" of the code, 1.5b and 1.5.b are identical.
> The difference appeared, because your patch uses isalpha() instead of
> some "next block is alpha?" computation. To be honest, this may be
> better than 1.5.b < 1.5, so you can skip this casuist note.
We need to take a step back here and examine things- RPM does a great
amount of work to make version comparison work as good as possible.

If we make a change to the core RPM code, we need to ask "why wasn't
this change made upstream" or "why don't they do it this way". I guess
that is how I see hardcoding things like "alpha" or "beta" into the
detection code- they don't do it and seem to be fine with that.

I feel for any changes proposed here, we need to explain why our
version should deviate from the upstream code (as we do in the case of
the pkgrel stuff) before I will really consider a patch to "fix"
behavior.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier 07-17-2008 04:35 PM

vercmp discussion (was: String freeze for 3.2 release)
 
On Thu, Jul 17, 2008 at 5:58 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
>
> OK. I believe that RPM guys are cool guys;-) I think they simply don't
> need this mplayer 1.0rc2 versus 1.0 stuff, because they use different
> versioning scheme (as I see):
> http://dag.wieers.com/rpm/packages/mplayer/
>
> I agree with you, that hacking vercmp is not a good idea, that's why I
> say that we should revert the whole stuff. The old code was tested by
> Archers for long time, and it seems to worked perfectly. (I know
> that in case of reverting, our work on new vercmp was just a waste of
> time, sry.) Personally I still don't see what is fixed by the new
> "imported" code. Or it is notably faster? I can be convinced, but not
> just saying "trust on RPM guys".
>

For what it is worth, I totally agree with Nagy. RPM people have to
deal with different and odd versioning schemes, so unfortunately, we
can't simply reuse their code as is.
If our need and usage were closer, it would be a very good idea to
follow upstream as close as possible, but since it is not the case, I
am not sure it is a good idea. We indeed had perfectly working code,
and now we are losing time on a code that didn't really need to be
changed.
I think that there are many parts of pacman which require a lot of
work and attention, but this is not one of them, so I am also in favor
or just reverting to the good old working code.

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

"Dan McGee" 07-18-2008 01:14 AM

vercmp discussion (was: String freeze for 3.2 release)
 
On Thu, Jul 17, 2008 at 11:35 AM, Xavier <shiningxc@gmail.com> wrote:
> On Thu, Jul 17, 2008 at 5:58 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
>>
>> OK. I believe that RPM guys are cool guys;-) I think they simply don't
>> need this mplayer 1.0rc2 versus 1.0 stuff, because they use different
>> versioning scheme (as I see):
>> http://dag.wieers.com/rpm/packages/mplayer/
>>
>> I agree with you, that hacking vercmp is not a good idea, that's why I
>> say that we should revert the whole stuff. The old code was tested by
>> Archers for long time, and it seems to worked perfectly. (I know
>> that in case of reverting, our work on new vercmp was just a waste of
>> time, sry.) Personally I still don't see what is fixed by the new
>> "imported" code. Or it is notably faster? I can be convinced, but not
>> just saying "trust on RPM guys".
>>
>
> For what it is worth, I totally agree with Nagy. RPM people have to
> deal with different and odd versioning schemes, so unfortunately, we
> can't simply reuse their code as is.
> If our need and usage were closer, it would be a very good idea to
> follow upstream as close as possible, but since it is not the case, I
> am not sure it is a good idea. We indeed had perfectly working code,
> and now we are losing time on a code that didn't really need to be
> changed.
> I think that there are many parts of pacman which require a lot of
> work and attention, but this is not one of them, so I am also in favor
> or just reverting to the good old working code.

I'll admit defeat, I tried. :)

Can someone put together a single revert patch to take care of this? I
know it took us at least two commits to get the vercmp code updated,
so we will probably need to do some manual work to get this reverted.
Obviously the vercmptest script should stay, and perhaps we should add
the proposed tests from Nagy to the mix.

If people do see any improvements that can be "backported" to the
previous code, that would be good to have.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier 07-18-2008 11:54 AM

vercmp discussion (was: String freeze for 3.2 release)
 
On Fri, Jul 18, 2008 at 3:14 AM, Dan McGee <dpmcgee@gmail.com> wrote:
>
> I'll admit defeat, I tried. :)
>
> Can someone put together a single revert patch to take care of this? I
> know it took us at least two commits to get the vercmp code updated,
> so we will probably need to do some manual work to get this reverted.
> Obviously the vercmptest script should stay, and perhaps we should add
> the proposed tests from Nagy to the mix.
>
> If people do see any improvements that can be "backported" to the
> previous code, that would be good to have.
>

I just looked a bit at the history of rpmvercmp function, in the git repo :
http://devel.linux.duke.edu/gitweb/?p=rpm.git;a=history;f=lib/rpmvercmp.c;h=0cdcc686608abc9de6115e7d4a6f7d8c0f2d 8cad;hb=HEAD

So the rpmvercmp in 4.0.4 (which I finally found here :
http://ftp-stud.hs-esslingen.de/Mirrors/ftp.rpm.org/rpm/dist/rpm-4.0.x/rpm-4.0.4.tar.gz
) is the same as the version from 2002 in that git repo :
http://devel.linux.duke.edu/gitweb/?p=rpm.git;a=blob;f=lib/rpmvercmp.c;hb=076a6e29c5c8a35a5f78ae2a15339d030cf e2fdf

And then very little changed between 2002 and now :
http://devel.linux.duke.edu/gitweb/?p=rpm.git;a=blobdiff;f=lib/rpmvercmp.c;h=0cdcc686608abc9de6115e7d4a6f7d8c0f2d 8cad;hp=27969d47ead9576e091e0f8ba99d2e692a47c747;h b=HEAD;hpb=076a6e29c5c8a35a5f78ae2a15339d030cfe2fd f

There were some slight behavior changes, but not much, and our code
diverted enough that they don't seem to be relevant.
Anyway, again, our usage is different, so we should only care about
problems we have with our version schemes, not their. Otherwise we are
going to fix irrelevant cases, and risk breaking the relevant ones.

Though I found one fix which is interesting :
/* if they are equal because there might be more segments to */
/* compare */
rc = strcmp(one, two);
- if (rc) return rc;
+ if (rc) return (rc < 1 ? -1 : 1);


Anyway my proposal is to just copy/paste the good old working vercmp
code we had, with only the above fix.

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

"Dan McGee" 07-18-2008 12:03 PM

vercmp discussion (was: String freeze for 3.2 release)
 
On Fri, Jul 18, 2008 at 6:54 AM, Xavier <shiningxc@gmail.com> wrote:
> On Fri, Jul 18, 2008 at 3:14 AM, Dan McGee <dpmcgee@gmail.com> wrote:
>>
>> I'll admit defeat, I tried. :)
>>
>> Can someone put together a single revert patch to take care of this? I
>> know it took us at least two commits to get the vercmp code updated,
>> so we will probably need to do some manual work to get this reverted.
>> Obviously the vercmptest script should stay, and perhaps we should add
>> the proposed tests from Nagy to the mix.
>>
>> If people do see any improvements that can be "backported" to the
>> previous code, that would be good to have.
>>
>
> I just looked a bit at the history of rpmvercmp function, in the git repo :
> http://devel.linux.duke.edu/gitweb/?p=rpm.git;a=history;f=lib/rpmvercmp.c;h=0cdcc686608abc9de6115e7d4a6f7d8c0f2d 8cad;hb=HEAD
>
> So the rpmvercmp in 4.0.4 (which I finally found here :
> http://ftp-stud.hs-esslingen.de/Mirrors/ftp.rpm.org/rpm/dist/rpm-4.0.x/rpm-4.0.4.tar.gz
> ) is the same as the version from 2002 in that git repo :
> http://devel.linux.duke.edu/gitweb/?p=rpm.git;a=blob;f=lib/rpmvercmp.c;hb=076a6e29c5c8a35a5f78ae2a15339d030cf e2fdf
>
> And then very little changed between 2002 and now :
> http://devel.linux.duke.edu/gitweb/?p=rpm.git;a=blobdiff;f=lib/rpmvercmp.c;h=0cdcc686608abc9de6115e7d4a6f7d8c0f2d 8cad;hp=27969d47ead9576e091e0f8ba99d2e692a47c747;h b=HEAD;hpb=076a6e29c5c8a35a5f78ae2a15339d030cfe2fd f
>
> There were some slight behavior changes, but not much, and our code
> diverted enough that they don't seem to be relevant.
> Anyway, again, our usage is different, so we should only care about
> problems we have with our version schemes, not their. Otherwise we are
> going to fix irrelevant cases, and risk breaking the relevant ones.
>
> Though I found one fix which is interesting :
> /* if they are equal because there might be more segments to */
> /* compare */
> rc = strcmp(one, two);
> - if (rc) return rc;
> + if (rc) return (rc < 1 ? -1 : 1);
>
>
> Anyway my proposal is to just copy/paste the good old working vercmp
> code we had, with only the above fix.

Ahh, yes. We do want vercmp to return -1, 1, or 0, and not negative,
positive, or 0. The old one didn't always do this.

-Dan

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier 07-18-2008 12:45 PM

vercmp discussion (was: String freeze for 3.2 release)
 
On Fri, Jul 18, 2008 at 1:54 PM, Xavier <shiningxc@gmail.com> wrote:
>
> Anyway my proposal is to just copy/paste the good old working vercmp
> code we had, with only the above fix.
>

So here it is.
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier 07-21-2008 09:25 AM

vercmp discussion (was: String freeze for 3.2 release)
 
On Fri, Jul 18, 2008 at 2:45 PM, Xavier <shiningxc@gmail.com> wrote:
> On Fri, Jul 18, 2008 at 1:54 PM, Xavier <shiningxc@gmail.com> wrote:
>>
>> Anyway my proposal is to just copy/paste the good old working vercmp
>> code we had, with only the above fix.
>>
>
> So here it is.
>

Apparently the static strings were a show stopper to Dan.
I don't have a problem with static strings, since it removes the
malloc/free overhead and also simplifies the code.
As always with dynamic strings, we have the choice between duplicating
all free calls, or using goto.

But anyway, here is a new patch using strdup/free instead of static arrays.
_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev

Xavier 07-21-2008 10:01 AM

vercmp discussion (was: String freeze for 3.2 release)
 
On Mon, Jul 21, 2008 at 11:25 AM, Xavier <shiningxc@gmail.com> wrote:
>
> Apparently the static strings were a show stopper to Dan.
> I don't have a problem with static strings, since it removes the
> malloc/free overhead and also simplifies the code.
> As always with dynamic strings, we have the choice between duplicating
> all free calls, or using goto.
>
> But anyway, here is a new patch using strdup/free instead of static arrays.
>

This breaks sync1000, sync1003 and upgrade075 pactest, but I have not
yet been able to figure out why.

_______________________________________________
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev


All times are GMT. The time now is 04:48 PM.

VBulletin, Copyright ©2000 - 2014, Jelsoft Enterprises Ltd.
Content Relevant URLs by vBSEO ©2007, Crawlability, Inc.