Hi, Ludovic!

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

> These would look nicer if ‘switch-system-program’ and
> ‘upgrade-services-program’ returns a <program-file> because you could
> just write:
>
>   (machine-remote-eval #~(primitive-load #$(switch-system-program …))
>                        machine)
>
> (I realize the order of arguments is reversed; to stick to what ‘eval’
> does, I’d tend to put the ‘machine’ argument second—but that’s a
> separate issue.  :-))

I'm using 'gexp->script', so they should be returning a 'program-file'.
I've just neglected the conveniences I'm afforded with ungexp, it seems.
#~(primitive-load #$(switch-system-program …)) is, indeed, quite a bit
cleaner :)

> Can we remove ‘with-output-to-string’? I’d rather see what’s going on.
> :-)
>
> If that’s too verbose, we can use ‘invoke/quiet’.

I'm not too concerned with verbosity; rather, in the case for 'guix
deploy', the script's output mixes with the REPL output and that causes
'remote-eval' to fail with a match error. I think it would be better to
continue using 'with-output-to-string', but to preseve its return value
so we can show it to the user from 'guix deploy' or 'guix system
reconfigure'. Users of 'guix deploy' would also be able to see the
script's output this way.

> It seems that this sort-of inlines parts of ‘shepherd-service-upgrade’
> but without traversing the service dependency graph to determine the
> compilete set of obsolete services, no? I feel that we should be
> reusing ‘shepherd-service-upgrade’ or similar bits. (I realize this is
> already in ‘master’ for ‘guix deploy’, but since this is going to be
> shared with ‘guix system’, we’d rather be extra cautious.)

Does 'live-service-requirement' not encompass the full service
dependency graph? Regardless, I'll look into reusing
'shepherd-service-upgrade' as it's well-testsed.

> Also, I think we should remove ‘false-if-exception’ around
> ‘unload-service’.

Agreed. When you have time to look at it, I've raised a few questions
about this in v2 of this series.

> I’d rather not swallow stdout and not use ‘error’. Or at least, code
> that runs ‘install-bootloader-program’ should be able to produce a
> meaningful (and i18n’d) error message. So the caller could do
> something like:
>
>   (define result
>     (machine-eval #~(…
>                      (guard (c ((message-condition? c)
>                                 (cons 'error (condition-message c))))
>                        (invoke/quiet #$(install-bootloader-program …))
>                        '(success)))
>                   machine))
>
>   (match result
>     (('error message)
>      (leave (G_ "failed to install bootloader:~%~a~%") message))
>     (('success)
>      #t))
>
> Does that make sense?

Yes, and thank you for providing that snippet :)

> That’s quite some boilerplate to the challenge will be to factorize
> it.
>
> Ultimately, the code in (guix scripts system reconfigure) should be
> parameterized by an evaluation procedure that would be either
> ‘machine-eval’ or some hypothetical ‘local-eval’ procedure to evaluate
> things locally.

Noted. That should be a relatively small change, so I'll see about
tackling that in my next revision for this series.

Regards,
Jakob