unofficial mirror of gwl-devel@gnu.org
 help / color / mirror / Atom feed
* Packages specification does not work
@ 2022-04-21 17:22 Olivier Dion via
  2022-04-21 18:25 ` Olivier Dion via
  2022-04-21 19:51 ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion
  0 siblings, 2 replies; 23+ messages in thread
From: Olivier Dion via @ 2022-04-21 17:22 UTC (permalink / raw)
  To: gwl-devel

Hi,

The documentation says that we can provide the list of packages as Guix
specification:

--8<---------------cut here---------------start------------->8---
process run
  packages "guile@3.0" "guile@3.0:debug"
  outputs "where-is-guile.txt"
  # { type -p "guile" >> {{outputs}} }

workflow test
  process run
--8<---------------cut here---------------end--------------->8---

however I got:

#v+
info: .07 Loading workflow file `./test.w'...
ice-9/boot-9.scm:1752:10: error: Could not find package `guile@3.0' with the current Guix
#v-

-- 
Olivier Dion
oldiob.dev



^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: Packages specification does not work
  2022-04-21 17:22 Packages specification does not work Olivier Dion via
@ 2022-04-21 18:25 ` Olivier Dion via
  2022-04-21 19:51 ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion
  1 sibling, 0 replies; 23+ messages in thread
From: Olivier Dion via @ 2022-04-21 18:25 UTC (permalink / raw)
  To: gwl-devel

On Thu, 21 Apr 2022, Olivier Dion <olivier.dion@polymtl.ca> wrote:
> Hi,
>
> The documentation says that we can provide the list of packages as Guix
> specification:

I might have a fix.  Still working on testing it though:

--8<---------------cut here---------------start------------->8---
diff --git a/gwl/packages.scm b/gwl/packages.scm
index 6fe82d4..60e90a7 100644
--- a/gwl/packages.scm
+++ b/gwl/packages.scm
@@ -71,13 +71,21 @@
             (set! connection (open-connection))
             connection)))))

-(define (lookup-package specification)
+(define (%lookup-package name+version output)
+  (values
+   (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) (%lookup-package name+version output))))

 (define (valid-package? val)
   (or (package? val)
--8<---------------cut here---------------end--------------->8---

-- 
Olivier Dion
oldiob.dev


^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v1 1/2] packages: Support for full Guix specification
  2022-04-21 17:22 Packages specification does not work Olivier Dion via
  2022-04-21 18:25 ` Olivier Dion via
@ 2022-04-21 19:51 ` Olivier Dion
  2022-04-21 19:51   ` [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
                     ` (2 more replies)
  1 sibling, 3 replies; 23+ messages in thread
From: Olivier Dion @ 2022-04-21 19:51 UTC (permalink / raw)
  To: Olivier Dion via; +Cc: Olivier Dion

Guix package specifications match:

  PACKAGE [@VERSION] [:OUTPUT]

thus the following are all valid package specifications:

  - "guile"
  - "guile@3.0.8"
  - "guile:debug"
  - "guile@3.0.8:debug"

This is not currently supported by gwl.  To do so, simply return in
`lookup-package` a list with `car` as the inferior's package and `cadr` as its
output.

The `simple-package` procedure can be used to remove the package's output from
the returned value of `lookup-package` which is often necessary for manipulating
the package itself and not its output.
---
 gwl/packages.scm        | 35 +++++++++++++++++++++++++----------
 gwl/processes.scm       |  2 +-
 gwl/workflows/graph.scm |  2 +-
 3 files changed, 27 insertions(+), 12 deletions(-)

diff --git a/gwl/packages.scm b/gwl/packages.scm
index 6fe82d4..295c31c 100644
--- a/gwl/packages.scm
+++ b/gwl/packages.scm
@@ -43,6 +43,7 @@
             lookup-package
             valid-package?
             package-name
+            simple-package
 
             bash-minimal
             build-time-guix
@@ -71,17 +72,31 @@
             (set! connection (open-connection))
             connection)))))
 
-(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) (%lookup-package name+version output))))
 
 (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))
 
 ;; Just like package-full-name from (guix packages) but for inferior
 ;; packages.
