Re: [PATCH 3/5] build: Add 'emacs-build-system'

[Ludovic Courtès]

Federico Beffa <address@hidden> skribis:

> From 725d42eb3e5c44bcec1bd81988d1f952e6be02a4 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <address@hidden>
> Date: Sun, 21 Jun 2015 10:10:05 +0200
> Subject: [PATCH 3/5] build: Add 'emacs-build-system'.
>
> * Makefile.am (MODULES): Add 'guix/build-system/emacs.scm' and
>   'guix/build/emacs-build-system.scm'.
> * guix/build-system/emacs.scm: New file.
> * guix/build/emacs-build-system.scm: New file.
> * doc/guix.texi (Build Systems): Document it.

Overall LGTM.
One last round of comments:

> address@hidden {Scheme Variable} emacs-build-system
> +This variable is exported by @code{(guix build-system emacs)}.  It
> +implements an installation procedure similar to the one of Emacs own
                                                                   ^
Add an apostrophe here.  —————————————————————————————————————————–’

> +packaging system.  It first creates the @code{PACKAGE-autoloads.el}
                   ^
Add an @pxref to the Emacs manual and then start a new paragraph.
Use @var{package} rather than PACKAGE.

> +file, then it byte compiles all Emacs Lisp files.  Differently from the
> +Emacs packaging system, the @code{info} documentation files are moved to

s/@code{info}/Info/

> +the standard documentation directory and the @code{dir} file is deleted.

@file{dir}

> +Each package is installed in its own directory under
> address@hidden/emacs/site-lisp/guix.d}.

@file as well.

> +(define-syntax lambda*-with-emacs-env
> +  (lambda (x)
> +    "Creates a 'lambda*' expression where the following variables are bound 
> to
> +the values expected by the 'emacs-build-system': 'emacs', 'out', 'name-ver',
> +'name', 'elpa-name-ver', 'elpa-name', 'info-dir', 'el-dir'.  The first
> +parameter of the syntax must be a list of symbols which become key parameters
> +of the procedure.  'inputs' and 'outputs' are automatically added to them.
> +The remaining parameters become the body of the procedure."

Interesting trick.

> +    (syntax-case x ()
> +      ((k (p ...) e1 e2 ...)
> +       (with-syntax (((outputs inputs emacs out name-ver name elpa-name-ver
> +                               elpa-name info-dir el-dir)
> +                      (map (cut datum->syntax #'k <>)
> +                           '(outputs inputs emacs out name-ver name
> +                                     elpa-name-ver elpa-name
> +                                     info-dir el-dir))))
> +         #'(lambda* (#:key outputs inputs p ... #:allow-other-keys)
> +             (let* ((emacs (string-append (assoc-ref inputs "emacs")
> +                                          "/bin/emacs"))
> +                    (out (assoc-ref outputs "out"))
> +                    (name-ver (store-dir->name-version out))
> +                    (name (package-name->name+version name-ver))
> +                    (elpa-name-ver (store-dir->elpa-name-version out))
> +                    (elpa-name (package-name->name+version elpa-name-ver))
> +                    (info-dir (string-append out "/share/info/" name-ver))
> +                    (el-dir (string-append out %install-suffix
> +                                           "/" elpa-name-ver)))
> +               e1 e2 ...)))))))

The problem is that this forcefully introduces bindings in an opaque way
(that is, regardless of whether the ‘outputs’ binding appears in the
source, there’s an ‘outputs’ binding that magically appears; this is
“unhygienic” or “non referentially transparent,” or just “bad”.  ;-))

Ideally, the identifiers that appear in the macro expansion should
either be in the source, or be unique (compiler-generated.)

What about starting from the ‘phase-lambda’ macro that Taylan proposed
at <https://lists.gnu.org/archive/html/guix-devel/2015-02/msg00712.html>?

It doesn’t solve everything, so perhaps you would need something like:

  (elpa-lambda ((emacs emacs-program) (el-dir emacs-lisp-directory))
    ...)

where ‘emacs-program’ and ‘emacs-lisp-directory’ are literals.

How does that sound?

> +(define (emacs-inputs inputs)
> +  "Retrive the list of Emacs packages from inputs."

“Retrieve” and “INPUTS”

> +  (filter (lambda (p)
> +            (and (pair? p)
> +                 (emacs-package? (package-name->name+version (first p)))))

(match-lambda
  ((label . directory)
   (emacs-package? (package-name+version directory))))

(Which means the ‘first’ above should have been ‘second’?)

For top-level bindings, try to consistently use non-abbreviated
words–e.g., ‘store-directory->name+version’ instead of
‘store-dir->name-version’ (also ‘+’ to denote the multiple-value return
in this example.)

Could you send an updated patch?

Thank you!

Ludo’.