GNU bug report logs - #43064
[PATCH] gexp: computed-file: Prevent mistakenly overriding default option values.

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 43064 in the body.
You can then email your comments to 43064 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH] gexp: computed-file: Prevent mistakenly overriding default
 option values.
Date: Thu, 27 Aug 2020 01:12:26 -0400

Because the options are passed as a list rather than actual keyword arguments,
omitting to repeat the default values in that list easily causes the default
values to be lost.

* guix/gexp.scm (%computed-file-default-options): New variable.
(alist->plist): New procedure.
(computed-file-combine-options-with-defaults): New procedure.
(computed-file): Use the above procedures to form the default OPTIONS value.
Update doc.  Use the COMPUTED-FILE-COMBINE-OPTIONS-WITH-DEFAULTS procedure to
combine the user options with the default options, when they aren't
overridden.
* tests/gexp.scm ("computed-file options defaults honored")
("computed-file options defaults overridden"): Add tests.
---
 guix/gexp.scm  | 42 +++++++++++++++++++++++++++++++++++++++---
 tests/gexp.scm | 12 ++++++++++++
 2 files changed, 51 insertions(+), 3 deletions(-)

diff --git a/guix/gexp.scm b/guix/gexp.scm
index 67b6121313..14e07e8fe6 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2018 Clément Lassieur <clement <at> lassieur.org>
 ;;; Copyright © 2018 Jan Nieuwenhuizen <janneke <at> gnu.org>
 ;;; Copyright © 2019, 2020 Mathieu Othacehe <m.othacehe <at> gmail.com>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -503,14 +504,49 @@ This is the declarative counterpart of 'text-file'."
   (guile      computed-file-guile)                ;<package>
   (options    computed-file-options))             ;list of arguments
 
+;;; Alist containing the default options for computed-file.
+(define %computed-file-default-options '((#:local-build? . #t)))
+
+(define (alist->plist alist)
+  "Transform an association list into a property list."
+  (fold (lambda (current acc)
+          (match current
+            ((x . y)
+             (append acc (list x y)))))
+        '()
+        alist))
+
+(define (computed-file-combine-options-with-defaults options)
+
+  (define alist->keys
+    (match-lambda
+      (((key . value) ...)
+       key)))
+
+  (define (plist->keys plist)
+    (filter keyword? plist))
+
+  (define (default-keys->plist keys)
+    (append-map (lambda (key)
+                  (list key (assq-ref %computed-file-default-options key)))
+                keys))
+
+  (let ((default-keys (lset-difference
+                       eq?
+                       (alist->keys %computed-file-default-options)
+                       (plist->keys options))))
+    (append options (default-keys->plist default-keys))))
+
 (define* (computed-file name gexp
-                        #:key guile (options '(#:local-build? #t)))
+                        #:key guile (options (alist->plist
+                                              %computed-file-default-options)))
   "Return an object representing the store item NAME, a file or directory
 computed by GEXP.  OPTIONS is a list of additional arguments to pass
-to 'gexp->derivation'.
+to 'gexp->derivation', which defaults to %COMPUTED-FILE-DEFAULT-OPTIONS.
 
 This is the declarative counterpart of 'gexp->derivation'."
