From mboxrd@z Thu Jan 1 00:00:00 1970 From: ludo@gnu.org (Ludovic =?utf-8?Q?Court=C3=A8s?=) Subject: Re: hackage importer Date: Fri, 05 Jun 2015 09:30:45 +0200 Message-ID: <87sia6pq6y.fsf@gnu.org> References: <87k2yiqqaw.fsf@gnu.org> <871tkbdhwc.fsf@gnu.org> <871tk7ykfj.fsf@gnu.org> <87zj6t9tq5.fsf@gnu.org> <87pp6j6t85.fsf@gnu.org> 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]:44064) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0m5d-0003MS-5q for guix-devel@gnu.org; Fri, 05 Jun 2015 03:30:58 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Z0m5Y-0005PC-Vx for guix-devel@gnu.org; Fri, 05 Jun 2015 03:30:53 -0400 Received: from fencepost.gnu.org ([2001:4830:134:3::e]:36857) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Z0m5Y-0005Os-Lj for guix-devel@gnu.org; Fri, 05 Jun 2015 03:30:48 -0400 In-Reply-To: (Federico Beffa's message of "Mon, 1 Jun 2015 17:20:06 +0200") 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-bounces+gcggd-guix-devel=m.gmane.org@gnu.org To: Federico Beffa Cc: Guix-devel Howdy! Federico Beffa skribis: > On Sat, May 2, 2015 at 2:48 PM, Ludovic Court=C3=A8s wrote: [...] >> This procedure is intimidating. I think this is partly due to its >> length, to the big let-values, the long identifiers, the many local >> variables, nested binds, etc. > > Ok, this procedure has now ... disappeared ... or rather it is now > hidden in a huge, but invisible macro ;-) > I've added support for braces delimited blocks. In so doing the > complexity of an ad-hoc solution increased further and decided that it > was time to study (and use) a proper parser. Yay, good idea! > But, a couple of words on your remarks: > > - Thanks to your comment about long list of local variables I > (re-)discovered the (test =3D> expr) form of cond clauses. Very useful! > - The nested use of the >>=3D function didn't look nice and the reason > is that it is really meant as a way to sequence monadic functions as > in (>>=3D m f1 f2 ...). Unfortunately the current version of >>=3D in > guile only accepts 2 arguments (1 function), hence the nesting. It > would be nice to correct that :-) Sure, I have it in my to-do list. :-) I looked at it quickly and it seems less trivial than initially envisioned though. >>> +(define-record-type >>> + (make-cabal-package name version license home-page source-repository >>> + synopsis description >>> + executables lib test-suites >>> + flags eval-environment) >>> + cabal-package? >>> + (name cabal-package-name) >>> + (version cabal-package-version) >>> + (license cabal-package-license) >>> + (home-page cabal-package-home-page) >>> + (source-repository cabal-package-source-repository) >>> + (synopsis cabal-package-synopsis) >>> + (description cabal-package-description) >>> + (executables cabal-package-executables) >>> + (lib cabal-package-library) ; 'library' is a Scheme keyword >> >> There are no keyboards in Scheme. :-) > > ?? Oops, these should have read =E2=80=9Ckeywords=E2=80=9D, not =E2=80=9Ckeybo= ards.=E2=80=9D :-) >> The existing tests here are fine, but they are more like integration >> tests (they test the whole pipeline.) Maybe it would be nice to >> directly exercise =E2=80=98read-cabal=E2=80=99 and =E2=80=98eval-cabal= =E2=80=99 individually? > > It is true that the tests are for the whole pipeline, but they catch > most of the problems (problems in any function along the chain) with > the smallest number of tests :-). I'm not very keen in doing fine > grained testing. Sorry. > > I've removed the test with TABs because the Cabal documentation says > explicitly that they are not allowed. > https://www.haskell.org/cabal/users-guide/developing-packages.html#packag= e-descriptions But are they actually used in practice? > I've changed the second test to check the use of braces (multi-line > values have still to be indented). OK. > From f422ea9aff3aa8425c80eaadf50628c24d54495a Mon Sep 17 00:00:00 2001 > From: Federico Beffa > Date: Sun, 26 Apr 2015 11:22:29 +0200 > Subject: [PATCH] import: hackage: Refactor parsing code and add new optio= ns. > > * guix/import/cabal.scm: New file. > * guix/import/hackage.scm: Update to use the new Cabal parsing module. > * tests/hackage.scm: Update tests. > * guix/scripts/import/hackage.scm: Add new '--cabal-environment' and '--s= tdin' > options. > * doc/guix.texi: ... and document them. > * Makefile.am (MODULES): Add 'guix/import/cabal.scm', > 'guix/import/hackage.scm' and 'guix/scripts/import/hackage.scm'. > (SCM_TESTS): Add 'tests/hackage.scm'. [...] > +(define (make-stack) > + "Creates a simple stack closure. Actions on the generated stack are > +requested by calling it with one of the following symbols as the first > +argument: 'empty?, 'push!, 'top, 'pop! and 'clear!. The action 'push! i= s the > +only one requiring a second argument corresponding to the object to be a= dded > +to the stack." > + (let ((stack '())) > + (lambda (msg . args) > + (cond ((eqv? msg 'empty?) (null? stack)) > + ((eqv? msg 'push!) (set! stack (cons (first args) stack))) > + ((eqv? msg 'top) (if (null? stack) '() (first stack))) > + ((eqv? msg 'pop!) (match stack > + ((e r ...) (set! stack (cdr stack)) e) > + (_ #f))) > + ((eqv? msg 'clear!) (set! stack '())) > + (else #f))))) Fair enough. :-) I wonder what happens exactly when trying to return monadic values in the parser. > +;; Stack to track the structure of nested blocks > +(define context-stack (make-stack)) What about making it either a SRFI-39 parameter, or a parameter to =E2=80=98make-cabal-parser=E2=80=99? I=E2=80=99ve only read through quickly, but the rest of the file looks lean= and clean! > +(define* (hackage->guix-package package-name #:key > + (include-test-dependencies? #t) > + (read-from-stdin? #f) > + (cabal-environment '())) Instead of #:read-from-stdin?, what about adding a #:port argument? That way, it would either read from PORT, or fetch from Hackage. That seems more idiomatic and more flexible. Also please mention it in the docstring. > -(test-assert "hackage->guix-package test 3" > - (eval-test-with-cabal test-cabal-3)) > - > -(test-assert "conditional->sexp-like" > - (match > - (eval-cabal-keywords > - (conditional->sexp-like test-cond-1) > - '(("debug" . "False"))) > - (('and ('or ('string-match "darwin" ('%current-system)) ('not '#f)) = '#t) > - #t) > - (x > - (pk 'fail x #f)))) I=E2=80=99m a bit scared when we add new code *and* remove tests. ;-) Could you add a couple of representative tests? I=E2=80=99m sure you ran t= ests by hand at the REPL, so it should be a matter of adding them in the file. Thanks for the nice refactoring & new features! Ludo=E2=80=99.