@@ -102,11 +117,11 @@ the version.  By default, DELIMITER is \"@\"."
 
 (define bash-minimal
   (mlambda ()
-    (lookup-package "bash-minimal")))
+    (simple-package (lookup-package "bash-minimal"))))
 
 (define build-time-guix
   (mlambda ()
-    (lookup-package "guix")))
+    (simple-package (lookup-package "guix"))))
 
 (define default-guile
   (mlambda ()
diff --git a/gwl/processes.scm b/gwl/processes.scm
index 3a05e03..02a38c6 100644
--- a/gwl/processes.scm
+++ b/gwl/processes.scm
@@ -657,7 +657,7 @@ PROCESS."
                          (set-search-paths (map sexp->search-path-specification
                                                 ',search-paths)
                                            (cons ,profile
-                                                 ',packages))))
+                                                 ',(map simple-package packages)))))
                 #$(if out `(setenv "out" ,out) "")
                 (setenv "_GWL_PROFILE" #$profile)
                 (use-modules (ice-9 match))
diff --git a/gwl/workflows/graph.scm b/gwl/workflows/graph.scm
index ea3fec9..bdfdb11 100644
--- a/gwl/workflows/graph.scm
+++ b/gwl/workflows/graph.scm
@@ -43,7 +43,7 @@ label=<<FONT POINT-SIZE=\"14\">~a</FONT><BR/>\
             (take-color)
             (string-upcase pretty-name)
             (process-synopsis process)
-            (match (process-packages process)
+            (match (map simple-package (process-packages process))
               (() "")
               (inputs (format #f "<BR/>Uses: ~{~a~^, ~}."
                               (map package-name inputs)))))))
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH
  2022-04-21 19:51 ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion
@ 2022-04-21 19:51   ` Olivier Dion
  2022-04-29 11:42     ` Ricardo Wurmus
  2022-04-21 20:10   ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion via
  2022-04-22 18:43   ` [PATCH v2 0/2] Support full package specifications Olivier Dion
  2 siblings, 1 reply; 23+ messages in thread
From: Olivier Dion @ 2022-04-21 19:51 UTC (permalink / raw)
  To: Olivier Dion via; +Cc: Olivier Dion

This can be useful for testing examples while developing.

`./pre-inst-env guix workflow run ./doc/examples/example-workflow.w`
---
 pre-inst-env.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pre-inst-env.in b/pre-inst-env.in
index fcb4460..e447e30 100644
--- a/pre-inst-env.in
+++ b/pre-inst-env.in
@@ -30,4 +30,7 @@ export PATH
 GWL_UNINSTALLED="t"
 export GWL_UNINSTALLED
 
+GUIX_EXTENSIONS_PATH="$abs_top_builddir/guix/extensions"
+export GUIX_EXTENSIONS_PATH
+
 exec "$@"
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 1/2] packages: Support for full Guix specification
  2022-04-21 19:51 ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion
  2022-04-21 19:51   ` [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
@ 2022-04-21 20:10   ` Olivier Dion via
  2022-04-22 18:43   ` [PATCH v2 0/2] Support full package specifications Olivier Dion
  2 siblings, 0 replies; 23+ messages in thread
From: Olivier Dion via @ 2022-04-21 20:10 UTC (permalink / raw)
  To: Olivier Dion via


The patch was tested on the following example:
--8<---------------cut here---------------start------------->8---
process list-binary-symbols (with pkg binary)
  synopsis "List all symbols of an executable."
  description "Given a package, returns a list of symbols of an executable."
  packages
    list "findutils" "elfutils:bin"
      string-append pkg ":debug"
  outputs
    string-append binary "-symbol-list.txt"
  # {
      eu-nm $(find -L $_GWL_PROFILE/lib/debug -name "{{binary}}.debug") > {{outputs}}
  }

workflow test
  processes
    list-binary-symbols "guile@3.0.8" "guile"
--8<---------------cut here---------------end--------------->8---

-- 
Olivier Dion
oldiob.dev


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 0/2] Support full package specifications
  2022-04-21 19:51 ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion
  2022-04-21 19:51   ` [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
  2022-04-21 20:10   ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion via
@ 2022-04-22 18:43   ` Olivier Dion
  2022-04-22 18:43     ` [PATCH v2 1/2] packages: Support for full Guix specification Olivier Dion
                       ` (2 more replies)
  2 siblings, 3 replies; 23+ messages in thread
From: Olivier Dion @ 2022-04-22 18:43 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

* Change since v1

  - lookup-package without output parameter returns a simple-package directly.

Olivier Dion (2):
  packages: Support for full Guix specification
  pre-inst-env.in: Export GUIX_EXTENSIONS_PATH

 gwl/packages.scm        | 31 +++++++++++++++++++++++--------
 gwl/processes.scm       |  2 +-
 gwl/workflows/graph.scm |  2 +-
 pre-inst-env.in         |  3 +++
 4 files changed, 28 insertions(+), 10 deletions(-)

-- 
2.35.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v2 1/2] packages: Support for full Guix specification
  2022-04-22 18:43   ` [PATCH v2 0/2] Support full package specifications Olivier Dion
@ 2022-04-22 18:43     ` Olivier Dion
  2022-04-26 18:11       ` Ricardo Wurmus
  2022-04-22 18:43     ` [PATCH v2 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
  2022-04-29 18:02     ` [PATCH v3 0/1] Support full package specifications Olivier Dion
  2 siblings, 1 reply; 23+ messages in thread
From: Olivier Dion @ 2022-04-22 18:43 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

Guix package specifications match:

  PACKAGE [@VERSION] [:OUTPUT]

thus the following are all valid package specifications:

  - "guile"
  - "guile@3.0.8"
  - "guile:debug"
  - "guile@3.0.8:debug"

This is not currently supported by gwl.  To do so, simply return in
`lookup-package` a list with `car` as the inferior's package and `cadr` as its
output.

The `simple-package` procedure can be used to remove the package's output from
the returned value of `lookup-package` which is often necessary for manipulating
the package itself and not its output.
---
 gwl/packages.scm        | 31 +++++++++++++++++++++++--------
 gwl/processes.scm       |  2 +-
 gwl/workflows/graph.scm |  2 +-
 3 files changed, 25 insertions(+), 10 deletions(-)

diff --git a/gwl/packages.scm b/gwl/packages.scm
index 6fe82d4..1658e03 100644
--- a/gwl/packages.scm
+++ b/gwl/packages.scm
@@ -43,6 +43,7 @@
             lookup-package
             valid-package?
             package-name
+            simple-package
 
             bash-minimal
             build-time-guix
@@ -71,17 +72,31 @@
             (set! connection (open-connection))
             connection)))))
 
-(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)))))
 
 (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))
 
 ;; Just like package-full-name from (guix packages) but for inferior
 ;; packages.
diff --git a/gwl/processes.scm b/gwl/processes.scm
index ce40d12..cdb0988 100644
--- a/gwl/processes.scm
+++ b/gwl/processes.scm
@@ -657,7 +657,7 @@ PROCESS."
                          (set-search-paths (map sexp->search-path-specification
                                                 ',search-paths)
                                            (cons ,profile
-                                                 ',packages))))
+                                                 ',(map simple-package packages)))))
                 #$(if out `(setenv "out" ,out) "")
                 (setenv "_GWL_PROFILE" #$profile)
                 (use-modules (ice-9 match))
diff --git a/gwl/workflows/graph.scm b/gwl/workflows/graph.scm
index ea3fec9..bdfdb11 100644
--- a/gwl/workflows/graph.scm
+++ b/gwl/workflows/graph.scm
@@ -43,7 +43,7 @@ label=<<FONT POINT-SIZE=\"14\">~a</FONT><BR/>\
             (take-color)
             (string-upcase pretty-name)
             (process-synopsis process)
-            (match (process-packages process)
+            (match (map simple-package (process-packages process))
               (() "")
               (inputs (format #f "<BR/>Uses: ~{~a~^, ~}."
                               (map package-name inputs)))))))
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* [PATCH v2 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH
  2022-04-22 18:43   ` [PATCH v2 0/2] Support full package specifications Olivier Dion
  2022-04-22 18:43     ` [PATCH v2 1/2] packages: Support for full Guix specification Olivier Dion
@ 2022-04-22 18:43     ` Olivier Dion
  2022-04-29  9:00       ` zimoun
  2022-04-29 18:02     ` [PATCH v3 0/1] Support full package specifications Olivier Dion
  2 siblings, 1 reply; 23+ messages in thread
From: Olivier Dion @ 2022-04-22 18:43 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

This can be useful for testing examples while developing.

`./pre-inst-env guix workflow run ./doc/examples/example-workflow.w`
---
 pre-inst-env.in | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/pre-inst-env.in b/pre-inst-env.in
index fcb4460..e447e30 100644
--- a/pre-inst-env.in
+++ b/pre-inst-env.in
@@ -30,4 +30,7 @@ export PATH
 GWL_UNINSTALLED="t"
 export GWL_UNINSTALLED
 
+GUIX_EXTENSIONS_PATH="$abs_top_builddir/guix/extensions"
+export GUIX_EXTENSIONS_PATH
+
 exec "$@"
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/2] packages: Support for full Guix specification
  2022-04-22 18:43     ` [PATCH v2 1/2] packages: Support for full Guix specification Olivier Dion
@ 2022-04-26 18:11       ` Ricardo Wurmus
  2022-04-26 18:59         ` Olivier Dion via
  0 siblings, 1 reply; 23+ messages in thread
From: Ricardo Wurmus @ 2022-04-26 18:11 UTC (permalink / raw)
  To: Olivier Dion; +Cc: gwl-devel


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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/2] packages: Support for full Guix specification
  2022-04-26 18:11       ` Ricardo Wurmus
@ 2022-04-26 18:59         ` Olivier Dion via
  2022-04-26 20:30           ` Ricardo Wurmus
  0 siblings, 1 reply; 23+ messages in thread
From: Olivier Dion via @ 2022-04-26 18:59 UTC (permalink / raw)
  To: Ricardo Wurmus; +Cc: gwl-devel

On Tue, 26 Apr 2022, Ricardo Wurmus <rekado@elephly.net> wrote:
> 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.

Oh I didn't knew that! Awesome!

>> +          ((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.

With (values ...)?  That's what (gnu packages) does I think.

>  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.

A record would be a better fit than a list yes.

>> +
>> +(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?

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.

For example, try the following:

--8<---------------cut here---------------start------------->8---
process test
  packages "coreutils" "make"
  # {
    ls $_GWL_PROFILE > "result.txt"
  }

workflow wf
  processes test
--8<---------------cut here---------------end--------------->8---

and you will see that there's no `lib/debug` directory.  So not all
outputs are in the profile.

What would you suggest I do in the meantime?  I have to publish for
december and I don't think we will see this feature very soon.  I can
keep this patch on my side for my use case, but it would be awesome if
we have a none ad-hoc solution by the time of publication :-).

Regards,
old

-- 
Olivier Dion
oldiob.dev


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/2] packages: Support for full Guix specification
  2022-04-26 18:59         ` Olivier Dion via
@ 2022-04-26 20:30           ` Ricardo Wurmus
  2022-04-26 21:52             ` Olivier Dion via
  0 siblings, 1 reply; 23+ messages in thread
From: Ricardo Wurmus @ 2022-04-26 20:30 UTC (permalink / raw)
  To: Olivier Dion; +Cc: gwl-devel

Hi again,

Olivier Dion <olivier.dion@polymtl.ca> writes:

>>> +          ((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.
>
> With (values ...)?  That's what (gnu packages) does I think.

I think I missed how you intended for this to work.  IIUC you’re letting
LOOKUP-PACKAGE return a list of a package and an output because that
will end up as an argument to PACKAGES->MANIFEST (in (@ (gwl processes)
process->script)).

PACKAGES->MANIFEST has this docstring:

 "Return a list of manifest entries, one for each item listed in PACKAGES.
Elements of PACKAGES can be either package objects or package/string tuples
denoting a specific output of a package."

So that’s why you’re making it return a tuple of package/string tuples –
for compatibility with that procedure.

My comment about returning multiple values or a record value totally
misses your intent.  Sorry!  Now I get it.

> 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.

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?

--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?

>  (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))))

