Package input loop detection

[Christopher Baines]

I've had this issue for a while now, while adding some packages, I'll
create a loop in the package graph, which causes Guix to just loop
infinitely when trying to generate derivations.

I've included a very rough patch which detects and informs the user what
has happened, this is an example of what this looks like (with a version
of the wip-rails branch I've broken):


$ guix build ruby-rails

error: input loop detected, error generating a derivation for #<package ruby-***@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:136 2d9f300>

This shouldn't happen with Guix packages, please consider reporting a bug.

Report bugs to: bug-***@gnu.org.
GNU Guix home page: <https://www.gnu.org/software/guix/>
General help using GNU software: <http://www.gnu.org/gethelp/>

If any of the packages below are not included in Guix, it could be that one of
them is causing the loop. The packages are listed in reverse order, so the
first package listed is a input to the second package for example, and the
start and end of the detected loop is highlighted with an arrow (--->).

---> #<package ruby-***@3.5.0 gnu/packages/ruby.scm:446 2b82d80>
#<package ruby-thread-***@1.1.0 gnu/packages/ruby.scm:6865 2bcc300>
#<package ruby-rspec-***@3.5.0 gnu/packages/ruby.scm:6822 2bcc3c0>
#<package ruby-rspec-***@3.5.4 gnu/packages/ruby.scm:325 2b6d300>
---> #<package ruby-***@3.5.0 gnu/packages/ruby.scm:446 2b82d80>
#<package ruby-***@1.0.5 gnu/packages/ruby.scm:4562 2bb3c00>
#<package ruby-***@5.1.4 gnu/packages/ruby.scm:2794 2ba0900>
#<package ruby-***@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:267 3b619c0>
#<package ruby-***@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:237 3b61c00>
#<package ruby-***@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:183 2d9f0c0>
#<package ruby-***@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:136 2d9f300>


I'm not particularly fond of the implementation, because the
package-derivation function is called from expand-input called from
bag->derivation, the information about the part of the graph that has
been traversed is passed through each function.

The seen-package-list argument could be removed, but the ordering
information is really useful when printing out the error message. I
think it should be still possible to generate this after finding the
issue by searching through the graph of packages, which would allow
removing this one argument.

One other thought I had is that this could be extracted to something in
guix lint, which would at least allow finding these problems, without
touching the core derivation related code.

What do people think?

Thanks,

Chris

[Ricardo Wurmus]

Hi,

Post by Christopher Baines
I've had this issue for a while now, while adding some packages, I'll
create a loop in the package graph, which causes Guix to just loop
infinitely when trying to generate derivations.

this is a great initiative. I’ve been having this issue in the past as
well, and I’d really like Guix to be a little smarter about it.

Post by Christopher Baines
I'm not particularly fond of the implementation, because the
package-derivation function is called from expand-input called from
bag->derivation, the information about the part of the graph that has
been traversed is passed through each function.
The seen-package-list argument could be removed, but the ordering
information is really useful when printing out the error message. I
think it should be still possible to generate this after finding the
issue by searching through the graph of packages, which would allow
removing this one argument.
One other thought I had is that this could be extracted to something in
guix lint, which would at least allow finding these problems, without
touching the core derivation related code.

I’d be in favour of keeping it out of the core and stash it away in a
separate tool. Not sure if that should be “guix lint” (what about “guix
graph”?), but I would prefer that over having the code run all the time.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6 2150 197A 5888 235F ACAC
https://elephly.net

[Gábor Boskovits]

Hi,

Post by Christopher Baines
I've had this issue for a while now, while adding some packages, I'll
create a loop in the package graph, which causes Guix to just loop
infinitely when trying to generate derivations.

this is a great initiative. I’ve been having this issue in the past as
well, and I’d really like Guix to be a little smarter about it.

I also welcome this idea.

Post by Christopher Baines
I'm not particularly fond of the implementation, because the
package-derivation function is called from expand-input called from
bag->derivation, the information about the part of the graph that has
been traversed is passed through each function.
The seen-package-list argument could be removed, but the ordering
information is really useful when printing out the error message. I
think it should be still possible to generate this after finding the
issue by searching through the graph of packages, which would allow
removing this one argument.
One other thought I had is that this could be extracted to something in
guix lint, which would at least allow finding these problems, without
touching the core derivation related code.

I’d be in favour of keeping it out of the core and stash it away in a
separate tool. Not sure if that should be “guix lint” (what about “guix
graph”?), but I would prefer that over having the code run all the time.

I feel that "guix graph" is a good suggestion.
Correct me if I'm mistaken, but it seems to me that we currently traverse
our graph breadth first. For this functionality a depth first traversal
would have
benefits. Could be extended to topologically short the whole DAG. WDYT?

--
Ricardo
GPG: BCA6 89B6 3655 3801 C3C6 2150 197A 5888 235F ACAC
https://elephly.net

[Ricardo Wurmus]

Post by Ricardo Wurmus

Post by Christopher Baines
I've had this issue for a while now, while adding some packages, I'll
create a loop in the package graph, which causes Guix to just loop
infinitely when trying to generate derivations.

this is a great initiative. I’ve been having this issue in the past as
well, and I’d really like Guix to be a little smarter about it.

I’m currently updating many Haskell packages and I applied this patch to
make this job easier. It works as advertised.

I think it could be less verbose (because something like this won’t
happen in a released version of Guix and is only useful to developers).
I’m no longer convinced that it should be a separate tool; it is an
improvement over having “guix build” hang indefinitely. I’d much rather
have “guix build” fail on its own than requiring me to lose patience and
hit Ctrl-C.

--
Ricardo

GPG: BCA6 89B6 3655 3801 C3C6 2150 197A 5888 235F ACAC
https://elephly.net

[Ludovic Courtès]

Hello!

Post by Ricardo Wurmus

Post by Christopher Baines
I've had this issue for a while now, while adding some packages, I'll
create a loop in the package graph, which causes Guix to just loop
infinitely when trying to generate derivations.

this is a great initiative. I’ve been having this issue in the past as
well, and I’d really like Guix to be a little smarter about it.

+1

Post by Ricardo Wurmus

Post by Christopher Baines
I'm not particularly fond of the implementation, because the
package-derivation function is called from expand-input called from
bag->derivation, the information about the part of the graph that has
been traversed is passed through each function.
The seen-package-list argument could be removed, but the ordering
information is really useful when printing out the error message. I
think it should be still possible to generate this after finding the
issue by searching through the graph of packages, which would allow
removing this one argument.
One other thought I had is that this could be extracted to something in
guix lint, which would at least allow finding these problems, without
touching the core derivation related code.

I’d be in favour of keeping it out of the core and stash it away in a
separate tool. Not sure if that should be “guix lint” (what about “guix
graph”?), but I would prefer that over having the code run all the time.

I’d be in favor of keeping it in the core, like Chris did, provided the
code is not too complex and provided there’s no significant performance
penalty. That way, problems would always be gracefully handled.

I’ll take a look at Chris’ code soonish.

Thanks!

Ludo’.

[Ricardo Wurmus]

Hi Ludo,

Post by Ludovic Courtès

Post by Ricardo Wurmus

Post by Christopher Baines
I'm not particularly fond of the implementation, because the
package-derivation function is called from expand-input called from
bag->derivation, the information about the part of the graph that has
been traversed is passed through each function.
The seen-package-list argument could be removed, but the ordering
information is really useful when printing out the error message. I
think it should be still possible to generate this after finding the
issue by searching through the graph of packages, which would allow
removing this one argument.
One other thought I had is that this could be extracted to something in
guix lint, which would at least allow finding these problems, without
touching the core derivation related code.

I’d be in favour of keeping it out of the core and stash it away in a
separate tool. Not sure if that should be “guix lint” (what about “guix
graph”?), but I would prefer that over having the code run all the time.

I’d be in favor of keeping it in the core, like Chris did, provided the
code is not too complex and provided there’s no significant performance
penalty. That way, problems would always be gracefully handled.

When the planned package cache feature is implemented, the performance
penalty would only apply when the cache is invalid, so I think it may
not be a problem.

I’d love to see this patch or a variant of it make it into the master
branch.

--
Ricardo

[Ludovic Courtès]

Hi again,

Thanks Ricardo for the reminder. :-)

