diff mbox series

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

Message ID 20200417023101.32685-1-anatol.pomozov@gmail.com
State Superseded, archived
Headers show
Series [pacman-dev] Introduce event types for start/end database list download | expand

Commit Message

Anatol Pomozov April 17, 2020, 2:31 a.m. UTC 
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.

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

Comments

Allan McRae April 29, 2020, 1:30 a.m. UTC | #1 
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

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

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 <anatol.pomozov@gmail.com>
> ---
>  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....

> +	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...

>  	ALPM_EVENT_RETRIEVE_START,

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

ALPM_EVENT_PKG_RETRIEVE_START

>  	/** 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 */
>
Anatol Pomozov May 6, 2020, 1:28 a.m. UTC | #2 
Hello

On Tue, Apr 28, 2020 at 6:31 PM Allan McRae <allan@archlinux.org> 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 <anatol.pomozov@gmail.com>
> > ---
> >  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 */
> >
Allan McRae May 6, 2020, 1:36 a.m. UTC | #3 
On 6/5/20 11:28 am, 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.
> 

Yes - but the errors are spread through the output.  I was thinking of
an update of 100 packages, and suddenly this gets spewed out at the end.

>> 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.

Of course - I was not talking about it happening in this patch.  Just
flagging something of future polishing.

A
diff mbox series

Patch

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. */
+	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. */
 	ALPM_EVENT_RETRIEVE_START,
 	/** 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 */