> +(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?"))))

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.

Ideally we’d also have tests for this.

What do you think of all this?

-- 
Ricardo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 1/2] packages: Support for full Guix specification
  2022-04-26 20:30           ` Ricardo Wurmus
@ 2022-04-26 21:52             ` Olivier Dion via
  0 siblings, 0 replies; 23+ messages in thread
From: Olivier Dion via @ 2022-04-26 21:52 UTC (permalink / raw)
  To: Ricardo Wurmus; +Cc: gwl-devel

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


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v2 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH
  2022-04-22 18:43     ` [PATCH v2 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
@ 2022-04-29  9:00       ` zimoun
  0 siblings, 0 replies; 23+ messages in thread
From: zimoun @ 2022-04-29  9:00 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion, Olivier Dion via

Hi,

On Fri, 22 Apr 2022 at 14:43, Olivier Dion <olivier.dion@polymtl.ca> wrote:

> diff --git a/pre-inst-env.in b/pre-inst-env.in

[...]

> +GUIX_EXTENSIONS_PATH="$abs_top_builddir/guix/extensions"
> +export GUIX_EXTENSIONS_PATH

I also have this in my own tree. :-)  Therefore, it seems useful.


Cheers,
simon


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH
  2022-04-21 19:51   ` [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
@ 2022-04-29 11:42     ` Ricardo Wurmus
  0 siblings, 0 replies; 23+ messages in thread
