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

  • Done
  • quality assurance status badge
Details
2 participants
  • Ludovic Courtès
  • Christopher Baines
Owner
unassigned
Submitted by
Christopher Baines
Severity
normal

Debbugs page

Christopher Baines wrote 7 years ago
(address . guix-patches@gnu.org)
20171011154029.16fe9aa4@cbaines.net
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(-)
-----BEGIN PGP SIGNATURE-----

iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAlneLV1fFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE
9XcUGRAAmbWOpED1IQJeMHaYs12TrQ+Pz9zbv5TtNtr6RXhb0KcXQcEL8Y08E+YZ
WzuVbhs2lqNSSmXgl2XAL5kNQDjbC3wUWKgUJipa2Dwe4TPmB8oe3ZLK+3cXMk9V
ZR317J8crrOf9t2drYaz5Tqyl1TJPEEhgcWoEb2MqdawHt9JLSQ+oW19C+Biwbei
bX0MOw9o24ppcN9M8MK7JaFi+ZqNaiYREooc8o8H80S8QyVnUHiLUm4vBoxBVbAw
3pPXQyZXZQHal+zim1GsbcZj//NtnJgwwIg5tVdYgsyeJfUljIQswejJ092I1lgd
Ajw5W65iullXTLKqXmlwuZOBqdVDsijA/NDYAAmPkbbYkWXk9Vmm2rzCoinKEcDu
PfrtRT+2OS44rdeg6m32HjnPmzWhIG3wRhZPV0NuwsC9xIhL2ZgKxmoHPUSojhYP
573ZfvDLzg+lYQsQUyw3Q1OWgnI5wIf3c5CMz69XS6gsOxnvcCm/nCHijHaTtgsv
hqGzjhaNAZIAcHMG5YEAo0PuTUtO4r7OtO8MxabbTzu0HL/GKuUFyo/ghiaMsMW+
iC0IzDUqnYSS24tNMs38UBs4j9vdoF1AMvLYYhqWD3FxiCwk3/utKib8VdrrPaLk
Dwg2Dtcf3xzddDL8wXGEBfQYX1p3uAX+P1K1avEUP8gbFo8uZJI=
=4QEI
-----END PGP SIGNATURE-----


Christopher Baines wrote 7 years ago
[PATCH 1/2] emacs-build-system: Handle missing programs when patching.
(address . 28787@debbugs.gnu.org)
20171011144218.26718-1-mail@cbaines.net
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(-)

Toggle diff (20 lines)
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
Christopher Baines wrote 7 years ago
[PATCH 2/2] emacs-build-system: Change how patch-el-files substitutes commands.
(address . 28787@debbugs.gnu.org)
20171011144218.26718-2-mail@cbaines.net
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(-)

