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

  • Done
  • quality assurance status badge
Details
4 participants
  • Taiju HIGASHI
  • Ludovic Courtès
  • Maxime Devos
  • Tobias Geerinckx-Rice
Owner
unassigned
Submitted by
Taiju HIGASHI
Severity
normal
T
T
Taiju HIGASHI wrote on 8 Jun 2022 12:21
(address . guix-patches@gnu.org)(name . Taiju HIGASHI)(address . higashi@taiju.info)
20220608102124.14865-1-higashi@taiju.info
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

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
T
T
Taiju HIGASHI wrote on 8 Jun 2022 12:22
[PATCH 1/1] ui: Improve pager selection logic when less is not installed.
(address . 55845@debbugs.gnu.org)(name . Taiju HIGASHI)(address . higashi@taiju.info)
20220608102257.15042-1-higashi@taiju.info
* 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(-)

Toggle diff (34 lines)
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
T
T
Tobias Geerinckx-Rice wrote on 8 Jun 2022 14:14
Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed
(name . Taiju HIGASHI)(address . higashi@taiju.info)
87wndrwc5m@nckx
Hi!

Taiju HIGASHI ???
Toggle quote (4 lines)
> 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.

Toggle quote (4 lines)
> 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.

Toggle quote (6 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYqCbdg0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15J6cA/RQGWhlpt1XXNYCSbQvLTOzELdbnq2yPc9ugwyIP
swScAQC4EhfrTujG0/3PIR1Igbvklb7ThJ8dxIzw1rEC9SyFAw==
=+rk9
-----END PGP SIGNATURE-----

T
T
Taiju HIGASHI wrote on 8 Jun 2022 15:12
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)
8735gf47ul.fsf@taiju.info
Hi ??,

Thank you for reviwing!

Toggle quote (22 lines)
> 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.

Toggle quote (8 lines)
> 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.

Toggle quote (7 lines)
> 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.

Toggle quote (10 lines)
> 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.

Toggle quote (6 lines)
> 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.

Toggle quote (10 lines)
>> 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
M
M
Maxime Devos wrote on 8 Jun 2022 15:18
Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.
c5c3c653a9184ea6793ab2f1c85ad5eba062eece.camel@telenet.be
Taiju HIGASHI schreef op wo 08-06-2022 om 19:22 [+0900]:
Toggle quote (7 lines)
> +(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
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqChnBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7j5RAPsEj5yYdIlfKWin4my2/ti4UWA2
QJt55+toXXZqqeRxtwEAjZmV/wVo9Um33TQVfFu3hk8Gwjhuw2O0Q+hass6Z+A8=
=pR+i
-----END PGP SIGNATURE-----


T
T
Tobias Geerinckx-Rice wrote on 8 Jun 2022 16:22
Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed
(name . Taiju HIGASHI)(address . higashi@taiju.info)
87k09rw74q@nckx
Hi again,

Taiju HIGASHI ???
Toggle quote (7 lines)
> 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?

Toggle quote (6 lines)
> 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.

Toggle quote (2 lines)
> 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.)

