GNU bug report logs - #54393
[PATCH 0/2] Add 'guix manifest' to "translate" commands to manifests

Previous Next

Full log

Message #56 received at 54393 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 54393 <at> debbugs.gnu.org
Subject: Re: bug#54393: [PATCH 0/2] Add 'guix manifest' to "translate"
 commands to manifests
Date: Mon, 04 Apr 2022 10:37:26 -0400

Hello,

Ludovic Courtès <ludo <at> gnu.org> writes:

> * guix/scripts/shell.scm (show-help, %options): Add '--export-manifest'.
> (manifest-entry-version-prefix, manifest->code*)
> (export-manifest): New procedures.
> (guix-shell): Honor '--export-manifest'.
> * tests/guix-shell-export-manifest.sh: New file.
> * Makefile.am (SH_TESTS): Add it.
> * doc/guix.texi (Invoking guix shell): Document '--export-manifest'.
> (Invoking guix environment): Link to it.
> (Invoking guix pack): Likewise.

[...]

> +
> +;;;
> +;;; Exporting a manifest.
> +;;;
> +
> +(define (manifest-entry-version-prefix entry)
> +  "Search among all the versions of ENTRY's package that are available, and
> +return the shortest unambiguous version prefix for this package."
> +  (package-unique-version-prefix (manifest-entry-name entry)
> +                                 (manifest-entry-version entry)))
> +
> +(define (manifest->code* manifest extra-manifests)
> +  "Like 'manifest->code', but insert a 'concatenate-manifests' call that
> +concatenates MANIFESTS, a list of expressions."
> +  (if (null? (manifest-entries manifest))
> +      (match extra-manifests
> +        ((one) one)
> +        (lst   `(concatenate-manifests ,@extra-manifests)))
> +      (match (manifest->code manifest
> +                             #:entry-package-version
> +                             manifest-entry-version-prefix)
> +        (('begin exp ... last)
> +         `(begin
> +            ,@exp
> +            ,(match extra-manifests
> +               (() last)
> +               (_  `(concatenate-manifests
> +                     (list ,last ,@extra-manifests)))))))))

Should an "else" clause be added here with a more useful error message
that the default 'no match for x' or similar?  If that'd be totally
unexpected and a bug, then it's fine as-is.

> +(define (export-manifest opts port)
> +  "Write to PORT a manifest corresponding to OPTS."
> +  (define (manifest-lift proc)
> +    (lambda (entry)
> +      (match (manifest-entry-item entry)
> +        ((? package? p)
> +         (manifest-entry
> +           (inherit (package->manifest-entry (proc p)))
> +           (output (manifest-entry-output entry))))
> +        (_
> +         entry))))
> +
> +  (define (validated-spec spec)
> +    ;; Return SPEC if it's validate package spec.

As this is an action (proc), perhaps it should be named "validate-spec".
The comment doc should also be worded as "if SPEC is a valid package
spec" or similar.

The rest LGTM.

Thank you for addressing the suggestion to reuse an existing sub-command
to try to keep things neatly organized instead of extending the already
large set of them :-).

Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.