Hi, Ludovic!

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

> Really nice that it becomes this concise.

Yeah, I think (and hope) this is a good sign that we've picked the
right abstraction for this :)

> I like to avoid exposing constructors so that one cannot “forge”
> invalid objects, but let’s see…

Should I use @@ for this, perhaps? Aside from one other place in the
test suite, it's a one-off use, and the objects are then only used
internally.

> I wonder it we should just use
>
>   #~(begin (use-modules (guix build utils)) (invoke …))
>
> here and in other places.
>
> That’s probably better longer-term (for example when we switch to
> Guile 3, that could ease the transition since the right Guile would be
> used) but we can keep it this way and revisit it later.

Oh that's a good point, I agree that we should do that. I'll submit a
separate patch once this gets merged.

> s/remote-exp/exp/
> ...
> A leftover?  :-)
>
> These two statements disappeared in the process, but I think they’re
> added back by one of the subsequent patches, right?

Good catches, thanks! Yes, the code is added back in the commits that
follow.

> OK, that makes sense here.
>
> (Once we’ve done that (guix graph) demonadification we discussed
> before, perhaps we can perform run ‘shepherd-service-upgrade’ entirely
> on the “other side”, and at that point we won’t need to expose the
> ‘live-service’ constructor.)

The main issue with calling 'shepherd-service-upgrade' on the other side
is that we'd need to send over the service objects (the current
'upgrade-services-program' deals with provision symbols rather than the
service objects themselves).

I'm certain it's possible, it's just easier said than done. I've got
time to think it through, though :)

> No need to repeat the file name here.
>
> However there are other changes no mentioned here, for example changes
> to the ‘install’ procedure. Could you add them to the log?
>
> While you’re at it, could you change it to:
>
>   (info (G_ "bootloader successfully installed on '~a'~%") …)
>
> ?

Yep, sure thing.

> What happens when ‘install-bootloader’ fails though? We should make
> sure that the error is diagnosed, and that the output of
> ‘grub-install’ or similar is shown when that happens.

> Note that there are now a few places where we call ‘built-derivations’
> without calling ‘show-what-to-build*’ first. That means the UX might
> be pretty bad since one has no idea what’s being built.
>
> Furthermore, that means substitutes may not be up-to-date, leading to
> many “updating substitutes” messages and HTTP round trips (as happened
> with <https://issues.guix.gnu.org/issue/36509>).
>
> Last, doing several ‘build-derivations’ call with just a couple of
> derivations is less efficient than doing a single call with many
> derivations; that also has an impact on the UI, if we were to call
> ‘show-what-to-build*’ once for ‘build-derivations’ call.
>
> What’s your experience with this in practice?

I haven't had too many issues with it since the G-Expressions tended to
have few inputs, but those are some valid concerns. Would it be better
to create derivations for locally-evaluated G-Expressions? For example,
with 'program-file' or 'gexp->script'? I thought that evaluating them
in-place might be better since that's one fewer store item that needs to
be built, but if we were to turn the G-Expression into a derivation, we
could add it to the call to 'show-what-to-build*' in 'guix system
reconfigure'.

> Eventually we should add it to (guix gexp).

Yeah, that seems to make more sense. I can move it when I address the
above.

> Last but not least, make sure to test this on your machine.  :-)
>
> It’s sensitive code that we’d rather not break.

Heh, indeed! I've run it several times in a virtual machine, but running
it on my desktop is the ultimate "I promise this works, and if it
doesn't, I'll eat my hat." I'll do an update on this machine and report
back.

> Perhaps you could also check the target of /run/current-system, and
> maybe check things like the set of user accounts (activation code)?
>
> Perhaps you could also check for the availability of a “replacement”
> slot (info "(shepherd) Slots of services") for services that exist
> both before and after the upgrade? This could be achieved by
> augmenting (gnu services herd) with a ‘live-service-replacement’
> procedure, I think.

Great ideas! In the interest of keeping this patch manageable, I'll
submit these improvements separately.

> No need for docstrings for inner procedures; a comment is enough.
> ...
> s/new/obsolete/, no?

I can address these in my corrections, though.

> I think you’ve reached the most difficult part of this whole endeavor.
> The good thing is that, once you’re past this, things will be much
> easier.

Agreed, I think this gives us a good framework for implementing
provisioning etc.

Regards,
Jakob