-  (%computed-file name gexp guile options))
+  (let ((options* (computed-file-combine-options-with-defaults options)))
+    (%computed-file name gexp guile options*)))
 
 (define-gexp-compiler (computed-file-compiler (file <computed-file>)
                                               system target)
diff --git a/tests/gexp.scm b/tests/gexp.scm
index 1beeb67c21..350065b58d 100644
--- a/tests/gexp.scm
+++ b/tests/gexp.scm
@@ -1,5 +1,6 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2014, 2015, 2016, 2017, 2018, 2019, 2020 Ludovic Courtès <ludo <at> gnu.org>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -62,6 +63,9 @@
                                      #:target target)
                   #:guile-for-build (%guile-for-build)))
 
+(define computed-file-combine-options-with-defaults
+  (@@ (guix gexp) computed-file-combine-options-with-defaults))
+
 (define %extension-package
   ;; Example of a package to use when testing 'with-extensions'.
   (dummy-package "extension"
@@ -1367,6 +1371,14 @@
     (return (and (derivation? drv1) (derivation? drv2)
                  (store-path? item)))))
 
+(test-equal "computed-file options defaults honored"
+  '(#:substitutable? #t #:local-build? #t)
+  (computed-file-combine-options-with-defaults '(#:substitutable? #t)))
+
+(test-equal "computed-file options defaults overridden"
+  '(#:local-build? #f)
+  (computed-file-combine-options-with-defaults '(#:local-build? #f)))
+
 (test-assertm "lower-object, computed-file"
   (let* ((text     (plain-file "foo" "Hello!"))
          (exp      #~(begin
-- 
2.27.0

Message #8 received at 43064 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 43064 <at> debbugs.gnu.org
Cc: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH] Revert "system: image: Do not offload image files."
Date: Thu, 27 Aug 2020 01:21:22 -0400

This reverts commit 6a9581741e4ee81226aeb2f1c997df76670a6aab, which is
obsoleted by the previous commit.
---
 gnu/system/image.scm | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/gnu/system/image.scm b/gnu/system/image.scm
index c1a718d607..36f56e237d 100644
--- a/gnu/system/image.scm
+++ b/gnu/system/image.scm
@@ -266,8 +266,7 @@ used in the image."
                                        #$output
                                        image-root)))))
         (computed-file "partition.img" image-builder
-                       #:options `(#:local-build? #t ;typically large file
-                                   #:references-graphs ,inputs))))
+                       #:options `(#:references-graphs ,inputs))))
 
     (define (partition->config partition)
       ;; Return the genimage partition configuration for PARTITION.
@@ -325,8 +324,7 @@ image ~a {
                    #~(symlink
                       (string-append #$image-dir "/" #$genimage-name)
                       #$output)
-                   #:options `(#:local-build? #t ;typically large file
-                               #:substitutable? ,substitutable?))))
+                   #:options `(#:substitutable? ,substitutable?))))
 
 
 ;;
@@ -403,8 +401,7 @@ used in the image. "
                                  #:volume-id #$root-label
                                  #:volume-uuid #$root-uuid)))))
     (computed-file name builder
-                   #:options `(#:local-build? #t ;typically large file
-                               #:references-graphs ,inputs
+                   #:options `(#:references-graphs ,inputs
                                #:substitutable? ,substitutable?))))
 
 
-- 
2.27.0

Message #11 received at 43064 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 43064 <at> debbugs.gnu.org
Subject: Re: [bug#43064] [PATCH] gexp: computed-file: Prevent mistakenly
 overriding default option values.
Date: Sun, 30 Aug 2020 21:41:11 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> Because the options are passed as a list rather than actual keyword arguments,
> omitting to repeat the default values in that list easily causes the default
> values to be lost.
>
> * guix/gexp.scm (%computed-file-default-options): New variable.
> (alist->plist): New procedure.
> (computed-file-combine-options-with-defaults): New procedure.
> (computed-file): Use the above procedures to form the default OPTIONS value.
> Update doc.  Use the COMPUTED-FILE-COMBINE-OPTIONS-WITH-DEFAULTS procedure to
> combine the user options with the default options, when they aren't
> overridden.
> * tests/gexp.scm ("computed-file options defaults honored")
> ("computed-file options defaults overridden"): Add tests.

How about exposing some of the relevant options as keywords?  We can
keep #:options as an “escape hatch” and add things like like
#:local-build? etc.  That would be simpler and more idiomatic.

Thanks,
Ludo’.

Message #14 received at 43064 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 43064 <at> debbugs.gnu.org
Cc: Ludovic Courtès <ludo <at> gnu.org>,
 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Subject: [PATCH v2] gexp: computed-file: Prevent mistakenly overriding default
 option values.
Date: Mon, 31 Aug 2020 00:37:44 -0400

In order to do so, default to an empty options list, and expose options whose
default values are sensitive directly as keyword arguments.

* guix/gexp.scm (computed-file): Extract the LOCAL-BUILD? parameter from the
OPTIONS parameter to make it a stand-alone keyword argument.  Introduce an
OPTIONS* binding which is obtained by combining the LOCAL-BUILD? keyword and
its value with OPTIONS.

Suggested-by: Ludovic Courtès <ludo <at> gnu.org>
---
 guix/gexp.scm | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/guix/gexp.scm b/guix/gexp.scm
index 67b6121313..9d3c52e783 100644
--- a/guix/gexp.scm
+++ b/guix/gexp.scm
@@ -3,6 +3,7 @@
 ;;; Copyright © 2018 Clément Lassieur <clement <at> lassieur.org>
 ;;; Copyright © 2018 Jan Nieuwenhuizen <janneke <at> gnu.org>
 ;;; Copyright © 2019, 2020 Mathieu Othacehe <m.othacehe <at> gmail.com>
+;;; Copyright © 2020 Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -504,13 +505,15 @@ This is the declarative counterpart of 'text-file'."
   (options    computed-file-options))             ;list of arguments
 
 (define* (computed-file name gexp
-                        #:key guile (options '(#:local-build? #t)))
+                        #:key guile (local-build? #t) (options '()))
   "Return an object representing the store item NAME, a file or directory
-computed by GEXP.  OPTIONS is a list of additional arguments to pass
-to 'gexp->derivation'.
+computed by GEXP.  When LOCAL-BUILD? is #t (the default), it ensures the
+corresponding derivation is built locally.  OPTIONS may be used to pass
+additional arguments to 'gexp->derivation'.
 
 This is the declarative counterpart of 'gexp->derivation'."
-  (%computed-file name gexp guile options))
+  (let ((options* `(#:local-build? ,local-build? ,@options)))
+    (%computed-file name gexp guile options*)))
 
 (define-gexp-compiler (computed-file-compiler (file <computed-file>)
                                               system target)
-- 
2.27.0

Message #17 received at 43064 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 43064 <at> debbugs.gnu.org
Subject: Re: [PATCH v2] gexp: computed-file: Prevent mistakenly overriding
 default option values.
Date: Mon, 31 Aug 2020 15:34:46 +0200

Hi,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> In order to do so, default to an empty options list, and expose options whose
> default values are sensitive directly as keyword arguments.
>
> * guix/gexp.scm (computed-file): Extract the LOCAL-BUILD? parameter from the
> OPTIONS parameter to make it a stand-alone keyword argument.  Introduce an
> OPTIONS* binding which is obtained by combining the LOCAL-BUILD? keyword and
> its value with OPTIONS.
>
> Suggested-by: Ludovic Courtès <ludo <at> gnu.org>

[...]

>  (define* (computed-file name gexp
> -                        #:key guile (options '(#:local-build? #t)))
> +                        #:key guile (local-build? #t) (options '()))
>    "Return an object representing the store item NAME, a file or directory
> -computed by GEXP.  OPTIONS is a list of additional arguments to pass
> -to 'gexp->derivation'.
> +computed by GEXP.  When LOCAL-BUILD? is #t (the default), it ensures the
> +corresponding derivation is built locally.  OPTIONS may be used to pass
> +additional arguments to 'gexp->derivation'.
>  
>  This is the declarative counterpart of 'gexp->derivation'."

Please update doc/guix.texi as well.

Otherwise LGTM, thanks!

Ludo’.

Message #22 received at 43064-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 43064-done <at> debbugs.gnu.org
Subject: Re: [PATCH v2] gexp: computed-file: Prevent mistakenly overriding
 default option values.
Date: Tue, 01 Sep 2020 09:00:12 -0400

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

> Hi,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> In order to do so, default to an empty options list, and expose options whose
>> default values are sensitive directly as keyword arguments.
>>
>> * guix/gexp.scm (computed-file): Extract the LOCAL-BUILD? parameter from the
>> OPTIONS parameter to make it a stand-alone keyword argument.  Introduce an
>> OPTIONS* binding which is obtained by combining the LOCAL-BUILD? keyword and
>> its value with OPTIONS.
>>
>> Suggested-by: Ludovic Courtès <ludo <at> gnu.org>

[...]

> Please update doc/guix.texi as well.
>
> Otherwise LGTM, thanks!
>
> Ludo’.

Done!

Thank you,

Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.