bug#31929: 27.0.50; Have make install copy the emacs-module.h to the destination install dir

bug#31929: 27.0.50; Have make install copy the emacs-module.h to the destination install dir

[Kaushal Modi]

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

Hello,

When building emacs locally, the emacs-module.h is not copied over to an
include/ directory in the destination installation directory. This prevents
one to have version-specific emacs-module.h for folks building emacs
themselves.

Ref: https://lists.gnu.org/r/help-gnu-emacs/2018-06/msg00303.html
-- 

Kaushal Modi

[-- Attachment #2: Type: text/html, Size: 581 bytes --]

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Philipp Stephani]

* Makefile.in (includedir): New variable.
(install-arch-indep): Install emacs-module.h.
(uninstall): Uninstall emacs-module.h.
---
 Makefile.in | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Makefile.in b/Makefile.in
index 19bf7c423f..5346429264 100644
--- a/Makefile.in
+++ b/Makefile.in
@@ -151,6 +151,9 @@ libexecdir=
 # Currently only used for the systemd service file.
 libdir=@libdir@
 
+# Where to install emacs-module.h.
+includedir=@includedir@
+
 # Where to install Emacs's man pages.
 # Note they contain cross-references that expect them to be in section 1.
 mandir=@mandir@
@@ -558,6 +561,8 @@ set_installuser=
 ## See also these comments from 2004 about cp -r working fine:
 ## https://lists.gnu.org/r/autoconf-patches/2004-11/msg00005.html
 install-arch-indep: lisp install-info install-man ${INSTALL_ARCH_INDEP_EXTRA}
+	$(MKDIR_P) -m 0755 $(includedir)
+	$(INSTALL_DATA) src/emacs-module.h $(includedir)/emacs-module.h
 	-set ${COPYDESTS} ; \
 	unset CDPATH; \
 	$(set_installuser); \
@@ -741,6 +746,7 @@ install-strip:
 ###
 ### Don't delete the lisp and etc directories if they're in the source tree.
 uninstall: uninstall-$(NTDIR) uninstall-doc
+	rm -f $(includedir)/emacs-module.h
 	$(MAKE) -C lib-src uninstall
 	-unset CDPATH; \
 	for dir in "$(DESTDIR)${lispdir}" "$(DESTDIR)${etcdir}" ; do 	\
-- 
2.19.0

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Fri, 21 Sep 2018 15:27:48 +0200
> Cc: Philipp Stephani <phst@google.com>
> 
> +	$(MKDIR_P) -m 0755 $(includedir)
> +	$(INSTALL_DATA) src/emacs-module.h $(includedir)/emacs-module.h

Thanks.  I wonder whether we should install in $(includedir)/emacs.
It sounds rude to me to invade the top-level include directory; other
packages install into package-specific subdirectories.

If you agree, we may need to change mod-test.c and perhaps also the
place where we keep emacs-module.h in the Emacs tree.

We should also think what will happen when we change the interface in
some backward-incompatible way: how do we allow end-users to compile
modules for several Emacs versions on the same system? does that
require a new version of the header, or can we provide a header that
will work with any Emacs version?

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Glenn Morris]

Philipp Stephani wrote:

> +	$(MKDIR_P) -m 0755 $(includedir)

I don't remember how portable that is, but everywhere else in Emacs's
Makefiles uses: umask 022 && ${MKDIR_P} "...".

And should this file only be installed in a --with-modules build?

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Philipp Stephani]

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

Eli Zaretskii <eliz@gnu.org> schrieb am Fr., 21. Sep. 2018 um 15:46 Uhr:

> > From: Philipp Stephani <p.stephani2@gmail.com>
> > Date: Fri, 21 Sep 2018 15:27:48 +0200
> > Cc: Philipp Stephani <phst@google.com>
> >
> > +     $(MKDIR_P) -m 0755 $(includedir)
> > +     $(INSTALL_DATA) src/emacs-module.h $(includedir)/emacs-module.h
>
> Thanks.  I wonder whether we should install in $(includedir)/emacs.
> It sounds rude to me to invade the top-level include directory; other
> packages install into package-specific subdirectories.
>

