[bug#47160] [PATCH] scripts: substitute: Add back some error handling.

["Ludovic Courtès" <ludo@gnu.org>]

Howdy!

Christopher Baines <mail@cbaines.net> skribis:

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

[...]

>> Though I’m still unsure what the patch series starting at
>> 7b812f7c84c43455cdd68a0e51b6ded018afcc8e was about.  What was the end
>> goal?
>
> So that was part of the creation of the (guix substitutes) module,
> unpicking the code in the script to separate out some of the connection
> caching was a prerequisite (discussion starts here
> https://issues.guix.gnu.org/45409#5 ).
>
> I think separating out that module is still a good thing. It's allowed
> for improvements in guix, the weather script doesn't now call in to the
> substitute script code for example. I'd also like the separation for
> things like the Guix Build Coordinator, which currently attempts to use
> the substitute code from Guix.

Right, I agree this is a worthy goal.  Untangling the stateful bits is
the hard part, as we see.  :-)

>> I also wonder if it introduced other issues.  For
>> example, 7b812f7c84c43455cdd68a0e51b6ded018afcc8e replaced a reference
>> to ‘open-connection-for-uri/cached’ by one to
>> ‘open-connection-for-uri/maybe’.  Are we still using cached
>> connections?
>
> At least on that commit, open-connection-for-uri/maybe calls
> open-connection-for-uri/cached, so yes, still using cached connections.

OK.

>> Commit f50f5751fff4cfc6d5abba9681054569694b7a5c no longer passes the
>> #:port parameter to ‘http-fetch’.
>
> Yeah, that change is sort of fine if you're just looking at how the
> port/connection is handled, but that area is being fixed up here, and
> because closing the port is something that happens, it's better to also
> pass the port in.

OK.

>> Commit 20c08a8a45d0f137ead7c05e720456b2aea44402 does other things but at
>> first sight I’m not sure what the effect is.
>
> So, open-connection-for-uri/maybe is like
> open-connection-for-uri/cached, but it catches a couple of exceptions
> relating to not being able to connect to a substitute server, it also
> remembers about showing the messages.
>
> The second commit here is changing that slightly, to not apply to
> process-substitution, however I do think that code might have applied in
> the past (as open-connection-for-uri/maybe was used I believe). But I
> think you're right in saying there's probably some overlap between the
> error handling here and done by with-networking.

Alright.

>> If you’re confident we can move forward to fix the bug, that’s great
>> (though we’ll need a good deal of testing), but I’d still like to
>> clarify these points later on.
>
> Well, the changes I'm suggesting here seem reasonable to me. As for
> testing, checking things basically work is easy enough, but I don't
> currently have many ideas for how to test for when fetching things
> doesn't go to plan (which can of course happen).

I’ll do some testing of v2 on my end and report back.

Thanks for the explanations!

Ludo’.