Hello Chris, Chris Marusich <cmmarus...@gmail.com> 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’.