Toggle diff (15 lines)
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
Christopher Baines wrote 7 years ago
control message for bug #28805
(address . control@debbugs.gnu.org)
8760bjd2gx.fsf@cbaines.net
block 28805 by 28787
Ludovic Courtès wrote 7 years ago
Re: [bug#28787] [PATCH 1/2] emacs-build-system: Handle missing programs when patching.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 28787@debbugs.gnu.org)
871sm6zpzh.fsf@gnu.org
Hello!

Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (26 lines)
> 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:
Toggle diff (16 lines)
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.
Otherwise LGTM!

Thanks,
Ludo’.
Ludovic Courtès wrote 7 years ago
Re: [bug#28787] [PATCH 2/2] emacs-build-system: Change how patch-el-files substitutes commands.
(name . Christopher Baines)(address . mail@cbaines.net)(address . 28787@debbugs.gnu.org)
87wp3yybdk.fsf@gnu.org
Christopher Baines <mail@cbaines.net> skribis:

Toggle quote (14 lines)
> 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'.
Christopher Baines wrote 7 years ago
Re: [bug#28787] [PATCH 1/2] emacs-build-system: Handle missing programs when patching.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 28787@debbugs.gnu.org)
20171015190608.7497eaeb@cbaines.net
On Fri, 13 Oct 2017 23:40:02 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

Toggle quote (32 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAlnjo5BfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE
9Xe0og/9H2/8Sd5wHzHGqpJppOZPVzIRTv6es0TdgX0nv6hsVtEX9IFwak1HjDl+
j+KCzjUUk+Z1RIRX8obkX4KOc9ikIeQyX/0Cm/9M1NtGuiqAPP0nxMOZJXBR1UR8
wGdJVCU9kxUdS9Nd0vLCLKYtG2F4IxvedpRHPNNXYUDN76CAbnYFx9gShqI2xzvG
iZVmmztdbfT6DmBi25FsAixNQVG/ZnUbTA5/b7EWBgkBmOXgFU0M9WxUIztWm+PB
ieZY4trT/hrTIg+hXIYnFZC9PPsCsDPRriocPODo8g7+VOZkpVsh/VEqSATAMFIr
ZSvztMGUk5dKTeMv3PKh8riPvSfKw7WYVkYeVjV8PD2dpel92Ta8XXvdR2EUstmY
3AOW88kV/8ez/Y+wgXKDKFet9O92PWNQSkfJoQPFn9KRo21mVprP+6WWgvJhezC2
UFtQIJ73iuHI5XD7XkpGDnBJV+BRvnMJbHzOBm03ZpfiRUIFHLDaCkKa8+RvWgpo
AWkPEA4bb8JPbU7NprbQ5P5Dy7WmijnJnXAtDtYhUnPwpuE2H1W6ZLiV6zccN/Bu
99kDNwTcPOSycS47WOh3hAhnQPc/uSxWel0QdxO1B4QMnOBjNcC7V71QXrtMnoMq
H0BsV3JS3H8gzgF/KcTr0Z2A9xWSu4P0cN6rg8KcKvjrH7nVqro=
=2XPv
-----END PGP SIGNATURE-----


Christopher Baines wrote 7 years ago
Re: [bug#28787] [PATCH 2/2] emacs-build-system: Change how patch-el-files substitutes commands.
(name . Ludovic Courtès)(address . ludo@gnu.org)(address . 28787-done@debbugs.gnu.org)
20171015190907.6053e529@cbaines.net
On Fri, 13 Oct 2017 23:40:55 +0200
ludo@gnu.org (Ludovic Courtès) wrote:

Toggle quote (18 lines)
> 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.
-----BEGIN PGP SIGNATURE-----

iQKTBAEBCgB9FiEEPonu50WOcg2XVOCyXiijOwuE9XcFAlnjpENfFIAAAAAALgAo
aXNzdWVyLWZwckBub3RhdGlvbnMub3BlbnBncC5maWZ0aGhvcnNlbWFuLm5ldDNF
ODlFRUU3NDU4RTcyMEQ5NzU0RTBCMjVFMjhBMzNCMEI4NEY1NzcACgkQXiijOwuE
9Xfd6A/+JumNi/wU8C30o9hhmU+ZntYp19da3ALqoSciwKFPpnfLpYS2LuesVx7w
4Zagu/TUUx34XdrSdR1kynkBTwROs8B0MSVuBYMnnMr+u0mFEidoXBtePkQUm/s7
7NWFFQqFh8oha2O8NEnOvm26hIqrMOKG42DiNDEVnUUWjaTBBctxYD9JnsNd0ax3
RMY8ZMD8HGkM/i1d2ErN5irJxIS3WkOHM9Z1FvHsf7YYJjHGxc+3iQBNFWvFZxUz
DPW9aI2NrC7zsxTIY+yp1RSc5KlotXR83dC4VWQ/rcqSHb9jAE+t9fzxFM2NNbV/
+LBbWJNjzhamEo3gHV61v2UBSjLHPgPiWDpfnyNY3T+DGpkSWNWlmxASXl2RHLsb
nH+xqIk7ik+rExFU+NqYZtCE5FflCnf0+ad/aXj7yRqo5sjf0QyTdFnLGPTQl/AI
FsKun6i5nMnlcJNR7vDN1E+lGcvD5nltqDW91RLLYca9rI3f6Nizyylg74acBEdc
IgkFdFN2N8C4BQw2OiAW/JEbkZU6L573m+dbn5Kpib+sECYpgFEv6VKvnlpobPXw
kMHQ84bBiZdycjycs2l15KVfQY/WEEMy5LCEpkf6BR7znxhEdA+NClbdQkG72Mh4
QYpC8YqZmu1LcMhywTyYSugdxGmT30FzB4YjJrEWJyX+xpqpeZU=
=t1wF
-----END PGP SIGNATURE-----


Closed
?
Your comment

This issue is archived.

To comment on this conversation send an email to 28787@debbugs.gnu.org

To respond to this issue using the mumi CLI, first switch to it
mumi current 28787
Then, you may apply the latest patchset in this issue (with sign off)
mumi am -- -s
Or, compose a reply to this issue
mumi compose
Or, send patches to this issue
mumi send-email *.patch
You may also tag this issue. See list of standard tags. For example, to set the confirmed and easy tags
mumi command -t +confirmed -t +easy
Or, remove the moreinfo tag and set the help tag
mumi command -t -moreinfo -t +help