Package: guix-patches;
Reported by: Graham James Addis <grahamjamesaddis <at> gmail.com>
Date: Mon, 19 Jun 2023 15:39:02 UTC
Severity: normal
Tags: patch
Merged with 64171
Message #12 received at 64173 <at> debbugs.gnu.org (full text, mbox):
From: Ludovic Courtès <ludo <at> gnu.org> To: Graham James Addis <grahamjamesaddis <at> gmail.com> Cc: Graham James Addis <graham <at> addis.org.uk>, Josselin Poiret <dev <at> jpoiret.xyz>, Tobias Geerinckx-Rice <me <at> tobias.gr>, 64173 <at> debbugs.gnu.org, Simon Tournier <zimon.toutoune <at> gmail.com>, Mathieu Othacehe <othacehe <at> gnu.org>, Christopher Baines <mail <at> cbaines.net>, 64171 <at> debbugs.gnu.org, Ricardo Wurmus <rekado <at> elephly.net> Subject: Re: bug#64173: [PATCH 0/1] guix: pack: add --entry-point-argument option Date: Thu, 17 Aug 2023 11:42:17 +0200
Hi Graham,
Apologies for the delay (holiday time!).
Graham James Addis <grahamjamesaddis <at> gmail.com> skribis:
> * guix/scripts/pack.scm: add support for parameters in entry-point
> (entry-point-argument-spec-option-parser): add function to parse --entry-point-argument
> (docker-image): Add function (form_entry_point) to handle --entry-point
> vs --entry-point-argument merging
> (docker-image): change call to (build-docker-image) to use (form-entry-point)
> (%default-options): add dafault for --entry-point-argument option
> (%docker-format-options): parser for --entry-point-argument
> (show-docker-format-options): help for --docker-format-options
> (show-docker-format-options/detailed): detailed help for --docker-format-options
> (%options): add flag for --help-docker-format and add parser for --docker-format-options
> (extra-options): add entry-point-argument option
This is really a minor issue and I don’t mind fixing it for you, but
note that the ChangeLog style just needs to say what has been
changed/added/removed. For example:
(entry-point-argument-spec-option-parser): New procedure.
> (guix.texi): add documentation
And here:
* doc/guix.texi (Inovking guix pack): Document it.
I like this new version. Here are a few things that I think would need
to be changed before we can push:
> +@cindex entry point arguments, for docker images
> +@item --entry-point-argument=@var{command}
> +@itemx -A @var{command}
Maybe @var{argument} for consistency and clarity?
> +Use @var{command} as an argument to @dfn{entry point} of the resulting pack.
> +This option is only valid in conjunction with @code{--entry-point} and can
> +appear multiple times on the command line.
Maybe add: “The example below shows illustrates that, passing
@option{--help} to the @command{guile} command:”
> +@example
> +guix pack -f docker --entry-point=bin/guile --entry-point-argument="--help" guile
> +@end example
[...]
> +(define (entry-point-argument-spec-option-parser opt name arg result)
> + "A SRFI-37 opion parser for the --entry-point-argument option. The spec
> +takes multiple occurances. The entries are used in the exec form for the
> +docker entry-point. The values are used as parameters in conjunction with
> +the --entry-point option which is used as the first value in the exec form."
> + (let ((entry-point-argument (assoc-ref result 'entry-point-argument)))
> + (alist-cons 'entry-point-argument
> + (append entry-point-argument (list arg))
> + (alist-delete 'entry-point-argument result eq?))))
I would just keep a regular option parser that does:
(alist-cons 'entry-point-argument arg result)
Later on, we’d collect all these arguments:
(reverse
(filter-map (match-lambda
(('entry-point-argument . arg) arg)
(_ #f))
opts))
I think this would be a bit clearer; this is what ‘guix repl’ does, for
instance.
> + (define (form-entry-point
> + prefix entry-point
> + entry-point-argument)
> + ;; construct entry-point parameter for build-docker-image
> + ;; the first entry is constructed by prefixing the entry-point
> + ;; with the supplied index subsequent entries are taken from
> + ;; the --entry-point-argument options
> + (cond
> + (entry-point
> + (cons*
> + (string-append prefix "/" entry-point)
> + entry-point-argument)
> + )
I’d avoid this extra procedure.
> + ('()))) ;empty list returned if no conditions are met
> +
> + (let-keywords '#$extra-options #f ((entry-point-argument #f))
> + (setenv "PATH" #+(file-append archiver "/bin"))
> +
> + (build-docker-image #$output
> + (map store-info-item
> + (call-with-input-file "profile"
> + read-reference-graph))
> + #$profile
> + #:repository (manifest->friendly-name
> + (profile-manifest #$profile))
> + #:database #+database
> + #:system (or #$target %host-type)
> + #:environment environment
> + #:entry-point (form-entry-point
> + #$profile
> + #$entry-point
> + entry-point-argument)
How about keeping it similar to what it looks like currently:
#:entry-point
#$(and entry-point
#~(cons (string-append #$profile "/"
#$entry-point)
'#$(or (assoc-ref extra-options
#:entry-point-arguments)
'())))
?
> @@ -1346,6 +1375,7 @@ (define %default-options
> (debug . 0)
> (verbosity . 1)
> (symlinks . ())
> + (entry-point-argument . ())
This can be omitted if you take the approach suggested above, with one
‘entry-point-argument’ pair per argument.
> +(define %docker-format-options
> + (list (option '(#\A "entry-point-argument") #t #f
> + entry-point-argument-spec-option-parser)))
> +
> +(define (show-docker-format-options)
> + (display (G_ "
> + --help-docker-format
> + list options specific to the DOCKER format")))
> +
> +(define (show-docker-format-options/detailed)
> + (display (G_ "
> + -A, --entry-point-argument=COMMAND/PARAMETER
> + Value(s) to use for the Docker EntryPoint arguments.
> + Multiple instances are accepted. This is only valid
> + in conjunction with the --entry-point option."))
> + (newline)
> + (exit 0))
> +
> (define %options
> ;; Specifications of the command-line options.
> (cons* (option '(#\h "help") #f #f
> @@ -1508,8 +1557,13 @@ (define %options
> (lambda args
> (show-rpm-format-options/detailed)))
>
> + (option '("help-docker-format") #f #f
> + (lambda args
> + (show-docker-format-options/detailed)))
> +
> (append %deb-format-options
> %rpm-format-options
> + %docker-format-options
> @@ -1528,6 +1582,7 @@ (define (show-help)
> (newline)
> (show-deb-format-options)
> (show-rpm-format-options)
> + (show-docker-format-options)
Leftover, can be removed.
> @@ -1696,6 +1751,8 @@ (define-command (guix-pack . args)
> (process-file-arg opts 'preun-file)
> #:postun-file
> (process-file-arg opts 'postun-file)))
> + ('docker
> + (list #:entry-point-argument (assoc-ref opts 'entry-point-argument)))
This would become #:entry-point-arguments (plural), with the
‘filter-map’ trick shown above.
Also, it should be possible to make it work for the Singularity backend,
right? We can keep it for a subsequent commit if you prefer, but then
please add a TODO comment.
Could you send an updated patch?
Thanks in advance!
Ludo’.
GNU bug tracking system
Copyright (C) 1999 Darren O. Benham,
1997,2003 nCipher Corporation Ltd,
1994-97 Ian Jackson.