[pacman-dev] [PATCH] Change the exit code for pacman_deptest() and some clean up.
Changed the exit code for missing deps from 1 to 128 because 1 is used for other errors. makepkg breaks if pacman exits with 1 for any reason other than a missing dep. Changed alpm_splitdeps( deptest ) so it doesn't change the value of deptest. Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- lib/libalpm/deps.c | 4 +++- scripts/makepkg.in | 2 +- src/pacman/deptest.c | 16 +++++----------- 3 files changed, 9 insertions(+), 13 deletions(-) diff --git a/lib/libalpm/deps.c b/lib/libalpm/deps.c index 524136b..b259c4b 100644 --- a/lib/libalpm/deps.c +++ b/lib/libalpm/deps.c @@ -442,7 +442,9 @@ pmdepend_t SYMEXPORT *alpm_splitdep(const char *depstring) if(depstring == NULL) { return(NULL); } - newstr = strdup(depstring); + + newstr = calloc(strlen(depstring)+1, sizeof(char)); + strncpy(newstr, depstring, strlen(depstring)); depend = malloc(sizeof(pmdepend_t)); if(depend == NULL) { diff --git a/scripts/makepkg.in b/scripts/makepkg.in index 8be44c6..12459f9 100644 --- a/scripts/makepkg.in +++ b/scripts/makepkg.in @@ -296,7 +296,7 @@ check_deps() { pmout=$(pacman $PACMAN_OPTS -T $*) ret=$? - if [ $ret -eq 1 ]; then #unresolved deps + if [ $ret -eq 128 ]; then #unresolved deps echo "$pmout" elif [ $ret -ne 0 ]; then error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout" diff --git a/src/pacman/deptest.c b/src/pacman/deptest.c index 264ba1c..8a731ae 100644 --- a/src/pacman/deptest.c +++ b/src/pacman/deptest.c @@ -27,6 +27,7 @@ #include <alpm.h> #include <alpm_list.h> +#include <deps.h> /* pacman */ #include "pacman.h" @@ -52,20 +53,14 @@ int pacman_deptest(alpm_list_t *targets) alpm_list_t *j, *provides; target = alpm_list_getdata(i); - - /* splitdep modifies the string... we'll compensate for now */ - char *saved_target = NULL; - saved_target = calloc(strlen(target)+1, sizeof(char)); - strncpy(saved_target, target, strlen(target)); - dep = alpm_splitdep(target); - pkg = alpm_db_get_pkg(alpm_option_get_localdb(), target); + pkg = alpm_db_get_pkg(alpm_option_get_localdb(), dep->name); if(pkg && alpm_depcmp(pkg, dep)) { found = 1; } else { /* not found, can we find anything that provides this in the local DB? */ - provides = alpm_db_whatprovides(alpm_option_get_localdb(), target); + provides = alpm_db_whatprovides(alpm_option_get_localdb(), dep->name); for(j = provides; j; j = alpm_list_next(j)) { pmpkg_t *pkg; pkg = alpm_list_getdata(j); @@ -78,10 +73,9 @@ int pacman_deptest(alpm_list_t *targets) } if(!found) { - printf("%s\n", saved_target); - retval = 1; + printf("%s\n", target); + retval = 128; } - free(saved_target); } return(retval); } -- 1.5.2.2
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
- newstr = strdup(depstring); + + newstr = calloc(strlen(depstring)+1, sizeof(char)); + strncpy(newstr, depstring, strlen(depstring));
hmm, that's basically the same, isn't it? :)
- - /* splitdep modifies the string... we'll compensate for now */ - char *saved_target = NULL; - saved_target = calloc(strlen(target)+1, sizeof(char)); - strncpy(saved_target, target, strlen(target)); -
Wow, now I'm confused, I didn't know about this code. I'm afraid I only looked at the backend code, and not frontend, when Dan asked this : http://www.archlinux.org/pipermail/pacman-dev/2007-June/008595.html Was that a workaround for a feature or for a bug ? commit is there : http://projects.archlinux.org/git/gitweb.cgi?p=pacman.git;a=commitdiff;h=6f8... Any reasons this wasn't directly made in splitdep instead, like Nagy did : http://www.archlinux.org/pipermail/pacman-dev/2007-June/008535.html ? It really looked like a bug to me, but this workaround confuses me.
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
- newstr = strdup(depstring); + + newstr = calloc(strlen(depstring)+1, sizeof(char)); + strncpy(newstr, depstring, strlen(depstring));
hmm, that's basically the same, isn't it? :)
- - /* splitdep modifies the string... we'll compensate for now */ - char *saved_target = NULL; - saved_target = calloc(strlen(target)+1, sizeof(char)); - strncpy(saved_target, target, strlen(target)); - As for the rest of the patch, looking at the code I saw this and thought it looked a bit out of place, the problem is in alpm_splitdeps() and I didn't see anything else that depended on this behaviour of alpm_splitdeps(). So I changed alpm_splitdeps() not to modify the string
First off I should have attached a BIG warning, this is my first attempt at doing anything serious in C beyond basic examples :p The original reason for the patch was to change the exit code. At the moment pacman uses 1 for missing deps & errors. This can confuse makepkg if pacman returns an error instead of the list of missing packages and you end up with pacman saying something like Checking deps... Missing deps: error: There was an error Xavier wrote: passed to it. Andrew
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
As for the rest of the patch, looking at the code I saw this and thought it looked a bit out of place, the problem is in alpm_splitdeps() and I didn't see anything else that depended on this behaviour of alpm_splitdeps(). So I changed alpm_splitdeps() not to modify the string passed to it.
Sorry if it wasn't very clear, I wasn't criticizing your patch, but the code that was there before :) Nagy also saw this problem in splitdeps and corrected it a while ago, by adding the strdup() function that you replaced by an equivalent. (I liked the strdup there though, because it's simpler ;) ) So I don't think it's needed to change splitdeps. I just find this code in pacman_deptest rather curious, because it indeed looks out of place. So I think your patch for this function is correct. Ah one little thing, I wonder if this free(saved_target) that you already removed could be replaced by free(dep).
Changed the exit code for missing deps from 1 to 128 because 1 is used for other errors. makepkg breaks if pacman exits with 1 for any reason other than a missing dep. Clean up some left over code from http://projects.archlinux.org/git/gitweb.cgi?p=pacman.git;a=commitdiff;h=765... Signed-off-by: Andrew Fyfe <andrew@neptune-one.net> --- scripts/makepkg.in | 2 +- src/pacman/deptest.c | 17 ++++++----------- 2 files changed, 7 insertions(+), 12 deletions(-) diff --git a/scripts/makepkg.in b/scripts/makepkg.in index 8be44c6..12459f9 100644 --- a/scripts/makepkg.in +++ b/scripts/makepkg.in @@ -296,7 +296,7 @@ check_deps() { pmout=$(pacman $PACMAN_OPTS -T $*) ret=$? - if [ $ret -eq 1 ]; then #unresolved deps + if [ $ret -eq 128 ]; then #unresolved deps echo "$pmout" elif [ $ret -ne 0 ]; then error "$(gettext "Pacman returned a fatal error (%i): %s")" "$ret" "$pmout" diff --git a/src/pacman/deptest.c b/src/pacman/deptest.c index 264ba1c..d587868 100644 --- a/src/pacman/deptest.c +++ b/src/pacman/deptest.c @@ -27,6 +27,7 @@ #include <alpm.h> #include <alpm_list.h> +#include <deps.h> /* pacman */ #include "pacman.h" @@ -52,20 +53,14 @@ int pacman_deptest(alpm_list_t *targets) alpm_list_t *j, *provides; target = alpm_list_getdata(i); - - /* splitdep modifies the string... we'll compensate for now */ - char *saved_target = NULL; - saved_target = calloc(strlen(target)+1, sizeof(char)); - strncpy(saved_target, target, strlen(target)); - dep = alpm_splitdep(target); - pkg = alpm_db_get_pkg(alpm_option_get_localdb(), target); + pkg = alpm_db_get_pkg(alpm_option_get_localdb(), dep->name); if(pkg && alpm_depcmp(pkg, dep)) { found = 1; } else { /* not found, can we find anything that provides this in the local DB? */ - provides = alpm_db_whatprovides(alpm_option_get_localdb(), target); + provides = alpm_db_whatprovides(alpm_option_get_localdb(), dep->name); for(j = provides; j; j = alpm_list_next(j)) { pmpkg_t *pkg; pkg = alpm_list_getdata(j); @@ -78,10 +73,10 @@ int pacman_deptest(alpm_list_t *targets) } if(!found) { - printf("%s\n", saved_target); - retval = 1; + printf("%s\n", target); + retval = 128; } - free(saved_target); + free(dep); } return(retval); } -- 1.5.2.2
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
As for the rest of the patch, looking at the code I saw this and thought it looked a bit out of place, the problem is in alpm_splitdeps() and I didn't see anything else that depended on this behaviour of alpm_splitdeps(). So I changed alpm_splitdeps() not to modify the string passed to it.
Sorry if it wasn't very clear, I wasn't criticizing your patch, but the code that was there before :) Nagy also saw this problem in splitdeps and corrected it a while ago, by adding the strdup() function that you replaced by an equivalent. (I liked the strdup there though, because it's simpler ;) ) So I don't think it's needed to change splitdeps. I need to do some more research before playing with the code :) I saw
Xavier wrote: the comment in pacman_deptest() and assumed there was still a problem with alpm_splitdeps().
I just find this code in pacman_deptest rather curious, because it indeed looks out of place. So I think your patch for this function is correct.
Ah one little thing, I wonder if this free(saved_target) that you already removed could be replaced by free(dep).
Done :) Andrew
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
I need to do some more research before playing with the code :) I saw the comment in pacman_deptest() and assumed there was still a problem with alpm_splitdeps().
I just find this code in pacman_deptest rather curious, because it indeed looks out of place. So I think your patch for this function is correct.
Ah one little thing, I wonder if this free(saved_target) that you already removed could be replaced by free(dep).
Done :)
Another thing I missed earlier, why is this deps.h needed ? Isn't it only in the backend anyway ? I don't think pacman should have access to it. Otherwise, I think it's fine now :) But since I don't have enough experience either, it would be better if someone looked at it also :D
On 6/29/07, Xavier <shiningxc@gmail.com> wrote:
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
I need to do some more research before playing with the code :) I saw the comment in pacman_deptest() and assumed there was still a problem with alpm_splitdeps().
I just find this code in pacman_deptest rather curious, because it indeed looks out of place. So I think your patch for this function is correct.
Ah one little thing, I wonder if this free(saved_target) that you already removed could be replaced by free(dep).
Done :)
Another thing I missed earlier, why is this deps.h needed ? Isn't it only in the backend anyway ? I don't think pacman should have access to it.
This is correct- The only headers that should need inclusion from the pacman frontend are those that were already there- alpm.h and alpm_list.h. The rest are not installed on the system. I can fix this if you want, otherwise I'll let you push a new version.
Otherwise, I think it's fine now :) But since I don't have enough experience either, it would be better if someone looked at it also :D
The rest of the patch looks fine if you make that one small change. Congrats on delving into C code. :P -Dan
On 6/29/07, Dan McGee <dpmcgee@gmail.com> wrote:
On 6/29/07, Xavier <shiningxc@gmail.com> wrote:
Another thing I missed earlier, why is this deps.h needed ? Isn't it only in the backend anyway ? I don't think pacman should have access to it.
This is correct- The only headers that should need inclusion from the pacman frontend are those that were already there- alpm.h and alpm_list.h. The rest are not installed on the system. I can fix this if you want, otherwise I'll let you push a new version.
Ah, there are bigger issues here, now aren't there. We don't have public accessor methods for pmdepend_t, even though we have them for pmdepmissing_t. Wow how I love libalpm. Do we really need both of these types? This seems a bit odd here. -Dan
2007/6/29, Dan McGee <dpmcgee@gmail.com>:
Ah, there are bigger issues here, now aren't there. We don't have public accessor methods for pmdepend_t, even though we have them for pmdepmissing_t. Wow how I love libalpm. Do we really need both of these types? This seems a bit odd here.
I'll answer by a new question (libalpm/conflict.c , line 59) * TODO WTF is a 'depmissing' doing indicating a conflict?? :) So this type probably needs to be thought again anyway. We have 3 things to represent here : 1) a package depending on another (pmdepend_t) 2) an error because a dependency is missing (pmdepmissing_t) 3) an error because of a conflict (pm_depmissing_t) Not sure if they could all be combined in one. Though, there is always an infinite number of ways to represent things, so ... Anyway, there is indeed a problem with accessors..
Xavier wrote:
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
I need to do some more research before playing with the code :) I saw the comment in pacman_deptest() and assumed there was still a problem with alpm_splitdeps().
I just find this code in pacman_deptest rather curious, because it indeed looks out of place. So I think your patch for this function is correct.
Ah one little thing, I wonder if this free(saved_target) that you already removed could be replaced by free(dep). Done :)
Another thing I missed earlier, why is this deps.h needed ? Isn't it only in the backend anyway ? I don't think pacman should have access to it.
I added deps.h so that I could access pmdepend_t->name, at the moment deptest is passing target (which could = foobar>=1.0) where the functions are expecting just the package name. So deptest is useless when dealing with versioned deps at the moment. Looking a little deeper I think pacman_deptest() should be using _alpm_checkdeps(). Are all _* functions private? Andrew
2007/6/30, Andrew Fyfe <andrew@neptune-one.net>:
I added deps.h so that I could access pmdepend_t->name, at the moment deptest is passing target (which could = foobar>=1.0) where the functions are expecting just the package name. So deptest is useless when dealing with versioned deps at the moment.
Yes indeed, that code is now totally broken, because it relied on a broken behavior, which has been fixed.
Looking a little deeper I think pacman_deptest() should be using _alpm_checkdeps().
That search for satisfier done by pacman_deptest() is very very common, and used everywhere, so there is some factoring work to do. There is no function for doing it yet, but there should be one. See third paragraph of Nagy's mail there : http://www.archlinux.org/pipermail/pacman-dev/2007-June/008539.html
Are all _* functions private?
Yes _alpm_* functions are private, while alpm_* functions are made public by the SYMEXPORT attribute. for example : lib/libalpm/deps.c:pmdepend_t SYMEXPORT *alpm_splitdep(const char *depstring)
2007/6/29, Andrew Fyfe <andrew@neptune-one.net>:
Changed the exit code for missing deps from 1 to 128 because 1 is used for other errors. makepkg breaks if pacman exits with 1 for any reason other than a missing dep.
I'm still reading the ML archives, and look what I found : http://www.archlinux.org/pipermail/pacman-dev/2007-February/007237.html Looks like it was 127 before :)
participants (3)
-
Andrew Fyfe
-
Dan McGee
-
Xavier