GNU bug report logs - #38754
[PATCH 0/2] Speed up the derivation linter.

Previous Next

Package: guix-patches;

Reported by: Christopher Baines <mail <at> cbaines.net>

Date: Thu, 26 Dec 2019 17:34:01 UTC

Severity: normal

Tags: patch

Done: Christopher Baines <mail <at> cbaines.net>

Bug is archived. No further changes may be made.

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 38754 in the body.
You can then email your comments to 38754 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

View this report as an mbox folder, status mbox, maintainer mbox


Report forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Thu, 26 Dec 2019 17:34:01 GMT) Full text and rfc822 format available.

Acknowledgement sent to Christopher Baines <mail <at> cbaines.net>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Thu, 26 Dec 2019 17:34:01 GMT) Full text and rfc822 format available.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: guix-patches <at> gnu.org
Subject: [PATCH 0/2] Speed up the derivation linter.
Date: Thu, 26 Dec 2019 17:33:07 +0000
[Message part 1 (text/plain, inline)]
These patches speed up the derivation linter by reducing the number of
times a connection to the guix-daemon is established. In testing on my
computer, the time to lint all guix packages with the derivation linter
drops from around 6 minutes to around 3 minutes.


Christopher Baines (2):
  guix: lint: Add an optional parameter for a store connection.
  scripts: lint: Set the %link-checker-store-connection parameter.

 guix/lint.scm         | 42 ++++++++++++++++++++++++++----------------
 guix/scripts/lint.scm | 22 +++++++++++++---------
 2 files changed, 39 insertions(+), 25 deletions(-)
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Thu, 26 Dec 2019 18:02:01 GMT) Full text and rfc822 format available.

Message #8 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 38754 <at> debbugs.gnu.org
Subject: [PATCH 1/2] guix: lint: Add an optional parameter for a store
 connection.
Date: Thu, 26 Dec 2019 18:01:03 +0000
Previously, the derivation lint checker establishes a connection to the store
for each supported system of each package. This change uses the same store
connection for all supported systems, with the option of setting a parameter
for a store connection which will be used instead of establishing a new
connection.

Previously, running the derivation linter for all packages would take around 6
and a half minutes, with this change, without setting the
%lint-checker-store-connection parameter, the time is reduced to around 4
minutes.

* guix/lint.scm (%lint-checker-store-connection): New parameter.
(check-derivation): Arrange the code so that it's possible to either run with
the store from the new parameter, or open a new connection via the with-store
syntax.
---
 guix/lint.scm | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index cd2ea571ed..19498db857 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -100,7 +100,9 @@
             lint-checker?
             lint-checker-name
             lint-checker-description
-            lint-checker-check))
+            lint-checker-check
+
+            %lint-checker-store-connection))
 
 
 ;;;
@@ -142,6 +144,9 @@
     ((_ package (G_ message) rest ...)
      (%make-warning package message rest ...))))
 
