Re: hackage importer

[Federico Beffa]

Thanks for the review!

I'm currently on a business trip. Will look carefully at your comments
when I will be back.

Regards,
Fede

On Sun, Mar 15, 2015 at 2:38 PM, Ludovic Courtès <address@hidden> wrote:
> Federico Beffa <address@hidden> 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’.