Post by Christopher Baines
I've included a very rough patch which detects and informs the user what
has happened, this is an example of what this looks like (with a version
$ guix build ruby-rails
This shouldn't happen with Guix packages, please consider reporting a bug.
GNU Guix home page: <https://www.gnu.org/software/guix/>
General help using GNU software: <http://www.gnu.org/gethelp/>
If any of the packages below are not included in Guix, it could be that one of
them is causing the loop. The packages are listed in reverse order, so the
first package listed is a input to the second package for example, and the
start and end of the detected loop is highlighted with an arrow (--->).

Neat.

Post by Christopher Baines
I'm not particularly fond of the implementation, because the
package-derivation function is called from expand-input called from
bag->derivation, the information about the part of the graph that has
been traversed is passed through each function.
The seen-package-list argument could be removed, but the ordering
information is really useful when printing out the error message. I
think it should be still possible to generate this after finding the
issue by searching through the graph of packages, which would allow
removing this one argument.

‘set->list’ preserves the order (actually the reverse order, but we
could fix that) of insertion, because it’s just a vhash, and a vhash is
just a list, which has a notion of ordering obviously:

--8<---------------cut here---------------start------------->8---
scheme@(guile-user)> (set->list (setq 'a 'b 'c 'd))
$4 = (d c b a)
scheme@(guile-user)> (set->list (apply set (map list (iota 4))))
$5 = ((3) (2) (1) (0))
scheme@(guile-user)> (set->list (apply set (iota 4)))
$6 = (3 2 1 0)
--8<---------------cut here---------------end--------------->8---

Thus we can get rid of ‘seen-package-list’.

Post by Christopher Baines
-(define* (expand-input store package input system #:optional cross-system)
+(define* (expand-input store package input system #:optional cross-system
+ #:key seen-packages seen-package-list)

Maybe s/seen-packages/visited/

(I’m not fond of passing an extra parameter around, but the only way to
avoid it would be to use a state monad, and that in turn would work
better once we’ve finally merged ‘wip-build-systems-gexp’…)

Post by Christopher Baines
+ (if (set-contains? seen-packages package)
+ (begin
+ (simple-format #t "\nerror: input loop detected, error generating a derivation for ~A\n"
+ (last seen-package-list))
+ (display "
+This shouldn't happen with Guix packages, please consider reporting a bug.\n")
+ (show-bug-report-information)
+ (display "
+If any of the packages below are not included in Guix, it could be that one of
+them is causing the loop. The packages are listed in reverse order, so the
+first package listed is a input to the second package for example, and the
+start and end of the detected loop is highlighted with an arrow (--->).\n\n")
+ (for-each (lambda (seen-package)
+ (if (eq? package seen-package)
+ (display " --->"))
+ (simple-format #t "\t~A\n" seen-package))
+ (cons package
+ seen-package-list))
+ (exit 1)))

Please just define a condition type and:

(raise (condition (&package-cycle …)))

with the UI part of it moved to (guix ui) in ‘call-with-error-handling’
with proper i18n.

I think we’d need two more things:

1. Timing and memory reported by, say:

time guix build libreoffice certbot pigx -d --no-grafts

before and after the change. We should make sure the overhead in
time and space is minimal.

2. One or two tests in tests/packages.scm that check whether the
exception is raised when it should.

Could you look into it, Chris?

Thanks, and apologies for the long delay!

Ludo’.