[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI]

Hi,

The problem rarely occurs, but when we run guix commands in an environment
where "less" is not installed we get an error.

This is the same problem reported at the following URL
https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=1012405

If "more" could be specified as an alternative program to "less", the problem
would be less likely to occur at least in a POSIX environment. Also, I would
like to avoid using the pager in special environments where "more" is not
installed at all.

I have written a patch to solve the above.

I am concerned about performance degradation due to more unnecessary
processing.
If you have another good solution, please let me know.
Also, if you feel that this is a minor issue and not worth addressing, please
feel free to dismiss it. (Still, a fix to make the error message more friendly
might be a good idea.)

Best Regards,

Taiju HIGASHI (1):
  ui: Improve pager selection logic when less is not installed.

 guix/ui.scm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

--
2.36.1

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

* guix/ui.scm (available-pager): New variable. Holds available pagers.
  (call-with-paginated-output-port): Get an alternative program from the
  available-pager variable when the environment variable is not set.
---
 guix/ui.scm | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index cb68a07c6c..22169a7eb8 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -17,6 +17,7 @@
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1672,11 +1673,18 @@ (define* (pager-wrapped-port #:optional (port (current-output-port)))
     (_
      #f)))
 
+(define available-pager
+  (if (which "less")
+      "less"
+      (if (which "more")
+          "more"
+          #f)))
+
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
   (let ((pager-command-line (or (getenv "GUIX_PAGER")
                                 (getenv "PAGER")
-                                "less")))
+                                available-pager)))
     ;; Setting PAGER to the empty string conventionally disables paging.
     (if (and (not (string-null? pager-command-line))
              (isatty?* (current-output-port)))
-- 
2.36.1

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 2190 bytes --]

Hi!

Taiju HIGASHI 写道：
> The problem rarely occurs, but when we run guix commands in an 
> environment
> where "less" is not installed we get an error.

True.  Odd that it's gone unreported(?) for so long.

> I am concerned about performance degradation due to more 
> unnecessary
> processing.

Since you asked…  :-)

One way that this is ‘expensive’ is that it always calls WHICH at 
least once, no matter what Guix was invoked to do.

If you're familiar with Haskell or Nix: Scheme is not that, it's 
not ‘lazy’ and will evaluate the (if (which "less") …) even when 
the value is never used.  Turning AVAILABLE-PAGER into a procedure 
would avoid that.

Also, you're looking up the final pager in $PATH twice: you call 
WHICH, but then discard its work by returning the relative string 
"less".

The final OPEN-PIPE* invokes a shell which will search $PATH 
again.  We could save it the trouble by returning an absolute file 
name: the result of WHICH.

And since WHICH returns #f on failure, you can replace the nested 
IFs with a single OR:

  (define (available-pager)
    (or (which "less")
        (which "more")))

And well, as you probably noticed by now, it's actually more clear 
and concise if we just in-line what little is left:

  (let ((pager-command-line (or (getenv "GUIX_PAGER")
                                (getenv "PAGER")
                                (which "less")
                                (which "more")
                                "")))
    …

Your original patch returns #f if no pages could be found.  I 
don't think that is handled, but "" is, so return that instead.

Now I think that's 100% equivalent to your original; let me know 
if I missed a spot.

> Also, if you feel that this is a minor issue and not worth 
> addressing, please
> feel free to dismiss it. (Still, a fix to make the error message 
> more friendly
> might be a good idea.)

It *is* minor, but then so is the fix, and as written above it 
doesn't add ‘overhead’.  I think it's a good idea to check for 
"more" (but no more) and silently disable paging otherwise.

Thanks!

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI]

Hi 写道,

Thank you for reviwing!

> Hi!
>
> Taiju HIGASHI 写道：
>> The problem rarely occurs, but when we run guix commands in an
>> environment
>> where "less" is not installed we get an error.
>
> True.  Odd that it's gone unreported(?) for so long.
>
>> I am concerned about performance degradation due to more unnecessary
>> processing.
>
> Since you asked…  :-)
>
> One way that this is ‘expensive’ is that it always calls WHICH at
> least once, no matter what Guix was invoked to do.
>
> If you're familiar with Haskell or Nix: Scheme is not that, it's not
> ‘lazy’ and will evaluate the (if (which "less") …) even when the
> value is never used.  Turning AVAILABLE-PAGER into a procedure would
> avoid that.

I understand that I can delay the evaluation timing if I make it a
procedure, but is my understanding correct that the number of calls will
remain the same because it will be evaluated each time the
`call-with-paginated-output-port` procedure is called?

I agree with your point that it would be better to make it a procedure,
as it would be more eco-friendly to not have to evaluate when GUIX_PAGER
or PAGER is specified.

> Also, you're looking up the final pager in $PATH twice: you call
> WHICH, but then discard its work by returning the relative string
> "less".
>
> The final OPEN-PIPE* invokes a shell which will search $PATH again.
> We could save it the trouble by returning an absolute file name: the
> result of WHICH.

I see, I did not understand that behavior. Thank you.

> And since WHICH returns #f on failure, you can replace the nested IFs
> with a single OR:
>
>  (define (available-pager)
>    (or (which "less")
>        (which "more")))

This one is also more readable. Thank you.

> And well, as you probably noticed by now, it's actually more clear and
> concise if we just in-line what little is left:
>
>  (let ((pager-command-line (or (getenv "GUIX_PAGER")
>                                (getenv "PAGER")
>                                (which "less")
>                                (which "more")
>                                "")))
>    …

You mean that the $PATH lookup in open-pipe can be suppressed?
Also, I misunderstood the string-null? spec; available-pager should have
returned an empty string.

> Your original patch returns #f if no pages could be found.  I don't
> think that is handled, but "" is, so return that instead.
>
> Now I think that's 100% equivalent to your original; let me know if I
> missed a spot.

I thought what you said was completely correct.

>> Also, if you feel that this is a minor issue and not worth
>> addressing, please
>> feel free to dismiss it. (Still, a fix to make the error message
>> more friendly
>> might be a good idea.)
>
> It *is* minor, but then so is the fix, and as written above it doesn't
> add ‘overhead’.  I think it's a good idea to check for "more" (but
> no more) and silently disable paging otherwise.

I will just write what you have told me, but may I continue to modify
the patch?

Thank,
--
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 900 bytes --]

Taiju HIGASHI schreef op wo 08-06-2022 om 19:22 [+0900]:
> +(define available-pager
> +  (if (which "less")
> +      "less"
> +      (if (which "more")
> +          "more"
> +          #f)))

Can be simplified to something like,:

(define (find-available-pager)
  "[appropriate docstring]"
  (or (getenv "GUIX_PAGER") ;; <-- simplify 'if' chains by using 'or'
      (getenv "PAGER")
      (which "less")
      (which "more")
      ;; <--- TODO: how to handle no pager being found?
      ))

and

   (let ((pager-command-line (available-pager)))
     [...])

I've thunked find-available-pager here, such that call-with-paginated-
output-port respects the $PATH that is set before call-with-paginated-
output-port instead of the $PATH from when "guix ui" was loaded?

Ideally there would be some regression tests as well.

Greetings,
Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Tobias Geerinckx-Rice via Guix-patches via]

[-- Attachment #1: Type: text/plain, Size: 1692 bytes --]

Hi again,

Taiju HIGASHI 写道：
> I understand that I can delay the evaluation timing if I make it 
> a
> procedure, but is my understanding correct that the number of 
> calls will
> remain the same because it will be evaluated each time the
> `call-with-paginated-output-port` procedure is called?

Previously, it would have been evaluated even if 
call-with-paginated-output-port was never called at all.

As for the >0 calls case: yes… but when do we expect 
call-with-paginated-output-port to be called more than once per 
run?

The use case for this code is to do something, then display it in 
a pager and exit.  I think calling it multiple times in one run 
would imply bad UX.

Do I misunderstand?

> I agree with your point that it would be better to make it a 
> procedure,
> as it would be more eco-friendly to not have to evaluate when 
> GUIX_PAGER
> or PAGER is specified.

I wish the rest of Guix were so efficient that it mattered :-)

[/me is waiting for ‘guix pull’ as I reply to multiple mails, on 
battery…]

Regardless, not calling a procedure at all is even more efficient 
and IMO more readable here.

> You mean that the $PATH lookup in open-pipe can be suppressed?

Yes.  OPEN-PIPE* won't need to stat $PATH at all if we give it 
"/run/current-system/profile/bin/less" instead of "less".

(It's not relevant to the above, but my previously reply 
mistakenly mentioned a shell — there is no shell involved with 
OPEN-PIPE*, only with OPEN-PIPE.  Sorry.)

> I will just write what you have told me, but may I continue to 
> modify
> the patch?

Of course!  Curious how, though.

Kind regards,

T G-R

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 247 bytes --]

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 836 bytes --]

[Please keep debbugs in CC]

Taiju HIGASHI schreef op wo 08-06-2022 om 22:33 [+0900]:
> > Ideally there would be some regression tests as well.
> 
> I think I can write tests if I can figure out how to give a minimal
> specific package to the test preconditions, do you have any test
> codes
> that are similar and can be used as a reference?

Not really, but FWIW it might be convenient to use the
with-environment-variables macro and mock the call to open-pipe* with
the 'mock' macro to make sure that open-pipe* is called with the
‘correct’ pager according to PATH (*).  Searching for 'mock' with "git
grep -F" should find some examples.

(*) call-with-temporary-directory + chmod + call-with-output-file may
be useful for setting up a simulated $PATH with a dummy 'less' and/or
'more'.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI]

