| Message ID | 6b9c634c024b6fd7a50d3b82edc244676a8ca8e8.1716038375.git.mail@cbaines.net |
|---|---|
| State | New |
| Headers | show |
| Series | Enable specifying the available builtin builders. | expand |
Hi Chris, On sam., 18 mai 2024 at 14:19, Christopher Baines <mail@cbaines.net> wrote: > diff --git a/guix/store.scm b/guix/store.scm > index 58ddaa8d15..0c734cdca7 100644 > --- a/guix/store.scm > +++ b/guix/store.scm > @@ -571,7 +571,7 @@ (define* (connect-to-daemon uri #:key non-blocking?) > > (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. Cheers, simon
Simon Tournier <zimon.toutoune@gmail.com> writes: > On sam., 18 mai 2024 at 14:19, Christopher Baines <mail@cbaines.net> wrote: > >> diff --git a/guix/store.scm b/guix/store.scm >> index 58ddaa8d15..0c734cdca7 100644 >> --- a/guix/store.scm >> +++ b/guix/store.scm >> @@ -571,7 +571,7 @@ (define* (connect-to-daemon uri #:key non-blocking?) >> >> (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.
Hi Chris, 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. Considering your patch, how do I know that I could used it, as [1]: --8<---------------cut here---------------start------------->8--- As in: (open-connection #:assume-available-builtin-builders '("download")) --8<---------------cut here---------------end--------------->8--- Because it is not clear from the docstring. And there is many procedures that would require some docstring update with this new procedure argument. :-) 1: bug#67250: builtin:git-download capability detection not working for the bordeaux build farm Ludovic Courtès <ludo@gnu.org> Wed, 22 Nov 2023 11:19:42 +0100 id:87bkbmm6o1.fsf@gnu.org https://issues.guix.gnu.org/67250 https://issues.guix.gnu.org/msgid/87bkbmm6o1.fsf@gnu.org https://yhetil.org/guix/87bkbmm6o1.fsf@gnu.org Cheers, simon
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.
diff --git a/guix/store.scm b/guix/store.scm index 58ddaa8d15..0c734cdca7 100644 --- a/guix/store.scm +++ b/guix/store.scm @@ -571,7 +571,7 @@ (define* (connect-to-daemon uri #:key non-blocking?) (define* (open-connection #:optional (uri (%daemon-socket-uri)) #:key port (reserve-space? #t) cpu-affinity - non-blocking?) + non-blocking? assume-available-builtin-builders) "Connect to the daemon at URI (a string), or, if PORT is not #f, use it as the I/O port over which to communicate to a build daemon. @@ -616,7 +616,9 @@ (define* (open-connection #:optional (uri (%daemon-socket-uri)) (when (>= (protocol-minor v) 11) (write-int (if reserve-space? 1 0) port)) (letrec* ((built-in-builders - (delay (%built-in-builders conn))) + (if assume-available-builtin-builders + (delay assume-available-builtin-builders) + (delay (%built-in-builders conn)))) (caches (make-vector (atomic-box-ref %store-connection-caches) @@ -635,7 +637,8 @@ (define* (open-connection #:optional (uri (%daemon-socket-uri)) conn)))))) (define* (port->connection port - #:key (version %protocol-version)) + #:key (version %protocol-version) + assume-available-builtin-builders) "Assimilate PORT, an input/output port, and return a connection to the daemon, assuming the given protocol VERSION. @@ -654,7 +657,9 @@ (define* (port->connection port (make-vector (atomic-box-ref %store-connection-caches) vlist-null) - (delay (%built-in-builders connection)))) + (if assume-available-builtin-builders + (delay assume-available-builtin-builders) + (delay (%built-in-builders connection))))) connection))