[bug#57704] [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.

[bug#57704] [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.

[Liliana Marie Prikler]

This makes it so that new-style inputs can be optional using regular Guile
patterns, e.g. (and (target-x86-64?) rust).

* guix/packages.scm (sanitize-inputs): Filter inputs by identity before adding
labels.
---
Note that this patch was prepared using master, but since it affects the
package record, it needs to go to core-updates.  I don' think there should
be a merge conflict here.

 guix/packages.scm | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/guix/packages.scm b/guix/packages.scm
index 94e464cd01..5bb2e81e18 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -430,11 +430,12 @@ (define %cuirass-supported-systems
 (define-inlinable (sanitize-inputs inputs)
   "Sanitize INPUTS by turning it into a list of name/package tuples if it's
 not already the case."
-  (cond ((null? inputs) inputs)
-        ((and (pair? (car inputs))
-              (string? (caar inputs)))
-         inputs)
-        (else (map add-input-label inputs))))
+  (let ((inputs (filter identity inputs)))
+    (cond ((null? inputs) inputs)
+          ((and (pair? (car inputs))
+                (string? (caar inputs)))
+           inputs)
+          (else (map add-input-label inputs)))))
 
 (define-syntax current-location-vector
   (lambda (s)
-- 
2.37.2

[bug#57704] [PATCH v2] guix: packages: Remove #f from inputs when sanitizing.

[Liliana Marie Prikler]

This makes it so that new-style inputs can be optional using regular Guile
patterns, e.g. (and (target-x86-64?) rust).

* guix/packages.scm (sanitize-inputs): Filter inputs by identity before adding
labels.
---
As noted by Maxime, this doesn't seem to be cause any rebuilds, so retargeting
master.  Also added missing documentation.

 guix/packages.scm | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/guix/packages.scm b/guix/packages.scm
index 94e464cd01..7569380610 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -428,13 +428,14 @@ (define %cuirass-supported-systems
   (fold delete %supported-systems '("mips64el-linux" "powerpc-linux" "riscv64-linux")))
 
 (define-inlinable (sanitize-inputs inputs)
-  "Sanitize INPUTS by turning it into a list of name/package tuples if it's
-not already the case."
-  (cond ((null? inputs) inputs)
-        ((and (pair? (car inputs))
-              (string? (caar inputs)))
-         inputs)
-        (else (map add-input-label inputs))))
+  "Sanitize INPUTS by removing falsy elements and turning it into a list of
+name/package tuples if it's not already the case."
+  (let ((inputs (filter identity inputs)))
+    (cond ((null? inputs) inputs)
+          ((and (pair? (car inputs))
+                (string? (caar inputs)))
+           inputs)
+          (else (map add-input-label inputs)))))
 
 (define-syntax current-location-vector
   (lambda (s)
-- 
2.37.2

[bug#57704] [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 1202 bytes --]

On 09-09-2022 17:56, Liliana Marie Prikler wrote:
> This makes it so that new-style inputs can be optional using regular Guile
> patterns, e.g. (and (target-x86-64?) rust).

Seems useful.

> * guix/packages.scm (sanitize-inputs): Filter inputs by identity before adding
> labels.

Documentation is missing.

> ---
> Note that this patch was prepared using master, but since it affects the
> package record, it needs to go to core-updates.  I don' think there should
> be a merge conflict here.

It does affect the package record, but it doesn't cause any rebuilds, so 
master should be fine:

* There aren't any current uses of #false:

(use-modules (guix packages) (gnu packages))
(package
   (inherit (specification->package "hello"))
   (inputs (list #false)))
;; guix build -f [...] --> package ‘hello@2.12.1’ has an invalid input

* In the absence of #false, the behaviour remains unchanged.

* guix/packages.scm is not used by any derivation
   (except for "guix pull" and the guix package)

As a test, I applied the patch and did
‘make && ./pre-inst-env guix build -n libreoffice’,
and it turned out I already have it installed.

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57704] [PATCH v2] guix: packages: Remove #f from inputs when sanitizing.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 957 bytes --]



On 09-09-2022 17:56, Liliana Marie Prikler wrote:
> This makes it so that new-style inputs can be optional using regular Guile
> patterns, e.g. (and (target-x86-64?) rust).
> 
> * guix/packages.scm (sanitize-inputs): Filter inputs by identity before adding
> labels.
> ---
> As noted by Maxime, this doesn't seem to be cause any rebuilds, so retargeting
> master.  Also added missing documentation.

The docstring is nice, but with documentation, I meant the manual, 
presumably in ‘(guix)package Reference’, maybe also in the packaging 
tutorial in the cookbook.

Also, something I forgot: performance.  sanitize-inputs is called for 
every package, and the change adds additional memory allocations (due to 
the use of 'filter'), is there an observable performance impact (maybe 
"GUIX_PROFILING=gc time guix refresh -l pkg-config" would be a good 
test)?  If there is, some optimisations may be in order

Greetings,
Maxime.

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57704] [PATCH v2] guix: packages: Remove #f from inputs when sanitizing.

[Liliana Marie Prikler]

Am Samstag, dem 10.09.2022 um 02:33 +0200 schrieb Maxime Devos:
> The docstring is nice, but with documentation, I meant the manual, 
> presumably in ‘(guix)package Reference’, maybe also in the packaging 
> tutorial in the cookbook.
I don't see the current practice documented, so I think we're actually
good on this front.

> Also, something I forgot: performance.  sanitize-inputs is called for
> every package, and the change adds additional memory allocations (due
> to the use of 'filter'), is there an observable performance impact
> (maybe "GUIX_PROFILING=gc time guix refresh -l pkg-config" would be a
> good test)?  If there is, some optimisations may be in order
Looking at the numbers below

Garbage collection statistics:
  heap size:        212.66 MiB
  allocated:        739.15 MiB
  GC times:         20
  time spent in GC: 5.30 seconds (65% of user time)

real	0m3,606s
user	0m8,140s
sys	0m0,109s

Garbage collection statistics:
  heap size:        276.29 MiB
  allocated:        1292.10 MiB
  GC times:         28
  time spent in GC: 10.48 seconds (64% of user time)

real	0m11,638s
user	0m16,422s
sys	0m0,308s

it does appear that this increases times by a factor of two.  Use of
filter! instead of filter brings only marginal benefits.  I'll check if
we could instead simply ignore unspecified? values when collecting the
inputs – that would allow the natural use of (when) and (unless).

Cheers 

[bug#57704] [PATCH v3] guix: Filter unspecified inputs when sanitizing.

[Liliana Marie Prikler]

* guix/packages.scm (sanitize-inputs): Filter inputs which are unspecified?
rather than adding a label.
---
 guix/packages.scm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/guix/packages.scm b/guix/packages.scm
index 94e464cd01..0975002c13 100644
--- a/guix/packages.scm
+++ b/guix/packages.scm
@@ -429,12 +429,15 @@ (define %cuirass-supported-systems
 
 (define-inlinable (sanitize-inputs inputs)
   "Sanitize INPUTS by turning it into a list of name/package tuples if it's
-not already the case."
+not already the case and removing unspecified inputs."
   (cond ((null? inputs) inputs)
         ((and (pair? (car inputs))
               (string? (caar inputs)))
          inputs)
-        (else (map add-input-label inputs))))
+        (else (filter-map (lambda (input)
+                            (if (unspecified? input) #f
+                                (add-input-label input)))
+                          inputs))))
 
 (define-syntax current-location-vector
   (lambda (s)
-- 
2.37.2

[bug#57704] [PATCH v2] guix: packages: Remove #f from inputs when sanitizing.

[Liliana Marie Prikler]

Am Samstag, dem 10.09.2022 um 08:40 +0200 schrieb Liliana Marie
Prikler:
> Looking at the numbers below
> [...]
> it does appear that this increases times by a factor of two.
It seems I've been comparing apples to oranges.  Running ./pre-inst-env
already increases the times for guix:

Garbage collection statistics:
  heap size:        276.29 MiB
  allocated:        1291.91 MiB
  GC times:         28
  time spent in GC: 9.39 seconds (66% of user time)

real	0m6,069s
user	0m14,172s
sys	0m0,140s

An alternative patch that I'll submit as v3 adds little to these times:

Garbage collection statistics:
  heap size:        276.29 MiB
  allocated:        1291.96 MiB
  GC times:         28
  time spent in GC: 9.32 seconds (66% of user time)

real	0m6,124s
user	0m14,138s
sys	0m0,147s

Cheers

[bug#57704] [PATCH v3] guix: Filter unspecified inputs when sanitizing.

[Maxime Devos]

[-- Attachment #1.1.1: Type: text/plain, Size: 2185 bytes --]

> Am Samstag, dem 10.09.2022 um 02:33 +0200 schrieb Maxime Devos:
>> The docstring is nice, but with documentation, I meant the manual, 
>> presumably in ‘(guix)package Reference’, maybe also in the packaging 
>> tutorial in the cookbook.
> I don't see the current practice documented, so I think we're actually
> good on this front.

That sounds bad to me -- the undocumented surface should be decreased, 
not increased.  Also, it is actually documented a little:

      ‘inputs’ (default: ‘'()’)
      ‘native-inputs’ (default: ‘'()’)
      ‘propagated-inputs’ (default: ‘'()’)
           These fields list dependencies of the package.  Each element
           of these lists is either a package, origin, or other
           “file-like object” (*note G-Expressions::); [...]

#false (or, in this case, *unspecified*) is neither a package, origin or 
other file-like object.  Maybe you can add that #false is also allowed 
but ignored?

On 10-09-2022 09:41, Liliana Marie Prikler wrote:
>            inputs)
> -        (else (map add-input-label inputs))))
> +        (else (filter-map (lambda (input)
> +                            (if (unspecified? input) #f
> +                                (add-input-label input)))
> +                          inputs))))

(when cond ...) / (unless cond ...) returning *unspecified* when (not 
cond)/cond is an implementation detail:

   * The return values(s) when (not cond)/cond is not documented in
     (guile)Conditionals
   * maybe: https://debbugs.gnu.org/cgi/bugreport.cgi?bug=56799#17

     There is an interest in letting it return zero values instead of
     *unspecified*, see e.g. 
https://scheme-reports.scheme-reports.narkive.com/QSQtJSAh/unspecified-values
     and a ‘bug’ on bugs.gnu.org I cannot find anymore about actually
     doing this change.

     By assuming that when/unless returns *unspecified* here, an
     additional backwards-compatibility concern is introduced.

As such, I don't think relying on this to be a good idea.

Alternative proposal: instead of (when cond package), maybe
(and cond package)?

Greetings,
Maxime

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 929 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 236 bytes --]

[bug#57704] [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.

[Ludovic Courtès]

Hi Liliana,

Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:

> This makes it so that new-style inputs can be optional using regular Guile
> patterns, e.g. (and (target-x86-64?) rust).

I’d rather avoid that and make sure input lists are just plain lists,
remaining strict, and keeping the sanitize procedure simple (notably so
it can be optimized in common cases).

That means we have to live with idioms like:

  (append (list x y z)
          (if (target-x86-64?) (list rust) '()))

The ‘openmpi’ package has sugar to make that more concise.

Thoughts?

Thanks,
Ludo’.

[bug#57704] [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.

[Maxim Cournoyer]

Hi Liliana,

Ludovic Courtès <ludo@gnu.org> writes:

> Hi Liliana,
>
> Liliana Marie Prikler <liliana.prikler@gmail.com> skribis:
>
>> This makes it so that new-style inputs can be optional using regular Guile
>> patterns, e.g. (and (target-x86-64?) rust).
>
> I’d rather avoid that and make sure input lists are just plain lists,
> remaining strict, and keeping the sanitize procedure simple (notably so
> it can be optimized in common cases).
>
> That means we have to live with idioms like:
>
>   (append (list x y z)
>           (if (target-x86-64?) (list rust) '()))
>
> The ‘openmpi’ package has sugar to make that more concise.
>
> Thoughts?

Any plans to revisit this, or should we close it?

-- 
Thanks,
Maxim