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

[Ricardo Wurmus]

Hi Olivier,

thank for the patch!

> -(define (lookup-package specification)
> +(define (%lookup-package name+version output)
> +  (list (match (apply lookup-inferior-packages
> +                      (cons (current-guix) (string-split name+version #\@)))

I don’t think we need the cons here.  As long as the last argument to
APPLY is a list everything’s fine.

> +          ((first . rest) first)
> +          (_ (raise (condition
> +                     (&gwl-package-error
> +                      (package-spec (string-append name+version output)))))))
> +        output))

I’d prefer to have this return multiple values instead of a compound
value.  And if it had to be a compound value for some reason I’d prefer
to use a dedicated data type (a record value) instead of a list.

> +
> +(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 not sure about forcing SIMPLE-PACKAGE to be used because the return
value might be an output.  The stuff in (guix inferior) also doesn’t
know about outputs, so I feel that we shouldn’t attempt to include
syntax for selecting outputs.  IIRC we’ll end up with all outputs in the
environment, so there’s no actual effect of picking a specific output.

I’d prefer to revisit this once (guix inferior) supports selecting
outputs.  What do you think?

>  (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)))
> +
> +(define (simple-package pkg)
> +  (if (list? pkg) (car pkg) pkg))

Generally, I prefer to use MATCH instead of CAR and CDR.  But in this
case I’d prefer not to overload the meaning of “package” to include
lists.

-- 
Ricardo