From: Ricardo Wurmus @ 2022-04-29 11:42 UTC (permalink / raw)
  To: Olivier Dion; +Cc: gwl-devel


Olivier Dion <olivier.dion@polymtl.ca> writes:

> This can be useful for testing examples while developing.
>
> `./pre-inst-env guix workflow run ./doc/examples/example-workflow.w`

I have applied it with commit 6f1241068c66a02c0d846a98b0ddac729054a200.

Thank you!

-- 
Ricardo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 0/1] Support full package specifications
  2022-04-22 18:43   ` [PATCH v2 0/2] Support full package specifications Olivier Dion
  2022-04-22 18:43     ` [PATCH v2 1/2] packages: Support for full Guix specification Olivier Dion
  2022-04-22 18:43     ` [PATCH v2 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
@ 2022-04-29 18:02     ` Olivier Dion
  2022-04-29 18:02       ` [PATCH v3 1/1] packages: Support for full Guix specification Olivier Dion
                         ` (2 more replies)
  2 siblings, 3 replies; 23+ messages in thread
From: Olivier Dion @ 2022-04-29 18:02 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

* Change since v1

  - lookup-package without output parameter returns a simple-package directly.

* Change since v2

  - Introduce <package-wrapper> record

  - Replace SIMPLE-PACKAGE with PACKAGE-UNWRAP

  - Disptach procedure using patterns matching

  NOTE: I'm not very happy with the naming I've come up with.

Olivier Dion (1):
  packages: Support for full Guix specification

 gwl/packages.scm        | 96 +++++++++++++++++++++++++++++++++--------
 gwl/processes.scm       |  6 +--
 gwl/workflows/graph.scm |  2 +-
 3 files changed, 81 insertions(+), 23 deletions(-)

-- 
2.35.1