Toggle quote (4 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIMEARYKACsWIQT12iAyS4c9C3o4dnINsP+IT1VteQUCYqC05Q0cbWVAdG9iaWFz
LmdyAAoJEA2w/4hPVW15P/AA/2cbBXHSGYEW1IztN5Vc8xA55LVoIwAiV8dY9xJn
jT2cAP4t32jz4Yh88FxgEfjYw77tgG05sqB971XkBni38fR+Bw==
=qZKk
-----END PGP SIGNATURE-----

M
M
Maxime Devos wrote on 8 Jun 2022 17:08
Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.
(name . Taiju HIGASHI)(address . higashi@taiju.info)(address . 55845@debbugs.gnu.org)
55fe6cdbed891707aca419ff4eedd7c37ef3eb03.camel@telenet.be
[Please keep debbugs in CC]

Taiju HIGASHI schreef op wo 08-06-2022 om 22:33 [+0900]:
Toggle quote (7 lines)
> > 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqC7XRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7h8jAP9B1zTmcC+e4dCLljBI+fLq8TVq
CsP2jacOxeIJD+rUYQD/Y8m5xt/oqWeJ8w60ZqdK/EnAgc6Cbi6mBQIf7Tq9XAw=
=eb9M
-----END PGP SIGNATURE-----


T
T
Taiju HIGASHI wrote on 8 Jun 2022 17:09
Re: [bug#55845] [PATCH 0/1] Improve pager selection logic when less is not installed
(name . Tobias Geerinckx-Rice)(address . me@tobias.gr)
87sfof19b7.fsf@taiju.info
Hi Tobias,

Thank you kindly for your detailed explanation.

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

Toggle quote (21 lines)
> 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.

Toggle quote (14 lines)
>> 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.

Toggle quote (9 lines)
>> 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.

Toggle quote (6 lines)
>> 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
T
T
Taiju HIGASHI wrote on 8 Jun 2022 17:17
Re: [bug#55845] [PATCH 1/1] ui: Improve pager selection logic when less is not installed.
(name . Maxime Devos)(address . maximedevos@telenet.be)(address . 55845@debbugs.gnu.org)
87k09r18xs.fsf@taiju.info
Maxime Devos <maximedevos@telenet.be> writes:

Toggle quote (2 lines)
> [Please keep debbugs in CC]

I'm sorry.

Toggle quote (18 lines)
> 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
M
M
Maxime Devos wrote on 8 Jun 2022 18:46
(name . Taiju HIGASHI)(address . higashi@taiju.info)(address . 55845@debbugs.gnu.org)
4ee2875e3f4c871779b734c1209a7e952222897d.camel@telenet.be
Taiju HIGASHI schreef op do 09-06-2022 om 00:17 [+0900]:
Toggle quote (4 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqDSdRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7ks8AQCP2siKdFtNuveWBw9+1YJJRfKb
kAKeRgajDDBIUcofHgEAtjJvzMG0okTkLXWk1IrIe6AJ7dSfzMO1g4gDDelL2AI=
=JarM
-----END PGP SIGNATURE-----


T
T
Taiju HIGASHI wrote on 9 Jun 2022 11:52
(address . 55845@debbugs.gnu.org)
87fskexiyc.fsf@taiju.info
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.
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(-)

Toggle diff (118 lines)
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
Please check it out.

Regards,
--
Taiju
T
T
Taiju HIGASHI wrote on 9 Jun 2022 12:23
(name . Maxime Devos)(address . maximedevos@telenet.be)
874k0uxhif.fsf@taiju.info
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
M
M
Maxime Devos wrote on 9 Jun 2022 21:43
(name . Taiju HIGASHI)(address . higashi@taiju.info)
5ccb68a4dc80a02baaf6d19fd782145b6b62e7b5.camel@telenet.be
Taiju HIGASHI schreef op do 09-06-2022 om 19:23 [+0900]:
Toggle quote (7 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqJNTxccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7nF9AP986LcNcjteB3h9iqA1wZE9CG4d
n/ttr1mi/ndsa2VRFAEAtgBnEHTdT1TPYRdbePVeQsfsLvKYrhag+OO4vZJsfAY=
=YEyy
-----END PGP SIGNATURE-----


T
T
Taiju HIGASHI wrote on 10 Jun 2022 02:39
(name . Maxime Devos)(address . maximedevos@telenet.be)
87a6alwdvn.fsf@taiju.info
Maxime Devos <maximedevos@telenet.be> writes:

Toggle quote (33 lines)
> 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: v3-0001-ui-Improve-pager-selection-logic-when-less-is-not.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,
--
Taiju
T
T
Taiju HIGASHI wrote on 10 Jun 2022 02:55
(name . Maxime Devos)(address . maximedevos@telenet.be)
871qvxwd47.fsf@taiju.info
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
M
M
Maxime Devos wrote on 10 Jun 2022 09:37
(name . Taiju HIGASHI)(address . higashi@taiju.info)
41a200f3464dd0d2da12c1fbd4ae2e33e2973075.camel@telenet.be
Taiju HIGASHI schreef op vr 10-06-2022 om 09:55 [+0900]:
Toggle quote (6 lines)
> 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.

Toggle quote (2 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqL0yBccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7vGAAP9M7ckwpnOIeEalT2lifPzXYRo7
l9zyHf3vXJhD8hijxAD+M+lrbBemC7133LgiuNNQJFVRNmtIotHUuEvNVrycFgI=
=f0dp
-----END PGP SIGNATURE-----


M
M
Maxime Devos wrote on 10 Jun 2022 09:47
(name . Taiju HIGASHI)(address . higashi@taiju.info)
d6ef40cdf1b7fef1cff49cd917f0e5c197c39db7.camel@telenet.be
Taiju HIGASHI schreef op vr 10-06-2022 om 09:39 [+0900]:
Toggle quote (3 lines)
> +     (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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqL3EhccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7kkUAP9jCAz++xtGHHpsixAkmFPt3Rtf
xUtx6hAunPPSFicLtAD8CoeegaML7XTCaFl4iLT7+8kzV2w35bN5Q/uLz9EjFgs=
=s+3y
-----END PGP SIGNATURE-----


T
T
Taiju HIGASHI wrote on 10 Jun 2022 10:40
(name . Maxime Devos)(address . maximedevos@telenet.be)
87r13wvrlz.fsf@taiju.info
Maxime Devos <maximedevos@telenet.be> writes:

Toggle quote (34 lines)
> 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*?

Toggle quote (13 lines)
> (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
T
T
Taiju HIGASHI wrote on 10 Jun 2022 10:52
(name . Maxime Devos)(address . maximedevos@telenet.be)
87czfgvr1j.fsf@taiju.info
Toggle quote (9 lines)
> 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...

Toggle quote (5 lines)
>> 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.


Thanks,
--
Taiju
M
M
Maxime Devos wrote on 10 Jun 2022 17:08
(name . Taiju HIGASHI)(address . higashi@taiju.info)
8250aad90dcf563aba4103127df95084a25fbd5d.camel@telenet.be
Taiju HIGASHI schreef op vr 10-06-2022 om 17:40 [+0900]:
Toggle quote (6 lines)
> 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
-----BEGIN PGP SIGNATURE-----

iIwEABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqNeiRccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7h5hAPdvOG4LHQODwtb20QLtifIFRj/k
NAbd2Ie+Uls9qsnMAP4lHfGr7EuTHvdVuZlHjUM3XUxz/kwR47eRigRGkkCbAQ==
=1mvq
-----END PGP SIGNATURE-----


T
T
Taiju HIGASHI wrote on 11 Jun 2022 13:26
(name . Maxime Devos)(address . maximedevos@telenet.be)
87bkuzsap7.fsf@taiju.info
Hi Maxime,

Toggle quote (12 lines)
>> 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: v4-0001-ui-Improve-pager-selection-logic-when-less-is-not.patch
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
T
T
Taiju HIGASHI wrote on 15 Jun 2022 01:58
(name . Maxime Devos)(address . maximedevos@telenet.be)
87k09iiypq.fsf@taiju.info
Hi Maxime,

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

Thanks,
--
Taiju
M
M
Maxime Devos wrote on 15 Jun 2022 10:02
(name . Taiju HIGASHI)(address . higashi@taiju.info)
51d3162e0e9230a7fdb112badb4e99c8c1507ae8.camel@telenet.be
Taiju HIGASHI schreef op wo 15-06-2022 om 08:58 [+0900]:
Toggle quote (2 lines)
>  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.
-----BEGIN PGP SIGNATURE-----

iI0EABYKADUWIQTB8z7iDFKP233XAR9J4+4iGRcl7gUCYqmSEhccbWF4aW1lZGV2
b3NAdGVsZW5ldC5iZQAKCRBJ4+4iGRcl7jBvAQC71D5WezoZwYg98DIJ2jyMOSoH
F5V7zCp7mGcKri8YCgD8CEB4G3eoJbZVVl393CbnBGUKNY48l4NzSbl01338DwI=
=V5zi
-----END PGP SIGNATURE-----


L
L
Ludovic Courtès wrote on 16 Jun 2022 23:43
Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less is not installed
(name . Taiju HIGASHI)(address . higashi@taiju.info)
87a6acjncd.fsf_-_@gnu.org
Hi,

Taiju HIGASHI <higashi@taiju.info> skribis:

Toggle quote (13 lines)
>>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’.
Attachment: file
L
L
Ludovic Courtès wrote on 16 Jun 2022 23:44
control message for bug #55845
(address . control@debbugs.gnu.org)
878rpwjnbx.fsf@gnu.org
close 55845
quit
T
T
Taiju HIGASHI wrote on 17 Jun 2022 02:38
Re: bug#55845: [PATCH 0/1] Improve pager selection logic when less is not installed
(name . Ludovic Courtès)(address . ludo@gnu.org)
8735g4nmyv.fsf@taiju.info
Hi Ludovic,

Toggle quote (3 lines)
> 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!

Toggle quote (9 lines)
> 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
:)

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
L
L
Ludovic Courtès wrote on 17 Jun 2022 14:39
(name . Taiju HIGASHI)(address . higashi@taiju.info)
87zgibh3au.fsf@gnu.org
Hello,

Taiju HIGASHI <higashi@taiju.info> skribis:

Toggle quote (7 lines)
> 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!

Toggle quote (5 lines)
> 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’.
T
T
Taiju HIGASHI wrote on 17 Jun 2022 15:36
(name . Ludovic Courtès)(address . ludo@gnu.org)
87wndfpg3c.fsf@taiju.info
Hi,

Toggle quote (13 lines)
>> 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
L
L
Ludovic Courtès wrote on 17 Jun 2022 17:12
(name . Taiju HIGASHI)(address . higashi@taiju.info)
87v8szfhnp.fsf@gnu.org
Hi!

Taiju HIGASHI <higashi@taiju.info> skribis:

Toggle quote (4 lines)
> 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’.
T
T
Taiju HIGASHI wrote on 18 Jun 2022 16:11
(name . Ludovic Courtès)(address . ludo@gnu.org)
87zgiam57f.fsf@taiju.info
Toggle quote (7 lines)
>> 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
?