+(define %lint-checker-store-connection
+  (make-parameter #f))
+
 
 ;;;
 ;;; Checkers
@@ -887,7 +892,7 @@ descriptions maintained upstream."
 
 (define (check-derivation package)
   "Emit a warning if we fail to compile PACKAGE to a derivation."
-  (define (try system)
+  (define (try store system)
     (catch #t
       (lambda ()
         (guard (c ((store-protocol-error? c)
@@ -900,25 +905,30 @@ descriptions maintained upstream."
                                  (G_ "failed to create ~a derivation: ~a")
                                  (list system
                                        (condition-message c)))))
-          (with-store store
-            ;; Disable grafts since it can entail rebuilds.
-            (parameterize ((%graft? #f))
-              (package-derivation store package system #:graft? #f)
-
-              ;; If there's a replacement, make sure we can compute its
-              ;; derivation.
-              (match (package-replacement package)
-                (#f #t)
-                (replacement
-                 (package-derivation store replacement system
-                                     #:graft? #f)))))))
+          ;; Disable grafts since it can entail rebuilds.
+          (parameterize ((%graft? #f))
+            (package-derivation store package system #:graft? #f)
+
+            ;; If there's a replacement, make sure we can compute its
+            ;; derivation.
+            (match (package-replacement package)
+              (#f #t)
+              (replacement
+               (package-derivation store replacement system
+                                   #:graft? #f))))))
       (lambda args
         (make-warning package
                       (G_ "failed to create ~a derivation: ~s")
                       (list system args)))))
 
-  (filter lint-warning?
-          (map try (package-supported-systems package))))
+  (define (check-with-store store)
+    (filter lint-warning?
+            (map (cut try store <>) (package-supported-systems package))))
+
+  (or (and=> (%lint-checker-store-connection)
+             check-with-store)
+      (with-store store
+        (check-with-store store))))
 
 (define (check-license package)
   "Warn about type errors of the 'license' field of PACKAGE."
-- 
2.24.1





Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Thu, 26 Dec 2019 18:02:02 GMT) Full text and rfc822 format available.

Message #11 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 38754 <at> debbugs.gnu.org
Subject: [PATCH 2/2] scripts: lint: Set the %link-checker-store-connection
 parameter.
Date: Thu, 26 Dec 2019 18:01:04 +0000
If set, this parameter provides a store connection used by the derivation
linter. Without this being set, the derivation linter establishes a new
connection for each package. With this change, I saw the time taken to lint
all packages with the derivation linter drop from over 4 minutes to around 3
minutes.

* guix/scripts/lint.scm (guix-lint): Set the %lint-checker-store-connection)
parameter.
---
 guix/scripts/lint.scm | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

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





Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Mon, 30 Dec 2019 22:02:01 GMT) Full text and rfc822 format available.

Message #14 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 38754 <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 0/2] Speed up the derivation linter.
Date: Mon, 30 Dec 2019 23:01:21 +0100
Hi,

Christopher Baines <mail <at> cbaines.net> skribis:

> These patches speed up the derivation linter by reducing the number of
> times a connection to the guix-daemon is established. In testing on my
> computer, the time to lint all guix packages with the derivation linter
> drops from around 6 minutes to around 3 minutes.

Neat.

> Christopher Baines (2):
>   guix: lint: Add an optional parameter for a store connection.
>   scripts: lint: Set the %link-checker-store-connection parameter.

LGTM, thanks!  :-)

Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Mon, 30 Dec 2019 22:13:01 GMT) Full text and rfc822 format available.

Message #17 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 38754 <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 2/2] scripts: lint: Set the
 %link-checker-store-connection parameter.
Date: Mon, 30 Dec 2019 23:12:45 +0100
Christopher Baines <mail <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Mon, 30 Dec 2019 23:35:02 GMT) Full text and rfc822 format available.

Message #20 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 38754 <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 2/2] scripts: lint: Set the
 %link-checker-store-connection parameter.
Date: Mon, 30 Dec 2019 23:34:21 +0000
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> gnu.org> writes:

> Christopher Baines <mail <at> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Tue, 31 Dec 2019 18:16:02 GMT) Full text and rfc822 format available.

Message #23 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 38754 <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 2/2] scripts: lint: Set the
 %link-checker-store-connection parameter.
Date: Tue, 31 Dec 2019 19:15:32 +0100
Hi Chris!

Christopher Baines <mail <at> cbaines.net> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Christopher Baines <mail <at> 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’.




Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Sun, 15 Mar 2020 21:07:01 GMT) Full text and rfc822 format available.

Message #26 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 38754 <at> debbugs.gnu.org
Subject: [PATCH 4/4] scripts: lint: Handle store connections for lint checkers.
Date: Sun, 15 Mar 2020 21:06:31 +0000
Rather than individual checkers opening up a connection to the store for each
package to check, if any checker requires a store connection, open a
connection and pass it to all checkers that would use it. This makes running
the derivation checker much faster for multiple packages.

* guix/scripts/lint.scm (run-checkers): Add a #:store argument, and pass the
store to checkers if they require a store connection.
(guix-lint): Establish a store connection if any checker requires one, and
pass it through to run-checkers.
---
 guix/scripts/lint.scm | 38 ++++++++++++++++++++++++++++----------
 1 file changed, 28 insertions(+), 10 deletions(-)

diff --git a/guix/scripts/lint.scm b/guix/scripts/lint.scm
index 8d08c484f5..97ffd57301 100644
--- a/guix/scripts/lint.scm
+++ b/guix/scripts/lint.scm
@@ -30,6 +30,7 @@
   #:use-module (guix packages)
   #:use-module (guix lint)
   #:use-module (guix ui)
+  #:use-module (guix store)
   #:use-module (guix scripts)
   #:use-module (guix scripts build)
   #:use-module (gnu packages)
@@ -53,7 +54,7 @@
              (lint-warning-message lint-warning))))
    warnings))
 
-(define (run-checkers package checkers)
+(define* (run-checkers package checkers #:key store)
   "Run the given CHECKERS on PACKAGE."
   (let ((tty? (isatty? (current-error-port))))
     (for-each (lambda (checker)
@@ -63,7 +64,9 @@
                           (lint-checker-name checker))
                   (force-output (current-error-port)))
                 (emit-warnings
-                 ((lint-checker-check checker) package)))
+                 (if (lint-checker-requires-store? checker)
+                     ((lint-checker-check checker) package #:store store)
+                     ((lint-checker-check checker) package))))
               checkers)
     (when tty?
       (format (current-error-port) "\x1b[K")
@@ -167,12 +170,27 @@ run the checkers on all packages.\n"))
                              (_ #f))
                            (reverse opts)))
          (checkers (or (assoc-ref opts 'checkers) %all-checkers)))
-    (cond
-     ((assoc-ref opts 'list?)
+
+    (when (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)))))
+
+    (let ((any-lint-checker-requires-store?
+           (any lint-checker-requires-store? checkers)))
+
+      (define (call-maybe-with-store proc)
+        (if any-lint-checker-requires-store?
+            (with-store store
+              (proc store))
+            (proc #f)))
+
+      (call-maybe-with-store
+       (lambda (store)
+         (cond
+          ((null? args)
+           (fold-packages (lambda (p r) (run-checkers p checkers
+                                                      #:store store)) '()))
+          (else
+           (for-each (lambda (spec)
+                       (run-checkers (specification->package spec) checkers
+                                     #:store store))
+                     args))))))))
-- 
2.25.0





Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Sun, 15 Mar 2020 21:07:02 GMT) Full text and rfc822 format available.

Message #29 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 38754 <at> debbugs.gnu.org
Subject: [PATCH 3/4] lint: Add a #:store argument to check-derivation
Date: Sun, 15 Mar 2020 21:06:30 +0000
This can then be used to avoid opening up a store connection each time a
package needs checking.

* guix/lint.scm (check-derivation): Add a #:store argument, and pull the
handling of the store connection out of the try function.
---
 guix/lint.scm | 36 ++++++++++++++++++++----------------
 1 file changed, 20 insertions(+), 16 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index b20510b45d..cfe3be2302 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -917,9 +917,9 @@ descriptions maintained upstream."
    (define exception-with-kind-and-args?
      (const #f))))
 
-(define (check-derivation package)
+(define* (check-derivation package #:key store)
   "Emit a warning if we fail to compile PACKAGE to a derivation."
-  (define (try system)
+  (define (try store system)
     (catch #t     ;TODO: Remove 'catch' when Guile 2.x is no longer supported.
       (lambda ()
         (guard (c ((store-protocol-error? c)
@@ -938,25 +938,29 @@ descriptions maintained upstream."
                                  (G_ "failed to create ~a derivation: ~a")
                                  (list system
                                        (condition-message c)))))
-          (with-store store
-            ;; Disable grafts since it can entail rebuilds.
-            (parameterize ((%graft? #f))
-              (package-derivation store package system #:graft? #f)
-
-              ;; If there's a replacement, make sure we can compute its
-              ;; derivation.
-              (match (package-replacement package)
-                (#f #t)
-                (replacement
-                 (package-derivation store replacement system
-                                     #:graft? #f)))))))
+          (parameterize ((%graft? #f))
+            (package-derivation store package system #:graft? #f)
+
+            ;; If there's a replacement, make sure we can compute its
+            ;; derivation.
+            (match (package-replacement package)
+              (#f #t)
+              (replacement
+               (package-derivation store replacement system
+                                   #:graft? #f))))))
       (lambda args
         (make-warning package
                       (G_ "failed to create ~a derivation: ~s")
                       (list system args)))))
 
-  (filter lint-warning?
-          (map try (package-supported-systems package))))
+  (define (check-with-store store)
+    (filter lint-warning?
+            (map (cut try store <>) (package-supported-systems package))))
+
+  ;; For backwards compatability, don't rely on store being set
+  (or (and=> store check-with-store)
+      (with-store store
+        (check-with-store store))))
 
 (define (check-license package)
   "Warn about type errors of the 'license' field of PACKAGE."
-- 
2.25.0





Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Sun, 15 Mar 2020 21:07:02 GMT) Full text and rfc822 format available.

Message #32 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 38754 <at> debbugs.gnu.org
Subject: [PATCH 2/4] lint: Mark the derivation checker as requiring a store
 connection.
Date: Sun, 15 Mar 2020 21:06:29 +0000
* guix/lint.scm (%local-checkers): Mark the derivation checker as requiring a
store connection.
---
 guix/lint.scm | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 2a084382c6..b20510b45d 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -1330,9 +1330,10 @@ or a list thereof")
      (description "Check for autogenerated tarballs")
      (check       check-source-unstable-tarball))
    (lint-checker
-     (name        'derivation)
-     (description "Report failure to compile a package to a derivation")
-     (check       check-derivation))
+     (name            'derivation)
+     (description     "Report failure to compile a package to a derivation")
+     (check           check-derivation)
+     (requires-store? #t))
    (lint-checker
     (name        'patch-file-names)
     (description "Validate file names and availability of patches")
-- 
2.25.0





Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Sun, 15 Mar 2020 21:07:03 GMT) Full text and rfc822 format available.

Message #35 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: 38754 <at> debbugs.gnu.org
Subject: [PATCH 1/4] lint: Add a requires-store? field to the checker record.
Date: Sun, 15 Mar 2020 21:06:28 +0000
This can then be used to mark checkers that require a store connection, which
will enable passing a connection in, avoiding the overhead of establishing a
connection inside the check function when it's run for lots of different
packages.

* guix/lint.scm (<lint-checker>): Add requires-store? to the record type.
---
 guix/lint.scm | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/guix/lint.scm b/guix/lint.scm
index 24fbf05202..2a084382c6 100644
--- a/guix/lint.scm
+++ b/guix/lint.scm
@@ -100,7 +100,8 @@
             lint-checker?
             lint-checker-name
             lint-checker-description
-            lint-checker-check))
+            lint-checker-check
+            lint-checker-requires-store?))
 
 
 ;;;
@@ -155,7 +156,9 @@
   ;; 'certainty' level.
   (name        lint-checker-name)
   (description lint-checker-description)
-  (check       lint-checker-check))
+  (check       lint-checker-check)
+  (requires-store? lint-checker-requires-store?
+                   (default #f)))
 
 (define (properly-starts-sentence? s)
   (string-match "^[(\"'`[:upper:][:digit:]]" s))
-- 
2.25.0





Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Sun, 15 Mar 2020 21:36:02 GMT) Full text and rfc822 format available.

Message #38 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 38754 <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 2/2] scripts: lint: Set the
 %link-checker-store-connection parameter.
Date: Sun, 15 Mar 2020 21:35:28 +0000
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi Chris!
>
> Christopher Baines <mail <at> cbaines.net> skribis:
>
>> Ludovic Courtès <ludo <at> gnu.org> writes:
>>
>>> Christopher Baines <mail <at> 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
[signature.asc (application/pgp-signature, inline)]

Information forwarded to guix-patches <at> gnu.org:
bug#38754; Package guix-patches. (Tue, 24 Mar 2020 10:21:02 GMT) Full text and rfc822 format available.

Message #41 received at 38754 <at> debbugs.gnu.org (full text, mbox):

From: Ludovic Courtès <ludo <at> gnu.org>
To: Christopher Baines <mail <at> cbaines.net>
Cc: 38754 <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 2/2] scripts: lint: Set the
 %link-checker-store-connection parameter.
Date: Tue, 24 Mar 2020 11:20:31 +0100
Hi,

Christopher Baines <mail <at> 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’.




Reply sent to Christopher Baines <mail <at> cbaines.net>:
You have taken responsibility. (Tue, 24 Mar 2020 19:51:02 GMT) Full text and rfc822 format available.

Notification sent to Christopher Baines <mail <at> cbaines.net>:
bug acknowledged by developer. (Tue, 24 Mar 2020 19:51:02 GMT) Full text and rfc822 format available.

Message #46 received at 38754-done <at> debbugs.gnu.org (full text, mbox):

From: Christopher Baines <mail <at> cbaines.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: 38754-done <at> debbugs.gnu.org
Subject: Re: [bug#38754] [PATCH 2/2] scripts: lint: Set the
 %link-checker-store-connection parameter.
Date: Tue, 24 Mar 2020 19:50:07 +0000
[Message part 1 (text/plain, inline)]
Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi,
>
> Christopher Baines <mail <at> 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.
[signature.asc (application/pgp-signature, inline)]

bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Wed, 22 Apr 2020 11:24:06 GMT) Full text and rfc822 format available.

This bug report was last modified 3 years and 341 days ago.

Previous Next


GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.