[PATCH] guix: scripts: Fix GUIX_BUILD_OPTIONS handling.

[PATCH] guix: scripts: Fix GUIX_BUILD_OPTIONS handling.

[Deck Pickard]

[-- Attachment #1.1: Type: text/plain, Size: 100 bytes --]

Perhaps, now it WILL twerk.

Drp,
-- 
(or ((,\ (x) `(,x x)) '(,\ (x) `(,x x))) (smth (that 'like)))

[-- Attachment #1.2: Type: text/html, Size: 149 bytes --]

[-- Attachment #2: 0001-guix-scripts-Fix-GUIX_BUILD_OPTIONS-handling.patch --]
[-- Type: application/octet-stream, Size: 4903 bytes --]

From 9c27d995e1a622de8457209d40031b392538e0f8 Mon Sep 17 00:00:00 2001
From: nebuli <nebu@kipple>
Date: Fri, 5 Dec 2014 01:28:12 +0100
Subject: [PATCH] guix: scripts: Fix GUIX_BUILD_OPTIONS handling.

Appending to "raw" args broke optional parameters in 'guix package -I'
and 'guix package -A', and possibly other places.  Therefore, switch to
parsing each set of options on its own and append resulting alists
together afterwards.

* guix/scripts/archive.scm (parse-options-from): Rename from
  (parse-options) and add explicit argument.  New form of (parse-options)
  using its old algorithm via -from function.
* guix/scripts/build.scm: Ditto.
* guix/scripts/environment.scm: Ditto.
* guix/scripts/package.scm: Ditto.
* guix/scripts/system.scm: Ditto.
---
 guix/scripts/archive.scm     | 8 ++++++--
 guix/scripts/build.scm       | 8 ++++++--
 guix/scripts/environment.scm | 9 +++++++--
 guix/scripts/package.scm     | 8 ++++++--
 guix/scripts/system.scm      | 8 ++++++--
 5 files changed, 31 insertions(+), 10 deletions(-)

diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
index 29a3ad1..781ffc5 100644
--- a/guix/scripts/archive.scm
+++ b/guix/scripts/archive.scm
@@ -293,8 +293,12 @@ the input port."
 (define (guix-archive . args)
   (define (parse-options)
     ;; Return the alist of option values.
-    (args-fold* (append args (environment-build-options))
-                %options
+    (append (parse-options-from args)
+            (parse-options-from (environment-build-options))))
+
+  (define (parse-options-from args)
+    ;; Actual parsing takes place here.
+    (args-fold* args %options
                 (lambda (opt name arg result)
                   (leave (_ "~A: unrecognized option~%") name))
                 (lambda (arg result)
diff --git a/guix/scripts/build.scm b/guix/scripts/build.scm
index 76a743f..26e9f42 100644
--- a/guix/scripts/build.scm
+++ b/guix/scripts/build.scm
@@ -401,8 +401,12 @@ arguments with packages that use the specified source."
 (define (guix-build . args)
   (define (parse-options)
     ;; Return the alist of option values.
-    (args-fold* (append args (environment-build-options))
-                %options
+    (append (parse-options-from args)
+            (parse-options-from (environment-build-options))))
+
+  (define (parse-options-from args)
+    ;; Actual parsing takes place here.
+    (args-fold* args %options
                 (lambda (opt name arg result)
                   (leave (_ "~A: unrecognized option~%") name))
                 (lambda (arg result)
diff --git a/guix/scripts/environment.scm b/guix/scripts/environment.scm
index a309dfa..c388b0c 100644
--- a/guix/scripts/environment.scm
+++ b/guix/scripts/environment.scm
@@ -213,8 +213,13 @@ packages."
 ;; Entry point.
 (define (guix-environment . args)
   (define (parse-options)
-    (args-fold* (append args (environment-build-options))
-                %options
+    ;; Return the alist of option values.
+    (append (parse-options-from args)
+            (parse-options-from (environment-build-options))))
+
+  (define (parse-options-from args)
+    ;; Actual parsing takes place here.
+    (args-fold* args %options
                 (lambda (opt name arg result)
                   (leave (_ "~A: unrecognized option~%") name))
                 (lambda (arg result)
diff --git a/guix/scripts/package.scm b/guix/scripts/package.scm
index 9ff4d17..21dc66c 100644
--- a/guix/scripts/package.scm
+++ b/guix/scripts/package.scm
@@ -668,8 +668,12 @@ removed from MANIFEST."
 (define (guix-package . args)
   (define (parse-options)
     ;; Return the alist of option values.
-    (args-fold* (append args (environment-build-options))
-                %options
+    (append (parse-options-from args)
+            (parse-options-from (environment-build-options))))
+
+  (define (parse-options-from args)
+    ;; Actual parsing takes place here.
+    (args-fold* args %options
                 (lambda (opt name arg result arg-handler)
                   (leave (_ "~A: unrecognized option~%") name))
                 (lambda (arg result arg-handler)
diff --git a/guix/scripts/system.scm b/guix/scripts/system.scm
index 8e049a4..eb987e1 100644
--- a/guix/scripts/system.scm
+++ b/guix/scripts/system.scm
@@ -471,8 +471,12 @@ Build the operating system declared in FILE according to ACTION.\n"))
 (define (guix-system . args)
   (define (parse-options)
     ;; Return the alist of option values.
-    (args-fold* (append args (environment-build-options))
-                %options
+    (append (parse-options-from args)
+            (parse-options-from (environment-build-options))))
+
+  (define (parse-options-from args)
+    ;; Actual parsing takes place here.
+    (args-fold* args %options
                 (lambda (opt name arg result)
                   (leave (_ "~A: unrecognized option~%") name))
                 (lambda (arg result)
-- 
2.1.2

Re: [PATCH] guix: scripts: Fix GUIX_BUILD_OPTIONS handling.

[Ludovic Courtès]

Deck Pickard <deck.r.pickard@gmail.com> skribis:

> From 9c27d995e1a622de8457209d40031b392538e0f8 Mon Sep 17 00:00:00 2001
> From: nebuli <nebu@kipple>
> Date: Fri, 5 Dec 2014 01:28:12 +0100
> Subject: [PATCH] guix: scripts: Fix GUIX_BUILD_OPTIONS handling.
>
> Appending to "raw" args broke optional parameters in 'guix package -I'
> and 'guix package -A', and possibly other places.  Therefore, switch to
> parsing each set of options on its own and append resulting alists
> together afterwards.
>
> * guix/scripts/archive.scm (parse-options-from): Rename from
>   (parse-options) and add explicit argument.  New form of (parse-options)
>   using its old algorithm via -from function.
> * guix/scripts/build.scm: Ditto.
> * guix/scripts/environment.scm: Ditto.
> * guix/scripts/package.scm: Ditto.
> * guix/scripts/system.scm: Ditto.

Good catch.  I’ve applied it along with a test case that reproduces the
problem and a clarification in the doc.

Thanks!

Ludo’.

Bugs in parsing build options

[Alex Kost]

Ludovic Courtès (2014-12-06 18:04 +0300) wrote:

> Deck Pickard <deck.r.pickard@gmail.com> skribis:
>
>> From 9c27d995e1a622de8457209d40031b392538e0f8 Mon Sep 17 00:00:00 2001
>> From: nebuli <nebu@kipple>
>> Date: Fri, 5 Dec 2014 01:28:12 +0100
>> Subject: [PATCH] guix: scripts: Fix GUIX_BUILD_OPTIONS handling.
>>
>> Appending to "raw" args broke optional parameters in 'guix package -I'
>> and 'guix package -A', and possibly other places.  Therefore, switch to
>> parsing each set of options on its own and append resulting alists
>> together afterwards.
>>
>> * guix/scripts/archive.scm (parse-options-from): Rename from
>>   (parse-options) and add explicit argument.  New form of (parse-options)
>>   using its old algorithm via -from function.
>> * guix/scripts/build.scm: Ditto.
>> * guix/scripts/environment.scm: Ditto.
>> * guix/scripts/package.scm: Ditto.
>> * guix/scripts/system.scm: Ditto.
>
> Good catch.  I’ve applied it along with a test case that reproduces the
> problem and a clarification in the doc.

Hello, I think I found a problem with this commit.

I noticed that "--no-grub" option has no effect.  For example,
"guix system --no-grub reconfigure" installs GRUB anyway.

Further investigation showed that there is a problem in parsing build
options.  After that commit (847391f) we have the following code:

    (append (parse-options-from args)
            (parse-options-from (environment-build-options)))

in several places.  But 'parse-options-from' returns default values for
unspecified options, e.g. if you didn't set GUIX_BUILD_OPTIONS, then
(parse-options-from '()) will return an alist of default options
(including ‘(install-grub? . #t)’).

So build commands will just ignore such options as "--no-grub" or
"--no-substitutes" if a user didn't tweak GUIX_BUILD_OPTIONS.

-- 
Alex

Re: Bugs in parsing build options

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> I noticed that "--no-grub" option has no effect.  For example,
> "guix system --no-grub reconfigure" installs GRUB anyway.
>
> Further investigation showed that there is a problem in parsing build
> options.  After that commit (847391f) we have the following code:
>
>     (append (parse-options-from args)
>             (parse-options-from (environment-build-options)))
>
> in several places.  But 'parse-options-from' returns default values for
> unspecified options, e.g. if you didn't set GUIX_BUILD_OPTIONS, then
> (parse-options-from '()) will return an alist of default options
> (including ‘(install-grub? . #t)’).

Indeed, good catch.  Commit 6e1a7d1 fixes it.

> So build commands will just ignore such options as "--no-grub" or
> "--no-substitutes" if a user didn't tweak GUIX_BUILD_OPTIONS.

The problem was in fact specific to the option handler for --no-grub;
the one for --no-substitutes (and all the others I checked) always
conses, so there’s no problem.

Thanks,
Ludo’.

Re: Bugs in parsing build options

[Alex Kost]

Ludovic Courtès (2015-02-25 01:45 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> I noticed that "--no-grub" option has no effect.  For example,
>> "guix system --no-grub reconfigure" installs GRUB anyway.
>>
>> Further investigation showed that there is a problem in parsing build
>> options.  After that commit (847391f) we have the following code:
>>
>>     (append (parse-options-from args)
>>             (parse-options-from (environment-build-options)))
>>
>> in several places.  But 'parse-options-from' returns default values for
>> unspecified options, e.g. if you didn't set GUIX_BUILD_OPTIONS, then
>> (parse-options-from '()) will return an alist of default options
>> (including ‘(install-grub? . #t)’).
>
> Indeed, good catch.  Commit 6e1a7d1 fixes it.

Not really, I'm afraid.

>> So build commands will just ignore such options as "--no-grub" or
>> "--no-substitutes" if a user didn't tweak GUIX_BUILD_OPTIONS.
>
> The problem was in fact specific to the option handler for --no-grub;
> the one for --no-substitutes (and all the others I checked) always
> conses, so there’s no problem.

Yes, I realized that, but I believe the real problem lies deeper – it is
in the current way of parsing build options in general (sorry, I should
have written more about that initially).

Suppose a user specifies "--no-substitutes" in his GUIX_BUILD_OPTIONS
and then he calls "guix system reconfigure".  What would happen?
Substitutes _will be used_ anyway, because:
‘(parse-options-from args)’ will contain ‘(substitutes? . #t)’ among
other things and it will shadow the false value for substitutes returned
by ‘(parse-options-from (environment-build-options))’.

Now (after your patch) the same will happen with "--no-grub":
“export GUIX_BUILD_OPTIONS=--no-grub” will not be honored unless a user
explicitly specifies "--no-grub" option one more time in a "guix system"
command.

That's why I didn't propose a patch initially: I don't really know what
would be the best way to fix all that.

-- 
Alex

Re: Bugs in parsing build options

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> Suppose a user specifies "--no-substitutes" in his GUIX_BUILD_OPTIONS
> and then he calls "guix system reconfigure".  What would happen?
> Substitutes _will be used_ anyway, because:
> ‘(parse-options-from args)’ will contain ‘(substitutes? . #t)’ among
> other things and it will shadow the false value for substitutes returned
> by ‘(parse-options-from (environment-build-options))’.
>
> Now (after your patch) the same will happen with "--no-grub":
> “export GUIX_BUILD_OPTIONS=--no-grub” will not be honored unless a user
> explicitly specifies "--no-grub" option one more time in a "guix system"
> command.

Oh, that’s right.  AFAICS, commit cf6ce3e fixes it.

It was a good opportunity to factorize all that and to add tests.

Thanks!

Ludo’.

Re: Bugs in parsing build options

[Alex Kost]

Ludovic Courtès (2015-02-26 02:05 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> Suppose a user specifies "--no-substitutes" in his GUIX_BUILD_OPTIONS
>> and then he calls "guix system reconfigure".  What would happen?
>> Substitutes _will be used_ anyway, because:
>> ‘(parse-options-from args)’ will contain ‘(substitutes? . #t)’ among
>> other things and it will shadow the false value for substitutes returned
>> by ‘(parse-options-from (environment-build-options))’.
>>
>> Now (after your patch) the same will happen with "--no-grub":
>> “export GUIX_BUILD_OPTIONS=--no-grub” will not be honored unless a user
>> explicitly specifies "--no-grub" option one more time in a "guix system"
>> command.
>
> Oh, that’s right.  AFAICS, commit cf6ce3e fixes it.

Yes, thank you, now I'm satisfied :-)

> It was a good opportunity to factorize all that and to add tests.
>
> Thanks!
>
> Ludo’.

Thank YOU for being so productive!

-- 
Alex

Re: Bugs in parsing build options

[Alex Kost]

Ludovic Courtès (2015-02-26 02:05 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> Suppose a user specifies "--no-substitutes" in his GUIX_BUILD_OPTIONS
>> and then he calls "guix system reconfigure".  What would happen?
>> Substitutes _will be used_ anyway, because:
>> ‘(parse-options-from args)’ will contain ‘(substitutes? . #t)’ among
>> other things and it will shadow the false value for substitutes returned
>> by ‘(parse-options-from (environment-build-options))’.
>>
>> Now (after your patch) the same will happen with "--no-grub":
>> “export GUIX_BUILD_OPTIONS=--no-grub” will not be honored unless a user
>> explicitly specifies "--no-grub" option one more time in a "guix system"
>> command.
>
> Oh, that’s right.  AFAICS, commit cf6ce3e fixes it.
>
> It was a good opportunity to factorize all that and to add tests.

I think I've found that there are still some problems in parsing options.

1. The following command:

  GUIX_BUILD_OPTIONS="--no-grub" guix package --help

gives the following error:

  guix package: error: no-grub: unrecognized option

So it makes impossible to use GUIX_BUILD_OPTIONS env var or did I miss
anything?

2. The manual says that ‘guix package’ «supports all the common build
options that ‘guix build’ supports» and the following command works
perfectly:

  guix build --system=i686-linux hello

However, the following command:

  guix package --system=i686-linux --install hello

gives an error again:

  guix package: error: system=i686-linux: unrecognized option

-- 
Alex

Re: Bugs in parsing build options

[Ludovic Courtès]

Alex Kost <alezost@gmail.com> skribis:

> 1. The following command:
>
>   GUIX_BUILD_OPTIONS="--no-grub" guix package --help
>
> gives the following error:
>
>   guix package: error: no-grub: unrecognized option
>
> So it makes impossible to use GUIX_BUILD_OPTIONS env var or did I miss
> anything?

GUIX_BUILD_OPTIONS is intended for options that related to building,
namely those listed in ‘%standard-build-options’ in (guix scripts
build).

Conversely, ‘--no-grub’ is specific to ‘guix system’.

> 2. The manual says that ‘guix package’ «supports all the common build
> options that ‘guix build’ supports» and the following command works
> perfectly:
>
>   guix build --system=i686-linux hello
>
> However, the following command:
>
>   guix package --system=i686-linux --install hello
>
> gives an error again:
>
>   guix package: error: system=i686-linux: unrecognized option

Same issue: the manual is somewhat vague, but ‘--system’ is not in
‘%standard-build-options’.

‘--system’ was intentionally not added to ‘guix package’ on the grounds
that it wouldn’t be very useful to install binaries built for another
system in a profile.

Now, I agree that the situation is different on “multi-personality”
systems, so maybe we should revise that choice?

Thoughts?

Ludo’.

Re: Bugs in parsing build options

[Alex Kost]

Ludovic Courtès (2015-05-31 22:32 +0300) wrote:

> Alex Kost <alezost@gmail.com> skribis:
>
>> 1. The following command:
>>
>>   GUIX_BUILD_OPTIONS="--no-grub" guix package --help
>>
>> gives the following error:
>>
>>   guix package: error: no-grub: unrecognized option
>>
>> So it makes impossible to use GUIX_BUILD_OPTIONS env var or did I miss
>> anything?
>
> GUIX_BUILD_OPTIONS is intended for options that related to building,
> namely those listed in ‘%standard-build-options’ in (guix scripts
> build).
>
> Conversely, ‘--no-grub’ is specific to ‘guix system’.

Ah, indeed!  I missed that GUIX_BUILD_OPTIONS is intended to have only
‘build’ options, not ‘system’ ones, sorry.

>> 2. The manual says that ‘guix package’ «supports all the common build
>> options that ‘guix build’ supports» and the following command works
>> perfectly:
>>
>>   guix build --system=i686-linux hello
>>
>> However, the following command:
>>
>>   guix package --system=i686-linux --install hello
>>
>> gives an error again:
>>
>>   guix package: error: system=i686-linux: unrecognized option
>
> Same issue: the manual is somewhat vague, but ‘--system’ is not in
> ‘%standard-build-options’.
>
> ‘--system’ was intentionally not added to ‘guix package’ on the grounds
> that it wouldn’t be very useful to install binaries built for another
> system in a profile.
>
> Now, I agree that the situation is different on “multi-personality”
> systems, so maybe we should revise that choice?
>
> Thoughts?

I believe that ‘guix package’ should support ‘--system’ option.  At
least I have a need to install some packages of another system to my
profile.

-- 
Alex