[bug#28787] [PATCH] emacs-build-system: Improve command patching.

[bug#28787] [PATCH] emacs-build-system: Improve command patching.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 353 bytes --]

I came across a couple of improvements when looking at packaging
emacs-org-plus-contrib.

Christopher Baines (2):
  emacs-build-system: Handle missing programs when patching.
  emacs-build-system: Change how patch-el-files substitutes commands.

 guix/build/emacs-build-system.scm | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

[bug#28787] [PATCH 1/2] emacs-build-system: Handle missing programs when patching.

[Christopher Baines]

Previously the string-append here would error, which isn't useful as it
doesn't tell you which command couldn't be found. To make the error
actionable, catch it earlier, and explicitly error.

* guix/build/emacs-build-system.scm (patch-el-files): Handle (which cmd)
  returning #f.
---
 guix/build/emacs-build-system.scm | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 2404dbddb..0260f15bb 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -93,7 +93,12 @@ store in '.el' files."
          (substitute-cmd (lambda ()
                            (substitute* (find-files "." "\\.el$")
                              (("\"/bin/([^.].*)\"" _ cmd)
-                              (string-append "\"" (which cmd) "\""))))))
+                              (string-append
+                               "\""
+                               (or
+                                (which cmd)
+                                (error "patch-el-files: unable to locate " 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.
-- 
2.14.2

[bug#28787] [PATCH 2/2] emacs-build-system: Change how patch-el-files substitutes commands.

[Christopher Baines]

Previously the regex would match from /bin/ to a closing quote. However, this
is greedy, so will match up until the last ". This causes problems when there
are several quotes on the same line, for example:

org-effectiveness.el:
196:      (call-process "/bin/bash" nil t nil "-c" strplot)

Therefore, change . to \S so that it doesn't include whitespace
characters. Changing to a lazy quantifier would be an option, if that were
supported.

* guix/build/emacs-build-system.scm (patch-el-files): Change the regular
  expression used.
---
 guix/build/emacs-build-system.scm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 0260f15bb..c1d36766e 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -92,7 +92,7 @@ store in '.el' files."
          (el-dir (string-append out %install-suffix "/" elpa-name-ver))
          (substitute-cmd (lambda ()
                            (substitute* (find-files "." "\\.el$")
-                             (("\"/bin/([^.].*)\"" _ cmd)
+                             (("\"/bin/([^.]\\S*)\"" _ cmd)
                               (string-append
                                "\""
                                (or
-- 
2.14.2

[bug#28787] [PATCH 1/2] emacs-build-system: Handle missing programs when patching.

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 1345 bytes --]

Hello!

Christopher Baines <mail@cbaines.net> skribis:

> Previously the string-append here would error, which isn't useful as it
> doesn't tell you which command couldn't be found. To make the error
> actionable, catch it earlier, and explicitly error.
>
> * guix/build/emacs-build-system.scm (patch-el-files): Handle (which cmd)
>   returning #f.
> ---
>  guix/build/emacs-build-system.scm | 7 ++++++-
>  1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
> index 2404dbddb..0260f15bb 100644
> --- a/guix/build/emacs-build-system.scm
> +++ b/guix/build/emacs-build-system.scm
> @@ -93,7 +93,12 @@ store in '.el' files."
>           (substitute-cmd (lambda ()
>                             (substitute* (find-files "." "\\.el$")
>                               (("\"/bin/([^.].*)\"" _ cmd)
> -                              (string-append "\"" (which cmd) "\""))))))
> +                              (string-append
> +                               "\""
> +                               (or
> +                                (which cmd)
> +                                (error "patch-el-files: unable to locate " cmd))
> +                               "\""))))))

For clarity I’d move the ‘error’ call out of the way:


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: Type: text/x-patch, Size: 852 bytes --]

diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
index 2404dbddb..bafb1060b 100644
--- a/guix/build/emacs-build-system.scm
+++ b/guix/build/emacs-build-system.scm
@@ -93,7 +93,10 @@ store in '.el' files."
          (substitute-cmd (lambda ()
                            (substitute* (find-files "." "\\.el$")
                              (("\"/bin/([^.].*)\"" _ cmd)
-                              (string-append "\"" (which cmd) "\""))))))
+                              (let ((cmd (which cmd)))
+                                (unless cmd
+                                  (error "..."))
+                                (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.

[-- Attachment #3: Type: text/plain, Size: 40 bytes --]


Otherwise LGTM!

Thanks,
Ludo’.

[bug#28787] [PATCH 2/2] emacs-build-system: Change how patch-el-files substitutes commands.

[Ludovic Courtès]

Christopher Baines <mail@cbaines.net> skribis:

> Previously the regex would match from /bin/ to a closing quote. However, this
> is greedy, so will match up until the last ". This causes problems when there
> are several quotes on the same line, for example:
>
> org-effectiveness.el:
> 196:      (call-process "/bin/bash" nil t nil "-c" strplot)
>
> Therefore, change . to \S so that it doesn't include whitespace
> characters. Changing to a lazy quantifier would be an option, if that were
> supported.
>
> * guix/build/emacs-build-system.scm (patch-el-files): Change the regular
>   expression used.

Good catch, LGTM!

Ludo'.

[bug#28787] [PATCH 1/2] emacs-build-system: Handle missing programs when patching.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 1614 bytes --]

On Fri, 13 Oct 2017 23:40:02 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Hello!
> 
> Christopher Baines <mail@cbaines.net> skribis:
> 
> > Previously the string-append here would error, which isn't useful as it
> > doesn't tell you which command couldn't be found. To make the error
> > actionable, catch it earlier, and explicitly error.
> >
> > * guix/build/emacs-build-system.scm (patch-el-files): Handle (which cmd)
> >   returning #f.
> > ---
> >  guix/build/emacs-build-system.scm | 7 ++++++-
> >  1 file changed, 6 insertions(+), 1 deletion(-)
> >
> > diff --git a/guix/build/emacs-build-system.scm b/guix/build/emacs-build-system.scm
> > index 2404dbddb..0260f15bb 100644
> > --- a/guix/build/emacs-build-system.scm
> > +++ b/guix/build/emacs-build-system.scm
> > @@ -93,7 +93,12 @@ store in '.el' files."
> >           (substitute-cmd (lambda ()
> >                             (substitute* (find-files "." "\\.el$")
> >                               (("\"/bin/([^.].*)\"" _ cmd)
> > -                              (string-append "\"" (which cmd) "\""))))))
> > +                              (string-append
> > +                               "\""
> > +                               (or
> > +                                (which cmd)
> > +                                (error "patch-el-files: unable to locate " cmd))
> > +                               "\""))))))  
> 
> For clarity I’d move the ‘error’ call out of the way:

This works for me. I've also changed the original cmd to cmd-name, to
avoid using the same name for two different variables.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]

bug#28787: [PATCH 2/2] emacs-build-system: Change how patch-el-files substitutes commands.

[Christopher Baines]

[-- Attachment #1: Type: text/plain, Size: 824 bytes --]

On Fri, 13 Oct 2017 23:40:55 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

> Christopher Baines <mail@cbaines.net> skribis:
> 
> > Previously the regex would match from /bin/ to a closing quote. However, this
> > is greedy, so will match up until the last ". This causes problems when there
> > are several quotes on the same line, for example:
> >
> > org-effectiveness.el:
> > 196:      (call-process "/bin/bash" nil t nil "-c" strplot)
> >
> > Therefore, change . to \S so that it doesn't include whitespace
> > characters. Changing to a lazy quantifier would be an option, if that were
> > supported.
> >
> > * guix/build/emacs-build-system.scm (patch-el-files): Change the regular
> >   expression used.  
> 
> Good catch, LGTM!

Great :) I've now pushed the updated version of these two patches.

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 963 bytes --]