Simon Tournier <zimon.toutoune@gmail.com> writes:

> On Sun, 26 May 2024 at 09:10, Christopher Baines <mail@cbaines.net> wrote:
>
>>>>  (define* (open-connection #:optional (uri (%daemon-socket-uri))
>>>>                            #:key port (reserve-space? #t) cpu-affinity
>>>> -                          non-blocking?)
>>>> +                          non-blocking? assume-available-builtin-builders)
>>>
>>> Why add the variable %assume-available-builtin-builders and default to
>>> it?
>>>
>>> Something like:
>>>
>>> --8<---------------cut here---------------start------------->8---
>>> (define %assume-available-builtin-builders
>>>   "List of builtin builders supported by the builder Guix daemon."
>>>   (list "download" "git-download"))
>>>
>>> (define* (open-connection #:optional (uri (%daemon-socket-uri))
>>>                           #:key port (reserve-space? #t) cpu-affinity
>>>                           non-blocking?)
>>>                           non-blocking?
>>>                           (assume-available-builtin-builders %assume-available-builtin-builders))
>>> --8<---------------cut here---------------end--------------->8---
>>>
>>> And then default to this %assume-available-builtin-builders elsewhere in
>>> [PATCH 2/2].  IMHO, it changes almost nothing but it would help to know
>>> (document) what to pass as argument.
>>
>> I think it's sensible to not use a fixed list by default, but check what
>> the daemon supports.
>
> Do you mean dynamically construct the proposal of
> %assume-available-builtin-builders?  Why not.
>
> Aside, my point is to provide a default value for the new argument and
> not let it free.  Because when reading the source code, not knowing its
> type, neither any meaningful value make it hard to remember what it use
> or how to use it, IMHO.  That’s why I am suggesting something like
> %assume-available-builtin-builders that collects the acceptable values
> – for the most recent daemon, indeed; well it would simplify the
> documentation of this new parameter / argument.

I'm not sure I follow. I guess open-connection could have a
#:build-in-builders argument that expects a procedure that takes the
store connection, and returns the list of strings. That would allow it
to have the default of %built-in-builders from (guix store).

While this adds the flexibility for users to provide their own way of
setting the builtin builders enabled on a connection, I'm not sure
there's a need for it currently and I don't think it addresses your
concern about not knowing what value to provide.

It sounds easier just to make it clear from the docstrings that you
provide a list of strings, and have it default to #f to indicate to the
daemon's buildin builders.