Re: [PATCH] gnu: Add scmutils.

[Ludovic Courtès]

Federico Beffa <address@hidden> skribis:

> From 1aacd03326def7b52b8166a80cc18c8e8226aa62 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Thu, 13 Aug 2015 18:58:01 +0200
> Subject: [PATCH] gnu: Add scmutils.
>
> * gnu/packages/scheme.scm (scmutils): New variable.

Overall LGTM.  The comments below are largely cosmetic:

> +;; FIXME: This function is temporarily in the engineering module and not
> +;; exported.  It will be moved to an utility module for general use.  Once
> +;; this is done, we should remove this definition.
> +(define broken-tarball-fetch
> +  (@@ (gnu packages engineering) broken-tarball-fetch))

Not important for this patch, but eventually we could always make a
(guix tarbomb-fetch) module that would export this procedure.

> +         (modify-phases %standard-phases

Since the phases are nontrivial, I would suggest adding a comment right
before the ‘lambda*’ line of each phase to briefly mention what it does.

> +           (add-before 'install 'fix-bin

Maybe ‘fix-directory-names’?

> +                    (lambda* (#:key outputs #:allow-other-keys)
> +                      (define* (copy-files-to-directory files dir
> +                                                        #:optional (delete? 
> #f))
> +                        (for-each (lambda (f)
> +                                    (copy-file f (string-append dir "/" f))

Note for later: once ‘core-updates’ is merged, we should write this as:

  (install-file f dir)

> +                                    (when delete? (delete-file f)))
> +                                  files))
> +
> +                      (let* ((out (assoc-ref outputs "out"))
> +                             (bin (string-append out "/bin"))
> +                             (doc (string-append out "/share/doc/"
> +                                                 ,name "-" ,version))
> +                             (lib (string-append out "/lib/mit-scheme-"
> +                                                 ,(system-suffix)
> +                                                 "/scmutils")))
> +                        (for-each (lambda (d) (mkdir-p d)) (list lib doc 
> bin))

This can be written as:

  (for-each mkdir-p (list …))

> +                        (with-directory-excursion "scmutils/scmutils"
> +                          (copy-files-to-directory '("COPYING" "LICENSE")
> +                                                   doc #t)
> +                          (for-each (lambda (f) (delete-file f))
> +                                    (find-files "." "\\.bin"))

  (for-each delete-file (find-files …))

This phase deletes a bunch of pre-compiled files, which are those Mark
and you were referring to, IIUC.  Could you move the removal of all the
pre-compiled files to a ‘snippet’?

This will ensure that ‘guix build -S scmutils’ provides only source code.

> +                               (emacs-lisp-dir
> +                                (string-append out "/share/emacs/site-lisp"))

This should be:

  (string-append out "/share/emacs/site-lisp/guix.d/" ,name "-" ,version)

That way, when installing the package from Emacs, it will automatically
be found and loaded (I realize the manual doesn’t explain it, but it
probably should.)

> +      (supported-systems '("x86_64-linux" "i686-linux"))

Please add a comment above explaining why this is the case.

> +      (synopsis "The Scmutils library for MIT Scheme")

-The

Well, this was not an easy package.  Thanks for working on it!

Ludo’.