Hello Ludovic!

ludo@gnu.org (Ludovic Courtès) writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer@gmail.com> skribis:
>
>> From 379cf143bb078c7785d104a41a762d6136f1508e Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> Date: Sat, 13 Jan 2018 17:54:18 -0500
>> Subject: [PATCH 1/4] emacs-build-system: Add set-emacs-load-path phase.
>>
>> This generalizes the mechanism by which the Emacs dependencies are made visible,
>> so that any build phase can make use of them.
>>
>> * guix/build/emacs-build-system.scm (%legacy-install-suffix): New variable.
>> (%install-suffix): Redefine in terms of %legacy-install-suffix.
>> (set-emacs-load-path): Add new phase used for dependency resolution.
>> (build): Remove ad-hoc dependency discovery mechanism.
>> (emacs-input->el-directory): Add new procedure.
>> (emacs-inputs-el-directories): Use it.
>> (package-name-version->elpa-name-version): Fix typo.
>> (%standard-phases): Include the new `set-emacs-load-path' phase. Refactor to
>> make the ordering of the phases clearer.
>> * guix/build/emacs-utils.scm (emacs-byte-compile-directory): Remove the
>> optional `dependency-dirs' argument, which is now obsoleted by the
>> `set-emacs-load-path' phase.
>
> Nice!  At first sight it looks good to me.  If you’ve checked on a
> sample that Emacs packages still build fine, and if nobody replies in
> the meantime, I’ll apply it in a day or two.
>
> This will trigger on the order of 200 rebuilds per architecture, but
> these are small packages, so I think it’s fine.

Yes, I did test this on a sample of random Emacs packages and they built
fine with these changes.

> Nitpick:
>
>>  (define (emacs-inputs-el-directories dirs)
>>    "Build the list of Emacs Lisp directories from the Emacs package directory
>>  DIRS."
>> -  (append-map (lambda (d)
>> -                (list (string-append d "/share/emacs/site-lisp")
>> -                      (string-append d %install-suffix "/"
>> -                                     (store-directory->elpa-name-version d))))
>> -              dirs))
>> +  (filter string? (map emacs-input->el-directory dirs)))
>
> This can be written as:
>
>   (filter-map emacs-input->el-directory dirs)

Done! It's good to know how to handle these pesky nils at last!

> [...]
>
>> +  ;; TODO: Remove after issue 30611 is fixed in master (see:
>> +  ;; https://debbugs.gnu.org/30116).
>
> Which number is correct?  :-)

30116, good catch!

> I’m not convinced we need special treatment for this case directly in
> emacs-build-system.  This has happened only once on 200+ packages, so I
> would rather leave the special case in the package definition itself.
>
> WDYT?

Hmm, I think I'd rather leave it there; the alternative implies
replacing the `patch-el-files' phase in the package definition, and this
would involve copy-pasting the modified phase code which is a bit
messy. I'd also rather spare someone from having to investigate this
problem again until 30116 is merged into master.

Does it make sense?

>> From 1e4a28920b17f7a3bf3e34a999b29e0245233942 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer@gmail.com>
>> Date: Mon, 11 Dec 2017 00:07:57 -0500
>> Subject: [PATCH 4/4] gnu: Add emacs-realgud.
>>
>> * gnu/packages/emacs.scm (emacs-test-simple, emacs-load-relative,
>> emacs-loc-changes, emacs-realgud): New public variables.
>
> LGTM.   However, there’s a tradition to add one package per commit, so
> it would be great if you could split it and send updated patches.

Done. All patches attached!

Thanks for reviewing :)

Maxim