GNU bug report logs - #66592
[PATCH] scripts: archive: Check compatibility of command line options.

Previous Next

To reply to this bug, email your comments to 66592 AT debbugs.gnu.org.

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

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

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: guix-patches <at> gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH] scripts: archive: Check compatibility of command line options.
Date: Tue, 17 Oct 2023 15:28:56 +0200

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.

* guix/scripts/archive.scm (compatible-option): New procedure.
(%options): Use it.
---
 guix/scripts/archive.scm | 23 ++++++++++++++++++++---
 1 file changed, 20 insertions(+), 3 deletions(-)

diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
index 2b5a55a23f..a1b2b73a0f 100644
--- a/guix/scripts/archive.scm
+++ b/guix/scripts/archive.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -115,6 +116,22 @@ (define %key-generation-parameters
       "(genkey (ecdsa (curve Ed25519) (flags rfc6979)))"
       "(genkey (rsa (nbits 4:4096)))"))
 
+(define* (compatible-option result
+                            #:optional
+                            (actions (list 'authorize 'export 'import)))
+  "Return the RESULT if it is compatible with the list of ACTIONS."
+  (let ((some-actions (fold (lambda (action answers)
+                              (if (assoc-ref result action)
+                                  (cons action answers)
+                                  answers))
+                            '()
+                            actions)))
+    (match some-actions
+      ((action)
+       result)
+      ((action other-action _ ...)
+       (leave (G_ "ambiguous options: ~s with ~s~%") action other-action)))))
+
 (define %options
   ;; Specifications of the command-line options.
   (cons* (option '(#\h "help") #f #f
@@ -126,13 +143,13 @@ (define %options
                    (show-version-and-exit "guix archive")))
          (option '("export") #f #f
                  (lambda (opt name arg result)
-                   (alist-cons 'export #t result)))
+                   (compatible-option (alist-cons 'export #t result))))
          (option '(#\r "recursive") #f #f
                  (lambda (opt name arg result)
                    (alist-cons 'export-recursive? #t result)))
          (option '("import") #f #f
                  (lambda (opt name arg result)
-                   (alist-cons 'import #t result)))
+                   (compatible-option (alist-cons 'import #t result))))
          (option '("missing") #f #f
                  (lambda (opt name arg result)
                    (alist-cons 'missing #t result)))
@@ -158,7 +175,7 @@ (define %options
                               (error-string err))))))
          (option '("authorize") #f #f
                  (lambda (opt name arg result)
-                   (alist-cons 'authorize #t result)))
+                   (compatible-option (alist-cons 'authorize #t result))))
 
          (option '(#\S "source") #f #f
                  (lambda (opt name arg result)

base-commit: dcc5c34504c94732c135a85fb4db40ca9796270e
-- 
2.38.1

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

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 66592 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH v2] scripts: archive: Check compatibility of command line
 options.
Date: Tue, 24 Oct 2023 18:33:10 +0200

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.

* guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
and use it.
---
 guix/scripts/archive.scm | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
index 2b5a55a23f..466aa9c4d7 100644
--- a/guix/scripts/archive.scm
+++ b/guix/scripts/archive.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -375,8 +376,24 @@ (define-command (guix-archive . args)
           (loop (read-line port)
                 (cons line result)))))
 
+  (define* (compatible-option options #:key actions)
+  "Return the OPTIONS if it is compatible with the list of ACTIONS."
+  (let ((some-actions (fold (lambda (action answers)
+                              (if (assoc-ref options action)
+                                  (cons action answers)
+                                  answers))
+                            '()
+                            actions)))
+    (match some-actions
+      ((action)
+       options)
+      ((action other-actions ...)
+       (leave (G_ "the options ~{'~s' ~}are exclusive~%") some-actions)))))
+
   (with-error-handling
-    (let ((opts (parse-command-line args %options (list %default-options))))
+    (let* ((opts (parse-command-line args %options (list %default-options)))
+           (opts (compatible-option opts
+                                    #:actions (list 'authorize 'export 'import))))
       (parameterize ((%graft? (assoc-ref opts 'graft?)))
         (cond ((assoc-ref opts 'generate-key)
                =>

base-commit: f3714b3d5f51aced4b31447c42d5e89c75e3079f
-- 
2.38.1

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: 66592 <at> debbugs.gnu.org
Subject: Re: [bug#66592] [PATCH v2] scripts: archive: Check compatibility of
 command line options.
Date: Tue, 31 Oct 2023 14:01:07 -0400

Hi Simon,

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

> Fixes <https://issues.guix.gnu.org/66358>.
> Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.
>
> * guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
> and use it.
> ---
>  guix/scripts/archive.scm | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
> index 2b5a55a23f..466aa9c4d7 100644
> --- a/guix/scripts/archive.scm
> +++ b/guix/scripts/archive.scm
> @@ -1,6 +1,7 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
>  ;;; Copyright © 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
> +;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -375,8 +376,24 @@ (define-command (guix-archive . args)
>            (loop (read-line port)
>                  (cons line result)))))
>  
> +  (define* (compatible-option options #:key actions)
> +  "Return the OPTIONS if it is compatible with the list of ACTIONS."
> +  (let ((some-actions (fold (lambda (action answers)
> +                              (if (assoc-ref options action)
> +                                  (cons action answers)
> +                                  answers))
> +                            '()
> +                            actions)))
> +    (match some-actions
> +      ((action)
> +       options)
> +      ((action other-actions ...)
> +       (leave (G_ "the options ~{'~s' ~}are exclusive~%") some-actions)))))
> +
>    (with-error-handling
> -    (let ((opts (parse-command-line args %options (list %default-options))))
> +    (let* ((opts (parse-command-line args %options (list %default-options)))
> +           (opts (compatible-option opts
> +                                    #:actions (list 'authorize 'export 'import))))
>        (parameterize ((%graft? (assoc-ref opts 'graft?)))
>          (cond ((assoc-ref opts 'generate-key)
>                 =>

Looks good from a cursory look, but it seems a nice to test in
tests/guix-archive.sh.

Could you please send a v3 with some tests for it?

-- 
Thanks,
Maxim

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

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 66592 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH v3] scripts: archive: Check compatibility of command line
 options.
Date: Thu, 30 Nov 2023 11:46:13 +0100

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.

* guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
and use it.
* tests/guix-archive.sh: Test it.
---
 guix/scripts/archive.scm | 18 +++++++++++++++++-
 tests/guix-archive.sh    |  5 +++++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
index 2b5a55a23f..b9cf78f981 100644
--- a/guix/scripts/archive.scm
+++ b/guix/scripts/archive.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -375,8 +376,23 @@ (define-command (guix-archive . args)
           (loop (read-line port)
                 (cons line result)))))
 
+  (define* (compatible-option options #:key actions)
+  "Return the OPTIONS if it is compatible with the list of ACTIONS."
+  (let ((some-actions (filter (lambda (action)
+                                (assoc-ref options action))
+                              actions)))
+    (match some-actions
+      ((action)
+       options)
+      ((action other-actions ...)
+       (leave (G_ "the options ~{'~s' ~}are exclusive~%") some-actions))
+      (_
+       options))))
+
   (with-error-handling
-    (let ((opts (parse-command-line args %options (list %default-options))))
+    (let* ((opts (parse-command-line args %options (list %default-options)))
+           (opts (compatible-option opts
+                                    #:actions (list 'authorize 'export 'import))))
       (parameterize ((%graft? (assoc-ref opts 'graft?)))
         (cond ((assoc-ref opts 'generate-key)
                =>
diff --git a/tests/guix-archive.sh b/tests/guix-archive.sh
index 0866b5a4d8..08c07684ad 100644
--- a/tests/guix-archive.sh
+++ b/tests/guix-archive.sh
@@ -79,4 +79,9 @@ guix archive -t < "$archive" | grep "^r /share/guile.*/boot-9\.scm"
 
 echo foo | guix archive --authorize && false
 
+# Check incompatible command-line options
+guix archive --authorize --export --import && false
+guix archive --export guile-bootstrap --authorize > "$archive" && false
+guix archive --authorize --import < "$archive" && false
+
 exit 0

base-commit: cd46757c1a0f886848fbb6828c028dd2a2532767
-- 
2.41.0

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

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 66592 <at> debbugs.gnu.org
Subject: Re: [bug#66592] [PATCH v2] scripts: archive: Check compatibility of
 command line options.
Date: Thu, 30 Nov 2023 11:46:55 +0100

Hi,

On mar., 31 oct. 2023 at 14:01, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote:

> Could you please send a v3 with some tests for it?

Done.  WDYT?

Cheers,
simon

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

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>, 66592 <at> debbugs.gnu.org
Subject: Re: [bug#66592] [PATCH v3] scripts: archive: Check compatibility of
 command line options.
Date: Mon, 04 Dec 2023 17:31:45 -0500

Hi,

Simon Tournier <zimon.toutoune <at> gmail.com> writes:

> Fixes <https://issues.guix.gnu.org/66358>.
> Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.
>
> * guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
> and use it.
> * tests/guix-archive.sh: Test it.
> ---
>  guix/scripts/archive.scm | 18 +++++++++++++++++-
>  tests/guix-archive.sh    |  5 +++++
>  2 files changed, 22 insertions(+), 1 deletion(-)
>
> diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
> index 2b5a55a23f..b9cf78f981 100644
> --- a/guix/scripts/archive.scm
> +++ b/guix/scripts/archive.scm
> @@ -1,6 +1,7 @@
>  ;;; GNU Guix --- Functional package management for GNU
>  ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
>  ;;; Copyright © 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
> +;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
>  ;;;
>  ;;; This file is part of GNU Guix.
>  ;;;
> @@ -375,8 +376,23 @@ (define-command (guix-archive . args)
>            (loop (read-line port)
>                  (cons line result)))))
>  
> +  (define* (compatible-option options #:key actions)
> +  "Return the OPTIONS if it is compatible with the list of ACTIONS."

Sorry for not mentioning this in my first review, but re-reading this
code, I think it should be named like: (check-compatibility options
actions).  There's no point making actions an optional argument since
the only point of using this procedure is when you have actions to check
compatibility with, right?

The docstring should mention that it exits with a user message when the
compatibility validation fails.

> +  (let ((some-actions (filter (lambda (action)
> +                                (assoc-ref options action))
> +                              actions)))
> +    (match some-actions
> +      ((action)
> +       options)
> +      ((action other-actions ...)
> +       (leave (G_ "the options ~{'~s' ~}are exclusive~%") some-actions))
> +      (_
> +       options))))
> +
>    (with-error-handling
> -    (let ((opts (parse-command-line args %options (list %default-options))))
> +    (let* ((opts (parse-command-line args %options (list %default-options)))
> +           (opts (compatible-option opts
> +                                    #:actions (list 'authorize 'export 'import))))
>        (parameterize ((%graft? (assoc-ref opts 'graft?)))
>          (cond ((assoc-ref opts 'generate-key)
>                 =>
> diff --git a/tests/guix-archive.sh b/tests/guix-archive.sh
> index 0866b5a4d8..08c07684ad 100644
> --- a/tests/guix-archive.sh
> +++ b/tests/guix-archive.sh
> @@ -79,4 +79,9 @@ guix archive -t < "$archive" | grep "^r /share/guile.*/boot-9\.scm"
>  
>  echo foo | guix archive --authorize && false
>  
> +# Check incompatible command-line options
> +guix archive --authorize --export --import && false
> +guix archive --export guile-bootstrap --authorize > "$archive" && false
> +guix archive --authorize --import < "$archive" && false

I'm puzzled here, because that's the approach used in commit
37dd69b44511dc73eb04bdebe8d82c9a0386338e, but I don't understand how
these checks work: if the command fails, it won't get to execute the
part after && (&& only proceeds if the left part exited with status 0),
and the test will report a failure even though this is the expected
result, no?

OK, it was explained by Eric in <https://issues.guix.gnu.org/62406#6>:

--8<---------------cut here---------------start------------->8--- 'cmd
&& false' [...] has the desired semantics under 'set -e' [...]

If 'cmd' fails, the return status is ignored by 'set -e', which
considers only the return status of a command following the final '&&'
or '||'.  And because 'cmd' failed the statement short-circuits without
executing the 'false.  Otherwise, if 'cmd' succeeds, the 'false' is
executed and the shell exits immediately.
--8<---------------cut here---------------end--------------->8---

Interesting (yet confusing) shell hack.

Would you mind sending a v4 with the above suggestion?  Then I think
we'd be good.

-- 
Thanks,
Maxim

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

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: Josselin Poiret <dev <at> jpoiret.xyz>, Mathieu Othacehe <othacehe <at> gnu.org>,
 Ludovic Courtès <ludo <at> gnu.org>,
 Tobias Geerinckx-Rice <me <at> tobias.gr>, Ricardo Wurmus <rekado <at> elephly.net>,
 Christopher Baines <guix <at> cbaines.net>, 66592 <at> debbugs.gnu.org
Subject: Re: [bug#66592] [PATCH v3] scripts: archive: Check compatibility of
 command line options.
Date: Fri, 12 Jan 2024 11:16:38 +0100

Hi Maxim,

On Mon, 04 Dec 2023 at 17:31, Maxim Cournoyer <maxim.cournoyer <at> gmail.com> wrote:

>> +  (define* (compatible-option options #:key actions)
>> +  "Return the OPTIONS if it is compatible with the list of ACTIONS."
>
> Sorry for not mentioning this in my first review, but re-reading this
> code, I think it should be named like: (check-compatibility options
> actions).  There's no point making actions an optional argument since
> the only point of using this procedure is when you have actions to check
> compatibility with, right?

Well, the point was not about an optional argument but about a key
argument – I find easier at call-location.  I do not know.

(BTW, I have not raised the issue for other commands, but this
’check-compatibility’ could be also applied.  Let as an exercise for
future potential contributor. ;-))

> Would you mind sending a v4 with the above suggestion?  Then I think
> we'd be good.

I will.


Cheers,
simon

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

From: Simon Tournier <zimon.toutoune <at> gmail.com>
To: 66592 <at> debbugs.gnu.org
Cc: Simon Tournier <zimon.toutoune <at> gmail.com>
Subject: [PATCH v4] scripts: archive: Check compatibility of command line
 options.
Date: Tue, 23 Jul 2024 16:45:09 +0200

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.

* guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
and use it.
* tests/guix-archive.sh: Test it.
---
 guix/scripts/archive.scm | 19 ++++++++++++++++++-
 tests/guix-archive.sh    |  5 +++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/guix/scripts/archive.scm b/guix/scripts/archive.scm
index 2b5a55a23f..29e4d3f7ba 100644
--- a/guix/scripts/archive.scm
+++ b/guix/scripts/archive.scm
@@ -1,6 +1,7 @@
 ;;; GNU Guix --- Functional package management for GNU
 ;;; Copyright © 2013, 2014, 2015, 2016, 2017, 2019, 2020, 2021 Ludovic Courtès <ludo <at> gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me <at> tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune <at> gmail.com>
 ;;;
 ;;; This file is part of GNU Guix.
 ;;;
@@ -375,8 +376,24 @@ (define-command (guix-archive . args)
           (loop (read-line port)
                 (cons line result)))))
 
+  (define* (compatible-option options #:key actions)
+  "Return the OPTIONS if it is compatible with the list of ACTIONS, else exit
+with message."
+  (let ((some-actions (filter (lambda (action)
+                                (assoc-ref options action))
+                              actions)))
+    (match some-actions
+      ((action)
+       options)
+      ((action other-actions ...)
+       (leave (G_ "the options ~{'~s' ~}are exclusive~%") some-actions))
+      (_
+       options))))
+
   (with-error-handling
-    (let ((opts (parse-command-line args %options (list %default-options))))
+    (let* ((opts (parse-command-line args %options (list %default-options)))
+           (opts (compatible-option opts
+                                    #:actions (list 'authorize 'export 'import))))
       (parameterize ((%graft? (assoc-ref opts 'graft?)))
         (cond ((assoc-ref opts 'generate-key)
                =>
diff --git a/tests/guix-archive.sh b/tests/guix-archive.sh
index 0866b5a4d8..08c07684ad 100644
--- a/tests/guix-archive.sh
+++ b/tests/guix-archive.sh
@@ -79,4 +79,9 @@ guix archive -t < "$archive" | grep "^r /share/guile.*/boot-9\.scm"
 
 echo foo | guix archive --authorize && false
 
+# Check incompatible command-line options
+guix archive --authorize --export --import && false
+guix archive --export guile-bootstrap --authorize > "$archive" && false
+guix archive --authorize --import < "$archive" && false
+
 exit 0

base-commit: d007b64356764f49677c78d82643f1125b5353b7
-- 
2.41.0

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Simon Tournier <zimon.toutoune <at> gmail.com>
Cc: Tobias Geerinckx-Rice <me <at> tobias.gr>, Christopher Baines <guix <at> cbaines.net>,
 Josselin Poiret <dev <at> jpoiret.xyz>, 66592 <at> debbugs.gnu.org,
 Mathieu Othacehe <othacehe <at> gnu.org>
Subject: Re: [bug#66592] [PATCH v4] scripts: archive: Check compatibility of
 command line options.
Date: Fri, 16 Aug 2024 18:00:41 +0200

Hello,

Simon Tournier <zimon.toutoune <at> gmail.com> skribis:

> Fixes <https://issues.guix.gnu.org/66358>.
> Reported by Perry, Daniel J <dperry45 <at> gatech.edu>.
>
> * guix/scripts/archive.scm (guix-archive)[compatible-option]: New procedure.
> and use it.
> * tests/guix-archive.sh: Test it.

LGTM, thanks!

Ludo’.

Previous Next

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