Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
On Mon, Jul 21, 2008 at 12:01 PM, Xavier [EMAIL PROTECTED] wrote:
On Mon, Jul 21, 2008 at 11:25 AM, Xavier [EMAIL PROTECTED] 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.
Well I found a fix, which looks correct to me, but I am still highly
confused about why the old vercmp function with static strings worked
fine.
Also, my vercmp function had problems when being called from checkdeps
(which is why it broke the 3 pactests above), and probably the two
arguments of vercmp in this case are dynamic strings.
But it didn't have any problems when being called from the vercmp tool
(src/util/vercmp) which use static strings, because all tests in
pactest/vercmptest still passed fine.
Anyway, after the fix, the calls from checkdeps and vercmp tool both work fine.
diff --git a/lib/libalpm/package.c b/lib/libalpm/package.c
index d0ca58a..8a36e56 100644
--- a/lib/libalpm/package.c
+++ b/lib/libalpm/package.c
@@ -593,12 +593,12 @@ int SYMEXPORT alpm_pkg_vercmp(const char *a,
const char *b)
/* lose the release number */
for(one = str1; *one *one != '-'; one++);
- if(one) {
+ if(*one) {
*one = '\0';
rel1 = ++one;
}
for(two = str2; *two *two != '-'; two++);
- if(two) {
+ if(*two) {
*two = '\0';
rel2 = ++two;
}
0001-Revert-vercmp-code-to-the-old-version.patch
Description: application/mbox
___
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
On Fri, Jul 18, 2008 at 6:54 AM, Xavier [EMAIL PROTECTED] wrote: On Fri, Jul 18, 2008 at 3:14 AM, Dan McGee [EMAIL PROTECTED] 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=0cdcc686608abc9de6115e7d4a6f7d8c0f2d8cad;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=076a6e29c5c8a35a5f78ae2a15339d030cfe2fdf 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=0cdcc686608abc9de6115e7d4a6f7d8c0f2d8cad;hp=27969d47ead9576e091e0f8ba99d2e692a47c747;hb=HEAD;hpb=076a6e29c5c8a35a5f78ae2a15339d030cfe2fdf 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
Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
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.
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?
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.
Bye
___
pacman-dev mailing list
pacman-dev@archlinux.org
http://archlinux.org/mailman/listinfo/pacman-dev
Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
On Thu, Jul 17, 2008 at 10:23 AM, Nagy Gabor [EMAIL PROTECTED] 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
Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
This note inspired me to test '1.0. == 1.0' You may think that this is useless, but image '1.0 ' versus '1.0' (extra spacebar, '\n' etc. character). The old code beats the new one again :-P I mean 3.1 vercmp code beats 3.2 vercmp code. Xav, your patch is excellent. (And I just joked here, of course.) Bye ___ pacman-dev mailing list pacman-dev@archlinux.org http://archlinux.org/mailman/listinfo/pacman-dev
Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
On Thu, Jul 17, 2008 at 5:58 PM, Nagy Gabor [EMAIL PROTECTED] 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
Re: [pacman-dev] vercmp discussion (was: String freeze for 3.2 release)
On Thu, Jul 17, 2008 at 11:35 AM, Xavier [EMAIL PROTECTED] wrote: On Thu, Jul 17, 2008 at 5:58 PM, Nagy Gabor [EMAIL PROTECTED] 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
