| Message ID | 9f5c22f6fe1f4feba5d3561cef011b0c3f55df06.1701340510.git.zimon.toutoune@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [bug#66592,v3] scripts: archive: Check compatibility of command line options. | expand |
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.
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
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