^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v3 1/1] packages: Support for full Guix specification
  2022-04-29 18:02     ` [PATCH v3 0/1] Support full package specifications Olivier Dion
@ 2022-04-29 18:02       ` Olivier Dion
  2022-05-22  6:43         ` Ricardo Wurmus
  2022-05-17 20:40       ` [PATCH v3 0/1] Support full package specifications Olivier Dion via
  2022-05-22 12:38       ` [PATCH v4] packages: Support for full Guix specification Olivier Dion
  2 siblings, 1 reply; 23+ messages in thread
From: Olivier Dion @ 2022-04-29 18:02 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

Guix package specifications match:

  PACKAGE [@VERSION] [:OUTPUT]

thus the following are all valid package specifications:

  - "guile"
  - "guile@3.0.8"
  - "guile:debug"
  - "guile@3.0.8:debug"

This is not currently supported by gwl.  To do so, packages and their output are
wrapped in a <package-wrapper> record.  The record can be unwrapped with
PACKAGE-UNWRAP to access the underlying Guix's package.  Patterns matching is
used for dispatching of procedure such as PACKAGE-NATIVE-INPUTS or PACKAGE-NAME.
---
 gwl/packages.scm        | 96 +++++++++++++++++++++++++++++++++--------
 gwl/processes.scm       |  6 +--
 gwl/workflows/graph.scm |  2 +-
 3 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/gwl/packages.scm b/gwl/packages.scm
index 6fe82d4..8016bd4 100644
--- a/gwl/packages.scm
+++ b/gwl/packages.scm
@@ -21,7 +21,9 @@
   #:use-module ((guix store)
                 #:select (open-connection close-connection))
   #:use-module ((guix packages)
-                #:select (package? package-full-name))
+                #:select (package?
+                          package-full-name
+                          (package-native-inputs . guix:package-native-inputs)))
   #:use-module ((guix inferior)
                 #:select (open-inferior
                           inferior?
@@ -31,9 +33,13 @@
                           inferior-package-version
                           inferior-package-native-inputs
                           inferior-package-derivation))
+  #:use-module ((guix ui)
+                #:select (package-specification->name+version+output))
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-9)
+  #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
@@ -43,11 +49,56 @@
             lookup-package
             valid-package?
             package-name
+            package-unwrap
+            package->package+output
 
             bash-minimal
             build-time-guix
             default-guile
-            default-guile-derivation))
+            default-guile-derivation
+            guile-gcrypt))
+
+(define-record-type <package-wrapper>
+  (make-package-wrapper package output)
+  package-wrapper?
+  (package package-wrapper-package)
+  (output package-wrapper-output))
+
+(define package-native-inputs
+  (match-lambda
+    ((? package? pkg)
+     (package-native-inputs pkg))
+    ((? inferior-package? pkg)
+     (inferior-package-native-inputs pkg))
+    ((? package-wrapper? pkg)
+     (package-native-inputs (package-wrapper-package pkg)))))
+
+(define package-name
+  (match-lambda
+    ((? package? pkg)
+     (package-full-name pkg))
+    ((? inferior-package? pkg)
+     (inferior-package-full-name pkg))
+    ((? package-wrapper? pkg)
+     (package-name (package-wrapper-package pkg)))))
+
+(define package-unwrap
+  (match-lambda
+    ((or (? package? pkg)
+         (? inferior-package? pkg))
+     pkg)
+    ((? package-wrapper? pkg)
+     (package-wrapper-package pkg))))
+
+(define package->package+output
+  (match-lambda
+    ((or (? package? pkg)
+         (? inferior-package? pkg))
+     (list pkg "out"))
+    ((? package-wrapper? pkg)
+     (list
+      (package-wrapper-package pkg)
+      (package-wrapper-output pkg)))))
 
 (define current-guix
   (let ((current-guix-inferior #false))
@@ -73,15 +124,25 @@
 
 (define (lookup-package specification)
   (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)))))))
+  (let-values (((name version output)
+                (package-specification->name+version+output
+                 specification)))
+    (let* ((inferior-package
+            (lookup-inferior-packages (current-guix)
+                                      name version))
+           (package (match inferior-package
+                      ((first . rest) first)
+                      (_ (raise (condition
+                                 (&gwl-package-error
+                                  (package-spec specification))))))))
+      (make-package-wrapper package output))))
 
 (define (valid-package? val)
-  (or (package? val)
-      (inferior-package? val)))
+  (match val
+    ((or (? package?)
+         (? inferior-package?)
+         (? package-wrapper?)) #t)
+    (_ #f)))
 
 ;; Just like package-full-name from (guix packages) but for inferior
 ;; packages.
@@ -93,27 +154,24 @@ the version.  By default, DELIMITER is \"@\"."
                  delimiter
                  (inferior-package-version inferior-package)))
 
-(define package-name
-  (match-lambda
-    ((? package? pkg)
-     (package-full-name pkg))
-    ((? inferior-package? pkg)
-     (inferior-package-full-name pkg))))
-
 (define bash-minimal
   (mlambda ()
-    (lookup-package "bash-minimal")))
+    (package-unwrap (lookup-package "bash-minimal"))))
+
+(define guile-gcrypt
+  (mlambda ()
+    (package-unwrap (lookup-package "guile-gcrypt"))))
 
 (define build-time-guix
   (mlambda ()
-    (lookup-package "guix")))
+    (package-unwrap (lookup-package "guix"))))
 
 (define default-guile
   (mlambda ()
     "Return the variant of Guile that was used to build the \"guix\"
 package, which provides all library features used by the GWL.  We use
 this Guile to run scripts."
-    (and=> (assoc-ref (inferior-package-native-inputs (build-time-guix))
+    (and=> (assoc-ref (package-native-inputs (build-time-guix))
                       "guile") first)))
 
 (define (default-guile-derivation)
diff --git a/gwl/processes.scm b/gwl/processes.scm
index ce40d12..dd5ed02 100644
--- a/gwl/processes.scm
+++ b/gwl/processes.scm
@@ -611,7 +611,7 @@ tags if WITH-TAGS? is #FALSE or missing."
   "Return a file that contains the list of references of ITEM."
   (if (struct? item)                              ;lowerable object
       (computed-file name
-                     (with-extensions (list (lookup-package "guile-gcrypt")) ;for store-copy
+                     (with-extensions (list (guile-gcrypt)) ;for store-copy
                        (with-imported-modules (source-module-closure
                                                '((guix build store-copy)))
                          #~(begin
@@ -643,7 +643,7 @@ PROCESS."
   (let* ((name         (process-full-name process))
          (packages     (cons (bash-minimal)
                              (process-packages process)))
-         (manifest     (packages->manifest packages))
+         (manifest     (packages->manifest (map package->package+output packages)))
          (profile      (profile (content manifest)))
          (search-paths (delete-duplicates
                         (map search-path-specification->sexp
@@ -657,7 +657,7 @@ PROCESS."
                          (set-search-paths (map sexp->search-path-specification
                                                 ',search-paths)
                                            (cons ,profile
-                                                 ',packages))))
+                                                 ',(map package-unwrap packages)))))
                 #$(if out `(setenv "out" ,out) "")
                 (setenv "_GWL_PROFILE" #$profile)
                 (use-modules (ice-9 match))
diff --git a/gwl/workflows/graph.scm b/gwl/workflows/graph.scm
index ea3fec9..c435644 100644
--- a/gwl/workflows/graph.scm
+++ b/gwl/workflows/graph.scm
@@ -43,7 +43,7 @@ label=<<FONT POINT-SIZE=\"14\">~a</FONT><BR/>\
             (take-color)
             (string-upcase pretty-name)
             (process-synopsis process)
-            (match (process-packages process)
+            (match (map package-unwrap (process-packages process))
               (() "")
               (inputs (format #f "<BR/>Uses: ~{~a~^, ~}."
                               (map package-name inputs)))))))
