Re: [pacman-dev] [PATCH] Introduce event types for start/end database list download

Hello On Tue, Apr 28, 2020 at 6:31 PM Allan McRae wrote:

On 17/4/20 12:31 pm, Anatol Pomozov wrote:

...

Multiplexed database/files downloads will use multiple progress bars. The UI logic is quite complicated and printing error messages while handling multiple progress bars is going to be challenging.

Instead we are going to save all ALPM error messages to a list and flush it at the end of the download process. Use on_progress variable that blocks error messages printing.

This is going to be fun... If you have a 100 package update and your first mirror is down, you will end up with 100 error messages at the end. e.g.

$ sudo ./build/pacman -Sy :: Synchronizing package databases... testing 45.4 KiB 24.5 KiB/s 00:02 [##############] 100% core 134.6 KiB 61.8 KiB/s 00:02 [##############] 100% extra 1646.0 KiB 741 KiB/s 00:02 [##############] 100% community-testing 621.3 KiB 468 KiB/s 00:01 [##############] 100% community 4.9 MiB 2.12 MiB/s 00:02 [##############] 100% error: failed retrieving file 'extra.db' from allanmcrae.com : The requested URL returned error: 404 error: failed retrieving file 'testing.db' from allanmcrae.com : The requested URL returned error: 404 error: failed retrieving file 'core.db' from allanmcrae.com : The requested URL returned error: 404 error: failed retrieving file 'community-testing.db' from allanmcrae.com : The requested URL returned error: 404 error: failed retrieving file 'community.db' from allanmcrae.com : The requested URL returned error: 404

I believe this is the same behavior as we currently have in the latest release.

This is patch is fine, but we should perhaps change the output to count the failures from each server.

Doing aggregation like this would be a bit tricky. The error message formatting happens at ALPM level when it calls _alpm_log() function. _alpm_log() calls handle->logcb() with a formatted string as an argument. So pacman has no reliable way to find out what was the error context. Pacman's responsibility at this point is just to collect the messages and print it without screwing up the UI. Thus the error aggregation need to happen in dload.c and would require some refactoring.

Also, it is not an error to fail retrieving a package or database if you get if from the next server. So this could be changed to a warning.

e.g. warning: failed retrieving 5 files from allanmcrae.com

This is nothing directly to do with this patch - just something we need to tidy.

...

Signed-off-by: Anatol Pomozov --- lib/libalpm/alpm.h | 6 ++++++ lib/libalpm/be_sync.c | 7 +++++++ src/pacman/callback.c | 13 +++++++++++-- 3 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/lib/libalpm/alpm.h b/lib/libalpm/alpm.h index 2cf20343..aee5aa0d 100644 --- a/lib/libalpm/alpm.h +++ b/lib/libalpm/alpm.h @@ -403,6 +403,12 @@ typedef enum _alpm_event_type_t { * arguments. */ ALPM_EVENT_SCRIPTLET_INFO, /** Files will be downloaded from a repository. */

/** Database files will be....

done

...

+ ALPM_EVENT_DB_RETRIEVE_START, + /** Files were downloaded from a repository. */ + ALPM_EVENT_DB_RETRIEVE_DONE, + /** Not all files were successfully downloaded from a repository. */ + ALPM_EVENT_DB_RETRIEVE_FAILED, + /** Files will be downloaded from a repository. */

/** Package files will be...

done

...

ALPM_EVENT_RETRIEVE_START,

As we are splitting db and package events, this should be changed to:

ALPM_EVENT_PKG_RETRIEVE_START

done

...

/** Files were downloaded from a repository. */ ALPM_EVENT_RETRIEVE_DONE, diff --git a/lib/libalpm/be_sync.c b/lib/libalpm/be_sync.c index 1be40650..826c11e6 100644 --- a/lib/libalpm/be_sync.c +++ b/lib/libalpm/be_sync.c @@ -308,6 +308,7 @@ int SYMEXPORT alpm_dbs_update(alpm_handle_t *handle, alpm_list_t *dbs, int force int ret = -1; mode_t oldmask; alpm_list_t *payloads = NULL; + alpm_event_t event;

/* Sanity checks */ CHECK_HANDLE(handle, return -1); @@ -382,10 +383,16 @@ int SYMEXPORT alpm_dbs_update(alpm_handle_t *handle, alpm_list_t *dbs, int force } }

+ event.type = ALPM_EVENT_DB_RETRIEVE_START; + EVENT(handle, &event); ret = _alpm_multi_download(handle, payloads, syncpath); if(ret < 0) { + event.type = ALPM_EVENT_DB_RETRIEVE_FAILED; + EVENT(handle, &event); goto cleanup; } + event.type = ALPM_EVENT_DB_RETRIEVE_DONE; + EVENT(handle, &event);

for(i = dbs; i; i = i->next) { alpm_db_t *db = i->data; diff --git a/src/pacman/callback.c b/src/pacman/callback.c index 8fb89b39..585eae09 100644 --- a/src/pacman/callback.c +++ b/src/pacman/callback.c @@ -280,8 +280,12 @@ void cb_event(alpm_event_t *event) case ALPM_EVENT_SCRIPTLET_INFO: fputs(event->scriptlet_info.line, stdout); break; + case ALPM_EVENT_DB_RETRIEVE_START: + on_progress = 1; + break; case ALPM_EVENT_RETRIEVE_START: colon_printf(_("Retrieving packages...\n")); + on_progress = 1; break; case ALPM_EVENT_DISKSPACE_START: if(config->noprogressbar) { @@ -338,6 +342,13 @@ void cb_event(alpm_event_t *event) } } break; + case ALPM_EVENT_DB_RETRIEVE_DONE: + case ALPM_EVENT_DB_RETRIEVE_FAILED: + case ALPM_EVENT_RETRIEVE_DONE: + case ALPM_EVENT_RETRIEVE_FAILED: + flush_output_list(); + on_progress = 0; + break; /* all the simple done events, with fallthrough for each */ case ALPM_EVENT_FILECONFLICTS_DONE: case ALPM_EVENT_CHECKDEPS_DONE: @@ -349,8 +360,6 @@ void cb_event(alpm_event_t *event) case ALPM_EVENT_KEY_DOWNLOAD_DONE: case ALPM_EVENT_LOAD_DONE: case ALPM_EVENT_DISKSPACE_DONE: - case ALPM_EVENT_RETRIEVE_DONE: - case ALPM_EVENT_RETRIEVE_FAILED: case ALPM_EVENT_HOOK_DONE: case ALPM_EVENT_HOOK_RUN_DONE: /* we can safely ignore those as well */