Re: [PATCH] build-system: Add haskell-build-system.

[Ludovic Courtès]

Federico Beffa <address@hidden> skribis:

> Please note that installed libraries need to be registered in a
> compiler specific database. The database consists of a directory with
> a configuration file for each library and *a single binary cache*
> file. To avoid clashes in profiles, the build system does not create
> the cache.

Isn’t there any environment variable akin to LD_LIBRARY_PATH or
GUILE_LOAD_PATH?  That would greatly simplify things.

> To overcome clash problems: at build time the build system creates a
> temporary library database to make GHC find input libraries. However,
> when a user installs a library in his profile we need to create a
> database cache.
>
> I would like to propose to use the same mechanism as used for the
> "dir" file of texinfo.
> WDYT?

If there’s no environment variable to specify the search path, we could
do that, yes.  It would be nice to check how Nixpkgs handles it.

> Binary files seems to include absolute path to libraries and do not
> require any special handling. This impression comes from inspecting
> haddock's derivation created with this build system.

That’s nice (nicer than propagating everything like we do for Python
modules, Perl modules, etc.)

> From b8c6b21e01c6eb7d9c5c7e9ec28a4ac7d12b0628 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Fri, 27 Mar 2015 09:36:56 +0100
> Subject: [PATCH] build-system: Add haskell-build-system.
>
> * guix/build-system/haskell.scm: New file.
> * guix/build/haskell-build-system.scm: New file.

Overall LGTM (assuming there’s no environment variable search path we
could use.)

Some superficial comments below:

> +;; Directory where we create the temporary libraries database with its cache
> +;; as required by the compiler.
> +(define %tmp-db-dir

It would be nice to explain a bit, as you did in this message, what the
library database is and why we’re creating a temporary database.

> +(define (call-setuphs command params)

Rather ‘invoke-setuphs’ or ‘run-setuphs’ (‘call’ is more for a function,
I think.)

> +        (begin
> +          (format
> +           #t
> +           "running \"runhaskell Setup.hs\" with command ~s and parameters 
> ~s~%"
> +           command params)

I would rather write it like this:

             (format #t "running \"runhaskell Setup.hs\" with command ~s \
and parameters ~s~%"
                     command params)

> +(define (compiler-name-version haskell-input)
> +  (let* ((base (basename haskell-input)))
> +    (string-drop base
> +                 (+ 1 (string-index base #\-)))))
> +
> +(define (grep rx port)
> +  (let ((line (read-line port)))
> +    (if (eof-object? line)
> +        #f
> +        (let ((rx-result (regexp-exec rx line)))
> +          (if rx-result
> +              (match:substring rx-result 1)
> +              (grep rx port))))))
> +
> +(define* (setup-compiler #:key system inputs outputs #:allow-other-keys)
> +  (let* ((haskell (assoc-ref inputs "haskell"))
> +         (name-version (compiler-name-version haskell)))
> +    (cond
> +     ((string-match "ghc" name-version)
> +      (setup-ghc system inputs outputs))
> +     (else
> +      (format #t
> +              "Compiler ~a not supported~%" name-version)))))
> +
> +(define (setup-ghc system inputs outputs)

Docstrings please.

Also s/setup-ghc/make-ghc-package-database/ or something like that.

> +  (let* ((haskell  (assoc-ref inputs "haskell"))
> +         (input-dirs (match inputs
> +                       (((_ . dir) ...)
> +                        dir)
> +                       (_ '())))
> +         (conf-dirs (search-path-as-list
> +                     `(,(string-append "lib/" system "-"
> +                                       (compiler-name-version haskell)
> +                                       "/package.conf.d"))
> +                     input-dirs)))
> +    (mkdir-p %tmp-db-dir)
> +    (for-each
> +     (lambda (dir)
> +       (let ((conf-files
> +              (find-files dir ".*\\.conf")))
> +         (unless (null? conf-files)
> +           (for-each
> +            (lambda (file)
> +              (copy-file file (string-append %tmp-db-dir "/" (basename 
> file))))
> +            conf-files))))
> +     conf-dirs)

The nested ‘for-each’ is unpleasant to the eye, IMO, and difficult to
reason about.

What about first constructing the list of files, and then iterating over
them with a single ‘for-each’, like:

  (let ((conf-files (append-map (cut find-files <> "\\.conf$") conf-dirs)))
    (for-each (lambda (conf)
                (copy-file conf ...))
              conf-files))

(Note also that "\\.conf$" is more accurate and shorter than
".*\\.conf".)

> +(define* (check #:key tests? test-target #:allow-other-keys)
> +  "Run the test suite of a given Haskell package."
> +  (if tests?
> +      (call-setuphs test-target '())
> +      #t))

Would be nice to print a message when tests are skipped, as done in
gnu-build-system.scm.

Thanks!

Ludo’.