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

  • Open
  • quality assurance status badge
Details
5 participants
  • Liliana Marie Prikler
  • Ludovic Courtès
  • Christopher Baines
  • Maxim Cournoyer
  • Maxime Devos
Owner
unassigned
Submitted by
Liliana Marie Prikler
Severity
normal
L
L
Liliana Marie Prikler wrote on 9 Sep 2022 17:56
(address . guix-patches@gnu.org)
b1b6504d725df23dc910cb04591a203979cdca7b.camel@gmail.com
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(-)

Toggle diff (24 lines)
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
M
M
Maxime Devos wrote on 9 Sep 2022 20:54
23051978-3831-94af-48ce-d67691248aa6@telenet.be
On 09-09-2022 17:56, Liliana Marie Prikler wrote:
Toggle quote (2 lines)
> This makes it so that new-style inputs can be optional using regular Guile
> patterns, e.g. (and (target-x86-64?) rust).
Seems useful.
Toggle quote (2 lines)
> * guix/packages.scm (sanitize-inputs): Filter inputs by identity before adding
> labels.
Documentation is missing.
Toggle quote (4 lines)
> ---
> 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: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
L
L
Liliana Marie Prikler wrote on 9 Sep 2022 17:56
[PATCH v2] guix: packages: Remove #f from inputs when sanitizing.
(address . 57704@debbugs.gnu.org)(name . Maxime Devos)(address . maximedevos@telenet.be)
bef2c9a9c67b2f6d6c50aa7f9fab3f263290d1b2.camel@gmail.com
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(-)

Toggle diff (28 lines)
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
M
M
Maxime Devos wrote on 10 Sep 2022 02:33
7c897510-3362-2ca1-641a-2728c013678f@telenet.be
On 09-09-2022 17:56, Liliana Marie Prikler wrote:
Toggle quote (8 lines)
> 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: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
L
L
Liliana Marie Prikler wrote on 10 Sep 2022 08:40
d53c94cb177679260842edc30212c245dea18bdb.camel@gmail.com
Am Samstag, dem 10.09.2022 um 02:33 +0200 schrieb Maxime Devos:
Toggle quote (3 lines)
> 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.

Toggle quote (5 lines)
> 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
L
L
Liliana Marie Prikler wrote on 10 Sep 2022 09:44
6c48d1c7aaad8562eeabb6b7a265530ebb39b26d.camel@gmail.com
Am Samstag, dem 10.09.2022 um 08:40 +0200 schrieb Liliana Marie
Prikler:
Toggle quote (3 lines)
> 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
L
L
Liliana Marie Prikler wrote on 10 Sep 2022 09:41
[PATCH v3] guix: Filter unspecified inputs when sanitizing.
(address . 57704@debbugs.gnu.org)(name . Maxime Devos)(address . maximedevos@telenet.be)
1b98ce69769d0366503c7fb7f956da7fa9ec5132.camel@gmail.com
* 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(-)

Toggle diff (24 lines)
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
M
M
Maxime Devos wrote on 10 Sep 2022 12:19
d3341dcd-fee3-204d-7949-6df5333bc5ea@telenet.be
Toggle quote (6 lines)
> 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:
Toggle quote (6 lines)
> 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
There is an interest in letting it return zero values instead of
*unspecified*, see e.g.
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: OpenPGP_0x49E3EE22191725EE.asc
Attachment: OpenPGP_signature
L
L
Ludovic Courtès wrote on 26 Sep 2022 22:51
Re: bug#57704: [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)(address . 57704@debbugs.gnu.org)
87bkr1kgf4.fsf@gnu.org
Hi Liliana,

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

Toggle quote (3 lines)
> 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’.
C
C
Christopher Baines wrote on 6 Oct 2022 15:29
control message for bug #57704
(address . control@debbugs.gnu.org)
875ygx3wrw.fsf@cbaines.net
tags 57704 + moreinfo
quit
M
M
Maxim Cournoyer wrote on 20 Jan 21:43 +0100
Re: bug#57704: [PATCH core-updates] guix: packages: Remove #f from inputs when sanitizing.
(name . Liliana Marie Prikler)(address . liliana.prikler@gmail.com)
877ck3enwm.fsf_-_@gmail.com
Hi Liliana,

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

Toggle quote (20 lines)
> 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
?
Your comment

Commenting via the web interface is currently disabled.

To comment on this conversation send an email to 57704@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 57704
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch