[pacman-dev] [PATCH 1/2] Combine add and removal package list display
There was no real reason for these to be done separately. Signed-off-by: Dan McGee <dan@archlinux.org> --- A bit of WIP here. Try it out to see what it does, but it addresses Allan's issue with upgrade size being all whacky when replaces are involved, among other things. http://paste.pocoo.org/show/444372/ for some example output with VerbosePkgLists on, http://paste.pocoo.org/show/444368/ with them off. Not sure what to do with the corny '[removal]' tag bit though. Notes: * The size column is now net changed size per package, so only new packages or totally removed packages will usually have big values (wireshark-cli is buggy in the package itself, as noticed by Dave and I). * Packages are now always sorted in alpha order, rather than the half-sorted, dep resolved mess we had before. * -R ops will show the same format as -U, even though the new version column will now always be blank. I think the consistency win is worth it. They will also always show the '[removal]' tag bit, so suggestions for that are welcome. * Yes, you get 4 different totals now on -S operations, but since the damn package list is so long anyway, who cares. Feedback/thoughts definitely welcome. I'll improve the commit message too before any final version goes live. -Dan src/pacman/remove.c | 2 +- src/pacman/sync.c | 3 +- src/pacman/upgrade.c | 3 +- src/pacman/util.c | 163 +++++++++++++++++++++++++++++++------------------ src/pacman/util.h | 7 ++- 5 files changed, 112 insertions(+), 66 deletions(-) diff --git a/src/pacman/remove.c b/src/pacman/remove.c index f0ac04e..95a728e 100644 --- a/src/pacman/remove.c +++ b/src/pacman/remove.c @@ -156,7 +156,7 @@ int pacman_remove(alpm_list_t *targets) } /* print targets and ask user confirmation */ - display_targets(pkglist, 0); + display_targets(); printf("\n"); if(yesno(_("Do you want to remove these packages?")) == 0) { retval = 1; diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 5e1643e..0facd6c 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -795,8 +795,7 @@ static int sync_trans(alpm_list_t *targets) goto cleanup; } - display_targets(alpm_trans_get_remove(config->handle), 0); - display_targets(alpm_trans_get_add(config->handle), 1); + display_targets(); printf("\n"); int confirm; diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index a99b137..55b5fa9 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -152,8 +152,7 @@ int pacman_upgrade(alpm_list_t *targets) trans_release(); return retval; } - display_targets(alpm_trans_get_remove(config->handle), 0); - display_targets(alpm_trans_get_add(config->handle), 1); + display_targets(); printf("\n"); int confirm = yesno(_("Proceed with installation?")); if(!confirm) { diff --git a/src/pacman/util.c b/src/pacman/util.c index 7065abd..a300f14 100644 --- a/src/pacman/util.c +++ b/src/pacman/util.c @@ -711,7 +711,7 @@ void signature_display(const char *title, alpm_sigresult_t *result) } /* creates a header row for use with table_display */ -static alpm_list_t *create_verbose_header(int install) +static alpm_list_t *create_verbose_header(void) { alpm_list_t *res = NULL; char *str; @@ -720,10 +720,8 @@ static alpm_list_t *create_verbose_header(int install) res = alpm_list_add(res, str); pm_asprintf(&str, "%s", _("Old Version")); res = alpm_list_add(res, str); - if(install) { - pm_asprintf(&str, "%s", _("New Version")); - res = alpm_list_add(res, str); - } + pm_asprintf(&str, "%s", _("New Version")); + res = alpm_list_add(res, str); pm_asprintf(&str, "%s", _("Size")); res = alpm_list_add(res, str); @@ -731,125 +729,170 @@ static alpm_list_t *create_verbose_header(int install) } /* returns package info as list of strings */ -static alpm_list_t *create_verbose_row(alpm_pkg_t *pkg, int install) +static alpm_list_t *create_verbose_row(pm_target_t *target) { char *str; - double size; + off_t size = 0; + double human_size; const char *label; alpm_list_t *ret = NULL; - alpm_db_t *ldb = alpm_option_get_localdb(config->handle); /* a row consists of the package name, */ - pm_asprintf(&str, "%s", alpm_pkg_get_name(pkg)); + if(target->install) { + pm_asprintf(&str, "%s", alpm_pkg_get_name(target->install)); + } else { + pm_asprintf(&str, "%s", alpm_pkg_get_name(target->remove)); + } ret = alpm_list_add(ret, str); /* old and new versions */ - if(install) { - alpm_pkg_t *oldpkg = alpm_db_get_pkg(ldb, alpm_pkg_get_name(pkg)); - pm_asprintf(&str, "%s", - oldpkg != NULL ? alpm_pkg_get_version(oldpkg) : ""); - ret = alpm_list_add(ret, str); - } + pm_asprintf(&str, "%s", + target->remove != NULL ? alpm_pkg_get_version(target->remove) : ""); + ret = alpm_list_add(ret, str); - pm_asprintf(&str, "%s", alpm_pkg_get_version(pkg)); + pm_asprintf(&str, "%s", + target->install != NULL ? alpm_pkg_get_version(target->install) : ""); ret = alpm_list_add(ret, str); /* and size */ - size = humanize_size(alpm_pkg_get_size(pkg), 'M', 1, &label); - pm_asprintf(&str, "%.2f %s", size, label); + size -= target->remove ? alpm_pkg_get_isize(target->remove) : 0; + size += target->install ? alpm_pkg_get_isize(target->install) : 0; + human_size = humanize_size(size, 'M', 1, &label); + pm_asprintf(&str, "%.2f %s", human_size, label); ret = alpm_list_add(ret, str); return ret; } /* prepare a list of pkgs to display */ -void display_targets(const alpm_list_t *pkgs, int install) +static void _display_targets(alpm_list_t *targets) { char *str; - const char *title, *label; + const char *label; double size; - const alpm_list_t *i; off_t isize = 0, rsize = 0, dlsize = 0; - alpm_list_t *j, *lp, *header = NULL, *targets = NULL; - alpm_db_t *db_local = alpm_option_get_localdb(config->handle); + alpm_list_t *i, *header = NULL, *rows = NULL; - if(!pkgs) { + if(!targets) { return; } - /* gather pkg infos */ - for(i = pkgs; i; i = alpm_list_next(i)) { - alpm_pkg_t *pkg = alpm_list_getdata(i); + /* gather package info */ + for(i = targets; i; i = alpm_list_next(i)) { + pm_target_t *target = alpm_list_getdata(i); - if(install) { - alpm_pkg_t *lpkg = alpm_db_get_pkg(db_local, alpm_pkg_get_name(pkg)); - dlsize += alpm_pkg_download_size(pkg); - if(lpkg) { - /* add up size of all removed packages */ - rsize += alpm_pkg_get_isize(lpkg); - } + if(target->install) { + dlsize += alpm_pkg_download_size(target->install); + isize += alpm_pkg_get_isize(target->install); + } + if(target->remove) { + /* add up size of all removed packages */ + rsize += alpm_pkg_get_isize(target->remove); } - isize += alpm_pkg_get_isize(pkg); if(config->verbosepkglists) { - targets = alpm_list_add(targets, create_verbose_row(pkg, install)); + rows = alpm_list_add(rows, create_verbose_row(target)); } else { - pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(pkg), - alpm_pkg_get_version(pkg)); - targets = alpm_list_add(targets, str); + if(target->install) { + pm_asprintf(&str, "%s-%s", alpm_pkg_get_name(target->install), + alpm_pkg_get_version(target->install)); + } else { + pm_asprintf(&str, "%s-%s [removal]", alpm_pkg_get_name(target->remove), + alpm_pkg_get_version(target->remove)); + } + rows = alpm_list_add(rows, str); } } /* print to screen */ - title = install ? _("Targets (%d):") : _("Remove (%d):"); - pm_asprintf(&str, title, alpm_list_count(pkgs)); + pm_asprintf(&str, _("Targets (%d):"), alpm_list_count(targets)); printf("\n"); if(config->verbosepkglists) { - header = create_verbose_header(install); - if(table_display(str, header, targets) != 0) { + header = create_verbose_header(); + if(table_display(str, header, rows) != 0) { config->verbosepkglists = 0; - display_targets(pkgs, install); + _display_targets(targets); goto out; } } else { - list_display(str, targets); + list_display(str, rows); } printf("\n"); - if(install) { + if(dlsize > 0) { size = humanize_size(dlsize, 'M', 1, &label); printf(_("Total Download Size: %.2f %s\n"), size, label); - if(!(config->flags & ALPM_TRANS_FLAG_DOWNLOADONLY)) { + } + if(!(config->flags & ALPM_TRANS_FLAG_DOWNLOADONLY)) { + if(isize > 0) { size = humanize_size(isize, 'M', 1, &label); printf(_("Total Installed Size: %.2f %s\n"), size, label); - /* only show this net value if different from raw installed size */ - if(rsize > 0) { - size = humanize_size(isize - rsize, 'M', 1, &label); - printf(_("Net Upgrade Size: %.2f %s\n"), size, label); - } } - } else { - size = humanize_size(isize, 'M', 1, &label); - printf(_("Total Removed Size: %.2f %s\n"), size, label); + if(rsize > 0) { + size = humanize_size(rsize, 'M', 1, &label); + printf(_("Total Removed Size: %.2f %s\n"), size, label); + } + /* only show this net value if different from raw installed size */ + if(isize > 0 && rsize > 0) { + size = humanize_size(isize - rsize, 'M', 1, &label); + printf(_("Net Upgrade Size: %.2f %s\n"), size, label); + } } out: /* cleanup */ if(config->verbosepkglists) { - /* targets is a list of lists of strings, free inner lists here */ - for(j = targets; j; j = alpm_list_next(j)) { - lp = alpm_list_getdata(j); + /* rows is a list of lists of strings, free inner lists here */ + for(i = rows; i; i = alpm_list_next(i)) { + alpm_list_t *lp = alpm_list_getdata(i); FREELIST(lp); } - alpm_list_free(targets); + alpm_list_free(rows); FREELIST(header); } else { - FREELIST(targets); + FREELIST(rows); } free(str); } +static int target_cmp(const void *p1, const void *p2) +{ + const pm_target_t *targ1 = p1; + const pm_target_t *targ2 = p2; + const char *name1 = targ1->install ? + alpm_pkg_get_name(targ1->install) : alpm_pkg_get_name(targ1->remove); + const char *name2 = targ2->install ? + alpm_pkg_get_name(targ2->install) : alpm_pkg_get_name(targ2->remove); + return strcmp(name1, name2); +} + +void display_targets(void) +{ + alpm_list_t *i, *targets = NULL; + alpm_db_t *db_local = alpm_option_get_localdb(config->handle); + + for(i = alpm_trans_get_add(config->handle); i; i = alpm_list_next(i)) { + alpm_pkg_t *pkg = alpm_list_getdata(i); + pm_target_t *targ = calloc(1, sizeof(pm_target_t)); + if(!targ) return; + targ->install = pkg; + targ->remove = alpm_db_get_pkg(db_local, alpm_pkg_get_name(pkg)); + targets = alpm_list_add(targets, targ); + } + for(i = alpm_trans_get_remove(config->handle); i; i = alpm_list_next(i)) { + alpm_pkg_t *pkg = alpm_list_getdata(i); + pm_target_t *targ = calloc(1, sizeof(pm_target_t)); + if(!targ) return; + targ->remove = pkg; + targets = alpm_list_add(targets, targ); + } + + targets = alpm_list_msort(targets, alpm_list_count(targets), target_cmp); + _display_targets(targets); + FREELIST(targets); +} + static off_t pkg_get_size(alpm_pkg_t *pkg) { switch(config->op) { diff --git a/src/pacman/util.h b/src/pacman/util.h index ee3dbd1..54c941d 100644 --- a/src/pacman/util.h +++ b/src/pacman/util.h @@ -39,6 +39,11 @@ /* update speed for the fill_progress based functions */ #define UPDATE_SPEED_SEC 0.2f +typedef struct _pm_target_t { + alpm_pkg_t *remove; + alpm_pkg_t *install; +} pm_target_t; + int trans_init(alpm_transflag_t flags, int check_valid); int trans_release(void); int needs_root(void); @@ -58,7 +63,7 @@ int table_display(const char *title, const alpm_list_t *header, const alpm_list_ void list_display(const char *title, const alpm_list_t *list); void list_display_linebreak(const char *title, const alpm_list_t *list); void signature_display(const char *title, alpm_sigresult_t *result); -void display_targets(const alpm_list_t *pkgs, int install); +void display_targets(void); int str_cmp(const void *s1, const void *s2); void display_new_optdepends(alpm_pkg_t *oldpkg, alpm_pkg_t *newpkg); void display_optdepends(alpm_pkg_t *pkg); -- 1.7.6
Pacman did a great job of having almost (but not quite) duplicate code paths through the sync and upgrade code. We can use the same logic in both upgrade in sync once the targets are resolved, so extract a function and delete a bunch of code. Signed-off-by: Dan McGee <dan@archlinux.org> --- Saw this when writing the previous patch and decided it should really get fixed. Duplicate code sucks. src/pacman/pacman.h | 1 + src/pacman/sync.c | 19 +++++--- src/pacman/upgrade.c | 113 +------------------------------------------------ 3 files changed, 16 insertions(+), 117 deletions(-) diff --git a/src/pacman/pacman.h b/src/pacman/pacman.h index 762e112..8bb9cb2 100644 --- a/src/pacman/pacman.h +++ b/src/pacman/pacman.h @@ -32,6 +32,7 @@ int pacman_query(alpm_list_t *targets); int pacman_remove(alpm_list_t *targets); /* sync.c */ int pacman_sync(alpm_list_t *targets); +int sync_prepare_execute(void); /* upgrade.c */ int pacman_upgrade(alpm_list_t *targets); diff --git a/src/pacman/sync.c b/src/pacman/sync.c index 0facd6c..eff0cea 100644 --- a/src/pacman/sync.c +++ b/src/pacman/sync.c @@ -712,9 +712,6 @@ cleanup: static int sync_trans(alpm_list_t *targets) { - int retval = 0; - alpm_list_t *data = NULL; - alpm_list_t *packages = NULL; alpm_list_t *i; /* Step 1: create a new transaction... */ @@ -726,8 +723,8 @@ static int sync_trans(alpm_list_t *targets) for(i = targets; i; i = alpm_list_next(i)) { char *targ = alpm_list_getdata(i); if(process_target(targ) == 1) { - retval = 1; - goto cleanup; + trans_release(); + return 1; } } @@ -736,11 +733,19 @@ static int sync_trans(alpm_list_t *targets) alpm_logaction(config->handle, "starting full system upgrade\n"); if(alpm_sync_sysupgrade(config->handle, config->op_s_upgrade >= 2) == -1) { pm_fprintf(stderr, ALPM_LOG_ERROR, "%s\n", alpm_strerror(alpm_errno(config->handle))); - retval = 1; - goto cleanup; + trans_release(); + return 1; } } + return sync_prepare_execute(); +} + +int sync_prepare_execute(void) +{ + alpm_list_t *i, *packages, *data = NULL; + int retval = 0; + /* Step 2: "compute" the transaction based on targets and flags */ if(alpm_trans_prepare(config->handle, &data) == -1) { enum _alpm_errno_t err = alpm_errno(config->handle); diff --git a/src/pacman/upgrade.c b/src/pacman/upgrade.c index 55b5fa9..e4b2e2b 100644 --- a/src/pacman/upgrade.c +++ b/src/pacman/upgrade.c @@ -41,9 +41,8 @@ */ int pacman_upgrade(alpm_list_t *targets) { - alpm_list_t *i, *data = NULL; + alpm_list_t *i; alpm_siglevel_t level = alpm_option_get_default_siglevel(config->handle); - int retval = 0; if(targets == NULL) { pm_printf(ALPM_LOG_ERROR, _("no targets specified (use -h for help)\n")); @@ -91,114 +90,8 @@ int pacman_upgrade(alpm_list_t *targets) } } - /* Step 2: "compute" the transaction based on targets and flags */ - /* TODO: No, compute nothing. This is stupid. */ - if(alpm_trans_prepare(config->handle, &data) == -1) { - enum _alpm_errno_t err = alpm_errno(config->handle); - pm_fprintf(stderr, ALPM_LOG_ERROR, _("failed to prepare transaction (%s)\n"), - alpm_strerror(err)); - switch(err) { - case ALPM_ERR_PKG_INVALID_ARCH: - for(i = data; i; i = alpm_list_next(i)) { - char *pkg = alpm_list_getdata(i); - printf(_(":: package %s does not have a valid architecture\n"), pkg); - } - break; - case ALPM_ERR_UNSATISFIED_DEPS: - for(i = data; i; i = alpm_list_next(i)) { - alpm_depmissing_t *miss = alpm_list_getdata(i); - char *depstring = alpm_dep_compute_string(miss->depend); - - /* TODO indicate if the error was a virtual package or not: - * :: %s: requires %s, provided by %s - */ - printf(_(":: %s: requires %s\n"), miss->target, depstring); - free(depstring); - } - break; - case ALPM_ERR_CONFLICTING_DEPS: - for(i = data; i; i = alpm_list_next(i)) { - alpm_conflict_t *conflict = alpm_list_getdata(i); - if(strcmp(conflict->package1, conflict->reason) == 0 || - strcmp(conflict->package2, conflict->reason) == 0) { - printf(_(":: %s and %s are in conflict\n"), - conflict->package1, conflict->package2); - } else { - printf(_(":: %s and %s are in conflict (%s)\n"), - conflict->package1, conflict->package2, conflict->reason); - } - } - break; - default: - break; - } - trans_release(); - FREELIST(data); - return 1; - } - - /* Step 3: perform the installation */ - alpm_list_t *packages = alpm_trans_get_add(config->handle); - - if(config->print) { - print_packages(packages); - trans_release(); - return 0; - } - - /* print targets and ask user confirmation */ - if(packages == NULL) { /* we are done */ - printf(_(" there is nothing to do\n")); - trans_release(); - return retval; - } - display_targets(); - printf("\n"); - int confirm = yesno(_("Proceed with installation?")); - if(!confirm) { - trans_release(); - return retval; - } - - if(alpm_trans_commit(config->handle, &data) == -1) { - enum _alpm_errno_t err = alpm_errno(config->handle); - pm_fprintf(stderr, ALPM_LOG_ERROR, _("failed to commit transaction (%s)\n"), - alpm_strerror(err)); - switch(err) { - case ALPM_ERR_FILE_CONFLICTS: - for(i = data; i; i = alpm_list_next(i)) { - alpm_fileconflict_t *conflict = alpm_list_getdata(i); - switch(conflict->type) { - case ALPM_FILECONFLICT_TARGET: - printf(_("%s exists in both '%s' and '%s'\n"), - conflict->file, conflict->target, conflict->ctarget); - break; - case ALPM_FILECONFLICT_FILESYSTEM: - printf(_("%s: %s exists in filesystem\n"), - conflict->target, conflict->file); - break; - } - } - break; - case ALPM_ERR_PKG_INVALID: - case ALPM_ERR_DLT_INVALID: - for(i = data; i; i = alpm_list_next(i)) { - char *filename = alpm_list_getdata(i); - printf(_("%s is invalid or corrupted\n"), filename); - } - break; - default: - break; - } - FREELIST(data); - trans_release(); - return 1; - } - - if(trans_release() == -1) { - retval = 1; - } - return retval; + /* now that targets are resolved, we can hand it all off to the sync code */ + return sync_prepare_execute(); } /* vim: set ts=2 sw=2 noet: */ -- 1.7.6
On 07/22/2011 07:19 PM, Dan McGee wrote:
There was no real reason for these to be done separately.
Signed-off-by: Dan McGee<dan@archlinux.org> ---
A bit of WIP here. Try it out to see what it does, but it addresses Allan's issue with upgrade size being all whacky when replaces are involved, among other things. http://paste.pocoo.org/show/444372/ for some example output with VerbosePkgLists on, http://paste.pocoo.org/show/444368/ with them off. Not sure what to do with the corny '[removal]' tag bit though.
Notes: * The size column is now net changed size per package, so only new packages or totally removed packages will usually have big values (wireshark-cli is buggy in the package itself, as noticed by Dave and I).
I'm not sure I agree with this. During a typical 'pacman -Syu', the net changed size will be ~0.00 MiB for nearly all packages, making the column more or less useless for the most commonly used operation. Unfortunately, I don't have an ideal solution. Currently, the size column displays installed size for removal lists and and package size for install / upgrade lists. I can think of two issues with this: 1) the column header is always 'Size' and it isn't clear enough about what's actually being displayed; and 2) displayed different things in the same column might be confusing/misleading; of course it gets much worse if removal and sync targets are shown in the same list. On the other hand, it feels correct to display sizes like this. During sync operations I want to know about package sizes, and when removing packages I want to know how much disk space is regained. It's also easy to pick out the largest packages during a large -Syu at a glance. The only other option I can think of with a merged removal / sync list is install size, which might be worth a try (even though I believe I'd prefer seeing download size for -S targets). tl;dr - in my opinion we should either stick to the old display with separate removal and sync lists (and fix the column header), or display install size in a merged list. P.S.: It might also be nice to keep the removal / sync target count instead of only displayed a total target count.
* Packages are now always sorted in alpha order, rather than the half-sorted, dep resolved mess we had before. * -R ops will show the same format as -U, even though the new version column will now always be blank. I think the consistency win is worth it. They will also always show the '[removal]' tag bit, so suggestions for that are welcome. * Yes, you get 4 different totals now on -S operations, but since the damn package list is so long anyway, who cares.
All of this sounds good to me.
On Wed, Jul 27, 2011 at 12:12 PM, Jakob Gruber <jakob.gruber@gmail.com> wrote:
On 07/22/2011 07:19 PM, Dan McGee wrote:
* The size column is now net changed size per package, so only new packages or totally removed packages will usually have big values (wireshark-cli is buggy in the package itself, as noticed by Dave and I).
I'm not sure I agree with this. During a typical 'pacman -Syu', the net changed size will be ~0.00 MiB for nearly all packages, making the column more or less useless for the most commonly used operation.
A "competing" product does this (trimmed for size): $ sudo yum update ... Package Arch Version Repository Size Updating: kernel-headers i386 2.6.18-238.19.1.el5 updates 1.1 M Transaction Summary Install 0 Package(s) Upgrade 1 Package(s) Total download size: 1.1 M $ sudo yum remove kernel-headers ... Package Arch Version Repository Size Removing: kernel-headers i386 2.6.18-238.19.1.el5 installed 2.2 M Removing for dependencies: gcc i386 4.1.2-50.el5 installed 9.6 M gcc-c++ i386 4.1.2-50.el5 installed 6.5 M glibc-devel i386 2.5-58.el5_6.4 installed 4.9 M glibc-headers i386 2.5-58.el5_6.4 installed 2.0 M Transaction Summary Remove 5 Package(s) Reinstall 0 Package(s) Downgrade 0 Package(s) So we do have the size mismatch there as well. Not sure what it does if you are installing an RPM directly.
Unfortunately, I don't have an ideal solution.
Currently, the size column displays installed size for removal lists and and package size for install / upgrade lists. I can think of two issues with this:
1) the column header is always 'Size' and it isn't clear enough about what's actually being displayed; and 2) displayed different things in the same column might be confusing/misleading; of course it gets much worse if removal and sync targets are shown in the same list.
On the other hand, it feels correct to display sizes like this. During sync operations I want to know about package sizes, and when removing packages I want to know how much disk space is regained. It's also easy to pick out the largest packages during a large -Syu at a glance.
The only other option I can think of with a merged removal / sync list is install size, which might be worth a try (even though I believe I'd prefer seeing download size for -S targets).
tl;dr - in my opinion we should either stick to the old display with separate removal and sync lists (and fix the column header), or display install size in a merged list.
P.S.: It might also be nice to keep the removal / sync target count instead of only displayed a total target count. Patches welcome; I could see these being added to the size display at
Anyone else? I'm still not sure what is right here, but I'm leaning toward a tad more explicit but still totally whacky: sync- show "Download Size", show 0.00/-0.00/blank for to-be-removed packages remove- show "Installed Size", all will be negative upgrade- show "Installed Size", all will be positive, unless inducing a removal of a different named package (do we even do this right?), then that one will show negative installed size the bottom. -Dan
On 07/29/2011 10:41 PM, Dan McGee wrote:
On Wed, Jul 27, 2011 at 12:12 PM, Jakob Gruber<jakob.gruber@gmail.com> wrote:
On 07/22/2011 07:19 PM, Dan McGee wrote:
* The size column is now net changed size per package, so only new packages or totally removed packages will usually have big values (wireshark-cli is buggy in the package itself, as noticed by Dave and I). I'm not sure I agree with this. During a typical 'pacman -Syu', the net changed size will be ~0.00 MiB for nearly all packages, making the column more or less useless for the most commonly used operation. A "competing" product does this (trimmed for size):
$ sudo yum update ... Package Arch Version Repository Size Updating: kernel-headers i386 2.6.18-238.19.1.el5 updates 1.1 M
Transaction Summary Install 0 Package(s) Upgrade 1 Package(s) Total download size: 1.1 M
$ sudo yum remove kernel-headers ... Package Arch Version Repository Size Removing: kernel-headers i386 2.6.18-238.19.1.el5 installed 2.2 M Removing for dependencies: gcc i386 4.1.2-50.el5 installed 9.6 M gcc-c++ i386 4.1.2-50.el5 installed 6.5 M glibc-devel i386 2.5-58.el5_6.4 installed 4.9 M glibc-headers i386 2.5-58.el5_6.4 installed 2.0 M
Transaction Summary Remove 5 Package(s) Reinstall 0 Package(s) Downgrade 0 Package(s)
So we do have the size mismatch there as well. Not sure what it does if you are installing an RPM directly.
Unfortunately, I don't have an ideal solution.
Currently, the size column displays installed size for removal lists and and package size for install / upgrade lists. I can think of two issues with this:
1) the column header is always 'Size' and it isn't clear enough about what's actually being displayed; and 2) displayed different things in the same column might be confusing/misleading; of course it gets much worse if removal and sync targets are shown in the same list.
On the other hand, it feels correct to display sizes like this. During sync operations I want to know about package sizes, and when removing packages I want to know how much disk space is regained. It's also easy to pick out the largest packages during a large -Syu at a glance.
The only other option I can think of with a merged removal / sync list is install size, which might be worth a try (even though I believe I'd prefer seeing download size for -S targets). Anyone else? I'm still not sure what is right here, but I'm leaning toward a tad more explicit but still totally whacky: sync- show "Download Size", show 0.00/-0.00/blank for to-be-removed packages remove- show "Installed Size", all will be negative upgrade- show "Installed Size", all will be positive, unless inducing a removal of a different named package (do we even do this right?), then that one will show negative installed size
After some more thought, I'd propose keeping the removal and install lists separate but with a single common summary section. Even when ignoring size inconsistencies, mixing both operations in a single list seems a little hard to read and confusing. A quick mockup: http://pastebin.com/EyVSZ2Zq Another alternative, this time keeping the single list to save a few lines of vertical space but explicitly marking removal/install targets and keeping all removal targets before install targets : http://pastebin.com/QKkWVNsJ What do you think?
On 23/07/11 03:19, Dan McGee wrote:
There was no real reason for these to be done separately.
Signed-off-by: Dan McGee<dan@archlinux.org> ---
A bit of WIP here. Try it out to see what it does, but it addresses Allan's issue with upgrade size being all whacky when replaces are involved, among other things. http://paste.pocoo.org/show/444372/ for some example output with VerbosePkgLists on, http://paste.pocoo.org/show/444368/ with them off. Not sure what to do with the corny '[removal]' tag bit though.
Notes: * The size column is now net changed size per package, so only new packages or totally removed packages will usually have big values (wireshark-cli is buggy in the package itself, as noticed by Dave and I). * Packages are now always sorted in alpha order, rather than the half-sorted, dep resolved mess we had before.
I am not sure I like this... Currently, "pacman -S foo" will always show "foo" as the last target. I find that makes it clear that the rest of the packages are the needed dependencies. With this patch, "foo" can end up anywhere in the package listing. For a package with lots of dependencies, it becomes difficult to see that "foo" is even being installed at a quick glance.
* -R ops will show the same format as -U, even though the new version column will now always be blank. I think the consistency win is worth it. They will also always show the '[removal]' tag bit, so suggestions for that are welcome. * Yes, you get 4 different totals now on -S operations, but since the damn package list is so long anyway, who cares.
Feedback/thoughts definitely welcome. I'll improve the commit message too before any final version goes live.
-Dan
On Fri, Jul 29, 2011 at 1:09 AM, Allan McRae <allan@archlinux.org> wrote:
On 23/07/11 03:19, Dan McGee wrote:
* Packages are now always sorted in alpha order, rather than the half-sorted, dep resolved mess we had before.
I am not sure I like this... Currently, "pacman -S foo" will always show "foo" as the last target. I find that makes it clear that the rest of the packages are the needed dependencies.
With this patch, "foo" can end up anywhere in the package listing. For a package with lots of dependencies, it becomes difficult to see that "foo" is even being installed at a quick glance.
How about a slight tweak, as I just hated the old seemingly random order. We'll do pulled package alpha-ordered, followed by explict alpha-ordered. This still makes my main beef, the -Syu case, print how I want while solving your problem as well.
On 30/07/11 05:37, Dan McGee wrote:
On Fri, Jul 29, 2011 at 1:09 AM, Allan McRae<allan@archlinux.org> wrote:
On 23/07/11 03:19, Dan McGee wrote:
* Packages are now always sorted in alpha order, rather than the half-sorted, dep resolved mess we had before.
I am not sure I like this... Currently, "pacman -S foo" will always show "foo" as the last target. I find that makes it clear that the rest of the packages are the needed dependencies.
With this patch, "foo" can end up anywhere in the package listing. For a package with lots of dependencies, it becomes difficult to see that "foo" is even being installed at a quick glance.
How about a slight tweak, as I just hated the old seemingly random order. We'll do pulled package alpha-ordered, followed by explict alpha-ordered. This still makes my main beef, the -Syu case, print how I want while solving your problem as well.
Agreed. That would be fine to me. Allan
participants (4)
-
Allan McRae
-
Dan McGee
-
Dan McGee
-
Jakob Gruber