-- 
2.35.1



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 0/1] Support full package specifications
  2022-04-29 18:02     ` [PATCH v3 0/1] Support full package specifications Olivier Dion
  2022-04-29 18:02       ` [PATCH v3 1/1] packages: Support for full Guix specification Olivier Dion
@ 2022-05-17 20:40       ` Olivier Dion via
  2022-05-22 12:38       ` [PATCH v4] packages: Support for full Guix specification Olivier Dion
  2 siblings, 0 replies; 23+ messages in thread
From: Olivier Dion via @ 2022-05-17 20:40 UTC (permalink / raw)
  To: gwl-devel; +Cc: Ricardo Wurmus


Any thoughts?

Regards,
old

-- 
Olivier Dion
oldiob.dev


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] packages: Support for full Guix specification
  2022-04-29 18:02       ` [PATCH v3 1/1] packages: Support for full Guix specification Olivier Dion
@ 2022-05-22  6:43         ` Ricardo Wurmus
  2022-05-22 12:33           ` Olivier Dion via
  0 siblings, 1 reply; 23+ messages in thread
From: Ricardo Wurmus @ 2022-05-22  6:43 UTC (permalink / raw)
  To: Olivier Dion; +Cc: gwl-devel


Hi Olivier,

thanks for the new patch and my apologies for the delay!

It looks fine, but I can’t help but feel a little confused about what
variables are actual package values and what are wrappers.  Sometimes we
have a variable called “packages”, but it’s seemingly always going to be
package wrappers, so we map “package-unwrap”.

I wonder if there’s a way to hide this machinery somewhat.  On the other
hand, “package-unwrap” is a no-op for actual packages, so it doesn’t
really matter.

> +(define package-native-inputs
> +  (match-lambda
> +    ((? package? pkg)
> +     (package-native-inputs pkg))
> +    ((? inferior-package? pkg)
> +     (inferior-package-native-inputs pkg))
> +    ((? package-wrapper? pkg)
> +     (package-native-inputs (package-wrapper-package pkg)))))

I’m confused about the naming.  Is this a recursive application of
PACKAGE-NATIVE-INPUTS?  Or are these two different implementations of
PACKAGE-NATIVE-INPUTS, one from GWL and the other from Guix?

>  (define (lookup-package specification)
>    (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)))))))
> +  (let-values (((name version output)
> +                (package-specification->name+version+output
> +                 specification)))
> +    (let* ((inferior-package
> +            (lookup-inferior-packages (current-guix)
> +                                      name version))
> +           (package (match inferior-package
> +                      ((first . rest) first)
> +                      (_ (raise (condition
> +                                 (&gwl-package-error
> +                                  (package-spec specification))))))))
> +      (make-package-wrapper package output))))

I think this would be slightly improved by using SRFI-71 instead of
LET-VALUES.  SRFI-71 replaces LET and LET* so that you can assign
multiple values without needing to be explicit about it.

Since this procedure is now a bit more complicated I think it would be
good to have a docstring that mentions the input and output values.

>  (define default-guile
>    (mlambda ()
>      "Return the variant of Guile that was used to build the \"guix\"
>  package, which provides all library features used by the GWL.  We use
>  this Guile to run scripts."
> -    (and=> (assoc-ref (inferior-package-native-inputs (build-time-guix))
> +    (and=> (assoc-ref (package-native-inputs (build-time-guix))
>                        "guile") first)))

Is this correct?  BUILD-TIME-GUIX has been changed so that the return
value is an unwrapped wrapper — so it really is an inferior package.
Does PACKAGE-NATIVE-INPUTS refer to the implementation here or that in
Guix?

I think it would be best if we can make all this unambiguous.

-- 
Ricardo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* Re: [PATCH v3 1/1] packages: Support for full Guix specification
  2022-05-22  6:43         ` Ricardo Wurmus
@ 2022-05-22 12:33           ` Olivier Dion via
  0 siblings, 0 replies; 23+ messages in thread
From: Olivier Dion via @ 2022-05-22 12:33 UTC (permalink / raw)
  To: Ricardo Wurmus; +Cc: gwl-devel

On Sun, 22 May 2022, Ricardo Wurmus <rekado@elephly.net> wrote:
> Hi Olivier,
>
> thanks for the new patch and my apologies for the delay!
>
> It looks fine, but I can’t help but feel a little confused about what
> variables are actual package values and what are wrappers.  Sometimes we
> have a variable called “packages”, but it’s seemingly always going to be
> package wrappers, so we map “package-unwrap”.
>
> I wonder if there’s a way to hide this machinery somewhat.  On the other
> hand, “package-unwrap” is a no-op for actual packages, so it doesn’t
> really matter.

Hi.  I've think of a way to accomplish this without introducing anything
ad-hoc.  You will see in my next patch.  Basically, I use object
properties to mark a package with its output.  By doing so, we only need
to change the code that actually need the package's output, that is the
call to `packages->manifest'.  The rest of the code simply manipulates a
package as usual.  I think it's technique called data layering and is
awesome!