Hi Tobias,

Thank you kindly for your detailed explanation.

Tobias Geerinckx-Rice <me@tobias.gr> writes:

> Hi again,
>
> Taiju HIGASHI 写道：
>> I understand that I can delay the evaluation timing if I make it a
>> procedure, but is my understanding correct that the number of calls
>> will
>> remain the same because it will be evaluated each time the
>> `call-with-paginated-output-port` procedure is called?
>
> Previously, it would have been evaluated even if
> call-with-paginated-output-port was never called at all.
>
> As for the >0 calls case: yes… but when do we expect
> call-with-paginated-output-port to be called more than once per run?
>
> The use case for this code is to do something, then display it in a
> pager and exit.  I think calling it multiple times in one run would
> imply bad UX.
>
> Do I misunderstand?

No, you don't.
As you said, my implementation was a bad idea, as
call-with-paginated-output-port is executed even when it is not needed.
It seems unlikely that call-with-paginated-output-port will be called
more than once in a single process. I did not have enough insight.

>> I agree with your point that it would be better to make it a
>> procedure,
>> as it would be more eco-friendly to not have to evaluate when
>> GUIX_PAGER
>> or PAGER is specified.
>
> I wish the rest of Guix were so efficient that it mattered :-)
>
> [/me is waiting for ‘guix pull’ as I reply to multiple mails, on
> battery…]
>
> Regardless, not calling a procedure at all is even more efficient and
> IMO more readable here.

I agree.

>> You mean that the $PATH lookup in open-pipe can be suppressed?
>
> Yes.  OPEN-PIPE* won't need to stat $PATH at all if we give it
> "/run/current-system/profile/bin/less" instead of "less".
>
> (It's not relevant to the above, but my previously reply mistakenly
> mentioned a shell ― there is no shell involved with OPEN-PIPE*, only
> with OPEN-PIPE.  Sorry.)

No problem. I'm sorry I'm the one who asked a ton of questions.

>> I will just write what you have told me, but may I continue to
>> modify
>> the patch?
>
> Of course!  Curious how, though.

I have also received a response from Maxime and plan to include the
following information.

(define (find-available-pager)
  "Returns the program name or path of an available pager.
If neither less nor more is installed, return an empty string so that
call-with-paginated-output-port will not call pager."
  (or (getenv "GUIX_PAGER")
      (getenv "PAGER")
      (which "less")
      (which "more")
      "" ;; Returns an empty string so that call-with-paginated-output-port does not call pager.
      ))

