Re: [PATCH v2 1/2] packages: Support for full Guix specification

[Olivier Dion via <gwl-devel@gnu.org>]

On Tue, 26 Apr 2022, Ricardo Wurmus <rekado@elephly.net> wrote:
> PACKAGES->MANIFEST 
> So that’s why you’re making it return a tuple of package/string tuples –
> for compatibility with that procedure.

Yes that's indeed what I had in mind.

>> I do think it would be better to wait for (guix inferior) to support
>> selecting outputs.  However, I do need selection of outputs for my
>> use case right now!  Specificaly, I need to have debug symbols of
>> many packages.  The quick hack above does the work for me but I
>> understand that it would be preferable if (guix inferior) has support
>> for outputs instead.
>
> I understand.
>
> So … I think we can figure something out that won’t be far removed from
> what you proposed.  I’d probably split it into smaller procedures,
> though, to make it a bit more obvious what’s going on.
>
> Let’s see the diff again…
>
>> -(define (lookup-package specification)
>> +(define (%lookup-package name+version output)
>> +  (list (match (apply lookup-inferior-packages
>> +                      (cons (current-guix) (string-split name+version #\@)))
>> +          ((first . rest) first)
>> +          (_ (raise (condition
>> +                     (&gwl-package-error
>> +                      (package-spec (string-append name+version output)))))))
>> +        output))
>>
>> +(define* (lookup-package specification #:optional (output "out"))
>>    (log-event 'guix (G_ "Looking up package `~a'~%") specification)
>> -  (match (lookup-inferior-packages (current-guix) specification)
>> -    ((first . rest) first)
>> -    (_ (raise (condition
>> -               (&gwl-package-error
>> -                (package-spec specification)))))))
>> +  (match (string-split specification #\:)
>> +    ((name+version sub-drv) (%lookup-package name+version sub-drv))
>> +    ((name+version) (simple-package (%lookup-package name+version output)))))
>
> I’m struggling to figure out a cleaner way to do this…
> Why are we processing the specification *and* accept an optional OUTPUT
> argument?  It seems to me that SUB-DRV and OUTPUT *should* be the same,
> but it’s possible to call LOOKUP-PACKAGE in a way that they differ,
> which doesn’t make much sense to me.

Hmm you're right.  I don't know why I've put this here.  I think I got
confused with how one can do e.g. `(,git "send-email") instead of
"git:send-email".  One use the package object followed by the output,
the other is a full speficication.  So really what we want here is no
optional output.

> Another thing that bothers me a bit is all that string splitting; once
> for version, again for the output.  The (guix ui) module has
> PACKAGE-SPECIFICATION->NAME+VERSION+OUTPUT, which is dedicated for this
> task.  It returns multiple values; let’s use LET* from SRFI-71.  What do
> you think of this?

Yes.  If there's a standard function in Guix, then it's better to use
it. Less burden on ourself.

> --8<---------------cut here---------------start------------->8---
> (import (srfi srfi-71)
> (define (lookup-package specification)
>   "Look up SPECIFICATION in an inferior and return a matching package.  If the
> specification declares a specific output return a tuple consisting of the
> package value and the output.  If no matching package is found, raise a
> &GWL-PACKAGE-ERROR."
>    (log-event 'guix (G_ "Looking up package `~a'~%") specification)
>    (let* ((name version output (package-specification->name+version+output specification))
>           (package
>             (match (lookup-inferior-packages (current-guix) name version)
>               ((first . rest) first)
>               (_ (raise (condition
>                          (&gwl-package-error
>                           (package-spec specification))))))))
>      (if output
>          (list package output)
>          package)))
> --8<---------------cut here---------------end--------------->8---
> What do you think of that?

Looks good. However, output is already "out" if no output is provided.
So the alternative branch is never took here.  I would change it to:
--8<---------------cut here---------------start------------->8---
(match output
  ("out" package)
  (specific (list package specific)))
--8<---------------cut here---------------end--------------->8---

>>  (define (valid-package? val)
>> -  (or (package? val)
>> -      (inferior-package? val)))
>> +  (or
>> +   (and (list? val)
>> +        (valid-package? (car val))
>> +        (string? (cadr val)))
>> +   (package? val)
>> +   (inferior-package? val)))
>> +
>
> I suggest rewriting this whole thing with MATCH so that the structure of
> VAL becomes apparent.  Perhaps something like this?
>
>    (match
>      ((maybe-package (? string? output))
>       (valid-package? maybe-package))
>      (_
>       (or (package? val)
>           (inferior-package? val))))

Yes.  Matching patterns are awesome!

>> +(define (simple-package pkg)
>> +  (if (list? pkg) (car pkg) pkg))
>
> I still don’t like this :)  Not only the implementation but the fact
> that it appears to be needed.  At least implementation-wise I’d prefer
> something like this:
>
> (define (just-package maybe-package+output)
>   (match maybe-package+output
>     (((? package? package) (? string? output)) package)
>     ((? package? package) package)
>     (_ (error "what is this?"))))

Yes match or record's getter are better.

>
> There are a few places where we need to be careful that we’re dealing
> with the right type and that we handle both cases equally well: when a
> tuple is encountered and when a plain package value is encountered.

If we need to deal in different way depending on the type, wouldn't
generics help here?  If not, we can use a record.  Then, it's just a
matter of using a getter for the underlying package without the output.

Ideally, we have to determined what we use here.  Generic, record or
matching patterns? 

> Ideally we’d also have tests for this.

For testing, I think you have a lots of unit testing.  Which is great on
its own.  However, I believe that the best way of testing a software is
to develop scenarios and run them, just like an user would.

We could start by adding a test suite that run all of the examples in
the documentation directory and the tutorial.  I believe that the latter
is even more crucial since this is what new user will try first.  If the
tutorial does not work for them, chance are they are going to uninstall
gwl before trying anything!

Finer grained scenarios could be added that way in the future.  This
also augment the set of examples for users.

What do you think?

-- 
Olivier Dion
oldiob.dev