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

  • Open
  • quality assurance status badge
Details
2 participants
  • Maxim Cournoyer
  • Simon Tournier
Owner
unassigned
Submitted by
Simon Tournier
Severity
normal
S
S
Simon Tournier wrote on 17 Oct 2023 15:28
(address . guix-patches@gnu.org)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
fb2bfb115372a037e22df39dc194deabf290a58f.1697549017.git.zimon.toutoune@gmail.com
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(-)

Toggle diff (64 lines)
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
S
S
Simon Tournier wrote on 24 Oct 2023 18:33
[PATCH v2] scripts: archive: Check compatibility of command line options.
(address . 66592@debbugs.gnu.org)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
5c26f17bbf1b4cf9872b4a782295260ce337d3fd.1698165008.git.zimon.toutoune@gmail.com
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(-)

Toggle diff (42 lines)
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
M
M
Maxim Cournoyer wrote on 31 Oct 2023 19:01
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)(address . 66592@debbugs.gnu.org)
87ttq6pt7g.fsf@gmail.com
Hi Simon,

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

Toggle quote (48 lines)
> 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
S
S
Simon Tournier wrote on 30 Nov 2023 11:46
[PATCH v3] scripts: archive: Check compatibility of command line options.
(address . 66592@debbugs.gnu.org)(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
9f5c22f6fe1f4feba5d3561cef011b0c3f55df06.1701340510.git.zimon.toutoune@gmail.com
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(-)

Toggle diff (55 lines)
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
S
S
Simon Tournier wrote on 30 Nov 2023 11:46
Re: [bug#66592] [PATCH v2] scripts: archive: Check compatibility of command line options.
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)(address . 66592@debbugs.gnu.org)
87leaf5xi8.fsf@gmail.com
Hi,

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

Toggle quote (2 lines)
> Could you please send a v3 with some tests for it?

Done. WDYT?

Cheers,
simon
M
M
Maxim Cournoyer wrote on 4 Dec 2023 23:31
Re: [bug#66592] [PATCH v3] scripts: archive: Check compatibility of command line options.
(name . Simon Tournier)(address . zimon.toutoune@gmail.com)
87r0k1r44u.fsf@gmail.com
Hi,

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

Toggle quote (30 lines)
> 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.

Toggle quote (32 lines)
> + (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:

Toggle snippet (9 lines)
&& 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.

Interesting (yet confusing) shell hack.

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

--
Thanks,
Maxim
S
S
Simon Tournier wrote on 12 Jan 11:16 +0100
(name . Maxim Cournoyer)(address . maxim.cournoyer@gmail.com)
87frz2yhyx.fsf@gmail.com
Hi Maxim,

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

Toggle quote (9 lines)
>> + (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. ;-))

Toggle quote (3 lines)
> Would you mind sending a v4 with the above suggestion? Then I think
> we'd be good.

I will.


Cheers,
simon
?