(define* (call-with-paginated-output-port proc
                                          #:key (less-options "FrX"))
  (let ((pager-command-line (find-available-pager)))
...

However, I can't submit the v2 patch yet because I don't know how to
implement the integration test.

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

Maxime Devos <maximedevos@telenet.be> writes:

> [Please keep debbugs in CC]

I'm sorry.

> Taiju HIGASHI schreef op wo 08-06-2022 om 22:33 [+0900]:
>> > Ideally there would be some regression tests as well.
>>
>> I think I can write tests if I can figure out how to give a minimal
>> specific package to the test preconditions, do you have any test
>> codes
>> that are similar and can be used as a reference?
>
> Not really, but FWIW it might be convenient to use the
> with-environment-variables macro and mock the call to open-pipe* with
> the 'mock' macro to make sure that open-pipe* is called with the
> ‘correct’ pager according to PATH (*).  Searching for 'mock' with "git
> grep -F" should find some examples.
>
> (*) call-with-temporary-directory + chmod + call-with-output-file may
> be useful for setting up a simulated $PATH with a dummy 'less' and/or
> 'more'.

Thanks for the implementation tips. I will try to implement the test as
well, although it will be after tomorrow. Is tests/ui.scm the right
place to implement the tests?

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 305 bytes --]

Taiju HIGASHI schreef op do 09-06-2022 om 00:17 [+0900]:
> Thanks for the implementation tips. I will try to implement the test as
> well, although it will be after tomorrow. Is tests/ui.scm the right
> place to implement the tests?

Looks like the right location to me ,yes.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

[-- Attachment #1: Type: text/plain, Size: 123 bytes --]

Hi,

I have created a v2 patch and have attached it to this email and also
added a unit test for the find-available-pager.

[-- Attachment #2: v2-patch --]
[-- Type: text/x-patch, Size: 4953 bytes --]

From a65818b99ea1b327313fea08cf2db229c55e4b21 Mon Sep 17 00:00:00 2001
From: Taiju HIGASHI <higashi@taiju.info>
Date: Wed, 8 Jun 2022 18:50:28 +0900
Subject: [PATCH v2] ui: Improve pager selection logic when less is not
 installed.

* guix/ui.scm (find-available-pager): New procedure. Return a available pager.
  (call-with-paginated-output-port): Change to use find-available-pager to
  select pager.
* tests/ui.scm: Add tests for find-available-pager.
---
 guix/ui.scm  | 16 ++++++++++++---
 tests/ui.scm | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 71 insertions(+), 3 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index cb68a07c6c..93707a7a4b 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -17,6 +17,7 @@
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1672,11 +1673,20 @@ (define* (pager-wrapped-port #:optional (port (current-output-port)))
     (_
      #f)))
 
+(define (find-available-pager)
+  "Returns the program name or path of an available pager.
+If neither less nor more is installed, return an empty string so that
+call-with-paginated-output-port will not call pager."
+  (or (getenv "GUIX_PAGER")
+      (getenv "PAGER")
+      (which "less")
+      (which "more")
+      "" ;; Returns an empty string so that call-with-paginated-output-port does not call pager.
+      ))
+
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
-  (let ((pager-command-line (or (getenv "GUIX_PAGER")
-                                (getenv "PAGER")
-                                "less")))
+  (let ((pager-command-line (find-available-pager)))
     ;; Setting PAGER to the empty string conventionally disables paging.
     (if (and (not (string-null? pager-command-line))
              (isatty?* (current-output-port)))
diff --git a/tests/ui.scm b/tests/ui.scm
index 3dc6952e1f..41de3c63da 100644
--- a/tests/ui.scm
+++ b/tests/ui.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -24,6 +25,7 @@ (define-module (test-ui)
   #:use-module (guix derivations)
   #:use-module ((gnu packages) #:select (specification->package))
   #:use-module (guix tests)
+  #:use-module (guix utils)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-19)
@@ -292,4 +294,60 @@ (define guile-2.0.9
          (>0 (package-relevance libb2
                                 (map rx '("crypto" "library")))))))
 
+(define make-dummy-file
+  (compose
+   close-port
+   open-output-file
+   (cut string-append <> "/" <>)))
+
+(test-equal "find-available-pager, All environment variables are specified and both less and more are installed"
+  "guix-pager"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" "guix-pager")
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       ((@@ (guix ui) find-available-pager))))))
+
+(test-equal "find-available-pager, GUIX_PAGER is not specified"
+  "pager"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       ((@@ (guix ui) find-available-pager))))))
+
+(test-equal "find-available-pager, All environment variables are not specified and both less and more are installed"
+  "less"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (basename ((@@ (guix ui) find-available-pager)))))))
+
+(test-equal "find-available-pager, All environment variables are not specified and more is installed"
+  "more"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       (make-dummy-file dir "more")
+       (basename ((@@ (guix ui) find-available-pager)))))))
+
+(test-equal "find-available-pager, All environment variables are not specified and both less and more are not installed"
+  ""
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       ((@@ (guix ui) find-available-pager))))))
+
 (test-end "ui")
-- 
2.36.1


