| Message ID | 20191226180104.10888-2-mail@cbaines.net |
|---|---|
| State | Accepted |
| Headers | show |
| Series | Speed up the derivation linter. | expand |
Christopher Baines <mail@cbaines.net> skribis: > + (with-store store > + (parameterize > + ((%lint-checker-store-connection store)) Actually it means that now ‘guix lint’ systematically connects to the daemon. I wonder if we could arrange to open the connection lazily, and to somehow carry state across linter invocations. Perhaps ‘check-derivation’ should be monadic, with a field in <checker> indicating that. Sounds complicated though. Thoughts? Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > Christopher Baines <mail@cbaines.net> skribis: > >> + (with-store store >> + (parameterize >> + ((%lint-checker-store-connection store)) > > Actually it means that now ‘guix lint’ systematically connects to the > daemon. I guess that's the effect, were you meaning this would make a better message in the commit? > I wonder if we could arrange to open the connection lazily, and to > somehow carry state across linter invocations. Perhaps > ‘check-derivation’ should be monadic, with a field in <checker> > indicating that. Sounds complicated though. > > Thoughts? I did wonder if the code could somehow transparently be made more efficient. Quite often database clients manage a pool of connections, and when you perform a database operation, a connection from the pool is checked out, and then returned once you're finished. But as you say, this could be complicated. I think parameters can be set with connections, and I'm not quiet sure what the interface should be. I also did think about somehow passing the store connection in to the lint checker more explicitly, but I'm not sure how to generalise that. At least to me, a parameter to store the connection in seemed like a simple if inelegant approach. I'm quite happy to look in to other approaches though if you have thoughts on what might be good. Thanks, Chris
Hi Chris! Christopher Baines <mail@cbaines.net> skribis: > Ludovic Courtès <ludo@gnu.org> writes: > >> Christopher Baines <mail@cbaines.net> skribis: >> >>> + (with-store store >>> + (parameterize >>> + ((%lint-checker-store-connection store)) >> >> Actually it means that now ‘guix lint’ systematically connects to the >> daemon. > > I guess that's the effect, were you meaning this would make a better > message in the commit? I mean that it’s a visible change. Before, you could run all the linters but this one without having a daemon running; now you need a daemon up and running. >> I wonder if we could arrange to open the connection lazily, and to >> somehow carry state across linter invocations. Perhaps >> ‘check-derivation’ should be monadic, with a field in <checker> >> indicating that. Sounds complicated though. >> >> Thoughts? > > I did wonder if the code could somehow transparently be made more > efficient. Quite often database clients manage a pool of connections, > and when you perform a database operation, a connection from the pool is > checked out, and then returned once you're finished. But as you say, > this could be complicated. I think parameters can be set with > connections, and I'm not quiet sure what the interface should be. > > I also did think about somehow passing the store connection in to the > lint checker more explicitly, but I'm not sure how to generalise that. There could be a <checker> field indicating either that (1) the procedure takes an optional store parameter, or that (2) the procedure is monadic in ‘%store-monad’. #2 seems more complicated to implement that #1 though. For #1, ‘guix lint’ could check whether: (any checker-require-store? checkers) is true, and if it is, it could open a connection and pass it on as needed. WDYT? If that seems good to you, I guess you can go ahead with it (let’s just not lose our hair on it!). Thanks, Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > Hi Chris! > > Christopher Baines <mail@cbaines.net> skribis: > >> Ludovic Courtès <ludo@gnu.org> writes: >> >>> Christopher Baines <mail@cbaines.net> skribis: >>> >>>> + (with-store store >>>> + (parameterize >>>> + ((%lint-checker-store-connection store)) >>> >>> Actually it means that now ‘guix lint’ systematically connects to the >>> daemon. >> >> I guess that's the effect, were you meaning this would make a better >> message in the commit? > > I mean that it’s a visible change. Before, you could run all the > linters but this one without having a daemon running; now you need a > daemon up and running. Ah, yeah, that's a good point. >>> I wonder if we could arrange to open the connection lazily, and to >>> somehow carry state across linter invocations. Perhaps >>> ‘check-derivation’ should be monadic, with a field in <checker> >>> indicating that. Sounds complicated though. >>> >>> Thoughts? >> >> I did wonder if the code could somehow transparently be made more >> efficient. Quite often database clients manage a pool of connections, >> and when you perform a database operation, a connection from the pool is >> checked out, and then returned once you're finished. But as you say, >> this could be complicated. I think parameters can be set with >> connections, and I'm not quiet sure what the interface should be. >> >> I also did think about somehow passing the store connection in to the >> lint checker more explicitly, but I'm not sure how to generalise that. > > There could be a <checker> field indicating either that (1) the > procedure takes an optional store parameter, or that (2) the procedure > is monadic in ‘%store-monad’. > > #2 seems more complicated to implement that #1 though. > > For #1, ‘guix lint’ could check whether: > > (any checker-require-store? checkers) > > is true, and if it is, it could open a connection and pass it on as > needed. > > WDYT? > > If that seems good to you, I guess you can go ahead with it (let’s just > not lose our hair on it!). I've finally got around to looking at this again. I've sent some new patches which add a field to the <lint-checker> record, and adjust the running of lint checkers as well as the derivation checker to use a single store connection. Let me know what you think? Thanks, Chris
Hi, Christopher Baines <mail@cbaines.net> skribis: > I've finally got around to looking at this again. I've sent some new > patches which add a field to the <lint-checker> record, and adjust the > running of lint checkers as well as the derivation checker to use a > single store connection. > > Let me know what you think? LGTM, thank you! Ludo’.
Ludovic Courtès <ludo@gnu.org> writes: > Hi, > > Christopher Baines <mail@cbaines.net> skribis: > >> I've finally got around to looking at this again. I've sent some new >> patches which add a field to the <lint-checker> record, and adjust the >> running of lint checkers as well as the derivation checker to use a >> single store connection. >> >> Let me know what you think? > > LGTM, thank you! Great, I've pushed this to master now as 57e12aad6dfc2d12567164144dd15161e66f32d5.
diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm index 8d08c484f5..47c104217d 100644 --- a/guix/scripts/lint.scm +++ b/guix/scripts/lint.scm @@ -28,6 +28,7 @@ (define-module (guix scripts lint) #:use-module (guix packages) + #:use-module (guix store) #:use-module (guix lint) #:use-module (guix ui) #:use-module (guix scripts) @@ -167,12 +168,15 @@ run the checkers on all packages.\n")) (_ #f)) (reverse opts))) (checkers (or (assoc-ref opts 'checkers) %all-checkers))) - (cond - ((assoc-ref opts 'list?) - (list-checkers-and-exit checkers)) - ((null? args) - (fold-packages (lambda (p r) (run-checkers p checkers)) '())) - (else - (for-each (lambda (spec) - (run-checkers (specification->package spec) checkers)) - args))))) + (with-store store + (parameterize + ((%lint-checker-store-connection store)) + (cond + ((assoc-ref opts 'list?) + (list-checkers-and-exit checkers)) + ((null? args) + (fold-packages (lambda (p r) (run-checkers p checkers)) '())) + (else + (for-each (lambda (spec) + (run-checkers (specification->package spec) checkers)) + args)))))))