Re: hackage importer

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

Federico Beffa <beffa@ieee.org> skribis:

> please find attached an initial version of an importer for packages
> from Hackage:
> http://hackage.haskell.org/

Woow, impressive piece of work!

[...]

> * The code handles dependencies with conditionals and tries to comply
> with the description at
> https://www.haskell.org/cabal/users-guide/developing-packages.html#configurations

Neat.

> * Cabal files include dependencies to libraries included with the
> complier (at least with GHC). For the moment I just filter those out
> assuming the packages are going to be compiled with GHC.

Sounds good.

> * The generated package name is prefixed with "haskell-" in a similar
> way to Perl and Python packages. However, the cabal file includes
> checks for the compiler implementation and the importer handles that
> and keep the test in the generated package (see eval-impl in the code
> and the description in the above link). If in the future there is
> interest in supporting other Haskell compilers, then maybe we should
> better prefix the packages according to the used compiler ("ghc-"
> ...).

I would use ‘ghc-’ right from the start, since that’s really what it
is.  WDYT?

> Obviously the tests in part 5 were used along the way and will be
> removed.

More precisely, they’ll have to be turned into tests/hackage.scm
(similar to tests/snix.scm and tests/pypi.scm.)  :-)

This looks like really nice stuff.  There are patterns that are not
sufficiently apparent IMO, like ‘read-cabal’ that would do the actual
parsing and return a first-class cabal object, and then ‘eval-cabal’ (or
similar) that would evaluate the conditionals in a cabal object.  For
the intermediate steps, I would expect conversion procedures from one
representation to another, typically ‘foo->bar’.

Some comments below.