[-- Attachment #3: Type: text/plain, Size: 42 bytes --]


Please check it out.

Regards,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

Hi Maxime,

I tried to mock open-pipe* and isatty?* using the mock macro and also
add a test to inspect the program coming across to open-pipe*, but gave
up because I could not get the return value of the
with-paginated-output-port macro.

I think we are one step closer, but it is not working.
I will share a piece of code in the process of verification just in
case.

(test-equal "with-paginated-output-port"
  "less"
  (call-with-temporary-directory
   (lambda (dir)
     (with-environment-variables
         `(("PATH" ,dir))
       (make-dummy-executable-file dir "less")
       (mock ((ice-9 popen) open-pipe*
              (lambda (mode command . args)
                (current-output-port)))
             (mock ((guix colors) isatty?* (const #t))
                   (with-paginated-output-port paginated
                     "less")))))))

I have debugged that the return value of dynamic-wind is "less", but I
could not successfully use it for assertions.

I also tried to inspect the value of the command argument using
test-equal in the open-pipe* mock replacement function, but it did not
work.

Is there a better way to do this?

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 1563 bytes --]

Taiju HIGASHI schreef op do 09-06-2022 om 19:23 [+0900]:
> Hi Maxime,
> 
> I tried to mock open-pipe* and isatty?* using the mock macro and also
> add a test to inspect the program coming across to open-pipe*, but gave
> up because I could not get the return value of the
> with-paginated-output-port macro.

The return value of 'with-paginated-output-port' is just whatever the
last expression put in that macro evaluates to.  Also 'close-pipe'
needs to be mocked, otherwise an error will result.

Try:

(test-assert "with-paginated-output-port: finds less in PATH"
  (call-with-temporary-directory
    (lambda (dir)
      (define used-command #false)
      (with-environment-variables
          `(("PATH" ,dir))
        (make-dummy-executable-file dir "less")
        (mock ((ice-9 popen) open-pipe*
               (lambda (mode command . args)
                 (when used-command ; <--- an extra test
                    (error "open-pipe* should only be called once"))
                 (set! used-command command) ; <--- this captures the passed command
                 (%make-void-port ""))) ; return a dummy port
              (mock ((ice-9 popen) close-pipe (const 'ok))
                 (mock ((guix colors) isatty?* (const #t))
                    (with-paginated-output-port port 'ok)))))
      (and (pk 'used-command used-command dir) ; <-- fails on my computer because a non-absolute path is passed and I haven't applied our patch
           (string=? (in-vicinity dir "less") used-command)))))

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

[-- Attachment #1: Type: text/plain, Size: 1674 bytes --]

Maxime Devos <maximedevos@telenet.be> writes:

> Taiju HIGASHI schreef op do 09-06-2022 om 19:23 [+0900]:
>> Hi Maxime,
>>
>> I tried to mock open-pipe* and isatty?* using the mock macro and also
>> add a test to inspect the program coming across to open-pipe*, but gave
>> up because I could not get the return value of the
>> with-paginated-output-port macro.
>
> The return value of 'with-paginated-output-port' is just whatever the
> last expression put in that macro evaluates to.  Also 'close-pipe'
> needs to be mocked, otherwise an error will result.
>
> Try:
>
> (test-assert "with-paginated-output-port: finds less in PATH"
>   (call-with-temporary-directory
>     (lambda (dir)
>       (define used-command #false)
>       (with-environment-variables
>           `(("PATH" ,dir))
>         (make-dummy-executable-file dir "less")
>         (mock ((ice-9 popen) open-pipe*
>                (lambda (mode command . args)
>                  (when used-command ; <--- an extra test
>                     (error "open-pipe* should only be called once"))
>                  (set! used-command command) ; <--- this captures the passed command
>                  (%make-void-port ""))) ; return a dummy port
>               (mock ((ice-9 popen) close-pipe (const 'ok))
>                  (mock ((guix colors) isatty?* (const #t))
>                     (with-paginated-output-port port 'ok)))))
>       (and (pk 'used-command used-command dir) ; <-- fails on my computer because a non-absolute path is passed and I haven't applied our patch
>            (string=? (in-vicinity dir "less") used-command)))))

Thank you very much! It worked as expected!

I made a v3 patch.

[-- Attachment #2: v3 patch --]
[-- Type: text/x-patch, Size: 6770 bytes --]

From b499be5cf73916005150ddf777ae705070f077d1 Mon Sep 17 00:00:00 2001
From: Taiju HIGASHI <higashi@taiju.info>
Date: Wed, 8 Jun 2022 18:50:28 +0900
Subject: [PATCH v3] ui: Improve pager selection logic when less is not
 installed.

* guix/ui.scm (find-available-pager): New procedure. Return a available pager.
  (call-with-paginated-output-port): Change to use find-available-pager to
  select pager.
* tests/ui.scm: Add tests for find-available-pager.
---
 guix/ui.scm  | 16 +++++++--
 tests/ui.scm | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 3 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index cb68a07c6c..93707a7a4b 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -17,6 +17,7 @@
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1672,11 +1673,20 @@ (define* (pager-wrapped-port #:optional (port (current-output-port)))
     (_
      #f)))
 
+(define (find-available-pager)
+  "Returns the program name or path of an available pager.
+If neither less nor more is installed, return an empty string so that
+call-with-paginated-output-port will not call pager."
+  (or (getenv "GUIX_PAGER")
+      (getenv "PAGER")
+      (which "less")
+      (which "more")
+      "" ;; Returns an empty string so that call-with-paginated-output-port does not call pager.
+      ))
+
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
-  (let ((pager-command-line (or (getenv "GUIX_PAGER")
-                                (getenv "PAGER")
-                                "less")))
+  (let ((pager-command-line (find-available-pager)))
     ;; Setting PAGER to the empty string conventionally disables paging.
     (if (and (not (string-null? pager-command-line))
              (isatty?* (current-output-port)))
diff --git a/tests/ui.scm b/tests/ui.scm
index 3dc6952e1f..ca01d8f03d 100644
--- a/tests/ui.scm
+++ b/tests/ui.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -24,6 +25,7 @@ (define-module (test-ui)
   #:use-module (guix derivations)
   #:use-module ((gnu packages) #:select (specification->package))
   #:use-module (guix tests)
+  #:use-module (guix utils)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-19)
@@ -292,4 +294,98 @@ (define guile-2.0.9
          (>0 (package-relevance libb2
                                 (map rx '("crypto" "library")))))))
 
+(define make-dummy-file
+  (compose
+   close-port
+   open-output-file
+   (cut in-vicinity <> <>)))
+
+(test-assert "find-available-pager, All environment variables are specified and both less and more are installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" "guix-pager")
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (string=? ((@@ (guix ui) find-available-pager)) "guix-pager")))))
+
+(test-assert "find-available-pager, GUIX_PAGER is not specified"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (string=? ((@@ (guix ui) find-available-pager)) "pager")))))
+
+(test-assert "find-available-pager, All environment variables are not specified and both less and more are installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (string=? ((@@ (guix ui) find-available-pager))
+                 (in-vicinity dir "less"))))))
+
+(test-assert "find-available-pager, All environment variables are not specified and more is installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       (make-dummy-file dir "more")
+       (string=? ((@@ (guix ui) find-available-pager))
+                 (in-vicinity dir "more"))))))
+
+(test-assert "find-available-pager, All environment variables are not specified and both less and more are not installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       (string=? ((@@ (guix ui) find-available-pager)) "")))))
+
+
+(test-assert "with-paginated-output-port: finds a pager in enviroment variables"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (define used-command #false)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" "guix-pager")
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (mock ((ice-9 popen) open-pipe*
+              (lambda (mode command . args)
+                (when used-command
+                  (error "open-pipe* should only be called once"))
+                (set! used-command command)
+                (%make-void-port "")))
+             (mock ((ice-9 popen) close-pipe (const 'ok))
+                   (mock ((guix colors) isatty?* (const #t))
+                         (with-paginated-output-port port 'ok)))))
+     (string=? "guix-pager" used-command))))
+
+(test-assert "with-paginated-output-port: finds less in PATH"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (define used-command #false)
+     (with-environment-variables
+         `(("PATH" ,dir))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (mock ((ice-9 popen) open-pipe*
+              (lambda (mode command . args)
+                (when used-command
+                  (error "open-pipe* should only be called once"))
+                (set! used-command command)
+                (%make-void-port "")))
+             (mock ((ice-9 popen) close-pipe (const 'ok))
+                   (mock ((guix colors) isatty?* (const #t))
+                         (with-paginated-output-port port 'ok)))))
+     (string=? (in-vicinity dir "less") used-command))))
+
 (test-end "ui")
-- 
2.36.1


[-- Attachment #3: Type: text/plain, Size: 266 bytes --]

Two tests have been added: one to select pager from the environment
variable and the other to select less from the PATH.

I also made some improvements to the existing tests based on your
answers.

There are many ways to do this. I learned a lot.

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

This is off-topic.

I feel I have a limited vocabulary available to me in Guile or Scheme
(as well as in English...) , but functions like pk were not included in
Guile's reference or Scheme's reference, so I thought my chances of
knowing them were quite limited. (I didn't know about in-vicinity
either, but now that I know it is in SRFI.)

Are these the kind of things you learn by reading the source?

-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 612 bytes --]

Taiju HIGASHI schreef op vr 10-06-2022 om 09:55 [+0900]:
> I feel I have a limited vocabulary available to me in Guile or Scheme
> (as well as in English...) , but functions like pk were not included in
> Guile's reference or Scheme's reference, so I thought my chances of
> knowing them were quite limited. (I didn't know about in-vicinity
> either, but now that I know it is in SRFI.)

Didn't know it was part of a SRFI.

> Are these the kind of things you learn by reading the source?

Yes, some things aren't documented.  Though if interested, feel free to
doucment them.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 1511 bytes --]

Taiju HIGASHI schreef op vr 10-06-2022 om 09:39 [+0900]:
> +     (with-environment-variables
> +         `(("PATH" ,dir))

Wait, looking at the definition of with-environment-variables, if PAGER
was set when running "make check" (try "GUIX_PAGER=less make check
TESTS=tests/ui.scm"), it will still be set when the test is run
(unverified).  Maybe it should be unset?  Proposal:

Change

                  (match-lambda
                    ((variable value)
                     (setenv variable value)

to

                  (match-lambda
                    ((variable #false)
                     (unsetenv variable))
                    ((variable value)
                     (setenv variable value)


and change the with-environment-variables to

   (with-environment-variables
      `(("PATH ,dir)
        ("PAGER" #false)
        ("GUIX_PAGER" #false))
      [...]).

and likewise for the other tests?

(string=? ((@@ (guix ui) find-available-pager)) "guix-pager")))))

Nitpick: find-available-pager is not an exported procedure, so due to
optimisations, it can dissappear (be inlined into call-with-paginated-
output-port).  So to be 100% robust, it needs to be exported, or a line

  (set! find-avaible-pager find-available-pager) ; used in tests/ui.scm

needs to be added in guix/ui.scm, or the tests needs to be adjusted
to always use with-paginated-output-port instead of
find-available-pager.

Otherwise, tests LGTM.

Greetings,
Maxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

Maxime Devos <maximedevos@telenet.be> writes:

> Taiju HIGASHI schreef op vr 10-06-2022 om 09:39 [+0900]:
>> +     (with-environment-variables
>> +         `(("PATH" ,dir))
>
> Wait, looking at the definition of with-environment-variables, if PAGER
> was set when running "make check" (try "GUIX_PAGER=less make check
> TESTS=tests/ui.scm"), it will still be set when the test is run
> (unverified).  Maybe it should be unset?  Proposal:
>
> Change
>
>                   (match-lambda
>                     ((variable value)
>                      (setenv variable value)
>
> to
>
>                   (match-lambda
>                     ((variable #false)
>                      (unsetenv variable))
>                     ((variable value)
>                      (setenv variable value)
>
>
> and change the with-environment-variables to
>
>    (with-environment-variables
>       `(("PATH ,dir)
>         ("PAGER" #false)
>         ("GUIX_PAGER" #false))
>       [...]).
>
> and likewise for the other tests?

Sorry, I easily used with-environment-variable*s* because the interface
looked convenient, but perhaps I should have used
with-environment-variable defined in guix/tests.scm.
However, using this one does not seem to solve the problem. Should I
modify with-environment-variable*s*?

> (string=? ((@@ (guix ui) find-available-pager)) "guix-pager")))))
>
> Nitpick: find-available-pager is not an exported procedure, so due to
> optimisations, it can dissappear (be inlined into call-with-paginated-
> output-port).  So to be 100% robust, it needs to be exported, or a line
>
>   (set! find-avaible-pager find-available-pager) ; used in tests/ui.scm
>
> needs to be added in guix/ui.scm, or the tests needs to be adjusted
> to always use with-paginated-output-port instead of
> find-available-pager.


Thank you. I will modify it in one of the ways you suggested.

I did not understand the optimization behavior. Thank you very much.

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

> Taiju HIGASHI schreef op vr 10-06-2022 om 09:55 [+0900]:
>> I feel I have a limited vocabulary available to me in Guile or Scheme
>> (as well as in English...) , but functions like pk were not included in
>> Guile's reference or Scheme's reference, so I thought my chances of
>> knowing them were quite limited. (I didn't know about in-vicinity
>> either, but now that I know it is in SRFI.)
>
> Didn't know it was part of a SRFI.

Yes, but in Guile it behaves in a way that makes it useful for
constructing path strings, but when I checked the SRFI specification[0],
the behavior seems to be different from that of the SRFI specification.
(in-vicinity = string-append)

I wonder if this is a subtle specification, since Gauche, which
implements many SRFIs, did not have it either...

>> Are these the kind of things you learn by reading the source?
>
> Yes, some things aren't documented.  Though if interested, feel free to
> doucment them.

Thank you!
This is not a complaint about the documentation. I just wanted to know
how people as proficient in Guile as you guys learned Guile.
I am glad to know that I can learn by getting suggestions for better
code implementation through code reviews, etc.

But, I thought it would be a good idea to include functions like pk in
the documentation, since the efficiency of development depends on
knowing them.

[0]: https://srfi.schemers.org/srfi-59/srfi-59.html

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 672 bytes --]

Taiju HIGASHI schreef op vr 10-06-2022 om 17:40 [+0900]:
> Sorry, I easily used with-environment-variable*s* because the interface
> looked convenient, but perhaps I should have used
> with-environment-variable defined in guix/tests.scm.
> However, using this one does not seem to solve the problem. Should I
> modify with-environment-variable*s*?

I didn't know about 'with-environment-variable' (it already does the
unsetenv!). Just use whatever works (a nested with-environment-variable
or a modified with-environment-variables), though FWIW I would expect
using the modified with-environment-variables to result in more compact
code.

Greetings,
Maxime

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

[-- Attachment #1: Type: text/plain, Size: 640 bytes --]

Hi Maxime,

>> Sorry, I easily used with-environment-variable*s* because the interface
>> looked convenient, but perhaps I should have used
>> with-environment-variable defined in guix/tests.scm.
>> However, using this one does not seem to solve the problem. Should I
>> modify with-environment-variable*s*?
>
> I didn't know about 'with-environment-variable' (it already does the
> unsetenv!). Just use whatever works (a nested with-environment-variable
> or a modified with-environment-variables), though FWIW I would expect
> using the modified with-environment-variables to result in more compact
> code.

I have attached the v4 patch.

[-- Attachment #2: v4 patch --]
[-- Type: text/x-patch, Size: 6680 bytes --]

From bf557600c549e22a06ccfb288b89b1a0736b0500 Mon Sep 17 00:00:00 2001
From: Taiju HIGASHI <higashi@taiju.info>
Date: Wed, 8 Jun 2022 18:50:28 +0900
Subject: [PATCH v4] ui: Improve pager selection logic when less is not
 installed.

* guix/ui.scm (find-available-pager): New procedure. Return a available pager.
  (call-with-paginated-output-port): Change to use find-available-pager to
  select pager.
* guix/utils.scm (call-with-environment-variables): Allow clearing of
specified environment variables.
* tests/ui.scm: Add tests for find-available-pager.
---
 guix/ui.scm    | 16 +++++++++--
 guix/utils.scm |  3 ++
 tests/ui.scm   | 74 ++++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 90 insertions(+), 3 deletions(-)

diff --git a/guix/ui.scm b/guix/ui.scm
index cb68a07c6c..93707a7a4b 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -17,6 +17,7 @@
 ;;; Copyright © 2020 Arun Isaac <arunisaac@systemreboot.net>
 ;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer@gmail.com>
 ;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -1672,11 +1673,20 @@ (define* (pager-wrapped-port #:optional (port (current-output-port)))
     (_
      #f)))
 
+(define (find-available-pager)
+  "Returns the program name or path of an available pager.
+If neither less nor more is installed, return an empty string so that
+call-with-paginated-output-port will not call pager."
+  (or (getenv "GUIX_PAGER")
+      (getenv "PAGER")
+      (which "less")
+      (which "more")
+      "" ;; Returns an empty string so that call-with-paginated-output-port does not call pager.
+      ))
+
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
-  (let ((pager-command-line (or (getenv "GUIX_PAGER")
-                                (getenv "PAGER")
-                                "less")))
+  (let ((pager-command-line (find-available-pager)))
     ;; Setting PAGER to the empty string conventionally disables paging.
     (if (and (not (string-null? pager-command-line))
              (isatty?* (current-output-port)))
diff --git a/guix/utils.scm b/guix/utils.scm
index 37b2e29800..5c36b15cfe 100644
--- a/guix/utils.scm
+++ b/guix/utils.scm
@@ -13,6 +13,7 @@
 ;;; Copyright © 2021 Chris Marusich <cmmarusich@gmail.com>
 ;;; Copyright © 2021 Maxime Devos <maximedevos@telenet.be>
 ;;; Copyright © 2018 Steve Sprang <scs@stevesprang.com>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -158,6 +159,8 @@ (define (call-with-environment-variables variables thunk)
     (dynamic-wind
       (lambda ()
         (for-each (match-lambda
+                    ((variable #false)
+                     (unsetenv variable))
                     ((variable value)
                      (setenv variable value)))
                   variables))
diff --git a/tests/ui.scm b/tests/ui.scm
index 3dc6952e1f..ff83e66a7e 100644
--- a/tests/ui.scm
+++ b/tests/ui.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020 Ludovic Courtès <ludo@gnu.org>
+;;; Copyright © 2022 Taiju HIGASHI <higashi@taiju.info>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -24,6 +25,7 @@ (define-module (test-ui)
   #:use-module (guix derivations)
   #:use-module ((gnu packages) #:select (specification->package))
   #:use-module (guix tests)
+  #:use-module (guix utils)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-19)
@@ -292,4 +294,76 @@ (define guile-2.0.9
          (>0 (package-relevance libb2
                                 (map rx '("crypto" "library")))))))
 
+(define make-dummy-file
+  (compose
+   close-port
+   open-output-file
+   (cut in-vicinity <> <>)))
+
+(define (assert-equals-find-available-pager expected)
+  (define used-command "")
+  (mock ((ice-9 popen) open-pipe*
+         (lambda (mode command . args)
+           (unless (string-null? used-command)
+             (error "open-pipe* should only be called once"))
+           (set! used-command command)
+           (%make-void-port "")))
+        (mock ((ice-9 popen) close-pipe (const 'ok))
+              (mock ((guix colors) isatty?* (const #t))
+                    (with-paginated-output-port port 'ok)
+                    (string=? expected used-command)))))
+
+
+(test-assert "find-available-pager, All environment variables are specified and both less and more are installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" "guix-pager")
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (assert-equals-find-available-pager "guix-pager")))))
+
+(test-assert "find-available-pager, GUIX_PAGER is not specified"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" #false)
+           ("PAGER" "pager"))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (assert-equals-find-available-pager "pager")))))
+
+(test-assert "find-available-pager, All environment variables are not specified and both less and more are installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" #false)
+           ("PAGER" #false))
+       (make-dummy-file dir "less")
+       (make-dummy-file dir "more")
+       (assert-equals-find-available-pager (in-vicinity dir "less"))))))
+
+(test-assert "find-available-pager, All environment variables are not specified and more is installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" #false)
+           ("PAGER" #false))
+       (make-dummy-file dir "more")
+       (assert-equals-find-available-pager (in-vicinity dir "more"))))))
+
+(test-assert "find-available-pager, All environment variables are not specified and both less and more are not installed"
+  (call-with-temporary-directory
+   (lambda (dir)
+     (with-environment-variables
+         `(("PATH" ,dir)
+           ("GUIX_PAGER" #false)
+           ("PAGER" #false))
+       (assert-equals-find-available-pager "")))))
+
 (test-end "ui")
-- 
2.36.1


[-- Attachment #3: Type: text/plain, Size: 290 bytes --]

I decided to fix with-environment-variables.

Also, the problem of find-available-pager tests being tests for
unexported functions has been changed to tests that use the exported
with-paginated-output-port to verify that the command is passed to
open-pipe* as expected.

Regards,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Taiju HIGASHI]

Hi Maxime,

Please let me know if you have any problems with the v4 patch I sent you
a few days ago :)

Thanks,
-- 
Taiju

[bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.

[Maxime Devos]

[-- Attachment #1: Type: text/plain, Size: 588 bytes --]

Taiju HIGASHI schreef op wo 15-06-2022 om 08:58 [+0900]:
>  Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed. (Van: Taiju HIGASHI <higashi@taiju.info>)Van:Taiju HIGASHI <higashi@taiju.info>Aan:Maxime Devos <maximedevos@telenet.be>Cc:me@tobias.gr, 55845@debbugs.gnu.orgOnderwerp:Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.Datum:Wed, 15 Jun 2022 08:58:57 +0900 (15-06-22 01:58:57)

The patches look fine from here (only reading them, not actually
running the tests myself).

Greetings,
MMaxime.

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 260 bytes --]

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1194 bytes --]

Hi,

Taiju HIGASHI <higashi@taiju.info> skribis:

>>From bf557600c549e22a06ccfb288b89b1a0736b0500 Mon Sep 17 00:00:00 2001
> From: Taiju HIGASHI <higashi@taiju.info>
> Date: Wed, 8 Jun 2022 18:50:28 +0900
> Subject: [PATCH v4] ui: Improve pager selection logic when less is not
>  installed.
>
> * guix/ui.scm (find-available-pager): New procedure. Return a available pager.
>   (call-with-paginated-output-port): Change to use find-available-pager to
>   select pager.
> * guix/utils.scm (call-with-environment-variables): Allow clearing of
> specified environment variables.
> * tests/ui.scm: Add tests for find-available-pager.

Applied with the cosmetic changes below, mostly aiming to visually
simplify the code and make it consistent with the rest.

It’s great that you went to great lengths to implement tests for this,
as Maxime had suggested.  To me, the complexity of a test must be
justified by its “bug-finding performance”; in this particular case, I
think we’re borderline: the tests are a little bit complex and unlikely
to find new bugs.

Thanks for all the work and for your feedback on your experience
programming with Guile!

Ludo’.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 5121 bytes --]

diff --git a/guix/ui.scm b/guix/ui.scm
index 93707a7a4b..a7acd41440 100644
--- a/guix/ui.scm
+++ b/guix/ui.scm
@@ -1674,15 +1674,13 @@ (define* (pager-wrapped-port #:optional (port (current-output-port)))
      #f)))
 
 (define (find-available-pager)
-  "Returns the program name or path of an available pager.
-If neither less nor more is installed, return an empty string so that
-call-with-paginated-output-port will not call pager."
+  "Return the program name of an available pager or the empty string if none is
+available."
   (or (getenv "GUIX_PAGER")
       (getenv "PAGER")
       (which "less")
       (which "more")
-      "" ;; Returns an empty string so that call-with-paginated-output-port does not call pager.
-      ))
+      ""))
 
 (define* (call-with-paginated-output-port proc
                                           #:key (less-options "FrX"))
diff --git a/tests/ui.scm b/tests/ui.scm
index ff83e66a7e..6a25a204ca 100644
--- a/tests/ui.scm
+++ b/tests/ui.scm
@@ -294,13 +294,12 @@ (define guile-2.0.9
          (>0 (package-relevance libb2
                                 (map rx '("crypto" "library")))))))
 
-(define make-dummy-file
-  (compose
-   close-port
-   open-output-file
-   (cut in-vicinity <> <>)))
+(define (make-empty-file directory file)
+  ;; Create FILE in DIRECTORY.
+  (close-port (open-output-file (in-vicinity directory file))))
 
 (define (assert-equals-find-available-pager expected)
+  ;; Use 'with-paginated-output-port' and return true if it invoked EXPECTED.
   (define used-command "")
   (mock ((ice-9 popen) open-pipe*
          (lambda (mode command . args)
@@ -314,56 +313,51 @@ (define used-command "")
                     (string=? expected used-command)))))
 
 
-(test-assert "find-available-pager, All environment variables are specified and both less and more are installed"
+(test-assert "find-available-pager, GUIX_PAGER takes precedence"
   (call-with-temporary-directory
    (lambda (dir)
-     (with-environment-variables
-         `(("PATH" ,dir)
-           ("GUIX_PAGER" "guix-pager")
-           ("PAGER" "pager"))
-       (make-dummy-file dir "less")
-       (make-dummy-file dir "more")
+     (with-environment-variables `(("PATH" ,dir)
+                                   ("GUIX_PAGER" "guix-pager")
+                                   ("PAGER" "pager"))
+       (make-empty-file dir "less")
+       (make-empty-file dir "more")
        (assert-equals-find-available-pager "guix-pager")))))
 
-(test-assert "find-available-pager, GUIX_PAGER is not specified"
+(test-assert "find-available-pager, PAGER takes precedence"
   (call-with-temporary-directory
    (lambda (dir)
-     (with-environment-variables
-         `(("PATH" ,dir)
-           ("GUIX_PAGER" #false)
-           ("PAGER" "pager"))
-       (make-dummy-file dir "less")
-       (make-dummy-file dir "more")
+     (with-environment-variables `(("PATH" ,dir)
+                                   ("GUIX_PAGER" #false)
+                                   ("PAGER" "pager"))
+       (make-empty-file dir "less")
+       (make-empty-file dir "more")
        (assert-equals-find-available-pager "pager")))))
 
-(test-assert "find-available-pager, All environment variables are not specified and both less and more are installed"
+(test-assert "find-available-pager, 'less' takes precedence"
   (call-with-temporary-directory
    (lambda (dir)
-     (with-environment-variables
-         `(("PATH" ,dir)
-           ("GUIX_PAGER" #false)
-           ("PAGER" #false))
-       (make-dummy-file dir "less")
-       (make-dummy-file dir "more")
+     (with-environment-variables `(("PATH" ,dir)
+                                   ("GUIX_PAGER" #false)
+                                   ("PAGER" #false))
+       (make-empty-file dir "less")
+       (make-empty-file dir "more")
        (assert-equals-find-available-pager (in-vicinity dir "less"))))))
 
-(test-assert "find-available-pager, All environment variables are not specified and more is installed"
+(test-assert "find-available-pager, 'more' takes precedence"
   (call-with-temporary-directory
    (lambda (dir)
-     (with-environment-variables
-         `(("PATH" ,dir)
-           ("GUIX_PAGER" #false)
-           ("PAGER" #false))
-       (make-dummy-file dir "more")
+     (with-environment-variables `(("PATH" ,dir)
+                                   ("GUIX_PAGER" #false)
+                                   ("PAGER" #false))
+       (make-empty-file dir "more")
        (assert-equals-find-available-pager (in-vicinity dir "more"))))))
 
-(test-assert "find-available-pager, All environment variables are not specified and both less and more are not installed"
+(test-assert "find-available-pager, no pager"
   (call-with-temporary-directory
    (lambda (dir)
-     (with-environment-variables
-         `(("PATH" ,dir)
-           ("GUIX_PAGER" #false)
-           ("PAGER" #false))
+     (with-environment-variables `(("PATH" ,dir)
+                                   ("GUIX_PAGER" #false)
+                                   ("PAGER" #false))
        (assert-equals-find-available-pager "")))))
 
 (test-end "ui")

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI]

Hi Ludovic,

> Applied with the cosmetic changes below, mostly aiming to visually
> simplify the code and make it consistent with the rest.

Thank you for the correction and application!

> It’s great that you went to great lengths to implement tests for this,
> as Maxime had suggested.  To me, the complexity of a test must be
> justified by its “bug-finding performance”; in this particular case, I
> think we’re borderline: the tests are a little bit complex and unlikely
> to find new bugs.
>
> Thanks for all the work and for your feedback on your experience
> programming with Guile!

I only discovered the problem, I was able to implement it mostly thanks
to Maxime and Tobias!
The code review experience was so good that I even posted the following
:)
https://fosstodon.org/web/@taiju/108458633893022791
https://fosstodon.org/web/@taiju/108458643302758263

I'd like to know for future contributions.
I like functional programming and I love compose, (ice-9
curried-definitions), and SRFI 26 in my programs, but should I use them
less in the code I put in Guix?

Thanks,
--
Taiju

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Ludovic Courtès]

Hello,

Taiju HIGASHI <higashi@taiju.info> skribis:

> I only discovered the problem, I was able to implement it mostly thanks
> to Maxime and Tobias!
> The code review experience was so good that I even posted the following
> :)
> https://fosstodon.org/web/@taiju/108458633893022791
> https://fosstodon.org/web/@taiju/108458643302758263

Heh, good to know that it was a positive experience!

> I'd like to know for future contributions.
> I like functional programming and I love compose, (ice-9
> curried-definitions), and SRFI 26 in my programs, but should I use them
> less in the code I put in Guix?

Probably.  It’s tempting to use these if you come with a Haskell
background, say.  But in some cases, they make things less readable;
that’s the case with the way ‘make-dummy-file’ was written IMO.

Guix code uses SRFI-26 in some places; I think (ice-9
curried-definitions) is not used anywhere but I think it’s fine to use
it if it helps.

Thanks,
Ludo’.

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI]

Hi,

>> I'd like to know for future contributions.
>> I like functional programming and I love compose, (ice-9
>> curried-definitions), and SRFI 26 in my programs, but should I use them
>> less in the code I put in Guix?
>
> Probably.  It’s tempting to use these if you come with a Haskell
> background, say.  But in some cases, they make things less readable;
> that’s the case with the way ‘make-dummy-file’ was written IMO.
>
> Guix code uses SRFI-26 in some places; I think (ice-9
> curried-definitions) is not used anywhere but I think it’s fine to use
> it if it helps.

Thank you for your answers!

I like Haskell and OCaml, but I don't have experience with them much. I
like Scheme and Common Lisp more :)

The point-free style is beautiful, but it is often just
self-satisfaction, so I think after hearing your opinion that better use
them less in shared code.
However, those features that make it possible are very good, and I will
continue to use them in the programs I write for personal use :)

Thanks,
-- 
Taiju

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Ludovic Courtès]

Hi!

Taiju HIGASHI <higashi@taiju.info> skribis:

> The point-free style is beautiful, but it is often just
> self-satisfaction, so I think after hearing your opinion that better use
> them less in shared code.

On this topic and others, the manual links to Riastradh’s style guide
for Scheme (info "(guix) Formatting Code"), which is worth a read!

Ludo’.

[bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed

[Taiju HIGASHI]

>> The point-free style is beautiful, but it is often just
>> self-satisfaction, so I think after hearing your opinion that better use
>> them less in shared code.
>
> On this topic and others, the manual links to Riastradh’s style guide
> for Scheme (info "(guix) Formatting Code"), which is worth a read!

Thanks for sharing the good information!

(I forgot to CC my reply, so I resent it.)
-- 
Taiju