This is the convention elsewhere and sounds like the right thing to do. * guix/ui.scm (call-with-paginated-output-port): Substitute OPEN-PIPE for OPEN-PIPE*. Reported by Daniel Brooks <db48x@db48x.net>. --- guix/ui.scm | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/guix/ui.scm b/guix/ui.scm index 4e686297e8..2b7d9dd64b 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -72,7 +72,7 @@ #:use-module (ice-9 match) #:use-module (ice-9 format) #:use-module (ice-9 regex) - #:autoload (ice-9 popen) (open-pipe* close-pipe) + #:autoload (ice-9 popen) (open-pipe close-pipe) #:autoload (system base compile) (compile-file) #:autoload (system repl repl) (start-repl) #:autoload (system repl debug) (make-debug stack->vector) @@ -1673,9 +1673,9 @@ zero means that PACKAGE does not match any of REGEXPS." ;; instead of 'r': this strips hyperlinks but allows 'less' to make a ;; good estimate of the line length. (let ((pager (with-environment-variables `(("LESS" ,less-options)) - (open-pipe* OPEN_WRITE - (or (getenv "GUIX_PAGER") (getenv "PAGER") - "less"))))) + (open-pipe (or (getenv "GUIX_PAGER") (getenv "PAGER") + "less") + OPEN_WRITE)))) (dynamic-wind (const #t) (lambda () (proc pager)) -- 2.29.2
[-- Attachment #1.1: Type: text/plain, Size: 702 bytes --] Guix, Tobias Geerinckx-Rice via Guix-patches via 写道: > This is the convention elsewhere and sounds like the right thing > to do. Before this patch, using PAGER= failed: $ PAGER= guix search e In execvp of : No such file or directory $ With it, it fails in a slightly worse way: $ PAGER= guix search e $ # nothing, because we spawn the shell that swallows all Attached are two possible solutions. One falls back to ‘less’, the other to no paging. I think I prefer the former (‘Ignore empty $PAGER variables’) because the concept of ‘unset but empty’ could confuse ‘users’. Is that too patronising? Do tell. Kind regards, T G-R [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1.2: 0001-ui-Ignore-empty-PAGER-variables.patch --] [-- Type: text/x-patch, Size: 1336 bytes --] From dc64aadd9b124df37fcdf2f6dc057b61cf05a473 Mon Sep 17 00:00:00 2001 From: Tobias Geerinckx-Rice <me@tobias.gr> Date: Sun, 15 Nov 2020 20:36:32 +0100 Subject: [PATCH] ui: Ignore empty $PAGER variables. * guix/ui.scm (call-with-paginated-output-port): Treat "" as unset. --- guix/ui.scm | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/guix/ui.scm b/guix/ui.scm index 66614eef7c..584afc65dd 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -1673,8 +1673,11 @@ zero means that PACKAGE does not match any of REGEXPS." ;; instead of 'r': this strips hyperlinks but allows 'less' to make a ;; good estimate of the line length. (let ((pager (with-environment-variables `(("LESS" ,less-options)) - (open-pipe (or (getenv "GUIX_PAGER") (getenv "PAGER") - "less") + ;; Ignore environment variables set to "" as if unset. + (open-pipe (find (lambda (s) (and s (not (string=? "" s)))) + (list (getenv "GUIX_PAGER") + (getenv "PAGER") + "less")) OPEN_WRITE)))) (dynamic-wind (const #t) -- 2.29.2 [-- Attachment #1.3: 0001-ui-Disable-paging-if-PAGER-is-set-to-the-empty-strin.patch --] [-- Type: text/x-patch, Size: 2961 bytes --] From e1cf7e852c4a4c0cfce8c0de5625d026229dd71b Mon Sep 17 00:00:00 2001 From: Tobias Geerinckx-Rice <me@tobias.gr> Date: Sun, 15 Nov 2020 20:26:54 +0100 Subject: [PATCH] ui: Disable paging if $PAGER is set to the empty string. * guix/ui.scm (call-with-paginated-output-port): Don't open a pipe if $PAGER is "". --- guix/ui.scm | 39 +++++++++++++++++++++------------------ 1 file changed, 21 insertions(+), 18 deletions(-) diff --git a/guix/ui.scm b/guix/ui.scm index 66614eef7c..bb03e06759 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -1663,24 +1663,27 @@ zero means that PACKAGE does not match any of REGEXPS." (define* (call-with-paginated-output-port proc #:key (less-options "FrX")) - (if (isatty?* (current-output-port)) - ;; Set 'LESS' so that 'less' exits if everything fits on the screen (F), - ;; lets ANSI escapes through (r), does not send the termcap - ;; initialization string (X). Set it unconditionally because some - ;; distros set it to something that doesn't work here. - ;; - ;; For things that produce long lines, such as 'guix processes', use 'R' - ;; instead of 'r': this strips hyperlinks but allows 'less' to make a - ;; good estimate of the line length. - (let ((pager (with-environment-variables `(("LESS" ,less-options)) - (open-pipe (or (getenv "GUIX_PAGER") (getenv "PAGER") - "less") - OPEN_WRITE)))) - (dynamic-wind - (const #t) - (lambda () (proc pager)) - (lambda () (close-pipe pager)))) - (proc (current-output-port)))) + (let ((command (or (getenv "GUIX_PAGER") (getenv "PAGER") + "less"))) + ;; If a user types ‘PAGER= guix foo’ their intention is probably to disable + ;; paging entirely, not to use Guix's default pager. + (if (and (not (string=? "" command)) + (isatty?* (current-output-port))) + ;; Set 'LESS' so that 'less' exits if everything fits on the screen (F), + ;; lets ANSI escapes through (r), does not send the termcap + ;; initialization string (X). Set it unconditionally because some + ;; distros set it to something that doesn't work here. + ;; + ;; For things that produce long lines, such as 'guix processes', use 'R' + ;; instead of 'r': this strips hyperlinks but allows 'less' to make a + ;; good estimate of the line length. + (let ((pager (with-environment-variables `(("LESS" ,less-options)) + (open-pipe command OPEN_WRITE)))) + (dynamic-wind + (const #t) + (lambda () (proc pager)) + (lambda () (close-pipe pager)))) + (proc (current-output-port))))) (define-syntax with-paginated-output-port (syntax-rules () -- 2.29.2 [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 247 bytes --]
Hi,
Tobias Geerinckx-Rice <me@tobias.gr> skribis:
> This is the convention elsewhere and sounds like the right thing to do.
>
> * guix/ui.scm (call-with-paginated-output-port): Substitute OPEN-PIPE
> for OPEN-PIPE*.
>
> Reported by Daniel Brooks <db48x@db48x.net>.
What’s the rationale though? Are there cases where this makes a
practical difference?
I’m all for avoiding the shell if there’s no need for it.
Thanks,
Ludo’.
Ludovic Courtès <ludo@gnu.org> writes:
> What’s the rationale though? Are there cases where this makes a
> practical difference?
The error I hit was effectively running PAGER=less -FXRS guix search
foo. Everything else that uses PAGER handles this case just fine.
db48x
Daniel Brooks <db48x@db48x.net> skribis:
> Ludovic Courtès <ludo@gnu.org> writes:
>
>> What’s the rationale though? Are there cases where this makes a
>> practical difference?
>
> The error I hit was effectively running PAGER=less -FXRS guix search
> foo. Everything else that uses PAGER handles this case just fine.
Oh, I see, hmm. I feel that going through the shell makes things more
brittle, but you describe a valid use case, so maybe we should just go
ahead and apply the patch Tobias posted.
Ludo’.
* guix/ui.scm (call-with-paginated-output-port): Empty PAGER values disable paging. Non-empty ones are split into command arguments. Reported by Daniel Brooks <db48x@db48x.net>. --- guix/ui.scm | 47 ++++++++++++++++++++++++++++------------------- 1 file changed, 28 insertions(+), 19 deletions(-) diff --git a/guix/ui.scm b/guix/ui.scm index a59be74ecd..37099eac7b 100644 --- a/guix/ui.scm +++ b/guix/ui.scm @@ -12,7 +12,7 @@ ;;; Copyright © 2018 Kyle Meyer <kyle@kyleam.com> ;;; Copyright © 2018 Ricardo Wurmus <rekado@elephly.net> ;;; Copyright © 2019 Chris Marusich <cmmarusich@gmail.com> -;;; Copyright © 2019 Tobias Geerinckx-Rice <me@tobias.gr> +;;; Copyright © 2019, 2020 Tobias Geerinckx-Rice <me@tobias.gr> ;;; Copyright © 2019 Simon Tournier <zimon.toutoune@gmail.com> ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net> ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com> @@ -1675,24 +1675,33 @@ zero means that PACKAGE does not match any of REGEXPS." (define* (call-with-paginated-output-port proc #:key (less-options "FrX")) - (if (isatty?* (current-output-port)) - ;; Set 'LESS' so that 'less' exits if everything fits on the screen (F), - ;; lets ANSI escapes through (r), does not send the termcap - ;; initialization string (X). Set it unconditionally because some - ;; distros set it to something that doesn't work here. - ;; - ;; For things that produce long lines, such as 'guix processes', use 'R' - ;; instead of 'r': this strips hyperlinks but allows 'less' to make a - ;; good estimate of the line length. - (let ((pager (with-environment-variables `(("LESS" ,less-options)) - (open-pipe* OPEN_WRITE - (or (getenv "GUIX_PAGER") (getenv "PAGER") - "less"))))) - (dynamic-wind - (const #t) - (lambda () (proc pager)) - (lambda () (close-pipe pager)))) - (proc (current-output-port)))) + (let ((pager-command-line (or (getenv "GUIX_PAGER") + (getenv "PAGER") + "less"))) + ;; Setting PAGER to the empty string conventionally disables paging. + (if (and (not (string-null? pager-command-line)) + (isatty?* (current-output-port))) + ;; Set 'LESS' so that 'less' exits if everything fits on the screen + ;; (F), lets ANSI escapes through (r), does not send the termcap + ;; initialization string (X). Set it unconditionally because some + ;; distros set it to something that doesn't work here. + ;; + ;; For things that produce long lines, such as 'guix processes', use + ;; 'R' instead of 'r': this strips hyperlinks but allows 'less' to + ;; make a good estimate of the line length. + (let* ((pager (with-environment-variables `(("LESS" ,less-options)) + (apply open-pipe* OPEN_WRITE + ;; Split into arguments. Treat runs of multiple + ;; whitespace characters as one. libpipeline- + ;; style "cmd one\ arg" escaping is unsupported. + (remove "" + (string-split pager-command-line + char-set:whitespace)))))) + (dynamic-wind + (const #t) + (lambda () (proc pager)) + (lambda () (close-pipe pager)))) + (proc (current-output-port))))) (define-syntax with-paginated-output-port (syntax-rules () -- 2.29.2
[-- Attachment #1: Type: text/plain, Size: 371 bytes --] > I feel that going through the shell makes things more brittle I don't think it's brittle but it's icky. Using OPEN-PIPE is just the easiest way to get the shell to do free parsing for us. It also allows using pipes and for-loops in PAGER so I agree it's not ideal. Here's a v2 that does more work for less result, but feels less dirty :-) Kind regards, T G-R [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 247 bytes --]
[-- Attachment #1: Type: text/plain, Size: 198 bytes --] ...it's also missing a #:use-module ((rnrs lists) #:select (remove)) that got dropped on the floor. This module already imports SRFI-1, so that's some bonus ick for you. Kind regards, T G-R [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 247 bytes --]
Hey,
Tobias Geerinckx-Rice <me@tobias.gr> writes:
> * guix/ui.scm (call-with-paginated-output-port): Empty PAGER values
> disable paging. Non-empty ones are split into command arguments.
>
> Reported by Daniel Brooks <db48x@db48x.net>.
> ---
> guix/ui.scm | 47 ++++++++++++++++++++++++++++-------------------
> 1 file changed, 28 insertions(+), 19 deletions(-)
I see this got pushed with a81258c12415b9cee52c951b8b53cbe46f27654f
about a year ago.
Closing!
Maxim