-- 
Olivier Dion
oldiob.dev


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v4] packages: Support for full Guix specification
  2022-04-29 18:02     ` [PATCH v3 0/1] Support full package specifications Olivier Dion
  2022-04-29 18:02       ` [PATCH v3 1/1] packages: Support for full Guix specification Olivier Dion
  2022-05-17 20:40       ` [PATCH v3 0/1] Support full package specifications Olivier Dion via
@ 2022-05-22 12:38       ` Olivier Dion
  2022-05-23 21:02         ` Ricardo Wurmus
  2022-05-23 21:45         ` [PATCH v5] " Olivier Dion
  2 siblings, 2 replies; 23+ messages in thread
From: Olivier Dion @ 2022-05-22 12:38 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

Guix package specifications match:

  PACKAGE [@VERSION] [:OUTPUT]

thus the following are all valid package specifications:

  - "guile"
  - "guile@3.0.8"
  - "guile:debug"
  - "guile@3.0.8:debug"

This is not currently supported by gwl.  To do so, simply mark the package with
its output through Guile's object properties infrastructure.

The `package-output' procedure can then be used to retrieve the package's output.
---
 gwl/packages.scm  | 24 +++++++++++++++++++-----
 gwl/processes.scm |  5 ++++-
 2 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/gwl/packages.scm b/gwl/packages.scm
index 6fe82d4..23bc09d 100644
--- a/gwl/packages.scm
+++ b/gwl/packages.scm
@@ -31,18 +31,22 @@
                           inferior-package-version
                           inferior-package-native-inputs
                           inferior-package-derivation))
+  #:use-module ((guix ui)
+                #:select (package-specification->name+version+output))
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
+  #:use-module (srfi srfi-71)
   #:export (current-guix
             inferior-store
 
             lookup-package
             valid-package?
             package-name
+            package-output
 
             bash-minimal
             build-time-guix
@@ -73,11 +77,18 @@
 
 (define (lookup-package specification)
   (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)))))))
+  (let ((name version output
+              (package-specification->name+version+output specification)))
+    (let* ((inferior-package
+            (lookup-inferior-packages (current-guix)
+                                      name version))
+           (package (match inferior-package
+                      ((first . rest) first)
+                      (_ (raise (condition
+                                 (&gwl-package-error
+                                  (package-spec specification))))))))
+      (set-object-property! package #:gwl/package-output output)
+      package)))
 
 (define (valid-package? val)
   (or (package? val)
@@ -100,6 +111,9 @@ the version.  By default, DELIMITER is \"@\"."
     ((? inferior-package? pkg)
      (inferior-package-full-name pkg))))
 
+(define (package-output pkg)
+  (object-property pkg #:gwl/package-output))
+
 (define bash-minimal
   (mlambda ()
     (lookup-package "bash-minimal")))
diff --git a/gwl/processes.scm b/gwl/processes.scm
index ce40d12..4fc4d6d 100644
--- a/gwl/processes.scm
+++ b/gwl/processes.scm
@@ -643,7 +643,10 @@ PROCESS."
   (let* ((name         (process-full-name process))
          (packages     (cons (bash-minimal)
                              (process-packages process)))
-         (manifest     (packages->manifest packages))
+         (manifest     (packages->manifest (map
+                                            (lambda (pkg)
+                                              (list pkg (package-output pkg)))
+                                            packages)))
          (profile      (profile (content manifest)))
          (search-paths (delete-duplicates
                         (map search-path-specification->sexp
-- 
2.36.0



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v4] packages: Support for full Guix specification
  2022-05-22 12:38       ` [PATCH v4] packages: Support for full Guix specification Olivier Dion
@ 2022-05-23 21:02         ` Ricardo Wurmus
  2022-05-23 21:45         ` [PATCH v5] " Olivier Dion
  1 sibling, 0 replies; 23+ messages in thread
From: Ricardo Wurmus @ 2022-05-23 21:02 UTC (permalink / raw)
  To: Olivier Dion; +Cc: gwl-devel


Hi Olivier,

thank you for the updated patch!  Using object properties is a neat
idea.

