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

[Federico Beffa]

Thanks for the review!

I've done the suggested modifications apart from the comments below.

On Thu, Jun 25, 2015 at 2:33 PM, Ludovic Courtès <address@hidden> wrote:
> Federico Beffa <address@hidden> skribis:
>> +(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.)

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
aimed at reducing boilerplate in this particular place only (where all
procedures need 'outputs' and most of the other variables) and every
introduced binding is documented. The name tells what it does in such
an obvious way that it makes the code shorter without degrading
readability.

In fact there are also popular general utilities promoted by highly
regarded programmers, which introduce what you call "bad" macros:
http://ep.yimg.com/ty/cdn/paulgraham/onlisp.pdf Chapter 14.

In any case, I've reverted to the boilerplate version (correcting it
according to the other comments).

>> +  (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.
'emacs-package?' checks for the agreed prefix in the name. Prior to
this check I discard the version suffix to make sure that, e.g.,
"emacs-123.456", is not confused for an emacs package.

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

Thanks,
Fede

0003-build-Add-emacs-build-system.patch
Description: Text Data