[bug#37978] [PATCH] guix: new command "guix time-machine"

[Ludovic Courtès]

Hello Konrad!

Konrad Hinsen <address@hidden> skribis:

> * guix/scripts/time-machine.scm: New file.
> * guix/scripts/pull.scm: Export function channel-list.
> * guix/inferior.scm: New function cached-guix-filetree-for-channels.
> * doc/guix.texi: Document "git time-machine"

Awesome.  :-)

Please also add time-machine.scm to Makefile.am.  In the commit log,
you’ll get bonus points if you mention the modified entities in
parentheses (see ‘git log’ for examples.)  :-)

> @@ -247,6 +247,7 @@ Utilities
>  * Invoking guix container::     Process isolation.
>  * Invoking guix weather::       Assessing substitute availability.
>  * Invoking guix processes::     Listing client processes.
> +* Invoking guix time-machine::  Running an older version of Guix.

How about moving this section a bit higher, because it’s more widely
useful than ‘guix processes’ for instance?  Actually, it could go under
“Package Management” right before “Inferiors”, WDYT?

>  The @command{guix describe --format=channels} command can even generate this
> -list of channels directly (@pxref{Invoking guix describe}).
> +list of channels directly (@pxref{Invoking guix describe}). The resulting
> +file can be used with the -C options of @command{guix pull}
> +(@pxref{Invoking guix pull}) or @command{guix time-machine}
> +(@pxref{Invoking guix time-machine}).

Nitpick: Please write two spaces after an end-of-sentence period.

> +The general syntax is:
> +
> +@example
> +guix time-machine @var{channels} -- @var{command} @var {arg}@dots{}
> +@end example

I think it should be “guix time-machine @var{options}@dots{} -- …”,
right?

IIUC, if you run:

  guix time-machine -- build hello

you build “hello” with the latest master, right?

Perhaps it would be good to add an example like this one actually, WDYT?

> +where @var{command} and @var{arg}@dots{} are passed unmodified to the
> +@command{guix} command in its old version.  The @var{channels} that define
> +this version can be specified using the following options:

Perhaps add “like for @command{guix pull}”.

> -(define* (inferior-for-channels channels
> -                                #:key
> -                                (cache-directory (%inferior-cache-directory))
> -                                (ttl (* 3600 24 30)))
> -  "Return an inferior for CHANNELS, a list of channels.  Use the cache at
> -CACHE-DIRECTORY, where entries can be reclaimed after TTL seconds.  This
> -procedure opens a new connection to the build daemon.
> -
> -This is a convenience procedure that people may use in manifests passed to
> -'guix package -m', for instance."
> +(define* (cached-guix-filetree-for-channels channels
> +                                            #:key
> +                                            (cache-directory 
> (%inferior-cache-directory))
> +                                            (ttl (* 3600 24 30)))
> +  "Return a directory containing a guix filetree defined by CHANNELS, a list 
> of channels.
> +The directory is a subdirectory of CACHE-DIRECTORY, where entries can be 
> reclaimed after TTL seconds.
> +This procedure opens a new connection to the build daemon."

How about (1) calling it ‘cached-channel-instance’ (or similar), and (2)
not opening a connection to the daemon?

Regarding (2), it means that procedure would be a monadic procedure and
it’s up to the user to do with-store + run-with-store or whatever.  The
general convention is to not open new connections on behalf of the user
(‘inferior-for-channels’ is one of the only exceptions to the rule
because it’s a convenience function for use in manifests.)

Perhaps this change should be a separate patch.

> +(define (guix-time-machine . args)
> +  (with-error-handling
> +    (let* ((opts         (parse-args args))
> +           (channels     (channel-list opts))
> +           (command-line (assoc-ref opts 'exec))
> +           (directory    (cached-guix-filetree-for-channels channels))
> +           (executable   (string-append directory "/bin/guix")))
> +      (apply system* (cons executable command-line)))))

I think this should be:

  (apply execl executable command-line)

so that we don’t create an extra process and actually get the exit code
for that sub-process.

Thanks,
Ludo’.