GNU bug report logs - #30119
[PATCH] Add emacs-realgud and varia

Previous Next

To add a comment to this bug, you must first unarchive it, by sending
a message to control AT debbugs.gnu.org, with unarchive 30119 in the body.
You can then email your comments to 30119 AT debbugs.gnu.org in the normal way.

Toggle the display of automated, internal messages from the tracker.

Message #5 received at submit <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: guix-patches <guix-patches <at> gnu.org>
Subject: [PATCH] Add emacs-realgud and varia
Date: Sun, 14 Jan 2018 23:03:37 -0500

[Message part 1 (text/plain, inline)]

Hello Guix!

This patch series introduces some changes and bugfixes to the Emacs
build system and goes on to add RealGUD and its dependencies.

Thank you,

Maxim

[0001-emacs-build-system-Add-set-emacs-load-path-phase.patch (text/x-patch, attachment)]

[0002-emacs-build-system-Reinstate-the-check-phase.patch (text/x-patch, attachment)]

[0003-emacs-build-system-Work-around-issue-30611.patch (text/x-patch, attachment)]

[0004-gnu-Add-emacs-realgud.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #8 received at 30119 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: 30119 <at> debbugs.gnu.org
Subject: Re: [bug#30119] [PATCHv2] Add emacs-realgud and varia
Date: Thu, 25 Jan 2018 00:45:31 -0500

[Message part 1 (text/plain, inline)]

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> writes:

> This patch series introduces some changes and bugfixes to the Emacs
> build system and goes on to add RealGUD and its dependencies.

I've just reworked those patches to include an improvement suggested by
Ludovic in another patch. I've also cleaned the #t and #f returned from
the build phases and used invoke instead of system*.

Maxim

[0001-emacs-build-system-Add-set-emacs-load-path-phase.patch (text/x-patch, attachment)]

[0002-emacs-build-system-Reinstate-the-check-phase.patch (text/x-patch, attachment)]

[0003-emacs-build-system-Work-around-issue-30611.patch (text/x-patch, attachment)]

[0004-gnu-Add-emacs-realgud.patch (text/x-patch, attachment)]

Message #11 received at 30119 <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 30119 <at> debbugs.gnu.org
Subject: Re: [bug#30119] [PATCHv2] Add emacs-realgud and varia
Date: Tue, 30 Jan 2018 22:05:53 +0100

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> From 379cf143bb078c7785d104a41a762d6136f1508e Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> 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.

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)

> From f76b5faee8b0752d1aae95b9df7a1e9e6d88bd08 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Sat, 13 Jan 2018 17:54:57 -0500
> Subject: [PATCH 2/4] emacs-build-system: Reinstate the check phase.
>
> * guix/build/emacs-build-system.scm (%standard-phases): Reinstate the check
> phase from the gnu-build-system.
> * guix/build-system/emacs.scm (emacs-build)[tests?]: But do not enable it by default.
> [parallel-tests?]: Add argument.

OK.

> From 50a671765b3d610e38f6e052a59b3eef316f4226 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Sun, 14 Jan 2018 22:38:20 -0500
> Subject: [PATCH 3/4] emacs-build-system: Work around issue 30611.
>
> This is a temporary workaround issue 30611, where substitute* crashes on
> files containing NUL characters.
>
> * guix/build/emacs-build-system.scm (patch-el-files): Filter out elisp files
> that contain NUL characters.

[...]

> +  ;; TODO: Remove after issue 30611 is fixed in master (see:
> +  ;; https://debbugs.gnu.org/30116).

Which number is correct?  :-)

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?

> From 1e4a28920b17f7a3bf3e34a999b29e0245233942 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> 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.

Thank you!

Ludo’.

Message #14 received at 30119 <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: 30119 <at> debbugs.gnu.org
Subject: Re: [bug#30119] [PATCHv2] Add emacs-realgud and varia
Date: Sun, 04 Feb 2018 22:41:54 -0500

[Message part 1 (text/plain, inline)]

Hello Ludovic!

ludo <at> gnu.org (Ludovic Courtès) writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> From 379cf143bb078c7785d104a41a762d6136f1508e Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer <at> 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 <at> 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

[0001-emacs-build-system-Add-set-emacs-load-path-phase.patch (text/x-patch, attachment)]

[0002-emacs-build-system-Reinstate-the-check-phase.patch (text/x-patch, attachment)]

[0003-emacs-build-system-Work-around-issue-30116.patch (text/x-patch, attachment)]

[0004-gnu-Add-emacs-test-simple.patch (text/x-patch, attachment)]

[0005-gnu-Add-emacs-load-relative.patch (text/x-patch, attachment)]

[0006-gnu-Add-emacs-loc-changes.patch (text/x-patch, attachment)]

[0007-gnu-Add-emacs-realgud.patch (text/x-patch, attachment)]

[signature.asc (application/pgp-signature, inline)]

Message #19 received at 30119-done <at> debbugs.gnu.org (full text, mbox):

From: ludo <at> gnu.org (Ludovic Courtès)
To: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
Cc: 30119-done <at> debbugs.gnu.org
Subject: Re: [bug#30119] [PATCHv2] Add emacs-realgud and varia
Date: Mon, 05 Feb 2018 16:57:51 +0100

[Message part 1 (text/plain, inline)]

Hi Maxim,

Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:

> From 4e9e0f1358b65c180218412f667a2dbb4e1b2b19 Mon Sep 17 00:00:00 2001
> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
> Date: Sun, 14 Jan 2018 22:38:20 -0500
> Subject: [PATCH 3/7] emacs-build-system: Work around issue 30116.
>
> This is a temporary workaround issue 30116, where substitute* crashes on
> files containing NUL characters.
>
> * guix/build/emacs-build-system.scm (patch-el-files): Filter out elisp files
> that contain NUL characters.

I applied the whole series but there was an issue in this one, so I took
the liberty to change it as follows:

  1. clarify comments;

  2. make sure ‘el-files’ is a list of absolute file names; previously
     it would fail beacuse ‘el-files’ was used with a different cwd.

Thanks!

Ludo’.

[Message part 2 (text/x-patch, inline)]

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 8156757be..b77984742 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -116,22 +116,21 @@ store in '.el' files."
          ;; strings containing NULs.  Filter out such files.  TODO: Remove
          ;; this workaround when <https://bugs.gnu.org/30116> is fixed.
          (el-files (remove file-contains-nul-char?
-                           (find-files "." "\\.el$")))
-
-         (substitute-cmd (lambda ()
+                           (find-files (getcwd) "\\.el$"))))
+    (define (substitute-program-names)
       (substitute* el-files
         (("\"/bin/([^.]\\S*)\"" _ cmd-name)
          (let ((cmd (which cmd-name)))
            (unless cmd
-                                  (error
-                                   "patch-el-files: unable to locate " cmd-name))
-                                (string-append "\"" cmd "\"")))))))
+             (error "patch-el-files: unable to locate " cmd-name))
+           (string-append "\"" cmd "\"")))))
+
     (with-directory-excursion el-dir
-      ;; Some old '.el' files (e.g., tex-buf.el in AUCTeX) are still encoded
-      ;; with the "ISO-8859-1" locale.
-      (unless (false-if-exception (substitute-cmd))
+      ;; Some old '.el' files (e.g., tex-buf.el in AUCTeX) are still
+      ;; ISO-8859-1-encoded.
+      (unless (false-if-exception (substitute-program-names))
         (with-fluids ((%default-port-encoding "ISO-8859-1"))
-          (substitute-cmd))))
+          (substitute-program-names))))
     #t))
 
 (define* (install #:key outputs

Message #22 received at 30119-done <at> debbugs.gnu.org (full text, mbox):

From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
To: ludo <at> gnu.org (Ludovic Courtès)
Cc: 30119-done <at> debbugs.gnu.org
Subject: Re: [bug#30119] [PATCHv2] Add emacs-realgud and varia
Date: Wed, 07 Feb 2018 08:42:50 -0500

Hi Ludovic,

ludo <at> gnu.org (Ludovic Courtès) writes:

> Hi Maxim,
>
> Maxim Cournoyer <maxim.cournoyer <at> gmail.com> skribis:
>
>> From 4e9e0f1358b65c180218412f667a2dbb4e1b2b19 Mon Sep 17 00:00:00 2001
>> From: Maxim Cournoyer <maxim.cournoyer <at> gmail.com>
>> Date: Sun, 14 Jan 2018 22:38:20 -0500
>> Subject: [PATCH 3/7] emacs-build-system: Work around issue 30116.
>>
>> This is a temporary workaround issue 30116, where substitute* crashes on
>> files containing NUL characters.
>>
>> * guix/build/emacs-build-system.scm (patch-el-files): Filter out elisp files
>> that contain NUL characters.
>
> I applied the whole series but there was an issue in this one, so I took
> the liberty to change it as follows:
>
>   1. clarify comments;
>
>   2. make sure ‘el-files’ is a list of absolute file names; previously
>      it would fail beacuse ‘el-files’ was used with a different cwd.

Thank you! I wonder why I had not encountered the issue; we used to use
that very (find-files . "\\.el$") expression before that change, so in
all cases it was already broken if it truly was :).

Thank you!

Maxim

Previous Next

GNU bug tracking system
Copyright (C) 1999 Darren O. Benham, 1997,2003 nCipher Corporation Ltd, 1994-97 Ian Jackson.