> +;; List of libraries distributed with ghc (7.8.4).
> +(define ghc-standard-libraries
> +  '("haskell98"        ; 2.0.0.3
> +    "hoopl"            ; 3.10.0.1

Maybe the version numbers in comments can be omitted since they’ll
become outdated eventually?

> +(define guix-name-prefix "haskell-")

s/guix-name-prefix/package-name-prefix/ and rather “ghc-” IMO.

> +;; Regular expression matching "key: value"
> +(define key-value-rx
> +  "([a-zA-Z0-9-]+): *(\\w?.*)$")
> +
> +;; Regular expression matching a section "head sub-head ..."
> +(define sections-rx
> +  "([a-zA-Z0-9\\(\\)-]+)")
> +
> +;; Cabal comment.
> +(define comment-rx
> +  "^ *--")

Use (make-regexp ...) directly, and then ‘regexp-exec’ instead of
‘string-match’.

> +;; Check if the current line includes a key
> +(define (has-key? line)
> +  (string-match key-value-rx line))
> +
> +(define (comment-line? line)
> +  (string-match comment-rx line))
> +
> +;; returns the number of indentation spaces and the rest of the line.
> +(define (line-indentation+rest line)

Please turn all the comments above procedures this into docstrings.

> +;; Part 1 main function: read a cabal fila and filter empty lines and comments.
> +;; Returns a list composed by the pre-processed lines of the file.
> +(define (read-cabal port)

s/fila/file/
s/Returns/Return/

I would expect ‘read-cabal’ to return a <cabal> record, say, that can be
directly manipulated (just like ‘read’ returns a Scheme object.)  But
here it seems to return an intermediate parsing result, right?

> +;; Parses a cabal file in the form of a list of lines as produced by
> +;; READ-CABAL and returns its content in the following form:
> +;;
> +;; (((head1 sub-head1 ... key1) (value))
> +;;  ((head2 sub-head2 ... key2) (value2))
> +;;  ...).
> +;;
> +;; where all elements are strings.
> +;;
> +;; We try do deduce the format from the following document:
> +;; https://www.haskell.org/cabal/users-guide/developing-packages.html
> +;;
> +;; Key values are case-insensitive. We therefore lowercase them. Values are
> +;; case-sensitive.
> +;;
> +;; Currently only only layout structured files are parsed.  Braces {}

“only indentation-structured files”

> +;; structured files are not handled.
> +(define (cabal->key-values lines)

I think this is the one I would call ‘read-cabal’.

> +;; Find if a string represent a conditional
> +(define condition-rx
> +  (make-regexp "^if +(.*)$"))

The comment should rather be “Regexp for conditionals.” and be placed
below ‘define’.

> +;; Part 3:
> +;;
> +;; So far we did read the cabal file and extracted flags information. Now we
> +;; need to evaluate the conditions and process the entries accordingly.

I would expect the conversion of conditional expressions to sexps to be
done in the parsing phase above, such that ‘read-cabal’ returns an
object with some sort of an AST for those conditionals.

Then this part would focus on the evaluation of those conditionals,
like:

  ;; Evaluate the conditionals in CABAL according to FLAGS.  Return an
  ;; evaluated Cabal object.
  (eval-cabal cabal flags)

WDYT?

> +(define (guix-name name)

Rather ‘hackage-name->package-name’?

> +;; Split the comma separated list of dependencies coming from the cabal file
> +;; and return a list with inputs suitable for the GUIX package.  Currently the
> +;; version information is discarded.

s/GUIX/Guix/

> +(define (split-dependencies ls)
> +  (define (split-at-comma d)
> +    (map
> +     (lambda (m)
> +       (let ((name (guix-name (match:substring m 1))))
> +         (list name (list 'unquote (string->symbol name)))))
> +     (list-matches dependencies-rx d)))

I think it could use simply:

  (map string-trim-both
       (string-tokenize d (char-set-complement (char-set #\,))))

> +;; Genrate an S-expression containing the list of dependencies.  The generated

“Generate”

> +;; S-expressions may include conditionals as defined in the cabal file.
> +;; During this process we discard the version information of the packages.
> +(define (dependencies-cond->sexp meta)

[...]

> +                  (match (match:substring rx-result 1)
> +                    ((? (cut member <>
> +                             ;; GUIX names are all lower-case.
> +                             (map (cut string-downcase <>)
> +                                  ghc-standard-libraries)))

s/GUIX/Guix/

I find it hard to read.  Typically, I would introduce:

 (define (standard-library? name)
   (member name ghc-standard-libraries))

and use it here (with the assumption that ‘ghc-standard-libraries’ is
already lowercase.)

> +;; Part 5:
> +;;
> +;; Some variables used to test the varisous functions.

“various”

> +(define test-dep-3
> +  '((("executable cabal" "if flag(cips)" "build-depends")
> +     ("fbe      >= 0.2"))
> +    (("executable cabal" "else" "build-depends")
> +     ("fbeee      >= 0.3"))
> +    ))

No hanging parens please.

> +;; Run some tests
> +
> +;; (display (cabal->key-values
> +;;           (call-with-input-file "mtl.cabal" read-cabal)))
> +;; (display (cabal->key-values
> +;;           (call-with-input-file "/home/beffa/tmp/cabal-install.cabal" read-cabal)))
> +;; (display (get-flags (pre-process-entries-keys (cabal->key-values test-5))))
> +;; (newline)
> +;; (display (conditional->sexp-like test-cond-2))
> +;; (newline)
> +;; (display
> +;;  (eval-flags (conditional->sexp-like test-cond-6)
> +;;              (get-flags (pre-process-entries-keys (cabal->key-values test-6)))))
> +;; (newline)
> +;; (key->values (cabal->key-values test-1) "name")
> +;; (newline)
> +;; (key-start-end->entries (cabal->key-values test-4) "Library" "CC-Options")
> +;; (newline)
> +;; (eval-tests (conditional->sexp-like test-cond-6))

This should definitely go to tests/hackage.scm.

> +  (display (_ "Usage: guix import hackage PACKAGE-NAME
> +Import and convert the Hackage package for PACKAGE-NAME.  If PACKAGE-NAME
> +includes a suffix constituted by a dash followed by a numerical version (as
> +used with GUIX packages), then a definition for the specified version of the

s/GUIX/Guix/

Also, guix/scripts/import.scm must be added to po/guix/POTFILES.in, for
i18n.

Thanks!

Ludo’.