> @@ -73,11 +77,18 @@
>  
>  (define (lookup-package specification)
>    (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)))))))
> +  (let ((name version output
> +              (package-specification->name+version+output specification)))
> +    (let* ((inferior-package
> +            (lookup-inferior-packages (current-guix)
> +                                      name version))
> +           (package (match inferior-package
> +                      ((first . rest) first)
> +                      (_ (raise (condition
> +                                 (&gwl-package-error
> +                                  (package-spec specification))))))))

You can merge the SRFI-71 “let” with “let*” to a single “let*” and thus
remove one layer of nested parentheses.

> +      (set-object-property! package #:gwl/package-output output)

The Guile manual has this to say about “set-object-property!”:

       Guile also implements a more traditional Lispy interface to
    properties, in which each object has an list of key-value pairs
    associated with it.  Properties in that list are keyed by symbols.  This
    is a legacy interface; you should use weak hash tables or object
    properties instead.

The preferred way to do this is to define a property, and then use
“set!” and the property getter on the object:

--8<---------------cut here---------------start------------->8---
(define package-output (make-object-property))

(define* (lookup-package …)
  …
 (set! (package-output package) output) ;store the output as a property
 …)

…

(package-output package) ; retrieve the output
--8<---------------cut here---------------end--------------->8---

-- 
Ricardo


^ permalink raw reply	[flat|nested] 23+ messages in thread

* [PATCH v5] packages: Support for full Guix specification
  2022-05-22 12:38       ` [PATCH v4] packages: Support for full Guix specification Olivier Dion
  2022-05-23 21:02         ` Ricardo Wurmus
@ 2022-05-23 21:45         ` Olivier Dion
  2022-06-01 13:07           ` Ricardo Wurmus
  1 sibling, 1 reply; 23+ messages in thread
From: Olivier Dion @ 2022-05-23 21:45 UTC (permalink / raw)
  To: Olivier Dion, Olivier Dion via

Guix package specifications match:

  PACKAGE [@VERSION] [:OUTPUT]

thus the following are all valid package specifications:

  - "guile"
  - "guile@3.0.8"
  - "guile:debug"
  - "guile@3.0.8:debug"

This is not currently supported by gwl.  To do so, simply mark the package with
its output through Guile's object properties infrastructure.

The `package-output' procedure can then be used to retrieve the package's output.
---
 gwl/packages.scm  | 22 +++++++++++++++++-----
 gwl/processes.scm |  5 ++++-
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/gwl/packages.scm b/gwl/packages.scm
index 6fe82d4..6a598ba 100644
--- a/gwl/packages.scm
+++ b/gwl/packages.scm
@@ -31,18 +31,22 @@
                           inferior-package-version
                           inferior-package-native-inputs
                           inferior-package-derivation))
+  #:use-module ((guix ui)
+                #:select (package-specification->name+version+output))
   #:use-module (ice-9 format)
   #:use-module (ice-9 match)
   #:use-module (srfi srfi-1)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-34)
   #:use-module (srfi srfi-35)
+  #:use-module (srfi srfi-71)
   #:export (current-guix
             inferior-store
 
             lookup-package
             valid-package?
             package-name
+            package-output
 
             bash-minimal
             build-time-guix
@@ -73,11 +77,17 @@
 
 (define (lookup-package specification)
   (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)))))))
+  (let* ((name version output
+               (package-specification->name+version+output specification))
+         (inferior-package
+          (lookup-inferior-packages (current-guix) name version))
+         (package (match inferior-package
+                    ((first . rest) first)
+                    (_ (raise (condition
+                               (&gwl-package-error
+                                (package-spec specification))))))))
+    (set! (package-output package) output)
+    package))
 
 (define (valid-package? val)
   (or (package? val)
@@ -100,6 +110,8 @@ the version.  By default, DELIMITER is \"@\"."
     ((? inferior-package? pkg)
      (inferior-package-full-name pkg))))
 
+(define package-output (make-object-property))
+
 (define bash-minimal
   (mlambda ()
     (lookup-package "bash-minimal")))
diff --git a/gwl/processes.scm b/gwl/processes.scm
index ce40d12..4fc4d6d 100644
--- a/gwl/processes.scm
+++ b/gwl/processes.scm
@@ -643,7 +643,10 @@ PROCESS."
   (let* ((name         (process-full-name process))
          (packages     (cons (bash-minimal)
                              (process-packages process)))
-         (manifest     (packages->manifest packages))
+         (manifest     (packages->manifest (map
+                                            (lambda (pkg)
+                                              (list pkg (package-output pkg)))
+                                            packages)))
          (profile      (profile (content manifest)))
          (search-paths (delete-duplicates
                         (map search-path-specification->sexp
-- 
2.36.0



^ permalink raw reply related	[flat|nested] 23+ messages in thread

* Re: [PATCH v5] packages: Support for full Guix specification
  2022-05-23 21:45         ` [PATCH v5] " Olivier Dion
@ 2022-06-01 13:07           ` Ricardo Wurmus
  0 siblings, 0 replies; 23+ messages in thread
From: Ricardo Wurmus @ 2022-06-01 13:07 UTC (permalink / raw)
  To: Olivier Dion; +Cc: gwl-devel

Hi Olivier,

thank you for this update.  I applied your patch!

-- 
Ricardo


^ permalink raw reply	[flat|nested] 23+ messages in thread

end of thread, other threads:[~2022-06-01 13:15 UTC | newest]

Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-04-21 17:22 Packages specification does not work Olivier Dion via
2022-04-21 18:25 ` Olivier Dion via
2022-04-21 19:51 ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion
2022-04-21 19:51   ` [PATCH v1 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
2022-04-29 11:42     ` Ricardo Wurmus
2022-04-21 20:10   ` [PATCH v1 1/2] packages: Support for full Guix specification Olivier Dion via
2022-04-22 18:43   ` [PATCH v2 0/2] Support full package specifications Olivier Dion
2022-04-22 18:43     ` [PATCH v2 1/2] packages: Support for full Guix specification Olivier Dion
2022-04-26 18:11       ` Ricardo Wurmus
2022-04-26 18:59         ` Olivier Dion via
2022-04-26 20:30           ` Ricardo Wurmus
2022-04-26 21:52             ` Olivier Dion via
2022-04-22 18:43     ` [PATCH v2 2/2] pre-inst-env.in: Export GUIX_EXTENSIONS_PATH Olivier Dion
2022-04-29  9:00       ` zimoun
2022-04-29 18:02     ` [PATCH v3 0/1] Support full package specifications Olivier Dion
2022-04-29 18:02       ` [PATCH v3 1/1] packages: Support for full Guix specification Olivier Dion
2022-05-22  6:43         ` Ricardo Wurmus
2022-05-22 12:33           ` Olivier Dion via
2022-05-17 20:40       ` [PATCH v3 0/1] Support full package specifications Olivier Dion via
2022-05-22 12:38       ` [PATCH v4] packages: Support for full Guix specification Olivier Dion
2022-05-23 21:02         ` Ricardo Wurmus
2022-05-23 21:45         ` [PATCH v5] " Olivier Dion
2022-06-01 13:07           ` Ricardo Wurmus

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).