[pacman-dev] String freeze for 3.2 release
Allan has finally bugged me enough that its time for our pacman codebase to see the light and a new 3.2 release. So here is the call for the string freeze. I've CC-ed this to the Arch General list to see if anyone there is interested in helping out. PLEASE reply to the pacman-dev mailing list if you have questions or want to help out but are not sure how. Our pacman translation policy is outlined here. The important sections for translators are the introduction and up to (and including) pre-release updates. http://www.archlinux.org/pacman/translation-help.html As stated in the above document, PO files have been made available to translate: http://code.toofishes.net/pacman/po_files/ I would like to get the release out in 7 days or so, so the sooner the better for the translations. Languages we currently have: cs:Vojtěch Gondžala <vojtech.gondzala@gmail.com> de: Matthias Gorissen <matthias@archlinux.de> en_GB: Jeff Bailes <thepizzaking@gmail.com> es: Juan Pablo Gonzalez <jotapesan@gmail.com>, Imanol Celaya <ilcra1989@gmail.com> fr: Xavier Chantry <shiningxc@gmail.com> hu: Nagy Gabor <ngaba@bibl.u-szeged.hu> it: Giovanni Scafora <linuxmania@gmail.com> pl: Jaroslaw Swierczynski <swiergot@gmail.com> pt_BR: João Felipe Santos <jfsantos@archlinux-br.org> ru: Sergey Tereschenko <serg.partizan@gmail.com> tr: Samed Beyribey <beyribey@gmail.com> zh_CN: 甘露(Lu.Gan) <rhythm.gan@gmail.com> If you wish to help translate, but your language is already listed as handled by someone, please contact them at the above addresses. If the language is listed as "no current translator", then please reply to ALL on this message stating you are doing the translation so we do not have duplicated effort (although you are more than welcome to work in teams). Once you have updated or completed a translation, simply tar and gzip the files up and send them back to pacman-dev@archlinux.org with a [translation] prefix in the message. For those that are used to the old method of updating translations (GIT patches), those are still acceptable as well. Please prefix the patch with "translation:" so it can be picked out easily. Any questions should be answered if you reply to this email on the list. -Dan
I sent my reply to Dan, but I wanted to send here (Dan: new 4. point;-).
I would like to get the release out in 7 days or so, so the sooner the better for the translations.
Wow. I bring up some thoughts here: 1. Some NEWS ideas. A. We should document API changes separated from other changes to help front-end writers. From my side I can recall 3 API changes: 8856146d71cb4cc512b0cf3414fbc231635822d3, d060e31be3586ce27382f80eaed7a9edf2c86aeb, 1fc83f4af6d827bf2e69c7a10e3d2010c9211974 (without deep investigation, alpm.h-diff should show almost everything). B. As user I would like to see new features separated from "memleak fixes" ;-). For example, new options: -Ru, --asexplicit, -S 'dep>=2.0' etc. 2. Xav said, that deltas are broken. Couldn't we move this out from alpm code, and using it XferCommand or whatever (or implement new "external helper", if needed)? 3. Front-ends should be able to free our new dynamic structs, e.g. pmdepmissing_t. 4. <flame> Revert vercmp code to 3.1 </flame> Bye
Nagy Gabor wrote:
2. Xav said, that deltas are broken. Couldn't we move this out from alpm code, and using it XferCommand or whatever (or implement new "external helper", if needed)?
How broken are they? More specifically, does this code need some minor fixes or a complete overhaul? BTW, this is on my big changes to get into Arch list, right behind getting a testing repo for community. I even maintain xdelta as a reminder. Allan
On Wed, Jul 16, 2008 at 6:39 AM, Allan McRae <allan@archlinux.org> wrote:
Nagy Gabor wrote:
2. Xav said, that deltas are broken. Couldn't we move this out from alpm code, and using it XferCommand or whatever (or implement new "external helper", if needed)?
How broken are they? More specifically, does this code need some minor fixes or a complete overhaul?
BTW, this is on my big changes to get into Arch list, right behind getting a testing repo for community. I even maintain xdelta as a reminder.
The libalpm code isn't broken at all, actually- it is just missing the scripting side of things in makepkg or repo-add. These scripts need the components to generate "delta 2.0" format, and we came across some problems with that, including compression, timestamps, and checksum mismatches. Xavier, do you remember where this all was? I thought this stuff was on the ML but I'm not sure. -Dan
On Wed, Jul 16, 2008 at 6:39 AM, Allan McRae <allan@archlinux.org> wrote:
Nagy Gabor wrote:
2. Xav said, that deltas are broken. Couldn't we move this out from alpm code, and using it XferCommand or whatever (or implement new "external helper", if needed)?
How broken are they? More specifically, does this code need some minor fixes or a complete overhaul?
BTW, this is on my big changes to get into Arch list, right behind getting a testing repo for community. I even maintain xdelta as a reminder.
The libalpm code isn't broken at all, actually- it is just missing the scripting side of things in makepkg or repo-add. These scripts need the components to generate "delta 2.0" format, and we came across some problems with that, including compression, timestamps, and checksum mismatches.
Xavier, do you remember where this all was? I thought this stuff was on the ML but I'm not sure.
-Dan
Oh, good to hear. Then I'm sorry about misleading info. (I don't follow this delta stuff at all, that's why I referred to Xavier). Bye
On Wed, Jul 16, 2008 at 2:06 PM, Nagy Gabor <ngaba@bibl.u-szeged.hu> wrote:
How broken are they? More specifically, does this code need some minor fixes or a complete overhaul?
BTW, this is on my big changes to get into Arch list, right behind getting a testing repo for community. I even maintain xdelta as a reminder.
The libalpm code isn't broken at all, actually- it is just missing the scripting side of things in makepkg or repo-add. These scripts need the components to generate "delta 2.0" format, and we came across some problems with that, including compression, timestamps, and checksum mismatches.
Xavier, do you remember where this all was? I thought this stuff was on the ML but I'm not sure.
-Dan
Oh, good to hear. Then I'm sorry about misleading info. (I don't follow this delta stuff at all, that's why I referred to Xavier).
The gzip / md5sum issue was discussed here between Nathan and I : http://www.nabble.com/Add-delta-creation-to-repo-add.-td15513733.html#a15838... To quote Nathan :
For the first patch, I added the -n flag to gzip in order to prevent the md5sum problem. The apply-delta script is not as efficient as it could be. There is an extra gunzip+gzip step that could be avoided if we create a delta on the .tar rather than the .tar.gz. Doing so would complicate makepkg a bit, so I stuck with the extra step.
This extra gunzip+gzip step is quite disappointing indeed, since the purpose of delta is efficiency. Working directly on tar would solve this problem but would cause another : increased disk usage, because the old and new packages would need to be both uncompressed at some point (and this affects both delta creation and application). Maybe we need new delta benchmarks taking this into account now, before spending more time? The second biggest problem is lack of motivation and interest from everyone. What is the point of having delta support if no one is going to use them? But if people are going to use them, then we need feedback about how it should work. I don't know whether it would be better to have delta being generated at makepkg step, or repo-add step, or both. We have to take into account both the simplicity of usage but also of implementation, and try to avoid duplication if we need delta support in both. In the end of the above thread, I proposed a quick summary of an implementation only at repo-add level, but I don't know if it is a good idea or if it is too limited or what.
Nagy Gabor wrote:
I sent my reply to Dan, but I wanted to send here (Dan: new 4. point;-).
I would like to get the release out in 7 days or so, so the sooner the better for the translations.
Wow. I bring up some thoughts here: 1. Some NEWS ideas. A. We should document API changes separated from other changes to help front-end writers. From my side I can recall 3 API changes: 8856146d71cb4cc512b0cf3414fbc231635822d3, d060e31be3586ce27382f80eaed7a9edf2c86aeb, 1fc83f4af6d827bf2e69c7a10e3d2010c9211974 (without deep investigation, alpm.h-diff should show almost everything). B. As user I would like to see new features separated from "memleak fixes" ;-). For example, new options: -Ru, --asexplicit, -S 'dep>=2.0' etc.
I am sure Dan would highly appreciate help on this :) The NEWS file is tracked by git too, so you can submit patches for that as well. You can provide the API changes and new features that you consider important, and it could be completed later by others.
3. Front-ends should be able to free our new dynamic structs, e.g. pmdepmissing_t.
We should definitively solve this problem. Even if these memleaks only happen on error cases, and they are relatively small. The question is how. Dan does not want to make these free functions public. Maybe we want to store these error lists in libalpm, and allow the frontend to get it. And then it would be freed when the transaction is freed? Otherwise, maybe this is where it would help to have a free function attached to the list structure.
4. <flame> Revert vercmp code to 3.1 </flame>
It would be nice to figure out what changed and why it doesn't work anymore rather than simply reverting.
only happen on error cases, and they are relatively small. The question is how. Dan does not want to make these free functions public.
Why? I can't see too much difference between alpm_miss_get_causingpkg (wow, new API change ;-) and alpm_miss_free.
... Maybe we want to store these error lists in libalpm, and allow the frontend to get it. And then it would be freed when the transaction is freed?
I don't like this. We should somehow differentiate pmdepmissing_t and pmconflict_t ... lists, and we have more chance to do memleaks here.
Otherwise, maybe this is where it would help to have a free function attached to the list structure.
4. <flame> Revert vercmp code to 3.1 </flame>
It would be nice to figure out what changed and why it doesn't work anymore rather than simply reverting.
nicer, but harder ;-) See 84283672853350a84d2a71b72dc06e180cad1587, search for 'type mismatch'. Bye
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: '\0' vs. alpha, as I see, this case was handled after comment /* see if we ran out of segments on one string */ Bye
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: '\0' 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.
I made a typo in one of the comment inside the patch : above casse -> above case .
Xavier wrote:
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: '\0' 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.
You do realize you just broke openssh version numbering Here is my vote for the best order: 1.0alpha < 1.0beta < 1.0pre < 1.0rc < 1.0 < 1.0a < 1.0b < 1.0c </massive flame!>
On Thu, Jul 17, 2008 at 2:54 PM, Allan McRae <allan@archlinux.org> wrote:
You do realize you just broke openssh version numbering
Here is my vote for the best order: 1.0alpha < 1.0beta < 1.0pre < 1.0rc < 1.0 < 1.0a < 1.0b < 1.0c
I think I just restored the old behavior, what we had before is : 1.0a < 1.0alpha < 1.0b < 1.0beta < 1.0c < 1.0pre < 1.0rc < 1.0 And as far as I can see, it never caused problems to openssh. Both old and new behaviors worked fine with it, because we always had : 4.3p1 < 4.3p2 < 4.4p1 According to the cvs history : http://repos.archlinux.org/viewvc.cgi/core/support/openssh/PKGBUILD?root=core&view=log and the source archives : ftp://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/ the scheme was always X.Y[.Z]pN . Now, maybe 1.0alpha < 1.0beta < 1.0pre < 1.0rc < 1.0 < 1.0a < 1.0b < 1.0c would be better, but we never had this behavior, and implementing would be (much?) more complex than what we have now.
Xavier wrote:
On Thu, Jul 17, 2008 at 2:54 PM, Allan McRae <allan@archlinux.org> wrote:
You do realize you just broke openssh version numbering
Here is my vote for the best order: 1.0alpha < 1.0beta < 1.0pre < 1.0rc < 1.0 < 1.0a < 1.0b < 1.0c
I think I just restored the old behavior, what we had before is : 1.0a < 1.0alpha < 1.0b < 1.0beta < 1.0c < 1.0pre < 1.0rc < 1.0
And as far as I can see, it never caused problems to openssh. Both old and new behaviors worked fine with it, because we always had : 4.3p1 < 4.3p2 < 4.4p1
According to the cvs history : http://repos.archlinux.org/viewvc.cgi/core/support/openssh/PKGBUILD?root=core&view=log and the source archives : ftp://ftp.openbsd.org/pub/OpenBSD/OpenSSH/portable/ the scheme was always X.Y[.Z]pN .
Yeah, poor example... In the v1.2ish era they went 1.2 -> 12.p2 -> 1.2p3 but that was a while ago... A better example is samba :)
Now, maybe 1.0alpha < 1.0beta < 1.0pre < 1.0rc < 1.0 < 1.0a < 1.0b < 1.0c would be better, but we never had this behavior, and implementing would be (much?) more complex than what we have now.
I was really trying to point out that you can't satisfy everyone... But maybe that order is quite good although it would be quite complex. Allan
2008/7/17 Allan McRae <allan@archlinux.org>:
Xavier wrote:
On Thu, Jul 17, 2008 at 2:54 PM, Allan McRae <allan@archlinux.org> wrote:
You do realize you just broke openssh version numbering
Here is my vote for the best order: 1.0alpha < 1.0beta < 1.0pre < 1.0rc < 1.0 < 1.0a < 1.0b < 1.0c
This is possible only with hardcoding the order, but that's overkill IMO.
I think I just restored the old behavior, what we had before is : 1.0a < 1.0alpha < 1.0b < 1.0beta < 1.0c < 1.0pre < 1.0rc < 1.0
Great! The most important point was that 1.0 was > than 1.0anything, and new behavior changed this (which I didn't like because of unneeded breakage of packages) So restoring to the old behavior is good.
Yeah, poor example... In the v1.2ish era they went 1.2 -> 12.p2 -> 1.2p3 but that was a while ago...
A better example is samba :)
samba used to have options=('force') so restoring to old behavior breaks nothing. Summary: [+] for restored old behavior, [-] for idea of hardcoding the order of version postfixes. -- Roman Kyrylych (Роман Кирилич)
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
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
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
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". Bye
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.
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
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=... 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... ) 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=07... 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... 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.
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=...
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... ) 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=07...
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...
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
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.
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.
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.
On Mon, Jul 21, 2008 at 12:01 PM, Xavier <shiningxc@gmail.com> wrote:
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.
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; }
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
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 Bye
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
On Tue, Jul 15, 2008 at 8:05 PM, Dan McGee <dpmcgee@gmail.com> wrote:
Allan has finally bugged me enough that its time for our pacman codebase to see the light and a new 3.2 release. So here is the call for the string freeze.
Anything I've received should be here (with some slight fixes to ensure they compiled correctly): http://code.toofishes.net/gitweb.cgi?p=pacman.git;a=shortlog;h=refs/heads/ne... -Dan
participants (5)
-
Allan McRae
-
Dan McGee
-
Nagy Gabor
-
Roman Kyrylych
-
Xavier