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

[Christopher Baines]

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

> Christopher Baines <mail@cbaines.net> skribis:
>
>> Ludovic Courtès <ludo@gnu.org> writes:
>
> [...]
>
>>> Regarding <https://issues.guix.gnu.org/47157>, I would lean towards
>>> perhaps reverting the connection/error-handling patch series and
>>> starting anew from that “known state”.
>>>
>>> This area is unfortunately quite tedious to test and to get right so I’d
>>> err on the path of conservative, incremental changes.
>>>
>>> Thought?
>>
>> My preference is still to try and move forward and to make the error
>> handling easier to see in the code.
>>
>> Particularly with this change, I think the problem was introduced in
>> this commit [1], but I think it's hard to tell from the diff, since the
>> error handling and retrying is within with-cached-connection.
>>
>> 1: 
>> https://git.savannah.gnu.org/cgit/guix.git/commit/?id=f50f5751fff4cfc6d5abba9681054569694b7a5c
>>
>> That commit was one of the commits where I was making small incremental
>> changes prior to actually getting to the changes I was looking at
>> making, but a breakage was still introduced.
>>
>> What I was thinking about with this patch was how to make the error
>> handling being added back here easier to see, and thus harder to
>> break/remove.
>
> OK.
>
> 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.

> 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.

> 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.

> 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.

> 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).

signature.asc
Description: PGP signature