GNU bug report logs - #37730
[PATCH] Topologically sort recursively-imported packages

Previous Next

Package: guix-patches;

Reported by: Brian Leung <bkleung89 <at> gmail.com>

Date: Sun, 13 Oct 2019 07:39:01 UTC

Severity: normal

Tags: patch

Done: Ludovic Courtès <ludo <at> gnu.org>

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 37730 in the body.
You can then email your comments to 37730 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#37730; Package guix-patches. (Sun, 13 Oct 2019 07:39:02 GMT) Full text and rfc822 format available.

Acknowledgement sent to Brian Leung <bkleung89 <at> gmail.com>:
New bug report received and forwarded. Copy sent to guix-patches <at> gnu.org. (Sun, 13 Oct 2019 07:39:02 GMT) Full text and rfc822 format available.

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

From: Brian Leung <bkleung89 <at> gmail.com>
To: guix-patches <at> gnu.org
Subject: [PATCH] Topologically sort recursively-imported packages
Date: Sun, 13 Oct 2019 00:37:59 -0700
[Message part 1 (text/plain, inline)]
Hi Guix,

Attached is a patch modifying the recursive importer in utils.scm.

Thanks,
Brian
[Message part 2 (text/html, inline)]
[0001-guix-utils-Topologically-sort-recursively-imported-r.patch (text/x-patch, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#37730; Package guix-patches. (Fri, 18 Oct 2019 09:32:02 GMT) Full text and rfc822 format available.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Brian Leung <bkleung89 <at> gmail.com>
Cc: 37730 <at> debbugs.gnu.org
Subject: Re: [bug#37730] [PATCH] Topologically sort recursively-imported
 packages
Date: Fri, 18 Oct 2019 11:31:26 +0200
Hi Brian,

Brian Leung <bkleung89 <at> gmail.com> skribis:

> From 6fec6a72a7938753307ccf3b7bdad8bff72e47f9 Mon Sep 17 00:00:00 2001
> From: Brian Leung <leungbk <at> mailfence.com>
> Date: Fri, 11 Oct 2019 23:18:03 -0700
> Subject: [PATCH] guix: utils: Topologically sort recursively-imported recipes.
>
> This output order, when it is well-defined, facilitates the process of
> deciding what to upstream next for a package with a large dependency closure.

That’s a great idea!

> * guix/import/utils.scm (recursive-import): Enforce topological sort.
>   Remove dependency on srfi-41. Reverse output here instead of in individual
>   importers.
> * guix/scripts/import/cran.scm (guix-import-cran): Unstreamify and don't
>   reverse here. Remove dependency on srfi-41.

Instead of “Unstreamify”, please write precisely what has changed, like
“Remove call to ‘stream-fold’ and call ‘foobar’ directly.”, “Remove call
to ‘stream->list’.”, etc.

> +  (define graph (make-hash-table))
> +  (define recipe-map (make-hash-table))
> +  (define stack (list package-name))
> +  (define accum '())
> +
> +  (while (not (null? stack))
> +    (let ((package-name (car stack)))
> +      (match (hash-ref graph package-name)
> +        ('()
> +         (set! stack (cdr stack))
> +         (set! accum (cons (hash-ref recipe-map package-name) accum)))
> +        ((dep . rest)
> +         (define (handle? dep)
> +           (and
> +            (not (equal? dep package-name))
> +            (not (hash-ref recipe-map dep))
> +            (not (exists? dep))))
> +         (hash-set! graph package-name rest)
> +         (when (handle? dep)
> +           (set! stack (cons dep stack))))
> +        (#f
> +         (receive (package-recipe . dependencies)
> +             (repo->guix-package package-name repo)
> +           (hash-set! graph package-name
> +                      (or (and (not (null? dependencies))
> +                               (car dependencies))
> +                          '()))
> +           (hash-set! recipe-map package-name
> +                      (or package-recipe '())))))))
> +
> +  (reverse accum))

Do you think you could rewrite this (1) in a functional style (you can
use vhashes instead of hash tables), and (2) using ‘match’ instead of
‘cdr’ & co.?

That would more closely match our conventions (info "(guix) Coding
Style") and would also probably allow for easier testing.

Regarding tests, you could make the topological sort code above a
separate procedure, and write a couple of tests that call it.

WDYT?

The rest LGTM.

Thank you!

Ludo’.




Information forwarded to guix-patches <at> gnu.org:
bug#37730; Package guix-patches. (Wed, 04 Dec 2019 16:51:02 GMT) Full text and rfc822 format available.

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

From: Brian Leung <bkleung89 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 
 Efraim Flashner <efraim <at> flashner.co.il>
Subject: Re: [bug#37730] [PATCH] Topologically sort recursively-imported
 packages
Date: Tue, 3 Dec 2019 14:03:02 -0800
[Message part 1 (text/plain, inline)]
Hi Ludo,

Sorry for putting this off; my Guix installation got corrupted and I wasn't
able to roll back. I'm writing this from within VirtualBox.

In the attached patch I've addressed most of your concerns, except for this
one:

> Regarding tests, you could make the topological sort code above a
> separate procedure, and write a couple of tests that call it.

I don't see how this would help. We would have to pass it the
`repo->guix-package` function and the `repo` variable as an arguments that
remain the same across all the tail-recursive invocations of `topo-sort`,
which would make it harder to read. And we'd have to come up with some
custom `repo->guix-package` function, when we already have one for the
(say) Crate test.

Efraim: I recall you mentioning a while back that topologically sorted
output would be nice to have. Please confirm this patch works as expected
for you.

Thanks,
Brian

On Fri, Oct 18, 2019 at 2:31 AM Ludovic Courtès <ludo <at> gnu.org> wrote:

> Hi Brian,
>
> Brian Leung <bkleung89 <at> gmail.com> skribis:
>
> > From 6fec6a72a7938753307ccf3b7bdad8bff72e47f9 Mon Sep 17 00:00:00 2001
> > From: Brian Leung <leungbk <at> mailfence.com>
> > Date: Fri, 11 Oct 2019 23:18:03 -0700
> > Subject: [PATCH] guix: utils: Topologically sort recursively-imported
> recipes.
> >
> > This output order, when it is well-defined, facilitates the process of
> > deciding what to upstream next for a package with a large dependency
> closure.
>
> That’s a great idea!
>
> > * guix/import/utils.scm (recursive-import): Enforce topological sort.
> >   Remove dependency on srfi-41. Reverse output here instead of in
> individual
> >   importers.
> > * guix/scripts/import/cran.scm (guix-import-cran): Unstreamify and don't
> >   reverse here. Remove dependency on srfi-41.
>
> Instead of “Unstreamify”, please write precisely what has changed, like
> “Remove call to ‘stream-fold’ and call ‘foobar’ directly.”, “Remove call
> to ‘stream->list’.”, etc.
>
> > +  (define graph (make-hash-table))
> > +  (define recipe-map (make-hash-table))
> > +  (define stack (list package-name))
> > +  (define accum '())
> > +
> > +  (while (not (null? stack))
> > +    (let ((package-name (car stack)))
> > +      (match (hash-ref graph package-name)
> > +        ('()
> > +         (set! stack (cdr stack))
> > +         (set! accum (cons (hash-ref recipe-map package-name) accum)))
> > +        ((dep . rest)
> > +         (define (handle? dep)
> > +           (and
> > +            (not (equal? dep package-name))
> > +            (not (hash-ref recipe-map dep))
> > +            (not (exists? dep))))
> > +         (hash-set! graph package-name rest)
> > +         (when (handle? dep)
> > +           (set! stack (cons dep stack))))
> > +        (#f
> > +         (receive (package-recipe . dependencies)
> > +             (repo->guix-package package-name repo)
> > +           (hash-set! graph package-name
> > +                      (or (and (not (null? dependencies))
> > +                               (car dependencies))
> > +                          '()))
> > +           (hash-set! recipe-map package-name
> > +                      (or package-recipe '())))))))
> > +
> > +  (reverse accum))
>
> Do you think you could rewrite this (1) in a functional style (you can
> use vhashes instead of hash tables), and (2) using ‘match’ instead of
> ‘cdr’ & co.?
>
> That would more closely match our conventions (info "(guix) Coding
> Style") and would also probably allow for easier testing.
>
> Regarding tests, you could make the topological sort code above a
> separate procedure, and write a couple of tests that call it.
>
> WDYT?
>
> The rest LGTM.
>
> Thank you!
>
> Ludo’.
>
[Message part 2 (text/html, inline)]
[0001-guix-utils-Topologically-sort-recursively-imported-r.patch (text/x-patch, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#37730; Package guix-patches. (Wed, 04 Dec 2019 16:51:02 GMT) Full text and rfc822 format available.

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

From: Brian Leung <bkleung89 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>, 
 Efraim Flashner <efraim <at> flashner.co.il>
Subject: Re: [bug#37730] [PATCH] Topologically sort recursively-imported
 packages
Date: Tue, 3 Dec 2019 15:06:51 -0800
[Message part 1 (text/plain, inline)]
Attached is a small edit to clarify the shape of something and rename
accordingly.

On Tue, Dec 3, 2019 at 2:03 PM Brian Leung <bkleung89 <at> gmail.com> wrote:

> Hi Ludo,
>
> Sorry for putting this off; my Guix installation got corrupted and I
> wasn't able to roll back. I'm writing this from within VirtualBox.
>
> In the attached patch I've addressed most of your concerns, except for
> this one:
>
> > Regarding tests, you could make the topological sort code above a
> > separate procedure, and write a couple of tests that call it.
>
> I don't see how this would help. We would have to pass it the
> `repo->guix-package` function and the `repo` variable as an arguments that
> remain the same across all the tail-recursive invocations of `topo-sort`,
> which would make it harder to read. And we'd have to come up with some
> custom `repo->guix-package` function, when we already have one for the
> (say) Crate test.
>
> Efraim: I recall you mentioning a while back that topologically sorted
> output would be nice to have. Please confirm this patch works as expected
> for you.
>
> Thanks,
> Brian
>
> On Fri, Oct 18, 2019 at 2:31 AM Ludovic Courtès <ludo <at> gnu.org> wrote:
>
>> Hi Brian,
>>
>> Brian Leung <bkleung89 <at> gmail.com> skribis:
>>
>> > From 6fec6a72a7938753307ccf3b7bdad8bff72e47f9 Mon Sep 17 00:00:00 2001
>> > From: Brian Leung <leungbk <at> mailfence.com>
>> > Date: Fri, 11 Oct 2019 23:18:03 -0700
>> > Subject: [PATCH] guix: utils: Topologically sort recursively-imported
>> recipes.
>> >
>> > This output order, when it is well-defined, facilitates the process of
>> > deciding what to upstream next for a package with a large dependency
>> closure.
>>
>> That’s a great idea!
>>
>> > * guix/import/utils.scm (recursive-import): Enforce topological sort.
>> >   Remove dependency on srfi-41. Reverse output here instead of in
>> individual
>> >   importers.
>> > * guix/scripts/import/cran.scm (guix-import-cran): Unstreamify and don't
>> >   reverse here. Remove dependency on srfi-41.
>>
>> Instead of “Unstreamify”, please write precisely what has changed, like
>> “Remove call to ‘stream-fold’ and call ‘foobar’ directly.”, “Remove call
>> to ‘stream->list’.”, etc.
>>
>> > +  (define graph (make-hash-table))
>> > +  (define recipe-map (make-hash-table))
>> > +  (define stack (list package-name))
>> > +  (define accum '())
>> > +
>> > +  (while (not (null? stack))
>> > +    (let ((package-name (car stack)))
>> > +      (match (hash-ref graph package-name)
>> > +        ('()
>> > +         (set! stack (cdr stack))
>> > +         (set! accum (cons (hash-ref recipe-map package-name) accum)))
>> > +        ((dep . rest)
>> > +         (define (handle? dep)
>> > +           (and
>> > +            (not (equal? dep package-name))
>> > +            (not (hash-ref recipe-map dep))
>> > +            (not (exists? dep))))
>> > +         (hash-set! graph package-name rest)
>> > +         (when (handle? dep)
>> > +           (set! stack (cons dep stack))))
>> > +        (#f
>> > +         (receive (package-recipe . dependencies)
>> > +             (repo->guix-package package-name repo)
>> > +           (hash-set! graph package-name
>> > +                      (or (and (not (null? dependencies))
>> > +                               (car dependencies))
>> > +                          '()))
>> > +           (hash-set! recipe-map package-name
>> > +                      (or package-recipe '())))))))
>> > +
>> > +  (reverse accum))
>>
>> Do you think you could rewrite this (1) in a functional style (you can
>> use vhashes instead of hash tables), and (2) using ‘match’ instead of
>> ‘cdr’ & co.?
>>
>> That would more closely match our conventions (info "(guix) Coding
>> Style") and would also probably allow for easier testing.
>>
>> Regarding tests, you could make the topological sort code above a
>> separate procedure, and write a couple of tests that call it.
>>
>> WDYT?
>>
>> The rest LGTM.
>>
>> Thank you!
>>
>> Ludo’.
>>
>
[Message part 2 (text/html, inline)]
[0001-guix-utils-Topologically-sort-recursively-imported-r.patch (text/x-patch, attachment)]

Information forwarded to guix-patches <at> gnu.org:
bug#37730; Package guix-patches. (Sun, 08 Dec 2019 17:10:02 GMT) Full text and rfc822 format available.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Brian Leung <bkleung89 <at> gmail.com>
Cc: Ricardo Wurmus <rekado <at> elephly.net>, 37730 <at> debbugs.gnu.org,
 Efraim Flashner <efraim <at> flashner.co.il>
Subject: Re: [bug#37730] [PATCH] Topologically sort recursively-imported
 packages
Date: Sun, 08 Dec 2019 18:09:14 +0100
[Message part 1 (text/plain, inline)]
Hi Brian,

Thanks for the updated patch!

Brian Leung <bkleung89 <at> gmail.com> skribis:

> From 915274d493116d063bfe2a953a9e855b8312711e Mon Sep 17 00:00:00 2001
> From: Brian Leung <leungbk <at> mailfence.com>
> Date: Fri, 11 Oct 2019 23:18:03 -0700
> Subject: [PATCH] guix: utils: Topologically sort recursively imported recipes.

[...]

> +  (define graph vlist-null)
> +  (define recipe-map vlist-null)
> +  (define stack (list package-name))
> +  (define accum '())
> +
> +  (define (topo-sort stack graph recipe-map accum)
> +    (if (null? stack)
> +        (reverse accum)
> +        (let ((head-package (car stack)))
> +          (match (vhash-assoc head-package graph)
> +            ((key . '())
> +             (let ((next-stack (cdr stack))
> +                   (next-accum (cons (cdr (vhash-assoc head-package recipe-map))
> +                                     accum)))
> +               (topo-sort next-stack
> +                          graph
> +                          recipe-map
> +                          next-accum)))
> +            ((key . (dep . rest))
> +             (define (handle? dep)
> +               (and
> +                (not (equal? dep head-package))
> +                (not (vhash-assoc dep recipe-map))
> +                (not (exists? dep))))
> +             (let* ((next-stack (if (handle? dep)
> +                                    (cons dep stack)
> +                                    stack))
> +                    (next-graph (vhash-cons key rest graph)))
> +               (topo-sort next-stack
> +                          next-graph
> +                          recipe-map
> +                          accum)))
> +            (#f
> +             (receive (package-recipe . dependencies) (repo->guix-package head-package repo)
> +               (let ((next-graph (vhash-cons head-package
> +                                             ;; dependencies has shape '(("package-a" "package-b" ...))
> +                                             (car dependencies)
> +                                             graph))
> +                     (next-recipe-map (vhash-cons head-package
> +                                                  (or
> +                                                   package-recipe
> +                                                   '())
> +                                                  recipe-map)))
> +                 (topo-sort stack
> +                            next-graph
> +                            next-recipe-map
> +                            accum))))))))
> +
> +  (topo-sort stack graph recipe-map accum))

I found this to be relatively complex (and part of this complexity was
already there before the patch) and quite different from the other
graph-walking procedures we have in different places, which got me
thinking why that is.

After a bit of researching and trying, I found that the attached patch
expresses the same thing, including topological sorting, in a hopefully
clearer way, or at least more consistent with other graph-walking
procedures in the code.  WDYT?

If that’s fine with you, I’d be willing to apply this patch, and then
apply other bits of your patch (the tests and stream removal) on top of
it.  How does that sound?

Returning a topologically-sorted set of packages means that nothing is
output until we’ve walked the whole dependency graph, so we indeed have
to get rid of streams.  I guess it’s a tradeoff.  Ricardo, how do you
feel about this change?

Thanks!

Ludo’.

[Message part 2 (text/x-patch, inline)]
diff --git a/guix/import/utils.scm b/guix/import/utils.scm
index 4694b6e7ef..bdce902d87 100644
--- a/guix/import/utils.scm
+++ b/guix/import/utils.scm
@@ -34,12 +34,14 @@
   #:use-module (guix gexp)
   #:use-module (guix store)
   #:use-module (guix download)
+  #:use-module (guix sets)
   #:use-module (gnu packages)
   #:use-module (ice-9 match)
   #:use-module (ice-9 rdelim)
   #:use-module (ice-9 receive)
   #:use-module (ice-9 regex)
   #:use-module (srfi srfi-1)
+  #:use-module (srfi srfi-9)
   #:use-module (srfi srfi-11)
   #:use-module (srfi srfi-26)
   #:use-module (srfi srfi-41)
@@ -377,40 +379,51 @@ separated by PRED."
                                       (chr (char-downcase chr)))
                                     name)))
 
+(define (topological-sort nodes
+                          node-dependencies
+                          node-name)
+  "Perform a breadth-first traversal of the graph rooted at NODES, a list of
+nodes, and return the list of nodes sorted in topological order.  Call
+NODE-DEPENDENCIES to obtain the dependencies of a node, and NODE-NAME to
+obtain a node's uniquely identifying \"key\"."
+  (let loop ((nodes nodes)
+             (result '())
+             (visited (set)))
+    (match nodes
+      (()
+       (reverse result))
+      ((head . tail)
+       (if (set-contains? visited (node-name head))
+           (loop tail result visited)
+           (let ((dependencies (node-dependencies head)))
+             (loop (append dependencies tail)
+                   (cons head result)
+                   (set-insert (node-name head) visited))))))))
+
 (define* (recursive-import package-name repo
                            #:key repo->guix-package guix-name
                            #:allow-other-keys)
   "Generate a stream of package expressions for PACKAGE-NAME and all its
 dependencies."
-  (define (exists? dependency)
-    (not (null? (find-packages-by-name (guix-name dependency)))))
-  (define initial-state (list #f (list package-name) (list)))
-  (define (step state)
-    (match state
-      ((prev (next . rest) done)
-       (define (handle? dep)
-         (and
-           (not (equal? dep next))
-           (not (member dep done))
-           (not (exists? dep))))
-       (receive (package . dependencies) (repo->guix-package next repo)
-         (list
-           (if package package '()) ;; default #f on failure would interrupt
-           (if package
-             (lset-union equal? rest (filter handle? (car dependencies)))
-             rest)
-           (cons next done))))
-      ((prev '() done)
-       (list #f '() done))))
+  (define-record-type <node>
+    (make-node name package dependencies)
+    node?
+    (name         node-name)
+    (package      node-package)
+    (dependencies node-dependencies))
 
-  ;; Generate a lazy stream of package expressions for all unknown
-  ;; dependencies in the graph.
-  (stream-unfold
-    ;; map: produce a stream element
-    (match-lambda ((latest queue done) latest))
-    ;; predicate
-    (match-lambda ((latest queue done) latest))
-    ;; generator: update the queue
-    step
-    ;; initial state
-    (step initial-state)))
+  (define (exists? name)
+    (not (null? (find-packages-by-name (guix-name name)))))
+
+  (define (lookup-node name)
+    (receive (package dependencies) (repo->guix-package name repo)
+      (make-node name package dependencies)))
+
+  (list->stream                                   ;TODO: remove streams
+   (map node-package
+        (topological-sort (list (lookup-node package-name))
+                          (lambda (node)
+                            (map lookup-node
+                                 (remove exists?
+                                         (node-dependencies node))))
+                          node-name))))

Information forwarded to guix-patches <at> gnu.org:
bug#37730; Package guix-patches. (Sun, 08 Dec 2019 21:11:02 GMT) Full text and rfc822 format available.

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

From: Brian Leung <bkleung89 <at> gmail.com>
To: Ludovic Courtès <ludo <at> gnu.org>
Subject: Re: [bug#37730] [PATCH] Topologically sort recursively-imported
 packages
Date: Sun, 8 Dec 2019 10:31:23 -0800
[Message part 1 (text/plain, inline)]
Hi Ludo,

> If that’s fine with you, I’d be willing to apply this patch, and then
> apply other bits of your patch (the tests and stream removal) on top of
> it.  How does that sound?

Sure, your patch seems clearer to me.

Thanks,
Brian

On Sun, Dec 8, 2019 at 9:09 AM Ludovic Courtès <ludo <at> gnu.org> wrote:

> Hi Brian,
>
> Thanks for the updated patch!
>
> Brian Leung <bkleung89 <at> gmail.com> skribis:
>
> > From 915274d493116d063bfe2a953a9e855b8312711e Mon Sep 17 00:00:00 2001
> > From: Brian Leung <leungbk <at> mailfence.com>
> > Date: Fri, 11 Oct 2019 23:18:03 -0700
> > Subject: [PATCH] guix: utils: Topologically sort recursively imported
> recipes.
>
> [...]
>
> > +  (define graph vlist-null)
> > +  (define recipe-map vlist-null)
> > +  (define stack (list package-name))
> > +  (define accum '())
> > +
> > +  (define (topo-sort stack graph recipe-map accum)
> > +    (if (null? stack)
> > +        (reverse accum)
> > +        (let ((head-package (car stack)))
> > +          (match (vhash-assoc head-package graph)
> > +            ((key . '())
> > +             (let ((next-stack (cdr stack))
> > +                   (next-accum (cons (cdr (vhash-assoc head-package
> recipe-map))
> > +                                     accum)))
> > +               (topo-sort next-stack
> > +                          graph
> > +                          recipe-map
> > +                          next-accum)))
> > +            ((key . (dep . rest))
> > +             (define (handle? dep)
> > +               (and
> > +                (not (equal? dep head-package))
> > +                (not (vhash-assoc dep recipe-map))
> > +                (not (exists? dep))))
> > +             (let* ((next-stack (if (handle? dep)
> > +                                    (cons dep stack)
> > +                                    stack))
> > +                    (next-graph (vhash-cons key rest graph)))
> > +               (topo-sort next-stack
> > +                          next-graph
> > +                          recipe-map
> > +                          accum)))
> > +            (#f
> > +             (receive (package-recipe . dependencies)
> (repo->guix-package head-package repo)
> > +               (let ((next-graph (vhash-cons head-package
> > +                                             ;; dependencies has shape
> '(("package-a" "package-b" ...))
> > +                                             (car dependencies)
> > +                                             graph))
> > +                     (next-recipe-map (vhash-cons head-package
> > +                                                  (or
> > +                                                   package-recipe
> > +                                                   '())
> > +                                                  recipe-map)))
> > +                 (topo-sort stack
> > +                            next-graph
> > +                            next-recipe-map
> > +                            accum))))))))
> > +
> > +  (topo-sort stack graph recipe-map accum))
>
> I found this to be relatively complex (and part of this complexity was
> already there before the patch) and quite different from the other
> graph-walking procedures we have in different places, which got me
> thinking why that is.
>
> After a bit of researching and trying, I found that the attached patch
> expresses the same thing, including topological sorting, in a hopefully
> clearer way, or at least more consistent with other graph-walking
> procedures in the code.  WDYT?
>
> If that’s fine with you, I’d be willing to apply this patch, and then
> apply other bits of your patch (the tests and stream removal) on top of
> it.  How does that sound?
>
> Returning a topologically-sorted set of packages means that nothing is
> output until we’ve walked the whole dependency graph, so we indeed have
> to get rid of streams.  I guess it’s a tradeoff.  Ricardo, how do you
> feel about this change?
>
> Thanks!
>
> Ludo’.
>
>
[Message part 2 (text/html, inline)]

Reply sent to Ludovic Courtès <ludo <at> gnu.org>:
You have taken responsibility. (Wed, 11 Dec 2019 13:02:02 GMT) Full text and rfc822 format available.

Notification sent to Brian Leung <bkleung89 <at> gmail.com>:
bug acknowledged by developer. (Wed, 11 Dec 2019 13:02:02 GMT) Full text and rfc822 format available.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Brian Leung <bkleung89 <at> gmail.com>
Cc: 37730-done <at> debbugs.gnu.org
Subject: Re: [bug#37730] [PATCH] Topologically sort recursively-imported
 packages
Date: Wed, 11 Dec 2019 12:26:12 +0100
Hi Brian,

Brian Leung <bkleung89 <at> gmail.com> skribis:

>> If that’s fine with you, I’d be willing to apply this patch, and then
>> apply other bits of your patch (the tests and stream removal) on top of
>> it.  How does that sound?
>
> Sure, your patch seems clearer to me.

I pushed patches that combine mine and yours:

  4982de4c32 import: crate: Add recursive import test.
  70a8e13277 import: utils: 'recursive-import' returns a list rather than a stream.
  ddd5915900 import: utils: 'recursive-import' returns packages in topological order.

Let me know if you notice anything wrong!

Thank you,
Ludo’.




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

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

From: Ricardo Wurmus <rekado <at> elephly.net>
To: Ludovic Courtès <ludo <at> gnu.org>
Cc: Brian Leung <bkleung89 <at> gmail.com>, 37730-done <at> debbugs.gnu.org
Subject: Re: bug#37730: [PATCH] Topologically sort recursively-imported
 packages
Date: Thu, 12 Dec 2019 16:15:01 +0100
Ludovic Courtès <ludo <at> gnu.org> writes:

> Hi Brian,
>
> Brian Leung <bkleung89 <at> gmail.com> skribis:
>
>>> If that’s fine with you, I’d be willing to apply this patch, and then
>>> apply other bits of your patch (the tests and stream removal) on top of
>>> it.  How does that sound?
>>
>> Sure, your patch seems clearer to me.
>
> I pushed patches that combine mine and yours:
>
>   4982de4c32 import: crate: Add recursive import test.
>   70a8e13277 import: utils: 'recursive-import' returns a list rather than a stream.
>   ddd5915900 import: utils: 'recursive-import' returns packages in topological order.

Thank you!

> Let me know if you notice anything wrong!

I believe the docstring of RECURSIVE-IMPORT in (guix import utils) needs
to be adjusted.  It still refers to streams.

-- 
Ricardo





Information forwarded to guix-patches <at> gnu.org:
bug#37730; Package guix-patches. (Thu, 12 Dec 2019 21:19:01 GMT) Full text and rfc822 format available.

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

From: Ludovic Courtès <ludo <at> gnu.org>
To: Ricardo Wurmus <rekado <at> elephly.net>
Cc: Brian Leung <bkleung89 <at> gmail.com>, 37730-done <at> debbugs.gnu.org
Subject: Re: bug#37730: [PATCH] Topologically sort recursively-imported
 packages
Date: Thu, 12 Dec 2019 22:18:17 +0100
Hi,

Ricardo Wurmus <rekado <at> elephly.net> skribis:

> Ludovic Courtès <ludo <at> gnu.org> writes:
>
>> Hi Brian,
>>
>> Brian Leung <bkleung89 <at> gmail.com> skribis:
>>
>>>> If that’s fine with you, I’d be willing to apply this patch, and then
>>>> apply other bits of your patch (the tests and stream removal) on top of
>>>> it.  How does that sound?
>>>
>>> Sure, your patch seems clearer to me.
>>
>> I pushed patches that combine mine and yours:
>>
>>   4982de4c32 import: crate: Add recursive import test.
>>   70a8e13277 import: utils: 'recursive-import' returns a list rather than a stream.
>>   ddd5915900 import: utils: 'recursive-import' returns packages in topological order.
>
> Thank you!
>
>> Let me know if you notice anything wrong!
>
> I believe the docstring of RECURSIVE-IMPORT in (guix import utils) needs
> to be adjusted.  It still refers to streams.

Oops, fixed!  (Will push shortly.)

Thanks,
Ludo’.




bug archived. Request was from Debbugs Internal Request <help-debbugs <at> gnu.org> to internal_control <at> debbugs.gnu.org. (Fri, 10 Jan 2020 12:24:04 GMT) Full text and rfc822 format available.

This bug report was last modified 4 years and 79 days ago.

Previous Next


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