I think it's not necessary to include "emacs" twice in the file name: using
"emacs-module.h" should already be unique enough, and including "emacs"
twice doesn't make it more unique.


>
> If you agree, we may need to change mod-test.c and perhaps also the
> place where we keep emacs-module.h in the Emacs tree.
>
> We should also think what will happen when we change the interface in
> some backward-incompatible way: how do we allow end-users to compile
> modules for several Emacs versions on the same system? does that
> require a new version of the header, or can we provide a header that
> will work with any Emacs version?
>

I don't think we can ever have backward-incompatible changes: only
additions are possible. This isn't different from other headers. That is,
the header for version N should also work for all versions older than N.

[-- Attachment #2: Type: text/html, Size: 2051 bytes --]

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Philipp Stephani]

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

Glenn Morris <rgm@gnu.org> schrieb am Fr., 21. Sep. 2018 um 17:36 Uhr:

> Philipp Stephani wrote:
>
> > +     $(MKDIR_P) -m 0755 $(includedir)
>
> I don't remember how portable that is, but everywhere else in Emacs's
> Makefiles uses: umask 022 && ${MKDIR_P} "...".
>

Good idea, done.


>
> And should this file only be installed in a --with-modules build?
>

I considered that, but I think it's simpler to install it unconditionally.
Users can use emacs-module.h to build modules even if their version of
Emacs doesn't support them.

[-- Attachment #2: Type: text/html, Size: 1012 bytes --]

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Philipp Stephani]

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

Since there were no more comments, I've installed (a tiny variant of) this
patch as 00ea749f2a.

Philipp Stephani <p.stephani2@gmail.com> schrieb am Fr., 21. Sep. 2018 um
15:28 Uhr:

> * Makefile.in (includedir): New variable.
> (install-arch-indep): Install emacs-module.h.
> (uninstall): Uninstall emacs-module.h.
> ---
>  Makefile.in | 6 ++++++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Makefile.in b/Makefile.in
> index 19bf7c423f..5346429264 100644
> --- a/Makefile.in
> +++ b/Makefile.in
> @@ -151,6 +151,9 @@ libexecdir=
>  # Currently only used for the systemd service file.
>  libdir=@libdir@
>
> +# Where to install emacs-module.h.
> +includedir=@includedir@
> +
>  # Where to install Emacs's man pages.
>  # Note they contain cross-references that expect them to be in section 1.
>  mandir=@mandir@
> @@ -558,6 +561,8 @@ set_installuser=
>  ## See also these comments from 2004 about cp -r working fine:
>  ## https://lists.gnu.org/r/autoconf-patches/2004-11/msg00005.html
>  install-arch-indep: lisp install-info install-man
> ${INSTALL_ARCH_INDEP_EXTRA}
> +       $(MKDIR_P) -m 0755 $(includedir)
> +       $(INSTALL_DATA) src/emacs-module.h $(includedir)/emacs-module.h
>         -set ${COPYDESTS} ; \
>         unset CDPATH; \
>         $(set_installuser); \
> @@ -741,6 +746,7 @@ install-strip:
>  ###
>  ### Don't delete the lisp and etc directories if they're in the source
> tree.
>  uninstall: uninstall-$(NTDIR) uninstall-doc
> +       rm -f $(includedir)/emacs-module.h
>         $(MAKE) -C lib-src uninstall
>         -unset CDPATH; \
>         for dir in "$(DESTDIR)${lispdir}" "$(DESTDIR)${etcdir}" ; do    \
> --
> 2.19.0
>
>

