From mboxrd@z Thu Jan 1 00:00:00 1970 From: Federico Beffa Subject: Re: hackage importer Date: Sun, 15 Mar 2015 22:29:51 +0000 Message-ID: References: <87k2yiqqaw.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]:52017) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1YXH2h-0001hv-MX for guix-devel@gnu.org; Sun, 15 Mar 2015 18:29:57 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1YXH2g-0007BA-5T for guix-devel@gnu.org; Sun, 15 Mar 2015 18:29:55 -0400 In-Reply-To: <87k2yiqqaw.fsf@gnu.org> 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: =?UTF-8?Q?Ludovic_Court=C3=A8s?= Cc: Guix-devel 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=C3=A8s wrote: > Federico Beffa 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#confi= gurations > > 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 =E2=80=98ghc-=E2=80=99 right from the start, since that=E2=80= =99s really what it > is. WDYT? > >> Obviously the tests in part 5 were used along the way and will be >> removed. > > More precisely, they=E2=80=99ll 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 =E2=80=98read-cabal=E2=80=99 that would d= o the actual > parsing and return a first-class cabal object, and then =E2=80=98eval-cab= al=E2=80=99 (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 =E2=80=98foo->bar=E2=80=99. > > 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=E2=80=99l= l > become outdated eventually? > >> +(define guix-name-prefix "haskell-") > > s/guix-name-prefix/package-name-prefix/ and rather =E2=80=9Cghc-=E2=80=9D= 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 =E2=80=98regexp-exec=E2=80=99 in= stead of > =E2=80=98string-match=E2=80=99. > >> +;; 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 c= omments. >> +;; 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 =E2=80=98read-cabal=E2=80=99 to return a record, s= ay, that can be > directly manipulated (just like =E2=80=98read=E2=80=99 returns a Scheme o= bject.) 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 {} > > =E2=80=9Conly indentation-structured files=E2=80=9D > >> +;; structured files are not handled. >> +(define (cabal->key-values lines) > > I think this is the one I would call =E2=80=98read-cabal=E2=80=99. > >> +;; Find if a string represent a conditional >> +(define condition-rx >> + (make-regexp "^if +(.*)$")) > > The comment should rather be =E2=80=9CRegexp for conditionals.=E2=80=9D a= nd be placed > below =E2=80=98define=E2=80=99. > >> +;; Part 3: >> +;; >> +;; So far we did read the cabal file and extracted flags information. N= ow 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 =E2=80=98read-cabal=E2=80=99 r= eturns 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 =E2=80=98hackage-name->package-name=E2=80=99? > >> +;; Split the comma separated list of dependencies coming from the cabal= file >> +;; and return a list with inputs suitable for the GUIX package. Curren= tly 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 ge= nerated > > =E2=80=9CGenerate=E2=80=9D > >> +;; S-expressions may include conditionals as defined in the cabal file. >> +;; During this process we discard the version information of the packag= es. >> +(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 =E2=80=98ghc-standard-libraries= =E2=80=99 is > already lowercase.) > >> +;; Part 5: >> +;; >> +;; Some variables used to test the varisous functions. > > =E2=80=9Cvarious=E2=80=9D > >> +(define test-dep-3 >> + '((("executable cabal" "if flag(cips)" "build-depends") >> + ("fbe >=3D 0.2")) >> + (("executable cabal" "else" "build-depends") >> + ("fbeee >=3D 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 tes= t-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-Opt= ions") >> +;; (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-NA= ME >> +includes a suffix constituted by a dash followed by a numerical version= (as >> +used with GUIX packages), then a definition for the specified version o= f the > > s/GUIX/Guix/ > > Also, guix/scripts/import.scm must be added to po/guix/POTFILES.in, for > i18n. > > Thanks! > > Ludo=E2=80=99.