[bug#36555] [PATCH v3 0/3] Refactor out common behavior for system reconfiguration.

[Christopher Lemmer Webber <cwebber@dustycloud.org>]

Jakob L. Kreuze writes:

> Hello to anyone reviewing this patch,
>
> I probably should've held off on sending this reroll out. After taking
> some more time to experiment with possible solutions, I was able to
> figure most of this out. Comments would still be appreciated, but the
> points I specifically asked for comments on no longer need special
> treatment. Also, if you haven't already started reviewing this, v4 will
> likely hit the mailing list tomorrow; everything's there, it just needs
> to be cleaned up.
>
> zerodaysfordays@sdf.lonestar.org (Jakob L. Kreuze) writes:
>
>> I still need to handle failed deployments in 'guix deploy'. I suspect
>> that, for now, it would make sense to implement remote roll-backs and
>> just roll-back the system on failure, at least until we've have some
>> dialog about the proper way to do atomic deployments.
>
> Well, except for this. I'll submit a separate patch series addressing
> this.

I think that's fine to do in a separate series, and a good idea too.

>> My biggest concern at the moment is error handling reporting in the
>> new 'guix system reconfigure'. I'd like to emulate what was done with
>> the previous version, but I'm at somewhat of a loss for how I'd go
>> about that, since the error reporting was mixed with the
>> reconfiguration code. So I'd like to ask for some suggestions: is the
>> best way to catch errors in '%store-monad' to do what
>> 'with-shepherd-error-handling' does, and then 'leave' on failure?
>>
>> Ludovic suggested guarding against 'message-condition' and having the
>> expression I send to 'remote-eval' return either ('error message) or
>> ('success). Would it make sense to just do this in all of the
>> reconfiguration procedures? Or is raising exceptions in the
>> reconfiguration procedures and catching them in the scripts' code the
>> way to go?
>
> Comments, if anyone has them, would be appreciated, but I feel that I'm
> in a good spot in terms of error handling now.

Or even:

  ('error <error-type-symbol> "error message here")

(I suppose in case of success, a value would never be returned?)

>> There's also a slight bug in the new 'guix system reconfigure' that
>> I'll need to figure out. At the moment, it installs a bootloader entry
>> for all but the newest generation.
>
> It wasn't actually a bug, I was misinterpreting the intended behavior of
> 'guix system reconfigure'. :)

Heh :)

>> Oh, how naïve I was four days ago. This reroll doesn't address this.
>> Having the procedures "parameterized by an evaluation procedure" can
>> be done in so many ways, and I think it would be best I put some
>> serious thought into which of those ways would be the best. A
>> 'local-eval' would clearly be much better than what I'm doing at the
>> present in 'system.scm', but the solution I came up with today
>> involved three layers of 'primitive-load', which I doubt is the way to
>> go about it. I had the idea to parameterize on a procedure that takes
>> a '<program-file>' rather than a G-Expression as I was making dinner
>> tonight, which seems to me like a sound idea, but we'll see if it
>> works tomorrow when I try to implement it.
>
> Actually, a more generalized 'eval' (taking a G-Expression) was the
> better way to go: it allowed me to simplify the interface to the
> reconfiguration procedures even further. And, thanks to Ludovic's recent
> patches with 'lower-gexp', I was able to collapse the Russian nesting
> doll of 'primitive-load' calls.

Yay!  Generalization!

>> Also, it hit me today that the safety checks done in 'guix system
>> reconfigure' -- 'check-mapped-devices',
>> 'check-file-system-availability', and 'check-initrd-modules' -- should
>> also be done in 'guix deploy'. It might make sense for me to submit that
>> change as a separate patch series so the code review for this doesn't
>> get too complicated, but since we're on the topic of unifying the code
>> between 'guix deploy' and 'guix system reconfigure', should I perhaps
>> reimplement those procedures as '<program-file>' objects like everything
>> else in '(guix scripts system reconfigure)'? They aren't really
>> effectful, but they concern system reconfiguration.
>
> Again, separate patch series.

Yes, please do.

My main worry is that such a patch series may be forgotten.  Would it be
inappropriate to make a "stub" patch issue for both of the followup
patch series, since both seem important and we don't want to forget them?

>> And, on the same note, should I go ahead and refactor the rest of the
>> reconfiguration code in 'system.scm' out into '(guix scripts system
>> reconfigure)'? I mean, this will probably be a separate patch series for
>> the same reason that the safety checks would be a separate patch series,
>> and I'll likely do this _after_ I come up with a decent way to
>> parameterize on an evaluation procedure, but I'd like to know if it's a
>> good idea or not before going ahead and ripping apart 'system.scm'.
>
> I'd still like comments on this, though.

I guess see above.

But I think we shouldn't wait, since I'd like to keep the energy up and
get this merged in.
 - Chris