[-- Attachment #2: Type: text/html, Size: 2228 bytes --]

bug#31929: closed (Re: [PATCH] Install emacs-module.h (Bug#31929))

[Kaushal Modi]

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

On Wed, Oct 3, 2018 at 4:03 PM GNU bug Tracking System <help-debbugs@gnu.org>
wrote:

> Your bug report
>
> #31929: 27.0.50; Have make install copy the emacs-module.h to the
> destination install dir
>
> which was filed against the emacs package, has been closed.Since there
> were no more comments, I've installed (a tiny variant of) this patch as
> 00ea749f2a.
>

Hi Philipp,

Thanks for fixing this. Interestingly this only the second email I got from
GNU debbugs. The first email was acknowledge email when I filed that bug.

Now when I see https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31929, I
notice that a lot of conversation happened, but I got none of those emails.

In any case, I see that this bug was closed in
http://git.savannah.gnu.org/cgit/emacs.git/commit/?id=00ea749f2af44bff6ea8c1259477fbf0ead8a306,
and that commit looks good.

Thanks again.

Kaushal

[-- Attachment #2: Type: text/html, Size: 1523 bytes --]

bug#31929: closed (Re: [PATCH] Install emacs-module.h (Bug#31929))

[Glenn Morris]

Kaushal Modi wrote:

> Thanks for fixing this. Interestingly this only the second email I got from
> GNU debbugs. The first email was acknowledge email when I filed that bug.
>
> Now when I see https://debbugs.gnu.org/cgi/bugreport.cgi?bug=31929, I
> notice that a lot of conversation happened, but I got none of those emails.

This is what happens when people don't use reply-to-all.

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Glenn Morris]

After the fact, I'm wondering if this should obey the EMACS_NAME
transformation? My initial feeling is, yes.

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Eli Zaretskii]

> From: Philipp Stephani <p.stephani2@gmail.com>
> Date: Wed, 3 Oct 2018 22:02:08 +0200
> Cc: Philipp Stephani <phst@google.com>
> 
> Since there were no more comments, I've installed (a tiny variant of) this patch as 00ea749f2a.

Please cherry-pick this to the release branch.

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Date: Wed, 03 Oct 2018 16:51:04 -0400
> Cc: p.stephani2@gmail.com, kaushal.modi@gmail.com
> 
> After the fact, I'm wondering if this should obey the EMACS_NAME
> transformation? My initial feeling is, yes.

I'm not sure.  If we edit the name, a given module couldn't be
compiled against differently-named Emacs distributions, which I think
is undesirable.  Also, compiling a module inside the Emacs tree and
outside it will need source-level changes to support both.  Lastly, we
name the file explicitly in the documentation.

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Glenn Morris]

Eli Zaretskii wrote:

>> After the fact, I'm wondering if this should obey the EMACS_NAME
>> transformation? My initial feeling is, yes.
>
> I'm not sure.  If we edit the name, a given module couldn't be
> compiled against differently-named Emacs distributions, which I think
> is undesirable. 

I think it is what users of the --program-prefix|suffix|transform
configure options will probably expect to happen.
Ie if someone wants "emacs" installed as "emacs27", I think they want
"emacs27-module.h" as well.

> Lastly, we name the file explicitly in the documentation.

The documentation also names eg the "emacs" and "emacslient"
executables, which are rather more fundamental. The documentation does
not account for the --program-transform etc options anywhere, and I
don't think it should. It's a specialized feature, and people who use it
should be able to deal with the consequences.

bug#31929: [PATCH] Install emacs-module.h (Bug#31929)

[Eli Zaretskii]

> From: Glenn Morris <rgm@gnu.org>
> Cc: 31929@debbugs.gnu.org,  p.stephani2@gmail.com,  kaushal.modi@gmail.com
> Date: Mon, 15 Oct 2018 16:04:49 -0400
> 
> > Lastly, we name the file explicitly in the documentation.
> 
> The documentation also names eg the "emacs" and "emacslient"

I mean the examples in which there's an #include statement.  It's not
the same as naming executables, IMO.

I think we should wait until we hear real-life use cases where the
fixed name is a problem, before we decide on a solution.