Subject: Re: bug#64173: [PATCH 0/1] guix: pack: add --entry-point-argument option

From: Ludovic Courtès <[email-masked]>
Date: Thu, 17 Aug 2023 11:42:17 +0200

Hi Graham,

Apologies for the delay (holiday time!).

Graham James Addis <[email-masked]> 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’.