| Message ID | 20201209185759.30937-1-mail@cbaines.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [bug#45146] scripts: substitute: Improve fetch-narinfos progress reporting. | expand |
| Context | Check | Description |
|---|---|---|
| cbaines/submitting builds | success | |
| cbaines/comparison | success | View comparision |
| cbaines/git branch | success | View Git branch |
| cbaines/applying patch | success | View Laminar job |
| cbaines/issue | success | View issue |
Hi, Christopher Baines <mail@cbaines.net> skribis: > At least in guix weather, these changes make the progress bar actually appear. > > * guix/scripts/substitute.scm (fetch-narinfos): Use (guix progress) for > progress reporting. Cool. I noticed that something was wrong with ‘guix weather’, but I suspected it had to do with the order in which the erase-line sequence and \r are sent. > - (lambda () > - (display "\r\x1b[K" (current-error-port)) ;erase current line > - (force-output (current-error-port)) > - (format (current-error-port) > - (G_ "updating substitutes from '~a'... ~5,1f%") > - url (* 100. (/ done total))) > - (set! done (+ 1 done))))) > + (define fetch-narinfos-progress-reporter > + (progress-reporter/bar (length paths))) The problem here is that we’d see a progress bar without knowing what it represents. Besides, currently output from ‘guix substitute’ is printed as is by client commands, regardless of whether stdout is a tty. The problem already exists but it would become a bit more visible as logs get filled with progress bars. Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > Christopher Baines <mail@cbaines.net> skribis: > >> At least in guix weather, these changes make the progress bar actually appear. >> >> * guix/scripts/substitute.scm (fetch-narinfos): Use (guix progress) for >> progress reporting. > > Cool. I noticed that something was wrong with ‘guix weather’, but I > suspected it had to do with the order in which the erase-line sequence > and \r are sent. > >> - (lambda () >> - (display "\r\x1b[K" (current-error-port)) ;erase current line >> - (force-output (current-error-port)) >> - (format (current-error-port) >> - (G_ "updating substitutes from '~a'... ~5,1f%") >> - url (* 100. (/ done total))) >> - (set! done (+ 1 done))))) >> + (define fetch-narinfos-progress-reporter >> + (progress-reporter/bar (length paths))) > > The problem here is that we’d see a progress bar without knowing what it > represents. > > Besides, currently output from ‘guix substitute’ is printed as is by > client commands, regardless of whether stdout is a tty. The problem > already exists but it would become a bit more visible as logs get filled > with progress bars. Maybe it's best to circle back to fixing guix weather after trying to restructure some of the guix substitute code. I've made an initial attempt at moving things around in [1]. If the underlying code can live in a module, and then the substitute, weather and challenge scripts use that code with whatever UI stuff they want, maybe that will allow for better addressing this weather specific issue. 1: https://issues.guix.info/45409
Christopher Baines <mail@cbaines.net> writes: > Ludovic Courtès <ludo@gnu.org> writes: > >> Christopher Baines <mail@cbaines.net> skribis: >> >>> At least in guix weather, these changes make the progress bar actually appear. >>> >>> * guix/scripts/substitute.scm (fetch-narinfos): Use (guix progress) for >>> progress reporting. >> >> Cool. I noticed that something was wrong with ‘guix weather’, but I >> suspected it had to do with the order in which the erase-line sequence >> and \r are sent. >> >>> - (lambda () >>> - (display "\r\x1b[K" (current-error-port)) ;erase current line >>> - (force-output (current-error-port)) >>> - (format (current-error-port) >>> - (G_ "updating substitutes from '~a'... ~5,1f%") >>> - url (* 100. (/ done total))) >>> - (set! done (+ 1 done))))) >>> + (define fetch-narinfos-progress-reporter >>> + (progress-reporter/bar (length paths))) >> >> The problem here is that we’d see a progress bar without knowing what it >> represents. >> >> Besides, currently output from ‘guix substitute’ is printed as is by >> client commands, regardless of whether stdout is a tty. The problem >> already exists but it would become a bit more visible as logs get filled >> with progress bars. > > Maybe it's best to circle back to fixing guix weather after trying to > restructure some of the guix substitute code. > > I've made an initial attempt at moving things around in [1]. If the > underlying code can live in a module, and then the substitute, weather > and challenge scripts use that code with whatever UI stuff they want, > maybe that will allow for better addressing this weather specific issue. > > 1: https://issues.guix.info/45409 I've sent a couple of updated patches for this. They're not particularly dependent on the above work to create the (guix substitutes) module, but I based the commits on that. These commits make more careful changes, the substitute script behaviour should remain the same.
Christopher Baines <mail@cbaines.net> writes: > Christopher Baines <mail@cbaines.net> writes: > >> Ludovic Courtès <ludo@gnu.org> writes: >> >>> Christopher Baines <mail@cbaines.net> skribis: >>> >>>> At least in guix weather, these changes make the progress bar actually appear. >>>> >>>> * guix/scripts/substitute.scm (fetch-narinfos): Use (guix progress) for >>>> progress reporting. >>> >>> Cool. I noticed that something was wrong with ‘guix weather’, but I >>> suspected it had to do with the order in which the erase-line sequence >>> and \r are sent. >>> >>>> - (lambda () >>>> - (display "\r\x1b[K" (current-error-port)) ;erase current line >>>> - (force-output (current-error-port)) >>>> - (format (current-error-port) >>>> - (G_ "updating substitutes from '~a'... ~5,1f%") >>>> - url (* 100. (/ done total))) >>>> - (set! done (+ 1 done))))) >>>> + (define fetch-narinfos-progress-reporter >>>> + (progress-reporter/bar (length paths))) >>> >>> The problem here is that we’d see a progress bar without knowing what it >>> represents. >>> >>> Besides, currently output from ‘guix substitute’ is printed as is by >>> client commands, regardless of whether stdout is a tty. The problem >>> already exists but it would become a bit more visible as logs get filled >>> with progress bars. >> >> Maybe it's best to circle back to fixing guix weather after trying to >> restructure some of the guix substitute code. >> >> I've made an initial attempt at moving things around in [1]. If the >> underlying code can live in a module, and then the substitute, weather >> and challenge scripts use that code with whatever UI stuff they want, >> maybe that will allow for better addressing this weather specific issue. >> >> 1: https://issues.guix.info/45409 > > I've sent a couple of updated patches for this. They're not particularly > dependent on the above work to create the (guix substitutes) module, but > I based the commits on that. > > These commits make more careful changes, the substitute script behaviour > should remain the same. I've gone ahead and pushed these patches now as f5ffb3bd9cd59cfff5b4625d6d65e8d116d0a25b.
diff --git a/guix/scripts/substitute.scm b/guix/scripts/substitute.scm index 25075eedff..5128310439 100755 --- a/guix/scripts/substitute.scm +++ b/guix/scripts/substitute.scm @@ -614,16 +614,8 @@ print a warning and return #f." (define (fetch-narinfos url paths) "Retrieve all the narinfos for PATHS from the cache at URL and return them." - (define update-progress! - (let ((done 0) - (total (length paths))) - (lambda () - (display "\r\x1b[K" (current-error-port)) ;erase current line - (force-output (current-error-port)) - (format (current-error-port) - (G_ "updating substitutes from '~a'... ~5,1f%") - url (* 100. (/ done total))) - (set! done (+ 1 done))))) + (define fetch-narinfos-progress-reporter + (progress-reporter/bar (length paths))) (define hash-part->path (let ((mapping (fold (lambda (path result) @@ -641,7 +633,7 @@ print a warning and return #f." (len (response-content-length response)) (cache (response-cache-control response)) (ttl (and cache (assoc-ref cache 'max-age)))) - (update-progress!) + (progress-reporter-report! fetch-narinfos-progress-reporter) ;; Make sure to read no more than LEN bytes since subsequent bytes may ;; belong to the next response. @@ -673,7 +665,7 @@ print a warning and return #f." (#f '()) (port - (update-progress!) + (start-progress-reporter! fetch-narinfos-progress-reporter) ;; Note: Do not check HTTPS server certificates to avoid depending ;; on the X.509 PKI. We can do it because we authenticate ;; narinfos, which provides a much stronger guarantee. @@ -683,7 +675,7 @@ print a warning and return #f." #:verify-certificate? #f #:port port))) (close-port port) - (newline (current-error-port)) + (stop-progress-reporter! fetch-narinfos-progress-reporter) result))))) ((file #f) (let* ((base (string-append (uri-path uri) "/"))