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

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

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

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

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

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,