Re: hackage importer

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

Howdy!

Federico Beffa <beffa@ieee.org> skribis:

> On Sat, May 2, 2015 at 2:48 PM, Ludovic Courtès <ludo@gnu.org> 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 => expr) form of cond clauses. Very useful!
> - The nested use of the >>= function didn't look nice and the reason
> is that it is really meant as a way to sequence monadic functions as
> in (>>= m f1 f2 ...).  Unfortunately the current version of >>= 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 <cabal-package>
>>> +  (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 “keywords”, not “keyboards.”  :-)

>> 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 ‘read-cabal’ and ‘eval-cabal’ 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#package-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 <beffa@fbengineering.ch>
> Date: Sun, 26 Apr 2015 11:22:29 +0200
> Subject: [PATCH] import: hackage: Refactor parsing code and add new options.
>
> * 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 '--stdin'
>   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! is the
> +only one requiring a second argument corresponding to the object to be added
> +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
‘make-cabal-parser’?

I’ve 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’m a bit scared when we add new code *and* remove tests.  ;-)

Could you add a couple of representative tests?  I’m sure you ran tests
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’.