diff mbox series

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

Message ID 87a6alwdvn.fsf@taiju.info
State Accepted
Headers show
Series [bug#55845,1/1] ui: Improve pager selection logic when less is not installed. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Taiju HIGASHI June 10, 2022, 12:39 a.m. UTC 
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.
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,

Comments

M June 10, 2022, 7:47 a.m. UTC | #1 
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.
Taiju HIGASHI June 10, 2022, 8:40 a.m. UTC | #2 
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,
M June 10, 2022, 3:08 p.m. UTC | #3 
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
diff mbox series

Patch

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