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

[ludo@gnu.org (Ludovic Courtès)]

Federico Beffa <beffa@ieee.org> skribis:

> On Thu, Jun 25, 2015 at 2:33 PM, Ludovic Courtès <ludo@gnu.org> wrote:
>> Federico Beffa <beffa@ieee.org> skribis:

[...]

>> 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.)
>
> I was so sure that you would say so, that I did a copy of the file
> before removing the 'let's and introducing the syntax.

:-)

> If this would be proposed as a general utility, then I would agree
> with you. But it's not. It is a module internal implementation detail

Sure, sure, but the problems here are not theoretical.  The possibility
of having one user introduce a binding that clashes without noticing is
real, and it can be hard to debug.  “But it’s all internal in a single
file,” right, but some future hacker might not notice the
unhygenically-introduced bindings.

Simply put, people may not expect such issues to arise in Scheme, so
it’s best to not violate the rule of least surprise, IMO.

>>> +  (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’?)
>
> I'm not sure I understand your comment:
> 'package-name->name+version' takes a package name, therefore I pass it
> the 1st element of each input.

The first element of an input is a label, which often happens to be a
package name, but doesn’t have to.  Would it be possible to extract the
package name from the directory name?

> (By the way, 'match-lambda' appears not to be documented in Guile.)

Oops.

> From e24d0b11280f4fcd106f371b98b7481f7c044eb0 Mon Sep 17 00:00:00 2001
> From: Federico Beffa <beffa@fbengineering.ch>
> 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.

[...]

> +(define (emacs-inputs-directories inputs)
> +  "Extract the list of Emacs package directories from INPUTS."
> +  (let ((emacs-ins (emacs-inputs inputs)))
> +    (match emacs-ins
> +      (((name . dir) ...) dir))))

Minor issue, but I would s/emacs-ins/inputs/, s/dir/directories/,
s/name/names/.  (See the “Naming” section in
<http://mumble.net/~campbell/scheme/style.txt>.)

OK to push with changes to this and ‘emacs-inputs’ as discussed above.

Thank you, and sorry for being as predictable and boring as you
expected.  ;-)

Ludo’.