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

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

[Simon Tournier]

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45@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@gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune@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

[bug#66592] [PATCH v2] scripts: archive: Check compatibility of command line options.

[Simon Tournier]

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45@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@gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune@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

[bug#66592] [PATCH v2] scripts: archive: Check compatibility of command line options.

[Maxim Cournoyer]

Hi Simon,

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

> Fixes <https://issues.guix.gnu.org/66358>.
> Reported by Perry, Daniel J <dperry45@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@gnu.org>
>  ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
> +;;; Copyright © 2023 Simon Tournier <zimon.toutoune@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

[bug#66592] [PATCH v3] scripts: archive: Check compatibility of command line options.

[Simon Tournier]

Fixes <https://issues.guix.gnu.org/66358>.
Reported by Perry, Daniel J <dperry45@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@gnu.org>
 ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
+;;; Copyright © 2023 Simon Tournier <zimon.toutoune@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

[bug#66592] [PATCH v2] scripts: archive: Check compatibility of command line options.

[Simon Tournier]

Hi,

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

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

Done.  WDYT?

Cheers,
simon

[bug#66592] [PATCH v3] scripts: archive: Check compatibility of command line options.

[Maxim Cournoyer]

Hi,

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

> Fixes <https://issues.guix.gnu.org/66358>.
> Reported by Perry, Daniel J <dperry45@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@gnu.org>
>  ;;; Copyright © 2020 Tobias Geerinckx-Rice <me@tobias.gr>
> +;;; Copyright © 2023 Simon Tournier <zimon.toutoune@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

[bug#66592] [PATCH v3] scripts: archive: Check compatibility of command line options.

[Simon Tournier]

Hi Maxim,

On Mon, 04 Dec 2023 at 17:31, Maxim Cournoyer <maxim.cournoyer@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