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 <l...@gnu.org> wrote: > Federico Beffa <be...@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’.
