From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: Package input loop detection Date: Mon, 23 Apr 2018 18:07:55 +0200 Message-ID: <87muxtykh0.fsf@gnu.org> References: <87eflzfkwh.fsf@cbaines.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable Return-path: Received: from eggs.gnu.org ([2001:4830:134:3::10]:36849) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1fAe0Z-0001qb-IH for guix-devel@gnu.org; Mon, 23 Apr 2018 12:08:04 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1fAe0V-0000oH-Du for guix-devel@gnu.org; Mon, 23 Apr 2018 12:08:03 -0400 Received: from hera.aquilenet.fr ([2a0c:e300::1]:50738) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1fAe0V-0000nq-2M for guix-devel@gnu.org; Mon, 23 Apr 2018 12:07:59 -0400 In-Reply-To: <87eflzfkwh.fsf@cbaines.net> (Christopher Baines's message of "Mon, 05 Feb 2018 16:42:22 +0000") List-Id: "Development of GNU Guix and the GNU System distribution." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: guix-devel-bounces+gcggd-guix-devel=m.gmane.org@gnu.org Sender: "Guix-devel" To: Christopher Baines Cc: guix-devel@gnu.org Hi again, Thanks Ricardo for the reminder. :-) Christopher Baines 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 # > > This shouldn't happen with Guix packages, please consider reporting a bug. > > Report bugs to: bug-guix@gnu.org. > GNU Guix home page: > General help using GNU software: > > If any of the packages below are not included in Guix, it could be that o= ne 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. > 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. =E2=80=98set->list=E2=80=99 preserves the order (actually the reverse order= , but we could fix that) of insertion, because it=E2=80=99s just a vhash, and a vhas= h 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 =3D (d c b a) scheme@(guile-user)> (set->list (apply set (map list (iota 4)))) $5 =3D ((3) (2) (1) (0)) scheme@(guile-user)> (set->list (apply set (iota 4))) $6 =3D (3 2 1 0) --8<---------------cut here---------------end--------------->8--- Thus we can get rid of =E2=80=98seen-package-list=E2=80=99. Another comment: > -(define* (expand-input store package input system #:optional cross-syste= m) > +(define* (expand-input store package input system #:optional cross-system > + #:key seen-packages seen-package-list) Maybe s/seen-packages/visited/ (I=E2=80=99m not fond of passing an extra parameter around, but the only wa= y to avoid it would be to use a state monad, and that in turn would work better once we=E2=80=99ve finally merged =E2=80=98wip-build-systems-gexp=E2= =80=99=E2=80=A6) > + (if (set-contains? seen-packages package) > + (begin > + (simple-format #t "\nerror: input loop detected, error generatin= g a derivation for ~A\n" > + (last seen-package-list)) > + (display " > +This shouldn't happen with Guix packages, please consider reporting a bu= g.\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 t= he > +first package listed is a input to the second package for example, and t= he > +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 =E2=80=A6))) with the UI part of it moved to (guix ui) in =E2=80=98call-with-error-handl= ing=E2=80=99 with proper i18n. I think we=E2=80=99d 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=E2=80=99.