Re: Package input loop detection

[ludo@gnu.org (Ludovic Courtès)]

Hi again,

Thanks Ricardo for the reminder.  :-)

Christopher Baines <mail@cbaines.net> skribis:

> 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-rails@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-guix@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-rspec@3.5.0 gnu/packages/ruby.scm:446 2b82d80>
> 	#<package ruby-thread-order@1.1.0 gnu/packages/ruby.scm:6865 2bcc300>
> 	#<package ruby-rspec-support@3.5.0 gnu/packages/ruby.scm:6822 2bcc3c0>
> 	#<package ruby-rspec-core@3.5.4 gnu/packages/ruby.scm:325 2b6d300>
>  --->	#<package ruby-rspec@3.5.0 gnu/packages/ruby.scm:446 2b82d80>
> 	#<package ruby-concurrent@1.0.5 gnu/packages/ruby.scm:4562 2bb3c00>
> 	#<package ruby-activesupport@5.1.4 gnu/packages/ruby.scm:2794 2ba0900>
> 	#<package ruby-actionview@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:267 3b619c0>
> 	#<package ruby-actionpack@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:237 3b61c00>
> 	#<package ruby-actioncable@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:183 2d9f0c0>
> 	#<package ruby-rails@5.0.0 /home/chris/Projects/Guix/guix-wip-rails/gnu/packages/rails.scm:136 2d9f300>

Neat.

> 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’.

Another comment:

> -(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’…)

> +  (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’.