Re: Update: Implementing guix system rollback / switch-generation

[Ludovic Courtès]

Hello Chris,

Chris Marusich <address@hidden> skribis:

> I've attached a preliminary patch which adds rudimentary roll-back and
> switch-generation commands to "guix system".  Currently, the commands
> just flip symlinks, which is part of the first milestone that Ludo
> suggested in the following thread:
>
> https://lists.gnu.org/archive/html/guix-devel/2016-06/msg00178.html

Awesome!

> Please let me know what you think.  After getting feedback, I intend to
> follow up a more complete patch that contains the following additional
> changes:
>
> * Regenerate grub.cfg, to complete the first milestone.  I'm not sure
>   exactly how this will play out, but I imagine I'll need to use the
>   store somehow to get it done.
>
> * Use a properly formatted ChangeLog-style Git commit message.
>
> * Mention these new commands in the manual.
>
> * Add tests.  I'm not new to automated testing, but I haven't done it
>   using autoconf/make or in Guile, so tips here would be welcome!
>
> * Add these new commands to the emacs interface.

Sounds good to me.

Automated tests for this will be a bit difficult because we don’t have
any ‘guix system’ tests yet.  I think this should be done in the new
system test infrastructure.

However, since you’ve done extensive manual testing, this part shouldn’t
block you.  We can add it at a later point.

> I've tested the changes on my bare metal GuixSD installation.  I would
> have used a VM, but my system has trouble launching VMs.  I first
> verified manually that the roll-back command switched the profile's
> symlink to the previous generation, by examining the output of
> list-generations and ls.  I then verified, in the same way, that the
> switch-generation command switched the profile's symlink to the
> specified generation.  I verified that switch-generation works for
> arbitrary numbers and relative numbers (note that a negative relative
> number must be preceded by the special "--" argument to prevent it from
> being interpreted as an option).  I also verified that if the user
> attempts to switch to a nonexistent generation (or to roll back from
> generation 1 in the case of "guix system"), an error is raised without
> modifying anything.  I repeated this manual verification process for
> both the "guix system" and "guix package" commands, since my changes
> involve some refactoring in "guix package".

Perfect.

> +(define (relative-generation-spec->number profile spec)
> +  "Return PROFILE's generation specified by SPEC, which is a string.  The 
> SPEC
> +may be a N, -N, or +N, where N is a number.  If the spec is N, then the 
> number
> +returned is N.  If it is -N, then the number returned is the profile's 
> current
> +generation number minus N.  If it is +N, then the number returned is the
> +profile's current generation number plus N.  Return #f if there is no such
> +generation."
> +  (let ((number (string->number spec)))
> +    (and number
> +         (case (string-ref spec 0)
> +           ((#\+ #\-)
> +            (relative-generation profile number))
> +           (else number)))))

(Refactored from (guix scripts package).)  Could you make this
refactoring in a separate patch?  It LGTM.

>  (define (switch-to-generation profile number)
>    "Atomically switch PROFILE to the generation NUMBER.  Return the number of
> -the generation that was current before switching."
> +the generation that was current before switching.  Raise a
> +&profile-not-found-error when the profile does not exist.  Raise a
> +&missing-generation-error when the generation does not exist."

Could you add the docstring in a separate patch?

(Isolating changes like this can be tedious but it simplifies review and
bisecting should a regression be introduced.)

>  (define (show-help)
> -  (display (_ "Usage: guix system [OPTION] ACTION [FILE]
> -Build the operating system declared in FILE according to ACTION.\n"))
> +  (display (_ "Usage: guix system [OPTION ...] ACTION [ARG ...] [FILE]
> +Build the operating system declared in FILE according to ACTION.
> +Some ACTIONS support additional ARGS.\n"))
>    (newline)
>    (display (_ "The valid values for ACTION are:\n"))
>    (newline)
>    (display (_ "\
> -   reconfigure      switch to a new operating system configuration\n"))
> +   reconfigure       switch to a new operating system configuration\n"))
> +  (display (_ "\
> +   roll-back         switch to the previous operating system 
> configuration\n"))
>    (display (_ "\
> -   list-generations list the system generations\n"))
> +   switch-generation switch to a generation matching a pattern\n"))
>    (display (_ "\
> -   build            build the operating system without installing 
> anything\n"))
> +   list-generations  list the system generations\n"))
>    (display (_ "\
> -   container        build a container that shares the host's store\n"))
> +   build             build the operating system without installing 
> anything\n"))
>    (display (_ "\
> -   vm               build a virtual machine image that shares the host's 
> store\n"))
> +   container         build a container that shares the host's store\n"))
>    (display (_ "\
> -   vm-image         build a freestanding virtual machine image\n"))
> +   vm                build a virtual machine image that shares the host's 
> store\n"))
>    (display (_ "\
> -   disk-image       build a disk image, suitable for a USB stick\n"))
> +   vm-image          build a freestanding virtual machine image\n"))
>    (display (_ "\
> -   init             initialize a root file system to run GNU\n"))
> +   disk-image        build a disk image, suitable for a USB stick\n"))
>    (display (_ "\
> -   extension-graph  emit the service extension graph in Dot format\n"))
> +   init              initialize a root file system to run GNU\n"))
>    (display (_ "\
> -   shepherd-graph   emit the graph of shepherd services in Dot format\n"))
> +   extension-graph   emit the service extension graph in Dot format\n"))
> +  (display (_ "\
> +   shepherd-graph    emit the graph of shepherd services in Dot format\n"))

Not sure what happened here.  :-)  Please avoid reformatting such
strings; they are translated so changing them makes translations stale.

Otherwise looks like Milestone #1 is already close to completion!

Thank you,
Ludo’.