[pacman-dev] [PATCH] A variety of small changes
1. makepkg - Reduces the missing arch error to a warning when only generating intergity checks (-g or --geninteg flag) 2. libalpm - remove unused handle->uid from pmhandle_t. The need to check permissions should be determined by the frontend (and is in pacman). 3. libalpm - fix comment on noextract in pmhandle_t. 4. pacman - only ask for removal confirmation when the recusre or cascade options add packages to the removal list Signed-off-by: Allan McRae <mcrae_allan@hotmail.com> --- lib/libalpm/handle.c | 1 - lib/libalpm/handle.h | 3 +-- scripts/makepkg.sh.in | 2 +- src/pacman/remove.c | 35 +++++++++++++++++++---------------- 4 files changed, 21 insertions(+), 20 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 5f209d4..2b871b5 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -54,7 +54,6 @@ pmhandle_t *_alpm_handle_new() handle->logstream = NULL; /* see if we're root or not */ - handle->uid = geteuid(); handle->root = NULL; handle->dbpath = NULL; handle->cachedirs = NULL; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5051917..9c537b1 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -30,7 +30,6 @@ typedef struct _pmhandle_t { /* internal usage */ - uid_t uid; /* current UID */ /* TODO is this used? */ pmdb_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (pmdb_t *) */ FILE *logstream; /* log file stream pointer */ @@ -50,7 +49,7 @@ typedef struct _pmhandle_t { /* package lists */ alpm_list_t *noupgrade; /* List of packages NOT to be upgraded */ - alpm_list_t *noextract; /* List of packages NOT to extract */ /*TODO is this used?*/ + alpm_list_t *noextract; /* List of files NOT to extract */ alpm_list_t *ignorepkg; /* List of packages to ignore */ alpm_list_t *holdpkg; /* List of packages which 'hold' pacman */ alpm_list_t *ignoregrp; /* List of groups to ignore */ diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc44c68..8009ef0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1315,7 +1315,7 @@ if [ "$arch" = 'any' ]; then fi if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" = "0" ]; then + if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgname" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH" diff --git a/src/pacman/remove.c b/src/pacman/remove.c index 4fe9bc8..9ae9262 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -121,23 +121,26 @@ int pacman_remove(alpm_list_t *targets) /* Warn user in case of dangerous operation */ if(config->flags & PM_TRANS_FLAG_RECURSE || config->flags & PM_TRANS_FLAG_CASCADE) { - /* list transaction targets */ - alpm_list_t *lst = NULL; - /* create a new list of package names only */ - for(i = alpm_trans_get_pkgs(); i; i = alpm_list_next(i)) { - pmpkg_t *pkg = alpm_list_getdata(i); - lst = alpm_list_add(lst, strdup(alpm_pkg_get_name(pkg))); - } - printf("\n"); - list_display(_("Targets:"), lst); - FREELIST(lst); - /* get confirmation */ - if(yesno(1, _("\nDo you want to remove these packages?")) == 0) { - trans_release(); - FREELIST(finaltargs); - return(1); + /* check if recurse or cascade added packages to remove */ + if(alpm_list_count(finaltargs) != alpm_list_count(alpm_trans_get_pkgs())) { + /* list transaction targets */ + alpm_list_t *lst = NULL; + /* create a new list of package names only */ + for(i = alpm_trans_get_pkgs(); i; i = alpm_list_next(i)) { + pmpkg_t *pkg = alpm_list_getdata(i); + lst = alpm_list_add(lst, strdup(alpm_pkg_get_name(pkg))); + } + printf("\n"); + list_display(_("Targets:"), lst); + FREELIST(lst); + /* get confirmation */ + if(yesno(1, _("\nDo you want to remove these packages?")) == 0) { + trans_release(); + FREELIST(finaltargs); + return(1); + } + printf("\n"); } - printf("\n"); } /* Step 3: actually perform the removal */ -- 1.5.5.1
On Fri, May 16, 2008 at 8:06 AM, Allan McRae <mcrae_allan@hotmail.com> wrote:
1. makepkg - Reduces the missing arch error to a warning when only generating intergity checks (-g or --geninteg flag) Probably smart, this seems good.
2. libalpm - remove unused handle->uid from pmhandle_t. The need to check permissions should be determined by the frontend (and is in pacman). Hmm- I almost think the backend should do something (such as verifying we have read/write perms on the relevant dbs if necessary), but you are right, it is unused now so it should go. Any headers that can be dropped in handle.c because of this change?
3. libalpm - fix comment on noextract in pmhandle_t. So we definitely use this and it still works? Heh. We might need to beef up pactests in this area.
4. pacman - only ask for removal confirmation when the recusre or cascade options add packages to the removal list I thought this came up a few months ago, and I believe I at least thought it wasn't the greatest of ideas. I wanted behavior to be consistent with the option, as some people might use these flags as "I know it will ask me for confirmation" step. I know it would confuse me (and scripts) if it asked sometimes and other times did not, and it is not immediately apparent to the end user why this is happening.
-Dan
Dan McGee wrote:
On Fri, May 16, 2008 at 8:06 AM, Allan McRae <mcrae_allan@hotmail.com> wrote:
1. makepkg - Reduces the missing arch error to a warning when only generating intergity checks (-g or --geninteg flag)
Probably smart, this seems good.
2. libalpm - remove unused handle->uid from pmhandle_t. The need to check permissions should be determined by the frontend (and is in pacman).
Hmm- I almost think the backend should do something (such as verifying we have read/write perms on the relevant dbs if necessary), but you are right, it is unused now so it should go. Any headers that can be dropped in handle.c because of this change?
Probably. I'll look into it.
3. libalpm - fix comment on noextract in pmhandle_t.
So we definitely use this and it still works? Heh. We might need to beef up pactests in this area.
Well, a quick search showed it is still used in the libalpm code. The option is documented in the pacman.conf man page so I assume it still works!
4. pacman - only ask for removal confirmation when the recusre or cascade options add packages to the removal list
I thought this came up a few months ago, and I believe I at least thought it wasn't the greatest of ideas. I wanted behavior to be consistent with the option, as some people might use these flags as "I know it will ask me for confirmation" step. I know it would confuse me (and scripts) if it asked sometimes and other times did not, and it is not immediately apparent to the end user why this is happening.
Fair enough. I just found it annoying to be asked when no additional packages were flagged. I was thinking this was consistent with the -R option no longer asking for confirmation. Anyway, it is a small annoyance and I will get over it.
Allan McRae wrote:
Dan McGee wrote:
On Fri, May 16, 2008 at 8:06 AM, Allan McRae <mcrae_allan@hotmail.com> wrote:
3. libalpm - fix comment on noextract in pmhandle_t.
So we definitely use this and it still works? Heh. We might need to beef up pactests in this area.
Well, a quick search showed it is still used in the libalpm code. The option is documented in the pacman.conf man page so I assume it still works!
And there is a pactest - upgrade070.py. We should probably have a sync one too.... Which brings up the issue of how it handles the file conflict. It seems I have a task for the day! Allan
1. makepkg - Reduces the missing arch error to a warning when only generating intergity checks (-g or --geninteg flag) 2. libalpm - remove unused handle->uid from pmhandle_t. The need to check permissions should be determined by the frontend (and is in pacman). 3. libalpm - fix comment on noextract in pmhandle_t. Signed-off-by: Allan McRae <mcrae_allan@hotmail.com> --- lib/libalpm/handle.c | 2 -- lib/libalpm/handle.h | 3 +-- scripts/makepkg.sh.in | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-) diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 5f209d4..94317bf 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -23,7 +23,6 @@ #include <stdlib.h> #include <string.h> -#include <unistd.h> #include <limits.h> #include <sys/types.h> #include <syslog.h> @@ -54,7 +53,6 @@ pmhandle_t *_alpm_handle_new() handle->logstream = NULL; /* see if we're root or not */ - handle->uid = geteuid(); handle->root = NULL; handle->dbpath = NULL; handle->cachedirs = NULL; diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5051917..9c537b1 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -30,7 +30,6 @@ typedef struct _pmhandle_t { /* internal usage */ - uid_t uid; /* current UID */ /* TODO is this used? */ pmdb_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (pmdb_t *) */ FILE *logstream; /* log file stream pointer */ @@ -50,7 +49,7 @@ typedef struct _pmhandle_t { /* package lists */ alpm_list_t *noupgrade; /* List of packages NOT to be upgraded */ - alpm_list_t *noextract; /* List of packages NOT to extract */ /*TODO is this used?*/ + alpm_list_t *noextract; /* List of files NOT to extract */ alpm_list_t *ignorepkg; /* List of packages to ignore */ alpm_list_t *holdpkg; /* List of packages which 'hold' pacman */ alpm_list_t *ignoregrp; /* List of groups to ignore */ diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc44c68..8009ef0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1315,7 +1315,7 @@ if [ "$arch" = 'any' ]; then fi if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" = "0" ]; then + if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgname" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH" -- 1.5.5.1
On Mon, May 19, 2008 at 1:57 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
1. makepkg - Reduces the missing arch error to a warning when only generating intergity checks (-g or --geninteg flag)
2. libalpm - remove unused handle->uid from pmhandle_t. The need to check permissions should be determined by the frontend (and is in pacman).
3. libalpm - fix comment on noextract in pmhandle_t.
Signed-off-by: Allan McRae <mcrae_allan@hotmail.com> --- lib/libalpm/handle.c | 2 -- lib/libalpm/handle.h | 3 +-- scripts/makepkg.sh.in | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 5f209d4..94317bf 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -23,7 +23,6 @@
#include <stdlib.h> #include <string.h> -#include <unistd.h> #include <limits.h> #include <sys/types.h> #include <syslog.h> @@ -54,7 +53,6 @@ pmhandle_t *_alpm_handle_new() handle->logstream = NULL;
/* see if we're root or not */ - handle->uid = geteuid(); handle->root = NULL; handle->dbpath = NULL; handle->cachedirs = NULL;
Maybe the comment should go as well?
diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5051917..9c537b1 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -30,7 +30,6 @@
typedef struct _pmhandle_t { /* internal usage */ - uid_t uid; /* current UID */ /* TODO is this used? */ pmdb_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (pmdb_t *) */ FILE *logstream; /* log file stream pointer */ @@ -50,7 +49,7 @@ typedef struct _pmhandle_t {
/* package lists */ alpm_list_t *noupgrade; /* List of packages NOT to be upgraded */ - alpm_list_t *noextract; /* List of packages NOT to extract */ /*TODO is this used?*/ + alpm_list_t *noextract; /* List of files NOT to extract */ alpm_list_t *ignorepkg; /* List of packages to ignore */ alpm_list_t *holdpkg; /* List of packages which 'hold' pacman */ alpm_list_t *ignoregrp; /* List of groups to ignore */
I wonder if NoUpgrade and NoExtract could not be somehow combined. But this requires a discussion to know in which situation these are used (I use none of them ...).
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc44c68..8009ef0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1315,7 +1315,7 @@ if [ "$arch" = 'any' ]; then fi
if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" = "0" ]; then + if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgname" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH"
I really don't understand the logic here :)
Xavier wrote:
On Mon, May 19, 2008 at 1:57 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
1. makepkg - Reduces the missing arch error to a warning when only generating intergity checks (-g or --geninteg flag)
2. libalpm - remove unused handle->uid from pmhandle_t. The need to check permissions should be determined by the frontend (and is in pacman).
3. libalpm - fix comment on noextract in pmhandle_t.
Signed-off-by: Allan McRae <mcrae_allan@hotmail.com> --- lib/libalpm/handle.c | 2 -- lib/libalpm/handle.h | 3 +-- scripts/makepkg.sh.in | 2 +- 3 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/lib/libalpm/handle.c b/lib/libalpm/handle.c index 5f209d4..94317bf 100644 --- a/lib/libalpm/handle.c +++ b/lib/libalpm/handle.c @@ -23,7 +23,6 @@
#include <stdlib.h> #include <string.h> -#include <unistd.h> #include <limits.h> #include <sys/types.h> #include <syslog.h> @@ -54,7 +53,6 @@ pmhandle_t *_alpm_handle_new() handle->logstream = NULL;
/* see if we're root or not */ - handle->uid = geteuid(); handle->root = NULL; handle->dbpath = NULL; handle->cachedirs = NULL;
Maybe the comment should go as well?
O_o maybe it should!
diff --git a/lib/libalpm/handle.h b/lib/libalpm/handle.h index 5051917..9c537b1 100644 --- a/lib/libalpm/handle.h +++ b/lib/libalpm/handle.h @@ -30,7 +30,6 @@
typedef struct _pmhandle_t { /* internal usage */ - uid_t uid; /* current UID */ /* TODO is this used? */ pmdb_t *db_local; /* local db pointer */ alpm_list_t *dbs_sync; /* List of (pmdb_t *) */ FILE *logstream; /* log file stream pointer */ @@ -50,7 +49,7 @@ typedef struct _pmhandle_t {
/* package lists */ alpm_list_t *noupgrade; /* List of packages NOT to be upgraded */ - alpm_list_t *noextract; /* List of packages NOT to extract */ /*TODO is this used?*/ + alpm_list_t *noextract; /* List of files NOT to extract */ alpm_list_t *ignorepkg; /* List of packages to ignore */ alpm_list_t *holdpkg; /* List of packages which 'hold' pacman */ alpm_list_t *ignoregrp; /* List of groups to ignore */
I wonder if NoUpgrade and NoExtract could not be somehow combined. But this requires a discussion to know in which situation these are used (I use none of them ...).
I don't either but when looking into making the pactests, they do serve slightly different purposes.... Maybe similar enough to combine though. In fact, at first I thought NoExtract should do things NoUpgrade does. I have added this to my TODO list so I will remember to look into it later.
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc44c68..8009ef0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1315,7 +1315,7 @@ if [ "$arch" = 'any' ]; then fi
if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" = "0" ]; then + if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgname" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH"
I really don't understand the logic here :)
I will add a comment there. Essentially, if we are only generating integrity checks, then we don't care if the arch line is missing. Well, at least not error level care. Maybe in that case a warning should still be printed. Opinions? Allan
On Mon, May 19, 2008 at 3:20 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc44c68..8009ef0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1315,7 +1315,7 @@ if [ "$arch" = 'any' ]; then fi
if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" = "0" ]; then + if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgname" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH"
I really don't understand the logic here :)
I will add a comment there. Essentially, if we are only generating integrity checks, then we don't care if the arch line is missing. Well, at least not error level care. Maybe in that case a warning should still be printed. Opinions?
It was more the implementation that I didn't understand : if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then Shouldn't it rather be something like this : if [ "$IGNOREARCH" = "0" -a "$GENINTEG" = "0" ]; then ? otherwise an additional if before that check : if [ "$GENINTEG" = "1" ]; then IGNOREARCH=1 fi But I think the current behavior is fine, just print an error in any cases. Are there really situations where you only want to generate md5sums, but not build the package or simply fix the pkgbuild so that others can build it?
Xavier wrote:
On Mon, May 19, 2008 at 3:20 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
diff --git a/scripts/makepkg.sh.in b/scripts/makepkg.sh.in index cc44c68..8009ef0 100644 --- a/scripts/makepkg.sh.in +++ b/scripts/makepkg.sh.in @@ -1315,7 +1315,7 @@ if [ "$arch" = 'any' ]; then fi
if ! in_array $CARCH ${arch[@]}; then - if [ "$IGNOREARCH" = "0" ]; then + if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then error "$(gettext "%s is not available for the '%s' architecture.")" "$pkgname" "$CARCH" plain "$(gettext "Note that many packages may need a line added to their %s")" "$BUILDSCRIPT" plain "$(gettext "such as arch=('%s').")" "$CARCH"
I really don't understand the logic here :)
I will add a comment there. Essentially, if we are only generating integrity checks, then we don't care if the arch line is missing. Well, at least not error level care. Maybe in that case a warning should still be printed. Opinions?
My bad.... it does actually print a warning as it is. That is in the else block of this if statement :palmface:
It was more the implementation that I didn't understand : if [ "$IGNOREARCH" = "0" -o "$GENINTEG" = "1" ]; then Shouldn't it rather be something like this : if [ "$IGNOREARCH" = "0" -a "$GENINTEG" = "0" ]; then ?
Yes. I have no idea who wrote the original code. An implementation that bad/wrong could not have come from me...
otherwise an additional if before that check : if [ "$GENINTEG" = "1" ]; then IGNOREARCH=1 fi
I think going the above way with a comment would be clearer.
But I think the current behavior is fine, just print an error in any cases. Are there really situations where you only want to generate md5sums, but not build the package or simply fix the pkgbuild so that others can build it?
When using the geninteg flag, it is not a error condition for the operation being run so it should continue. It is an edge case but I am sure I saw someone else suggest this as well so that is my justification of usefulness... Found it: http://wiki.archlinux.org/index.php/User:Romashka#Pacman Allan
On Mon, May 19, 2008 at 4:24 PM, Allan McRae <mcrae_allan@hotmail.com> wrote:
When using the geninteg flag, it is not a error condition for the operation being run so it should continue. It is an edge case but I am sure I saw someone else suggest this as well so that is my justification of usefulness... Found it: http://wiki.archlinux.org/index.php/User:Romashka#Pacman
Yeah ok. By the way, the way it is formulated there : "should not be checked at this step" made me think that we could maybe reorder the way things are done. Either delay arch check after integ check, or put integ check before arch check. But it is obviously harder to do this, so it might be not worth it.
2008/5/19 Allan McRae <mcrae_allan@hotmail.com>:
Xavier wrote:
But I think the current behavior is fine, just print an error in any cases. Are there really situations where you only want to generate md5sums, but not build the package or simply fix the pkgbuild so that others can build it?
When using the geninteg flag, it is not a error condition for the operation being run so it should continue. It is an edge case but I am sure I saw someone else suggest this as well so that is my justification of usefulness... Found it: http://wiki.archlinux.org/index.php/User:Romashka#Pacman
whoa! someone reads my ancient outdated never-done todos 8-) -- Roman Kyrylych (Роман Кирилич)
Roman Kyrylych wrote:
2008/5/19 Allan McRae <mcrae_allan@hotmail.com>:
Xavier wrote:
But I think the current behavior is fine, just print an error in any cases. Are there really situations where you only want to generate md5sums, but not build the package or simply fix the pkgbuild so that others can build it?
When using the geninteg flag, it is not a error condition for the operation being run so it should continue. It is an edge case but I am sure I saw someone else suggest this as well so that is my justification of usefulness... Found it: http://wiki.archlinux.org/index.php/User:Romashka#Pacman
whoa! someone reads my ancient outdated never-done todos 8-)
Not really... It was just what google came up with when I was trying to find where I originally saw this. I thought it was on the forums.
participants (4)
-
Allan McRae
-
Dan McGee
-
Roman Kyrylych
-
Xavier