Merging feature/android

Merging feature/android

[Po Lu]

Please take a look at the feature/android branch.

I would like to merge it into master in the next couple of days, but
before I do so I want everyone else to be happy with its contents as
well.  So would everyone please do the usual with the diff between the
branch and master?

Thanks.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Date: Thu, 02 Mar 2023 12:05:23 +0800
> 
> Please take a look at the feature/android branch.
> 
> I would like to merge it into master in the next couple of days, but
> before I do so I want everyone else to be happy with its contents as
> well.  So would everyone please do the usual with the diff between the
> branch and master?

How long was this branch kept in its finished state, and how many
people tried it?  Does it build on the main supported systems?

If the branch in its current state is relatively "young", I'd prefer
to delay merging it for a month or so, to give more people chance to
build and try it.  There's no rush in landing it.

Some specific comments below.

The INSTALL.android file:

 . It should be in a subdirectory, like we do with nt/INSTALL (and
   NEWS should be updated to that effect).
 . The NDK BUILD SYSTEM IMPLEMENTATION section doesn't belong in
   INSTALL, IMO.  It should be a separate file, since it's mainly of
   interest to Emacs developers.
 . The PATCH FOR LIBXML2 part and similar parts for patching other
   libraries and components should also be in separate files, suitable
   for submitting to Patch or similar utilities, and INSTALL should
   only mention the need for these patch and refer to those files. 
 . The copying conditions should be really at the end, not in the
   middle.

admin/merge-gnulib: I don't think we should change this without a
review from Paul Eggert.  The additional modules you add may need to
be disabled in nt/gnulib-cfg.mk if for some reason they are compiled
without being needed.

-OPTION_DEFAULT_ON([modules],[don't compile with dynamic modules support])
+OPTION_DEFAULT_IFAVAILABLE([modules],[don't compile with dynamic modules support])

Why was this changed to "ifavailable"?

The changes in configure.ac that disable various warning options:

+  nw="$nw -Wunknown-warning-option" # Let #pragma GCC ignore work properly
+                                   # even when the compiler in use doesn't
+                                   # support the option.

+  # If Emacs is being built for Android and many functions are
+  # currently stubbed out for operation on the build machine, disable
+  # -Wsuggest-attribute=noreturn.
+
+  nw="$nw -Wsuggest-attribute=noreturn"

+    gl_WARN_ADD([-Wno-shift-overflow])

If these are specific to Android, they should have suitable conditions
for when to apply them.

The gecos test in configure.ac:

+# Check for pw_gecos in struct passwd; this is known to be missing on
+# Android.
+
+AC_CHECK_MEMBERS([struct passwd.pw_gecos], [], [], [#include <pwd.h>])

This could fail the MS-Windows build -- did you check that it doesn't
have any adverse effect on that?

CM_OBJ setting in configure.ac:

-CM_OBJ="cm.o"

Why was this deleted?

+  Does Emacs use Android?                                 ${ANDROID}

This should say "Is Emacs being built for Android?" instead.

The files in cross/lib/ seem to be from Gnulib?  If so, do we really
need another copy of them in the tree? any way to reuse the sources in
lib/ instead?

General remark about *.texi files: it looks like you used TABs there;
you should only use spaces for alignment in Texinfo.

The code in from_unicode_buffer that is used only on Android should be
under an appropriate #ifdef.  Likewise with other such code, if any.

Thanks.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> How long was this branch kept in its finished state, and how many
> people tried it?  Does it build on the main supported systems?

It builds on GNU/Linux and Haiku, I've not tested this on other systems
yet.  I plan to fix the MS-DOS build a while after it is merged.

> If the branch in its current state is relatively "young", I'd prefer
> to delay merging it for a month or so, to give more people chance to
> build and try it.  There's no rush in landing it.

Thanks, then I guess I'll ask in April.  It's been finished for about a
week now.

> Some specific comments below.
>
> The INSTALL.android file:
>
>  . It should be in a subdirectory, like we do with nt/INSTALL (and
>    NEWS should be updated to that effect).
>  . The NDK BUILD SYSTEM IMPLEMENTATION section doesn't belong in
>    INSTALL, IMO.  It should be a separate file, since it's mainly of
>    interest to Emacs developers.

Sure.

>  . The PATCH FOR LIBXML2 part and similar parts for patching other
>    libraries and components should also be in separate files, suitable
>    for submitting to Patch or similar utilities, and INSTALL should
>    only mention the need for these patch and refer to those files.

What would be a good place to put these patch files? admin/notes
perhaps?

> admin/merge-gnulib: I don't think we should change this without a
> review from Paul Eggert.  The additional modules you add may need to
> be disabled in nt/gnulib-cfg.mk if for some reason they are compiled
> without being needed.

Okay.  Paul, the Android port really needs the `printf-posix' and
`vasprintf-posix' modules (as Android's printf ranges from ``completely
broken'' to ``just missing %td'' depending on the OS version being
used), but stpncpy and getline are only ``nice-to-have''s.  Is there any
downside to depending on those additional gnulib modules?  And will they
build on MS Windows as well?

> -OPTION_DEFAULT_ON([modules],[don't compile with dynamic modules support])
> +OPTION_DEFAULT_IFAVAILABLE([modules],[don't compile with dynamic modules support])
>
> Why was this changed to "ifavailable"?

Because the modules code has a dependency on the GCC cleanup function
extension, and fails to compile when it is not present.  I rewrote the
configury to detect the presence of the extension and not build with
modules when that is in effect.

> The changes in configure.ac that disable various warning options:
>
> +  nw="$nw -Wunknown-warning-option" # Let #pragma GCC ignore work properly
> +                                   # even when the compiler in use doesn't
> +                                   # support the option.
>
> +  # If Emacs is being built for Android and many functions are
> +  # currently stubbed out for operation on the build machine, disable
> +  # -Wsuggest-attribute=noreturn.
> +
> +  nw="$nw -Wsuggest-attribute=noreturn"
>
> +    gl_WARN_ADD([-Wno-shift-overflow])
>
> If these are specific to Android, they should have suitable conditions
> for when to apply them.

OK, thanks for catching this.

> The gecos test in configure.ac:
>
> +# Check for pw_gecos in struct passwd; this is known to be missing on
> +# Android.
> +
> +AC_CHECK_MEMBERS([struct passwd.pw_gecos], [], [], [#include <pwd.h>])
>
> This could fail the MS-Windows build -- did you check that it doesn't
> have any adverse effect on that?

It should not, since the result of the check is only used on Unix
systems.

> CM_OBJ setting in configure.ac:
>
> -CM_OBJ="cm.o"
>
> Why was this deleted?

It wasn't, I simply moved it further up.

> +  Does Emacs use Android?                                 ${ANDROID}
>
> This should say "Is Emacs being built for Android?" instead.

OK, thanks.

> The files in cross/lib/ seem to be from Gnulib?  If so, do we really
> need another copy of them in the tree? any way to reuse the sources in
> lib/ instead?

I tried multiple times, but the gnulib stuff kept trying to include
generated headers from the wrong copy of gnulib, so in the end I
couldn't find any way around having to keep two copies of gnulib
in-tree.

In the past cross/lib was also used to hold patches for Android, but the
gnulib folks have now fixed all of the problems which required patches.

> General remark about *.texi files: it looks like you used TABs there;
> you should only use spaces for alignment in Texinfo.
>
> The code in from_unicode_buffer that is used only on Android should be
> under an appropriate #ifdef.  Likewise with other such code, if any.
>
> Thanks.

I will fix these too, thanks.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org, Paul Eggert <eggert@cs.ucla.edu>
> Date: Thu, 02 Mar 2023 18:19:50 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >  . The PATCH FOR LIBXML2 part and similar parts for patching other
> >    libraries and components should also be in separate files, suitable
> >    for submitting to Patch or similar utilities, and INSTALL should
> >    only mention the need for these patch and refer to those files.
> 
> What would be a good place to put these patch files? admin/notes
> perhaps?

In the same android/ subdirectory, I think.

> > -OPTION_DEFAULT_ON([modules],[don't compile with dynamic modules support])
> > +OPTION_DEFAULT_IFAVAILABLE([modules],[don't compile with dynamic modules support])
> >
> > Why was this changed to "ifavailable"?
> 
> Because the modules code has a dependency on the GCC cleanup function
> extension, and fails to compile when it is not present.  I rewrote the
> configury to detect the presence of the extension and not build with
> modules when that is in effect.

Rewriting the configury is OK, but that still doesn't need
ifavailable, AFAIU.  For example, we build with HarfBuzz if that is
available, but we don't use ifavailable for it.  How is this one
different?

> > The files in cross/lib/ seem to be from Gnulib?  If so, do we really
> > need another copy of them in the tree? any way to reuse the sources in
> > lib/ instead?
> 
> I tried multiple times, but the gnulib stuff kept trying to include
> generated headers from the wrong copy of gnulib, so in the end I
> couldn't find any way around having to keep two copies of gnulib
> in-tree.

What do you mean by "from the wrong copy"?  The two copies are
identical, no?  They are generated for the same platform, right?

In any case, before we include two copies, we should ask Gnulib folks
for help in this matter.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Rewriting the configury is OK, but that still doesn't need
> ifavailable, AFAIU.  For example, we build with HarfBuzz if that is
> available, but we don't use ifavailable for it.  How is this one
> different?

Well, it would be a nasty shock for someone to configure --with-modules,
and not get an error when dynamic modules cannot be used.

> What do you mean by "from the wrong copy"?  The two copies are
> identical, no?  They are generated for the same platform, right?

They are generated for different platforms: the first copy is generated
for the system which is building Emacs (where we need an emacs binary to
build the ELCs), whilst the second copy in cross/ is generated for the
Android system.

> In any case, before we include two copies, we should ask Gnulib folks
> for help in this matter.

Perhaps Paul has a better idea, yes.  Thanks in advance.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Thu, 02 Mar 2023 21:42:30 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Rewriting the configury is OK, but that still doesn't need
> > ifavailable, AFAIU.  For example, we build with HarfBuzz if that is
> > available, but we don't use ifavailable for it.  How is this one
> > different?
> 
> Well, it would be a nasty shock for someone to configure --with-modules,
> and not get an error when dynamic modules cannot be used.

I'm talking about the default case, where there's no --with-modules
option explicitly in the configure command line.  If the user does
explicitly asks for modules, then emitting an error message is
reasonable.  But if the user didn't ask for that explicitly, why not
silently disable the feature, like we do with HarfBuzz and others?

> > What do you mean by "from the wrong copy"?  The two copies are
> > identical, no?  They are generated for the same platform, right?
> 
> They are generated for different platforms: the first copy is generated
> for the system which is building Emacs (where we need an emacs binary to
> build the ELCs), whilst the second copy in cross/ is generated for the
> Android system.
> 
> > In any case, before we include two copies, we should ask Gnulib folks
> > for help in this matter.
> 
> Perhaps Paul has a better idea, yes.  Thanks in advance.

I'd be surprised if Gnulib didn't have a way of supporting cross
builds similar to this case.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
>> Date: Thu, 02 Mar 2023 21:42:30 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > Rewriting the configury is OK, but that still doesn't need
>> > ifavailable, AFAIU.  For example, we build with HarfBuzz if that is
>> > available, but we don't use ifavailable for it.  How is this one
>> > different?
>> 
>> Well, it would be a nasty shock for someone to configure --with-modules,
>> and not get an error when dynamic modules cannot be used.
>
> I'm talking about the default case, where there's no --with-modules
> option explicitly in the configure command line.  If the user does
> explicitly asks for modules, then emitting an error message is
> reasonable.  But if the user didn't ask for that explicitly, why not
> silently disable the feature, like we do with HarfBuzz and others?

The HarfBuzz check doesn't behave that way, since it doesn't use
ifavailable.  ``--with-modules'' does, however.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Fri, 03 Mar 2023 08:51:11 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> >> Date: Thu, 02 Mar 2023 21:42:30 +0800
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > Rewriting the configury is OK, but that still doesn't need
> >> > ifavailable, AFAIU.  For example, we build with HarfBuzz if that is
> >> > available, but we don't use ifavailable for it.  How is this one
> >> > different?
> >> 
> >> Well, it would be a nasty shock for someone to configure --with-modules,
> >> and not get an error when dynamic modules cannot be used.
> >
> > I'm talking about the default case, where there's no --with-modules
> > option explicitly in the configure command line.  If the user does
> > explicitly asks for modules, then emitting an error message is
> > reasonable.  But if the user didn't ask for that explicitly, why not
> > silently disable the feature, like we do with HarfBuzz and others?
> 
> The HarfBuzz check doesn't behave that way, since it doesn't use
> ifavailable.  ``--with-modules'' does, however.

I don't understand your reasoning, and more importantly, I don't
understand the conclusion.  Do you agree that ifavailable shouldn't be
needed in this case?  If not, please explain more, because I don't
think I understand what you say above.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> If not, please explain more, because I don't think I understand what
> you say above.

I was saying that the HarfBuzz check currently doesn't show an error
when it is enabled with ``--with-harfbuzz'' and HarfBuzz is not found.

The reason is precisely that the option doesn't default to ifavailable;
as a result, configure cannot tell apart the two cases where
$with_harfbuzz is specified on the command line, and when it is simply
set to yes as its default value.  That's why ifavailable is required.

Thanks.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Fri, 03 Mar 2023 16:03:38 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > If not, please explain more, because I don't think I understand what
> > you say above.
> 
> I was saying that the HarfBuzz check currently doesn't show an error
> when it is enabled with ``--with-harfbuzz'' and HarfBuzz is not found.

That's strange, because image libraries use the same OPTION_DEFAULT_ON
way, and I definitely remember seeing an error message when I used the
"--with-gif" option and the library wasn't available.  The message
suggested using ifavailable instead.

> The reason is precisely that the option doesn't default to ifavailable;
> as a result, configure cannot tell apart the two cases where
> $with_harfbuzz is specified on the command line, and when it is simply
> set to yes as its default value.  That's why ifavailable is required.

Do you see the same with image libraries?

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
>> Date: Fri, 03 Mar 2023 16:03:38 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > If not, please explain more, because I don't think I understand what
>> > you say above.
>> 
>> I was saying that the HarfBuzz check currently doesn't show an error
>> when it is enabled with ``--with-harfbuzz'' and HarfBuzz is not found.
>
> That's strange, because image libraries use the same OPTION_DEFAULT_ON
> way, and I definitely remember seeing an error message when I used the
> "--with-gif" option and the library wasn't available.  The message
> suggested using ifavailable instead.
>
>> The reason is precisely that the option doesn't default to ifavailable;
>> as a result, configure cannot tell apart the two cases where
>> $with_harfbuzz is specified on the command line, and when it is simply
>> set to yes as its default value.  That's why ifavailable is required.
>
> Do you see the same with image libraries?

Image libraries default to being on, while also accepting
``ifavailable'' as an option.  The default values of the options for the
common image libraries is not ``ifavailable'', because we want configure
to error out by default if the library is not present.

Thanks.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Fri, 03 Mar 2023 17:51:08 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> >> Date: Fri, 03 Mar 2023 16:03:38 +0800
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > If not, please explain more, because I don't think I understand what
> >> > you say above.
> >> 
> >> I was saying that the HarfBuzz check currently doesn't show an error
> >> when it is enabled with ``--with-harfbuzz'' and HarfBuzz is not found.
> >
> > That's strange, because image libraries use the same OPTION_DEFAULT_ON
> > way, and I definitely remember seeing an error message when I used the
> > "--with-gif" option and the library wasn't available.  The message
> > suggested using ifavailable instead.
> >
> >> The reason is precisely that the option doesn't default to ifavailable;
> >> as a result, configure cannot tell apart the two cases where
> >> $with_harfbuzz is specified on the command line, and when it is simply
> >> set to yes as its default value.  That's why ifavailable is required.
> >
> > Do you see the same with image libraries?
> 
> Image libraries default to being on, while also accepting
> ``ifavailable'' as an option.  The default values of the options for the
> common image libraries is not ``ifavailable'', because we want configure
> to error out by default if the library is not present.

So why shouldn't it be the same with modules?  Don't we want the user
to be aware that modules support will be absent?

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> So why shouldn't it be the same with modules?  Don't we want the user
> to be aware that modules support will be absent?

No, I don't think so: that would amount to making Emacs not build
by default without a recent GCC.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Fri, 03 Mar 2023 21:28:27 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > So why shouldn't it be the same with modules?  Don't we want the user
> > to be aware that modules support will be absent?
> 
> No, I don't think so: that would amount to making Emacs not build
> by default without a recent GCC.

We had this in emacs-module.c for a long time, including two major
releases.  How come it was never a problem until now?

Re: Merging feature/android

[Paul Eggert]

On 2023-03-02 02:19, Po Lu wrote:

> Paul, the Android port really needs the `printf-posix' and
> `vasprintf-posix' modules (as Android's printf ranges from ``completely
> broken'' to ``just missing %td'' depending on the OS version being
> used), but stpncpy and getline are only ``nice-to-have''s.  Is there any
> downside to depending on those additional gnulib modules?  And will they
> build on MS Windows as well?

They should build. They'll bring in a lot of support modules, but if we 
play our cards right those modules will be built only on Android so it's 
only a matter of library code clutter.

> I tried multiple times, but the gnulib stuff kept trying to include
> generated headers from the wrong copy of gnulib, so in the end I
> couldn't find any way around having to keep two copies of gnulib
> in-tree.

This should be doable by having two build directories, but only one copy 
of the Gnulib source should be needed. The two build directories would 
have different config.h files. You'd run 'configure' twice (or have two 
'configure' files if you want to be fancier). That sort of thing.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> This should be doable by having two build directories, but only one
> copy of the Gnulib source should be needed. The two build directories
> would have different config.h files. You'd run 'configure' twice (or
> have two 'configure' files if you want to be fancier). That sort of
> thing.

They do, yes, but the problem is the C compiler doesn't understand GNU
make vpath directives, so the includes in lib/ are:

  -I$(srcdir) -I.

while the includes in cross/src/lib are:

  -I$(top_srcdir)/lib -I$(srcdir) -I.

and as a result, when a file in -I$(top_srcdir)/lib then includes:

  #include "stdio.h"

it gets the stdio.h in lib, and not cross/lib, leading to silent
problems later on.

Thanks.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
>> Date: Fri, 03 Mar 2023 21:28:27 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > So why shouldn't it be the same with modules?  Don't we want the user
>> > to be aware that modules support will be absent?
>> 
>> No, I don't think so: that would amount to making Emacs not build
>> by default without a recent GCC.
>
> We had this in emacs-module.c for a long time, including two major
> releases.  How come it was never a problem until now?

I don't know, but it doesn't surprise me, since most people use GCC.

Re: Merging feature/android

[Eli Zaretskii]

> Date: Fri, 3 Mar 2023 13:17:00 -0800
> Cc: emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 2023-03-02 02:19, Po Lu wrote:
> 
> > Paul, the Android port really needs the `printf-posix' and
> > `vasprintf-posix' modules (as Android's printf ranges from ``completely
> > broken'' to ``just missing %td'' depending on the OS version being
> > used), but stpncpy and getline are only ``nice-to-have''s.  Is there any
> > downside to depending on those additional gnulib modules?  And will they
> > build on MS Windows as well?
> 
> They should build. They'll bring in a lot of support modules, but if we 
> play our cards right those modules will be built only on Android so it's 
> only a matter of library code clutter.

Just having lib/ files that aren't built on some platforms is not a
problem, from my POV.  We have that already, actually: many of those
files aren't built on GNU/Linux with new enough kernel and glibc.

> > I tried multiple times, but the gnulib stuff kept trying to include
> > generated headers from the wrong copy of gnulib, so in the end I
> > couldn't find any way around having to keep two copies of gnulib
> > in-tree.
> 
> This should be doable by having two build directories, but only one copy 
> of the Gnulib source should be needed. The two build directories would 
> have different config.h files. You'd run 'configure' twice (or have two 
> 'configure' files if you want to be fancier). That sort of thing.

I think running configure twice could be a minor annoyance, but we
could arrange for the configure script or for some Makefile to run
another configure script when needed.  The Sourceware build tree does
something like that when you build GCC, Binutils, or GDB.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  emacs-devel@gnu.org
> Date: Sat, 04 Mar 2023 08:06:34 +0800
> 
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
> > This should be doable by having two build directories, but only one
> > copy of the Gnulib source should be needed. The two build directories
> > would have different config.h files. You'd run 'configure' twice (or
> > have two 'configure' files if you want to be fancier). That sort of
> > thing.
> 
> They do, yes, but the problem is the C compiler doesn't understand GNU
> make vpath directives, so the includes in lib/ are:
> 
>   -I$(srcdir) -I.
> 
> while the includes in cross/src/lib are:
> 
>   -I$(top_srcdir)/lib -I$(srcdir) -I.
> 
> and as a result, when a file in -I$(top_srcdir)/lib then includes:
> 
>   #include "stdio.h"
> 
> it gets the stdio.h in lib, and not cross/lib, leading to silent
> problems later on.

Can you perhaps use -isystem or -iquote or -dirafter compiler switches
to work around these problems?

Anyway, I think we should fix these issues before we land the branch.
I'd really hate to have 2 copies of Gnulib files in the repository.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sat, 04 Mar 2023 08:07:27 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> >> Date: Fri, 03 Mar 2023 21:28:27 +0800
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> > So why shouldn't it be the same with modules?  Don't we want the user
> >> > to be aware that modules support will be absent?
> >> 
> >> No, I don't think so: that would amount to making Emacs not build
> >> by default without a recent GCC.
> >
> > We had this in emacs-module.c for a long time, including two major
> > releases.  How come it was never a problem until now?
> 
> I don't know, but it doesn't surprise me, since most people use GCC.

macOS builds don't use GCC.

But if this problem is basically limited to Android, how about solving
it in emacs-module.c, via preprocessor directives, instead?  Or even
unconditionally disable modules in the configure script for Android?
I think making changes that affect all the platforms for the benefit
of Android should be limited to the absolute minimum.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> macOS builds don't use GCC.

They use Clang, which pretends to be GCC well enough to support this
extension.

> But if this problem is basically limited to Android, how about solving
> it in emacs-module.c, via preprocessor directives, instead?  Or even
> unconditionally disable modules in the configure script for Android?
> I think making changes that affect all the platforms for the benefit
> of Android should be limited to the absolute minimum.

This problem in emacs-module.c affects all platforms except MS-DOS.
Basically any non-GCC and non-Clang compiler cannot compile Emacs by
default.

Also, some versions of Android's GCC do support the cleanup attribute,
while others are new enough to support it but signal an ICE compiling
any code that actually uses it.  And Android's clang is a hit and miss I
have not yet figured out.

In general, it is better to write tests for a feature than tests for a
version of one tool known to support a feature.  That's why Autoconf was
created.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Can you perhaps use -isystem or -iquote or -dirafter compiler switches
> to work around these problems?

I tried, but GCC always looks in the directory where the file lies
first.

> Anyway, I think we should fix these issues before we land the branch.
> I'd really hate to have 2 copies of Gnulib files in the repository.

If gnulib provided a list of its C files, we could arrange for autoconf
to symlink the files in lib/ to cross/lib/ during configuration.  But I
could not find any such list.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> Date: Fri, 3 Mar 2023 13:17:00 -0800
>> Cc: emacs-devel@gnu.org
>> From: Paul Eggert <eggert@cs.ucla.edu>
>> 
>> On 2023-03-02 02:19, Po Lu wrote:
>> 
>> > Paul, the Android port really needs the `printf-posix' and
>> > `vasprintf-posix' modules (as Android's printf ranges from ``completely
>> > broken'' to ``just missing %td'' depending on the OS version being
>> > used), but stpncpy and getline are only ``nice-to-have''s.  Is there any
>> > downside to depending on those additional gnulib modules?  And will they
>> > build on MS Windows as well?
>> 
>> They should build. They'll bring in a lot of support modules, but if we 
>> play our cards right those modules will be built only on Android so it's 
>> only a matter of library code clutter.
>
> Just having lib/ files that aren't built on some platforms is not a
> problem, from my POV.  We have that already, actually: many of those
> files aren't built on GNU/Linux with new enough kernel and glibc.
>
>> > I tried multiple times, but the gnulib stuff kept trying to include
>> > generated headers from the wrong copy of gnulib, so in the end I
>> > couldn't find any way around having to keep two copies of gnulib
>> > in-tree.
>> 
>> This should be doable by having two build directories, but only one copy 
>> of the Gnulib source should be needed. The two build directories would 
>> have different config.h files. You'd run 'configure' twice (or have two 
>> 'configure' files if you want to be fancier). That sort of thing.
>
> I think running configure twice could be a minor annoyance, but we
> could arrange for the configure script or for some Makefile to run
> another configure script when needed.  The Sourceware build tree does
> something like that when you build GCC, Binutils, or GDB.

We already run configure twice: if you configure --with-android, then
very early on (after initializing the Java compiler stuff), configure
calls a stripped-down version of itself to generate Makefiles using the
Android compiler.  This includes gnulib.mk and lib/Makefile.in, which
are copied to cross/lib and used there to cross-compile gnulib.

So ``cross'' itself kind of serves as this second build directory.  The
problem is that the ``first'' build directory happens to be the same
directory containing the gnulib sources.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-04 00:08, Po Lu wrote:
> If gnulib provided a list of its C files, we could arrange for autoconf
> to symlink the files in lib/ to cross/lib/ during configuration.  But I
> could not find any such list.

m4/gnulib-comp.m4 has gl_FILE_LIST. Although this lists all files not 
just C files, it should be easy enough to extract the C files.


> the C compiler doesn't understand GNU
> make vpath directives, so the includes in lib/ are:
> 
>   -I$(srcdir) -I.
> 
> while the includes in cross/src/lib are:
> 
>   -I$(top_srcdir)/lib -I$(srcdir) -I.
> 
> and as a result, when a file in -I$(top_srcdir)/lib then includes:
> 
>   #include "stdio.h"
> 
> it gets the stdio.h in lib, and not cross/lib, leading to silent
> problems later on.

I'm not quite following, since I'm not sure whether we're talking about 
an in-tree build or an out-of-tree build. That being said, can we 
address the problem by changing the order of the includes in 
cross/src/lib/Makefile?

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> m4/gnulib-comp.m4 has gl_FILE_LIST. Although this lists all files not
> just C files, it should be easy enough to extract the C files.

Thanks!

>> the C compiler doesn't understand GNU
>> make vpath directives, so the includes in lib/ are:
>>   -I$(srcdir) -I.
>> while the includes in cross/src/lib are:
>>   -I$(top_srcdir)/lib -I$(srcdir) -I.
>> and as a result, when a file in -I$(top_srcdir)/lib then includes:
>>   #include "stdio.h"
>> it gets the stdio.h in lib, and not cross/lib, leading to silent
>> problems later on.
>
> I'm not quite following, since I'm not sure whether we're talking
> about an in-tree build or an out-of-tree build. That being said, can
> we address the problem by changing the order of the includes in
> cross/src/lib/Makefile?

Nope, that doesn't work.  I think things would become much clearer if
you read the code in cross/lib/Makefile.in; I'm bad at explaining
things.

We're talking about a half in-tree, half out-of-tree build: the cross
compiled part is built ``out-of-tree'', while the libgnu.a used on the
build machine can either be built in-tree or out-of-tree.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sat, 04 Mar 2023 16:05:19 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > macOS builds don't use GCC.
> 
> They use Clang, which pretends to be GCC well enough to support this
> extension.
> 
> > But if this problem is basically limited to Android, how about solving
> > it in emacs-module.c, via preprocessor directives, instead?  Or even
> > unconditionally disable modules in the configure script for Android?
> > I think making changes that affect all the platforms for the benefit
> > of Android should be limited to the absolute minimum.
> 
> This problem in emacs-module.c affects all platforms except MS-DOS.
> Basically any non-GCC and non-Clang compiler cannot compile Emacs by
> default.

What are those "non-GCC and non-Clang compilers" that are prone to
this problem?

> Also, some versions of Android's GCC do support the cleanup attribute,
> while others are new enough to support it but signal an ICE compiling
> any code that actually uses it.  And Android's clang is a hit and miss I
> have not yet figured out.

<Shrug> It is strange that GCC and Clang for Android behave
differently in this regard from the same compilers configured for
other platforms.  The cleanup attribute is not supposed to be so
platform-dependent, AFAIU.

> In general, it is better to write tests for a feature than tests for a
> version of one tool known to support a feature.  That's why Autoconf was
> created.

I'm okay with writing tests for this, but can be arrange for those
tests to be run only in the Android build?  Once again, I'd prefer not
to make non-trivial changes in our configury with a clear test case,
and we don't have any such case that doesn't involve the Android
build, AFAIU.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> What are those "non-GCC and non-Clang compilers" that are prone to
> this problem?

tcc, for instance.  Or the various compilers found on Unix systems.

> I'm okay with writing tests for this, but can be arrange for those
> tests to be run only in the Android build?  Once again, I'd prefer not
> to make non-trivial changes in our configury with a clear test case,
> and we don't have any such case that doesn't involve the Android
> build, AFAIU.

We should not assume that all the world is any specific compiler, be it
GCC or Clang.  Especially not in a configure script, whose job is to
make sure Emacs can adapt itself to any reasonable Unix system,
including ones that may show up in the future, such as a tcc-based
GNU/Linux system.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sat, 04 Mar 2023 20:12:59 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > What are those "non-GCC and non-Clang compilers" that are prone to
> > this problem?
> 
> tcc, for instance.  Or the various compilers found on Unix systems.

And Emacs can be, and is actually, built with them?

> > I'm okay with writing tests for this, but can be arrange for those
> > tests to be run only in the Android build?  Once again, I'd prefer not
> > to make non-trivial changes in our configury with a clear test case,
> > and we don't have any such case that doesn't involve the Android
> > build, AFAIU.
> 
> We should not assume that all the world is any specific compiler, be it
> GCC or Clang.

We don't.  Function attributes are widely used feature nowadays.

> Especially not in a configure script, whose job is to make sure
> Emacs can adapt itself to any reasonable Unix system, including ones
> that may show up in the future, such as a tcc-based GNU/Linux
> system.

Our configury is complicated enough for us to avoid solving any
theoretical problems by further complicating it.  I'm mainly bothered
by any possible effect this could have on users who configure Emacs,
and would like to avoid any unnecessary risks.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> And Emacs can be, and is actually, built with them?

Yes.  Emacs builds fine with TCC, lcc, Sun C, etc, given that you
disable the dynamic module support.

> We don't.  Function attributes are widely used feature nowadays.

The cleanup attribute specifically is a GCC and Clang feature not found
in other compilers.  Along with most other __attribute__ syntax.

> Our configury is complicated enough for us to avoid solving any
> theoretical problems by further complicating it.  I'm mainly bothered
> by any possible effect this could have on users who configure Emacs,
> and would like to avoid any unnecessary risks.

Well, Emacs currently doesn't work on any number of systems for this
reason.  And the test will get a suitable amount of testing on master
before being released.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-04 16:06, Po Lu wrote:
> Emacs builds fine with TCC, lcc, Sun C, etc, given that you
> disable the dynamic module support.

Actually dynamic module support should build with Sun C 5.15 (i.e., 
Oracle Solaris Studio 12.6), because Sun C supports GNU C's 
__attribute__ ((cleanup)) extension. I think icc also supports that 
extension. Not that I've ever used modules on those platforms.

As I vaguely recall, this issue came up in 2015 when Emacs modules were 
added, and it was decided that there wasn't a practical way to get Emacs 
modules to work if the compiler did not support a cleanup attribute of 
some sort.

I'm a bit surprised that the Android NDK doesn't support the attribute, 
though, as I thought it was based on Clang. Perhaps there's an alternate 
compiler you can use to build on Android.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> Actually dynamic module support should build with Sun C 5.15 (i.e.,
> Oracle Solaris Studio 12.6), because Sun C supports GNU C's
> __attribute__ ((cleanup)) extension. I think icc also supports that
> extension. Not that I've ever used modules on those platforms.

Well, I do recall /opt/SUNwspro/bin/cc choking on emacs-module.c at some
point in the past.  I didn't think much of it at the time and just gave:

  ./configure CC=gcc7

but that's not something we want to make everyone do.  In retrospect, it
probably failed because the ifdefs in emacs-module.c did not detect the
attribute support correctly.

> As I vaguely recall, this issue came up in 2015 when Emacs modules
> were added, and it was decided that there wasn't a practical way to
> get Emacs modules to work if the compiler did not support a cleanup
> attribute of some sort.

Right, so the correct solution is to check whether or not the compiler
supports such an extension, and disable dynamic module support should it
not.  Which is why we use configure in the first place, instead of
performing ``checks'' with `GNUC_PREREQ'.

> I'm a bit surprised that the Android NDK doesn't support the
> attribute, though, as I thought it was based on Clang. Perhaps there's
> an alternate compiler you can use to build on Android.

The attribute leads to a compiler error generating while generating
debug information, or alternatively the

 #error "__attribute__ ((cleanup)) not supported by this compiler; try GCC"

in emacs-module.c being triggered, depending on the version of the NDK
you use.

r21 and r25 are free of this illness, but they don't support old
versions of Android that Emacs does, such as Android 4.4 or 2.2.

  (And the other day someone asked me for a build of Emacs that runs on
   Android 4.1, so these versions are evidently being used.)

Re: Merging feature/android

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > We should not assume that all the world is any specific compiler, be it
  > GCC or Clang.

We have no responsibility to support any particular compiler other
than GCC.  We can do so when we wish.

If someone sends us patches to support compiler BARCC, and they are
simple enough, we will probably install them just to be helpful.  But
if it isn't easy. we should say "no thanks".

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Merging feature/android

[Po Lu]

Richard Stallman <rms@gnu.org> writes:

> We have no responsibility to support any particular compiler other
> than GCC.  We can do so when we wish.

That's not what the GNU Coding Standards say:

  An exception to this rule are the large, established programs (such as
  Emacs) which run on a great variety of systems. Using GNU extensions
  in such programs would make many users unhappy, so we don’t do that.

  Another exception is for programs that are used as part of
  compilation: anything that must be compiled with other compilers in
  order to bootstrap the GNU compilation facilities. If these require
  the GNU compiler, then no one can compile them without having them
  installed already. That would be extremely troublesome in certain
  cases.

> If someone sends us patches to support compiler BARCC, and they are
> simple enough, we will probably install them just to be helpful.  But
> if it isn't easy. we should say "no thanks".

In this specific case, the patch being discussed is a configure test to
disable an Emacs feature when a GCC extension is not present.  That's
very easy.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sun, 05 Mar 2023 08:06:04 +0800
> 
> > Our configury is complicated enough for us to avoid solving any
> > theoretical problems by further complicating it.  I'm mainly bothered
> > by any possible effect this could have on users who configure Emacs,
> > and would like to avoid any unnecessary risks.
> 
> Well, Emacs currently doesn't work on any number of systems for this
> reason.

Why haven't we heard about those problems until now?

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
>> Date: Sun, 05 Mar 2023 08:06:04 +0800
>> 
>> > Our configury is complicated enough for us to avoid solving any
>> > theoretical problems by further complicating it.  I'm mainly bothered
>> > by any possible effect this could have on users who configure Emacs,
>> > and would like to avoid any unnecessary risks.
>> 
>> Well, Emacs currently doesn't work on any number of systems for this
>> reason.
>
> Why haven't we heard about those problems until now?

Because most users shrug and look for GCC, or even a complete GNU/Linux
system, to install Emacs on instead, instead of reporting problems they
see with configure.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sun, 05 Mar 2023 15:53:09 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> >> Date: Sun, 05 Mar 2023 08:06:04 +0800
> >> 
> >> > Our configury is complicated enough for us to avoid solving any
> >> > theoretical problems by further complicating it.  I'm mainly bothered
> >> > by any possible effect this could have on users who configure Emacs,
> >> > and would like to avoid any unnecessary risks.
> >> 
> >> Well, Emacs currently doesn't work on any number of systems for this
> >> reason.
> >
> > Why haven't we heard about those problems until now?
> 
> Because most users shrug and look for GCC, or even a complete GNU/Linux
> system, to install Emacs on instead, instead of reporting problems they
> see with configure.

Which is OK in my book.

So please make this change specific to Android.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-04 19:16, Po Lu wrote:
> Paul Eggert <eggert@cs.ucla.edu> writes:
> 
>> Actually dynamic module support should build with Sun C 5.15 (i.e.,
>> Oracle Solaris Studio 12.6), because Sun C supports GNU C's
>> __attribute__ ((cleanup)) extension. I think icc also supports that
>> extension. Not that I've ever used modules on those platforms.
> 
> Well, I do recall /opt/SUNwspro/bin/cc choking on emacs-module.c at some
> point in the past.

That's likely an earlier version of Sun C that does not support 
__attribute__ ((cleanup)). I doubt whether it's worth worrying about 
these older Sun C versions. People can get an up-to-date compiler, or 
(better yet) use GCC. It's not worth our time to port to obsolete Sun C.


> the correct solution is to check whether or not the compiler
> supports such an extension, and disable dynamic module support should it
> not.  Which is why we use configure in the first place, instead of
> performing ``checks'' with `GNUC_PREREQ'.

Emacs uses HAS_ATTRIBUTE, which should use Clang's __has_attribute, 
which should work with a recent-enough Android NDK (as these are based 
on Clang).

If you're using an older Android NDK based on GCC, yes that does use 
GNUC_PREREQ, but again this should work unless the older Android NDKs 
are squirrelly about GCC version numbers (are they?).

It's true that the Autoconf Way is to write a little program to test for 
__attribute__((cleanup)) directly, and that may be a good way to go. 
However, I would suggest first understanding why the current approach 
does not work. (But please see below; perhaps we don't need to worry 
about this at all.)


> r21 and r25 are free of this illness, but they don't support old
> versions of Android that Emacs does, such as Android 4.4 or 2.2.
> 
>    (And the other day someone asked me for a build of Emacs that runs on
>     Android 4.1, so these versions are evidently being used.)

Although I'm sure that ancient Android versions are still used 
somewhere, I suggest that we not worry about porting to Android versions 
so old that Google itself no longer supports them. This is the general 
guideline we've used for most other ports, and it should be a good 
guideline for Android too. We have limited development resources and 
it's generally not worth our time to port to an OS if it's not even 
worth the OS maintainer's time to support the OS.

Plus, Emacs is likely to have known security holes if we try to port it 
to no-longer-supported Android, and that would be bad for our users and 
bad publicity for Emacs. Android is one of the biggest malware targets 
out there.


> The attribute leads to a compiler error generating while generating
> debug information

If this Android NDK is so old that Google no longer supports it, I 
wouldn't worry about it. If not, perhaps we'll need to build Emacs 
without the debug-info option that is making that buggy compiler crash.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Which is OK in my book.

Why so?

IOW, what makes you think this change is unsafe, exactly?  How is this
any different for the __attribute__ ((aligned)) check, or any other
check for a compiler extension under which an Emacs feature is
conditional?

And why are all of the far more extensive checks which are made by
gnulib any different?

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> That's likely an earlier version of Sun C that does not support
> __attribute__ ((cleanup)). I doubt whether it's worth worrying about
> these older Sun C versions. People can get an up-to-date compiler, or
> (better yet) use GCC. It's not worth our time to port to obsolete Sun
> C.

But nothing prevents us from making this tiny change to our configury to
make Emacs work with those old compilers.

>> the correct solution is to check whether or not the compiler
>> supports such an extension, and disable dynamic module support should it
>> not.  Which is why we use configure in the first place, instead of
>> performing ``checks'' with `GNUC_PREREQ'.
>
> Emacs uses HAS_ATTRIBUTE, which should use Clang's __has_attribute,
> which should work with a recent-enough Android NDK (as these are based
> on Clang).
>
> If you're using an older Android NDK based on GCC, yes that does use
> GNUC_PREREQ, but again this should work unless the older Android NDKs
> are squirrelly about GCC version numbers (are they?).

The problem with GNUC_PREREQ is that while it works as intended, the
check in emacs-module.c doesn't catch the r16 gcc which throws ICEs
compiling a program, and the fallback is not to disable dynamic module
support, but to cause a compilation error.

> It's true that the Autoconf Way is to write a little program to test
> for __attribute__((cleanup)) directly, and that may be a good way to
> go. However, I would suggest first understanding why the current
> approach does not work. (But please see below; perhaps we don't need
> to worry about this at all.)

It is best to follow the Autoconf Way.  Otherwise, we will be heading
back to the age where configure looked for the a m/ header and then ran
Makefile.c through sed and cpp.

> Although I'm sure that ancient Android versions are still used
> somewhere, I suggest that we not worry about porting to Android
> versions so old that Google itself no longer supports them. This is
> the general guideline we've used for most other ports, and it should
> be a good guideline for Android too. We have limited development
> resources and it's generally not worth our time to port to an OS if
> it's not even worth the OS maintainer's time to support the OS.
>
> Plus, Emacs is likely to have known security holes if we try to port
> it to no-longer-supported Android, and that would be bad for our users
> and bad publicity for Emacs. Android is one of the biggest malware
> targets out there.

I cannot agree with this statement when I see every day my relatives and
coworkers using such old versions of Android, which are also supported
by many proprietary software developers.

An old version of the NDK works fine, and is in fact still supported by
Google (r16 is provided in the SDK manager's downloads page as an LTS
release), aside from having this nasty compiler problem.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sun, 05 Mar 2023 18:29:54 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Which is OK in my book.
> 
> Why so?

Because I don't mind at all if more people start using GCC.

> IOW, what makes you think this change is unsafe, exactly?  How is this
> any different for the __attribute__ ((aligned)) check, or any other
> check for a compiler extension under which an Emacs feature is
> conditional?

We made modules supported by default 2 releases ago, and I don't want
to risk silently dropping that in configurations that up till now
didn't need the test at all.

And there's also what Paul said: we don't really understand why the
existing conditions don't work in this case.  We should at least
understand that before we make non-trivial changes.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-05 02:40, Po Lu wrote:

> But nothing prevents us from making this tiny change to our configury to
> make Emacs work with those old compilers.

It may not be as simple as we'd like. If it involves testing the Android 
runtime then we could well be out of luck. If it's merely testing 
whether the compiler crashes then we may be OK (it partly depends on how 
"reliably" the compiler crashes :-).


> It is best to follow the Autoconf Way.  Otherwise, we will be heading
> back to the age where configure looked for the a m/ header and then ran
> Makefile.c through sed and cpp.

In Gnulib we prefer to follow the Autoconf Way. However, sometimes it's 
easier not to, such as when there are hard-to-test runtime errors or 
flaky compiler errors; in these cases we don't.


> I cannot agree with this statement when I see every day my relatives and
> coworkers using such old versions of Android, which are also supported
> by many proprietary software developers.

In the end it's up to you as to how you will spend your own time. 
However, I can't recommend that we significantly complicate Emacs for 
every developer, merely to cater to people running old, unsupported 
versions of Android with well-known security bugs that bad actors are 
targeting. Overall I expect that would be a net minus for the GNU project.


> An old version of the NDK works fine, and is in fact still supported by
> Google (r16 is provided in the SDK manager's downloads page as an LTS
> release), aside from having this nasty compiler problem.

Simply publishing downloads of old releases is not the same as 
supporting the releases. By "supported" I mean Google will fix serious 
bugs, such as security bugs, in the old releases. We don't want people 
building Emacs with serious security bugs.

As I understand it, r16 is no longer supported by Google. I'm basing my 
understanding on the listing of the r16b download on the Android NDK 
"Unsupported Downloads" wiki 
<https://github.com/android/ndk/wiki/Unsupported-Downloads>. If I'm 
wrong, please feel free to correct me; I'm certainly no expert on the NDK.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> We made modules supported by default 2 releases ago, and I don't want
> to risk silently dropping that in configurations that up till now
> didn't need the test at all.

What about printing a warning after configuration, if configure
concluded that the compiler does not support the attribute?  Then it
wouldn't be so silent.

BTW, I'd like to see the discussion where it was concluded that the
cleanup attribute is absolutely required.  It doesn't do fancy things
like clean up after Lisp signals under the hood, so all of the cleanup
can be done in portable C as well.

> And there's also what Paul said: we don't really understand why the
> existing conditions don't work in this case.  We should at least
> understand that before we make non-trivial changes.

I tried to explain that already.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-05 03:25, Po Lu wrote:
> I'd like to see the discussion where it was concluded that the
> cleanup attribute is absolutely required.  It doesn't do fancy things
> like clean up after Lisp signals under the hood, so all of the cleanup
> can be done in portable C as well.

As I recall there wasn't much discussion, unfortunately. I very vaguely 
recall that there was some worry that Emacs modules would be written in 
C++, and that they would invoke Elisp code, and that this meant it'd be 
hard to do the cleanup in portable C.

One option that was discussed was to require a C++ compiler to compile 
emacs-modules.cc (i.e., to write the emacs-modules interface in C++). I 
expect this would have addressed the cleanup issue in a different way. 
But RMS was very strongly against requiring a C++ compiler.

You may find this patch useful:

https://lists.gnu.org/archive/html/emacs-devel/2019-04/msg01075.html

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: emacs-devel@gnu.org,  eggert@cs.ucla.edu
> Date: Sun, 05 Mar 2023 19:25:44 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > We made modules supported by default 2 releases ago, and I don't want
> > to risk silently dropping that in configurations that up till now
> > didn't need the test at all.
> 
> What about printing a warning after configuration, if configure
> concluded that the compiler does not support the attribute?  Then it
> wouldn't be so silent.

Warnings during the run of configure go unnoticed, since configure is
extremely chatty and shows a lot of routine messages.

I originally suggested to fail with an error, like we do for image
libraries, and you objected.  I still think that is the best
alternative.  It will make sure users are aware of the issue, and can
decide whether to look and solve the problem or use ifavailable
instead.

> BTW, I'd like to see the discussion where it was concluded that the
> cleanup attribute is absolutely required.  It doesn't do fancy things
> like clean up after Lisp signals under the hood, so all of the cleanup
> can be done in portable C as well.

AFAICT, this attribute was used from the very beginning.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> As I recall there wasn't much discussion, unfortunately. I very
> vaguely recall that there was some worry that Emacs modules would be
> written in C++, and that they would invoke Elisp code, and that this
> meant it'd be hard to do the cleanup in portable C.
>
> One option that was discussed was to require a C++ compiler to compile
> emacs-modules.cc (i.e., to write the emacs-modules interface in
> C++). I expect this would have addressed the cleanup issue in a
> different way. But RMS was very strongly against requiring a C++
> compiler.

We build emacs with -fexceptions?  That's the only case where GCC turns:

  foo ()
  {
    type *k __attribute__((cleanup, cleanup_func));
    k = /* ... */;
  }

into something other than:

  foo ()
  {
    type *k __attribute__((cleanup, cleanup_func));
    k = /* ... */;
    cleanup_func (k);
  }

and I really hope we are not building emacs with -fexceptions, because
it really generates a lot of extra code that increases binary size.

> You may find this patch useful:
>
> https://lists.gnu.org/archive/html/emacs-devel/2019-04/msg01075.html

This is essentially what was installed on feature/android (except that
check also works while cross compiling, and is only run when modules are
enabled.)

This says that the change was dismissed by virtue of modules being
disabled by default, but now that has changed, so wouldn't it be the
right time to reconsider the ``makes configure slower'' argument?

BTW, with a cache file, you only pay the price of the test once.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2023-03-05 02:40, Po Lu wrote:
>
>> But nothing prevents us from making this tiny change to our configury to
>> make Emacs work with those old compilers.
>
> It may not be as simple as we'd like. If it involves testing the
> Android runtime then we could well be out of luck. If it's merely
> testing whether the compiler crashes then we may be OK (it partly
> depends on how "reliably" the compiler crashes :-).

It only depends on checking for the crash, which happens reliably with
the test I wrote.  ``cleanup'' is also a compiler only feature, as long
as you do not build C++ code or use -fexceptions, so the C++ runtime is
not involved at all.

>> I cannot agree with this statement when I see every day my relatives and
>> coworkers using such old versions of Android, which are also supported
>> by many proprietary software developers.
>
> In the end it's up to you as to how you will spend your own
> time. However, I can't recommend that we significantly complicate
> Emacs for every developer, merely to cater to people running old,
> unsupported versions of Android with well-known security bugs that bad
> actors are targeting. Overall I expect that would be a net minus for
> the GNU project.

[...]

> Simply publishing downloads of old releases is not the same as
> supporting the releases. By "supported" I mean Google will fix serious
> bugs, such as security bugs, in the old releases. We don't want people
> building Emacs with serious security bugs.
>
> As I understand it, r16 is no longer supported by Google. I'm basing
> my understanding on the listing of the r16b download on the Android
> NDK "Unsupported Downloads" wiki
> <https://github.com/android/ndk/wiki/Unsupported-Downloads>. If I'm
> wrong, please feel free to correct me; I'm certainly no expert on the
> NDK.

I'm not aware of a serious security bug in Google GCC or Clang which
affects compiled code.

Either way, if we go by what the Google does (or does not) support, then
in one or two years we will no longer be able to support the latest
version of Replicant.  Judging by that, the speed at which support is
removed for older Android versions is dropped is quite unreasonable.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Warnings during the run of configure go unnoticed, since configure is
> extremely chatty and shows a lot of routine messages.

Right, but we can display the warning after configure finishes.  Users
definitely listen to those, such as the movemail warning message.

> AFAICT, this attribute was used from the very beginning.

Please see my other reply to Paul.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-05 04:06, Po Lu wrote:

> We build emacs with -fexceptions?

Sorry, this is an area I know little about. (But I'll pontificate 
anyway. :-)

Perhaps the idea was that if you wanted modules written in C++, you 
could compile Emacs with -fexceptions, even though this is not the 
default build procedure. If so, I wouldn't worry too much about that, as 
I don't think anybody does that.

If we can simply stop using __attribute__ ((cleanup)), then I'm all for 
it. One less portability thing to worry about.

If on the other hand it's straightforward (albeit perhaps tedious) for 
Emacs to replace __attribute__ ((cleanup)) with a portable C idiom, then 
I'd be for that too. This change can be done independently of Android, 
though of course it will simplify support on Android.

Another possibility is that perhaps Emacs itself could stop using 
__attribute__ ((cleanup)), but modules would become responsible for 
cleanup. I can see problems with this approach, though.


>> https://lists.gnu.org/archive/html/emacs-devel/2019-04/msg01075.html
...
> This says that the change was dismissed by virtue of modules being
> disabled by default, but now that has changed, so wouldn't it be the
> right time to reconsider the ``makes configure slower'' argument?

Yes, that sounds right.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-05 04:13, Po Lu wrote:

> I'm not aware of a serious security bug in Google GCC or Clang which
> affects compiled code.

That wasn't the sort of security bug I was worried about. By not 
supporting these obsolete NDKs, Google is saying that apps built for 
older Android versions are unreliable or insecure or whatever. We can't 
expect users to fix those old Android bugs, and we likely lack resources 
to modify Emacs to work around them.


> Either way, if we go by what the Google does (or does not) support, then
> in one or two years we will no longer be able to support the latest
> version of Replicant.

Is this referring to Replicant 6? Though I know little about Replicant, 
I suspect it'll be better for everyone concerned if Replicant 11 is the 
porting target, as that's where active Replicant development is (even 
though no official version has been released). Replicant 6 is pretty old 
and has known and seemingly unfixable security issues.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2023-03-05 04:13, Po Lu wrote:
>
>> I'm not aware of a serious security bug in Google GCC or Clang which
>> affects compiled code.
>
> That wasn't the sort of security bug I was worried about. By not
> supporting these obsolete NDKs, Google is saying that apps built for
> older Android versions are unreliable or insecure or whatever. We
> can't expect users to fix those old Android bugs, and we likely lack
> resources to modify Emacs to work around them.

Security problems shouldn't be specific to the version of the NDK you
build with, since all symbols are in /system/lib/libc.so.  Android
doesn't do symbol versioning AFAIK.

> Is this referring to Replicant 6? Though I know little about
> Replicant, I suspect it'll be better for everyone concerned if
> Replicant 11 is the porting target, as that's where active Replicant
> development is (even though no official version has been
> released). Replicant 6 is pretty old and has known and seemingly
> unfixable security issues.

I'd rather we stop arguing about this, but please keep in mind that my
stance on Android version support will not change, since I see people
using old, unsupported versions every day.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

>
> Sorry, this is an area I know little about. (But I'll pontificate
> anyway. :-)
>
> Perhaps the idea was that if you wanted modules written in C++, you
> could compile Emacs with -fexceptions, even though this is not the
> default build procedure. If so, I wouldn't worry too much about that,
> as I don't think anybody does that.

I'm not sure of the point of this since Emacs cannot survive a C++
exception anyway.  Perhaps someone else will enlighten us.

Re: Merging feature/android

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > > We have no responsibility to support any particular compiler other
  > > than GCC.  We can do so when we wish.

  > That's not what the GNU Coding Standards say:

That part of the Coding Standards is about a different question, more
specific.  Namely, about whether to use GNU extensions (to C, for
instance) in the code of GNU packages.

My previous message addressed a related but different question:
whether to make it a goal to support compiling Emacs with a compiler
other than GCC.

These two questions are in the same area, indeed they overlap,
However, they are not the same question.  There is no conflict between
what I wrote recently about one, and what the Coding Standards say
about the other.

 
-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Merging feature/android

[Richard Stallman]

[[[ To any NSA and FBI agents reading my email: please consider    ]]]
[[[ whether defending the US Constitution against all enemies,     ]]]
[[[ foreign or domestic, requires you to follow Snowden's example. ]]]

  > That wasn't the sort of security bug I was worried about. By not 
  > supporting these obsolete NDKs, Google is saying that apps built for 
  > older Android versions are unreliable or insecure or whatever. We can't 
  > expect users to fix those old Android bugs, and we likely lack resources 
  > to modify Emacs to work around them.

Those statements are true, interpreted strictly.  We can't "expect
users to fix" the bugs that manifest on old versions of Android, and
we can't fix them ourselves,  And we should not feel obliged to try.

But our users often _do_ fix bugs that involve old versions of systes
that they continue to use.  Although we do not have an obligation to
help develop those fixes, it would be a gratuitous unkindness to slam
the door on accepting those fixes.

-- 
Dr Richard Stallman (https://stallman.org)
Chief GNUisance of the GNU Project (https://gnu.org)
Founder, Free Software Foundation (https://fsf.org)
Internet Hall-of-Famer (https://internethalloffame.org)

Re: Merging feature/android

[Po Lu]

Richard Stallman <rms@gnu.org> writes:

> [[[ To any NSA and FBI agents reading my email: please consider    ]]]
> [[[ whether defending the US Constitution against all enemies,     ]]]
> [[[ foreign or domestic, requires you to follow Snowden's example. ]]]
>
>   > > We have no responsibility to support any particular compiler other
>   > > than GCC.  We can do so when we wish.
>
>   > That's not what the GNU Coding Standards say:
>
> That part of the Coding Standards is about a different question, more
> specific.  Namely, about whether to use GNU extensions (to C, for
> instance) in the code of GNU packages.
>
> My previous message addressed a related but different question:
> whether to make it a goal to support compiling Emacs with a compiler
> other than GCC.
>
> These two questions are in the same area, indeed they overlap,
> However, they are not the same question.  There is no conflict between
> what I wrote recently about one, and what the Coding Standards say
> about the other.

The problem we are discussing is an extension being used by
emacs-module.c, not whether or not we should support compilers other
than GCC.

Re: Merging feature/android

[Arsen Arsenović]

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


Po Lu <luangruo@yahoo.com> writes:

> Paul Eggert <eggert@cs.ucla.edu> writes:
>
>>
>> Sorry, this is an area I know little about. (But I'll pontificate
>> anyway. :-)
>>
>> Perhaps the idea was that if you wanted modules written in C++, you
>> could compile Emacs with -fexceptions, even though this is not the
>> default build procedure. If so, I wouldn't worry too much about that,
>> as I don't think anybody does that.
>
> I'm not sure of the point of this since Emacs cannot survive a C++
> exception anyway.  Perhaps someone else will enlighten us.

-fexceptions is useful if it is possible to get a call stack which
invokes C++ through a C function through a C++ function, for instance,
if the comparator of a qsort could throw.  This provides the user of a
given C API to gracefully handle thrown errors.  Alternatively, it also
allows the C API developers to (mostly) gracefully clean up when a C++
exception remains uncaught.  This is why libc is compiled with
-fexceptions for instance.

__attribute__((cleanup)) will run cleanups during stack unwinding:

‘cleanup (CLEANUP_FUNCTION)’
     The ‘cleanup’ attribute runs a function when the variable goes out
     of scope.  This attribute can only be applied to auto function
     scope variables; it may not be applied to parameters or variables
     with static storage duration.  The function must take one
     parameter, a pointer to a type compatible with the variable.  The
     return value of the function (if any) is ignored.

     If ‘-fexceptions’ is enabled, then CLEANUP_FUNCTION is run during
     the stack unwinding that happens during the processing of the
     exception.  Note that the ‘cleanup’ attribute does not allow the
     exception to be caught, only to perform an action.  It is undefined
     what happens if CLEANUP_FUNCTION does not return normally.

IMO, the price of exceptions is overblown, so it could be worthwhile
adopting this, if I understand the context right.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: Merging feature/android

[Arsen Arsenović]

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


Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2023-03-05 03:25, Po Lu wrote:
>> I'd like to see the discussion where it was concluded that the
>> cleanup attribute is absolutely required.  It doesn't do fancy things
>> like clean up after Lisp signals under the hood, so all of the cleanup
>> can be done in portable C as well.
>
> As I recall there wasn't much discussion, unfortunately. I very vaguely recall
> that there was some worry that Emacs modules would be written in C++, and that
> they would invoke Elisp code, and that this meant it'd be hard to do the
> cleanup in portable C.
>
> One option that was discussed was to require a C++ compiler to compile
> emacs-modules.cc (i.e., to write the emacs-modules interface in C++). I expect
> this would have addressed the cleanup issue in a different way. But RMS was
> very strongly against requiring a C++ compiler.

Depending on how long ago that was, it might be worth reconsidering.
GCC has been C++ for a decade now, for instance, so C++ compilers are
likely fairly widespread.  One could make the case that using portable
C++ in addition to portable C is a more portable way to do cleanups than
GNU C cleanups.

If still undesirable, I strongly suggest at least using GNU C cleanups.
They are a decent workaround.

> You may find this patch useful:
>
> https://lists.gnu.org/archive/html/emacs-devel/2019-04/msg01075.html


-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: Merging feature/android

[Po Lu]

Arsen Arsenović <arsen@aarsen.me> writes:

> Depending on how long ago that was, it might be worth reconsidering.
> GCC has been C++ for a decade now, for instance, so C++ compilers are
> likely fairly widespread.  One could make the case that using portable
> C++ in addition to portable C is a more portable way to do cleanups than
> GNU C cleanups.
>
> If still undesirable, I strongly suggest at least using GNU C cleanups.
> They are a decent workaround.

GNU C cleanups only work when compiling with -fexceptions.

C++ is an ugly and bloated language, and not as widely available as you
think.  To use it in Emacs for something as important as modules would
be a shame.

Either way, we don't expect to survive a C++ exception, so why cater
specifically to C++'s stack unwinding?

Re: Merging feature/android

[Po Lu]

Arsen Arsenović <arsen@aarsen.me> writes:

> -fexceptions is useful if it is possible to get a call stack which
> invokes C++ through a C function through a C++ function, for instance,
> if the comparator of a qsort could throw.  This provides the user of a
> given C API to gracefully handle thrown errors.  Alternatively, it also
> allows the C API developers to (mostly) gracefully clean up when a C++
> exception remains uncaught.  This is why libc is compiled with
> -fexceptions for instance.
>
> __attribute__((cleanup)) will run cleanups during stack unwinding:

Only if you build Emacs with -fexceptions.

> ‘cleanup (CLEANUP_FUNCTION)’
>      The ‘cleanup’ attribute runs a function when the variable goes out
>      of scope.  This attribute can only be applied to auto function
>      scope variables; it may not be applied to parameters or variables
>      with static storage duration.  The function must take one
>      parameter, a pointer to a type compatible with the variable.  The
>      return value of the function (if any) is ignored.
>
>      If ‘-fexceptions’ is enabled, then CLEANUP_FUNCTION is run during
>      the stack unwinding that happens during the processing of the
>      exception.  Note that the ‘cleanup’ attribute does not allow the
>      exception to be caught, only to perform an action.  It is undefined
>      what happens if CLEANUP_FUNCTION does not return normally.
>
> IMO, the price of exceptions is overblown, so it could be worthwhile
> adopting this, if I understand the context right.

If a C++ exception is thrown inside a module function, then we should
IMO just let everything blow up.  How are they different from any other
kind of unexpected error in a dynamic module, such as for instance an
arithmetic trap, memory access error, or perhaps a Pascal exception?

Re: Merging feature/android

[Arsen Arsenović]

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


Po Lu <luangruo@yahoo.com> writes:

> Arsen Arsenović <arsen@aarsen.me> writes:
>
>> Depending on how long ago that was, it might be worth reconsidering.
>> GCC has been C++ for a decade now, for instance, so C++ compilers are
>> likely fairly widespread.  One could make the case that using portable
>> C++ in addition to portable C is a more portable way to do cleanups than
>> GNU C cleanups.
>>
>> If still undesirable, I strongly suggest at least using GNU C cleanups.
>> They are a decent workaround.
>
> GNU C cleanups only work when compiling with -fexceptions.

This is not the case.  They work under all conditions, and are used to
great effect in many C codebases.  As an example, the following:

  int
  f (void (*foo) (void))
  {
    __attribute__ ((cleanup (test))) int* n = g ();
    foo ();
  }

... generates:

  f:
        pushq   %rbx
        xorl    %eax, %eax
        movq    %rdi, %rbx
        subq    $16, %rsp
        call    g
        movq    %rax, 8(%rsp)
        call    *%rbx
        leaq    8(%rsp), %rdi
        call    test             /* cleanup */
        addq    $16, %rsp
        popq    %rbx
        ret

... with -O3 -fno-exceptions.  The latter is the default for C, but I
wanted to be explicit anyway.

> C++ is an ugly and bloated language, and not as widely available as you
> think.  To use it in Emacs for something as important as modules would
> be a shame.

It is likely as widely available as cleanup attributes.  Certainly as
widely as any supported (or many unsupported) versions of GCC are.

I won't argue on the merits of the language, as that usually doesn't go
anywhere, and the cleanup attribute seems like an accepted solution
anyway.  Note that it's no coincidence that it is being adopted by some
GNU projects.

> Either way, we don't expect to survive a C++ exception, so why cater
> specifically to C++'s stack unwinding?

I agree that it isn't necessary.  I was just answering the question of
what '-fexceptions' would do as someone with the know-how, because it
was brought up.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: Merging feature/android

[Arsen Arsenović]

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


Po Lu <luangruo@yahoo.com> writes:

> Arsen Arsenović <arsen@aarsen.me> writes:
>
>> -fexceptions is useful if it is possible to get a call stack which
>> invokes C++ through a C function through a C++ function, for instance,
>> if the comparator of a qsort could throw.  This provides the user of a
>> given C API to gracefully handle thrown errors.  Alternatively, it also
>> allows the C API developers to (mostly) gracefully clean up when a C++
>> exception remains uncaught.  This is why libc is compiled with
>> -fexceptions for instance.
>>
>> __attribute__((cleanup)) will run cleanups during stack unwinding:
>
> Only if you build Emacs with -fexceptions.
>
>> ‘cleanup (CLEANUP_FUNCTION)’
>>      The ‘cleanup’ attribute runs a function when the variable goes out
>>      of scope.  This attribute can only be applied to auto function
>>      scope variables; it may not be applied to parameters or variables
>>      with static storage duration.  The function must take one
>>      parameter, a pointer to a type compatible with the variable.  The
>>      return value of the function (if any) is ignored.
>>
>>      If ‘-fexceptions’ is enabled, then CLEANUP_FUNCTION is run during
>>      the stack unwinding that happens during the processing of the
>>      exception.  Note that the ‘cleanup’ attribute does not allow the
>>      exception to be caught, only to perform an action.  It is undefined
>>      what happens if CLEANUP_FUNCTION does not return normally.
>>
>> IMO, the price of exceptions is overblown, so it could be worthwhile
>> adopting this, if I understand the context right.
>
> If a C++ exception is thrown inside a module function, then we should
> IMO just let everything blow up.  How are they different from any other
> kind of unexpected error in a dynamic module, such as for instance an
> arithmetic trap, memory access error, or perhaps a Pascal exception?

I agree that it is unnecessary to support them.  The context that I took
to understand is specifically wanting to support C++ modules.  If that
is not the case, then -fexceptions is not worth enabling, indeed.
-- 
Arsen Arsenović

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 381 bytes --]

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Paul Eggert <eggert@cs.ucla.edu>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org
> Date: Mon, 06 Mar 2023 18:39:59 +0800
> 
> Arsen Arsenović <arsen@aarsen.me> writes:
> 
> If a C++ exception is thrown inside a module function, then we should
> IMO just let everything blow up.

But we don't want to, and we have decided long ago to use this
mechanism.

So please stop arguing for its removal, because I'm not going to agree
to it.

Once again, let's please make this work like configure does with image
libraries, i.e. error out by default if modules aren't supported, and
suggest using ifavailable.  This will solve the problem cleanly and in
a way that cannot be missed by chance.

Re: Merging feature/android

[Po Lu]

Arsen Arsenović <arsen@aarsen.me> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Arsen Arsenović <arsen@aarsen.me> writes:
>>
>>> Depending on how long ago that was, it might be worth reconsidering.
>>> GCC has been C++ for a decade now, for instance, so C++ compilers are
>>> likely fairly widespread.  One could make the case that using portable
>>> C++ in addition to portable C is a more portable way to do cleanups than
>>> GNU C cleanups.
>>>
>>> If still undesirable, I strongly suggest at least using GNU C cleanups.
>>> They are a decent workaround.
>>
>> GNU C cleanups only work when compiling with -fexceptions.
>
> This is not the case.  They work under all conditions, and are used to
> great effect in many C codebases.  As an example, the following:
>
>   int
>   f (void (*foo) (void))
>   {
>     __attribute__ ((cleanup (test))) int* n = g ();
>     foo ();
>   }
>
> ... generates:
>
>   f:
>         pushq   %rbx
>         xorl    %eax, %eax
>         movq    %rdi, %rbx
>         subq    $16, %rsp
>         call    g
>         movq    %rax, 8(%rsp)
>         call    *%rbx
>         leaq    8(%rsp), %rdi
>         call    test             /* cleanup */
>         addq    $16, %rsp
>         popq    %rbx
>         ret
>
> ... with -O3 -fno-exceptions.  The latter is the default for C, but I
> wanted to be explicit anyway.

I meant that they only work to handle C++ exceptions when you compile
with -fexceptions, so it is rather pointless to use it as we do now.
Without -fexceptions, that code is no different from:

f (foo)
  int (*foo) ();
{
  int *n;

  n = g ();
  foo ();
  test (fn);
}

> It is likely as widely available as cleanup attributes.  Certainly as
> widely as any supported (or many unsupported) versions of GCC are.
>
> I won't argue on the merits of the language, as that usually doesn't go
> anywhere, and the cleanup attribute seems like an accepted solution
> anyway.  Note that it's no coincidence that it is being adopted by some
> GNU projects.

The GNU Coding Standards recommend against relying on GNU C extensions
in Emacs.

> I agree that it isn't necessary.  I was just answering the question of
> what '-fexceptions' would do as someone with the know-how, because it
> was brought up.

OK, then let's not waste any more time here.  Thanks.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> But we don't want to, and we have decided long ago to use this
> mechanism.

The problem is that, as we are using it now, that mechanism does nothing
different from the more portable equivalents.  It will only handle C++
exceptions if Emacs is built with -fexceptions, and Emacs is not.

> Once again, let's please make this work like configure does with image
> libraries, i.e. error out by default if modules aren't supported, and
> suggest using ifavailable.  This will solve the problem cleanly and in
> a way that cannot be missed by chance.

How about only on GNU/Linux systems, and maybe Mac OS and Windows?

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Mon, 06 Mar 2023 21:12:35 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > But we don't want to, and we have decided long ago to use this
> > mechanism.
> 
> The problem is that, as we are using it now, that mechanism does nothing
> different from the more portable equivalents.  It will only handle C++
> exceptions if Emacs is built with -fexceptions, and Emacs is not.
> 
> > Once again, let's please make this work like configure does with image
> > libraries, i.e. error out by default if modules aren't supported, and
> > suggest using ifavailable.  This will solve the problem cleanly and in
> > a way that cannot be missed by chance.
> 
> How about only on GNU/Linux systems, and maybe Mac OS and Windows?

I don't understand what you propose, in practical terms.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
>> Date: Mon, 06 Mar 2023 21:12:35 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > But we don't want to, and we have decided long ago to use this
>> > mechanism.
>> 
>> The problem is that, as we are using it now, that mechanism does nothing
>> different from the more portable equivalents.  It will only handle C++
>> exceptions if Emacs is built with -fexceptions, and Emacs is not.
>> 
>> > Once again, let's please make this work like configure does with image
>> > libraries, i.e. error out by default if modules aren't supported, and
>> > suggest using ifavailable.  This will solve the problem cleanly and in
>> > a way that cannot be missed by chance.
>> 
>> How about only on GNU/Linux systems, and maybe Mac OS and Windows?
>
> I don't understand what you propose, in practical terms.

Basically, to only default to the error when opsys is set to systems
which use GCC.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Tue, 07 Mar 2023 08:36:16 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> > Once again, let's please make this work like configure does with image
> >> > libraries, i.e. error out by default if modules aren't supported, and
> >> > suggest using ifavailable.  This will solve the problem cleanly and in
> >> > a way that cannot be missed by chance.
> >> 
> >> How about only on GNU/Linux systems, and maybe Mac OS and Windows?
> >
> > I don't understand what you propose, in practical terms.
> 
> Basically, to only default to the error when opsys is set to systems
> which use GCC.

Why the distinction? what bad things will happen if we do this for any
compiler?

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
>> Date: Tue, 07 Mar 2023 08:36:16 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> > Once again, let's please make this work like configure does with image
>> >> > libraries, i.e. error out by default if modules aren't supported, and
>> >> > suggest using ifavailable.  This will solve the problem cleanly and in
>> >> > a way that cannot be missed by chance.
>> >> 
>> >> How about only on GNU/Linux systems, and maybe Mac OS and Windows?
>> >
>> > I don't understand what you propose, in practical terms.
>> 
>> Basically, to only default to the error when opsys is set to systems
>> which use GCC.
>
> Why the distinction? what bad things will happen if we do this for any
> compiler?

Emacs will not install by just running:

      ./configure
      make
      su
      make install

from the distribution.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> Date: Tue, 07 Mar 2023 21:03:06 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
> >> Date: Tue, 07 Mar 2023 08:36:16 +0800
> >> 
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >> 
> >> >> > Once again, let's please make this work like configure does with image
> >> >> > libraries, i.e. error out by default if modules aren't supported, and
> >> >> > suggest using ifavailable.  This will solve the problem cleanly and in
> >> >> > a way that cannot be missed by chance.
> >> >> 
> >> >> How about only on GNU/Linux systems, and maybe Mac OS and Windows?
> >> >
> >> > I don't understand what you propose, in practical terms.
> >> 
> >> Basically, to only default to the error when opsys is set to systems
> >> which use GCC.
> >
> > Why the distinction? what bad things will happen if we do this for any
> > compiler?
> 
> Emacs will not install by just running:
> 
>       ./configure
>       make
>       su
>       make install
> 
> from the distribution.

We are going in circles: as I already said before, Emacs will also not
install by just running the above if, say, the XPM library is not
available.

Re: Merging feature/android

[Po Lu]

XPM is actually a good example of what I'm talking about: it is used by default only if the system in question typically has libXpm preinstalled.  Otherwise, the built-in XPM code in image.c is used instead.  This happens on PGTK, Android and Mac OS.

On March 7, 2023 9:36:14 PM GMT+08:00, Eli Zaretskii <eliz@gnu.org> wrote:
>> From: Po Lu <luangruo@yahoo.com>
>> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
>> Date: Tue, 07 Mar 2023 21:03:06 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> >> From: Po Lu <luangruo@yahoo.com>
>> >> Cc: arsen@aarsen.me,  eggert@cs.ucla.edu,  emacs-devel@gnu.org
>> >> Date: Tue, 07 Mar 2023 08:36:16 +0800
>> >> 
>> >> Eli Zaretskii <eliz@gnu.org> writes:
>> >> 
>> >> >> > Once again, let's please make this work like configure does with image
>> >> >> > libraries, i.e. error out by default if modules aren't supported, and
>> >> >> > suggest using ifavailable.  This will solve the problem cleanly and in
>> >> >> > a way that cannot be missed by chance.
>> >> >> 
>> >> >> How about only on GNU/Linux systems, and maybe Mac OS and Windows?
>> >> >
>> >> > I don't understand what you propose, in practical terms.
>> >> 
>> >> Basically, to only default to the error when opsys is set to systems
>> >> which use GCC.
>> >
>> > Why the distinction? what bad things will happen if we do this for any
>> > compiler?
>> 
>> Emacs will not install by just running:
>> 
>>       ./configure
>>       make
>>       su
>>       make install
>> 
>> from the distribution.
>
>We are going in circles: as I already said before, Emacs will also not
>install by just running the above if, say, the XPM library is not
>available.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-07 15:55, Po Lu wrote:
> XPM is actually a good example of what I'm talking about: it is used by default only if the system in question typically has libXpm preinstalled.

That's not true for JPEG, PNG, GIF, TIFF, GnuTLS, and probably other 
libraries.

I routinely run into the sort of messages that Eli is talking about if I 
run plain 'configure' on some platforms. Solaris 10, for example, 
typically doesn't have a JPEG library installed. It's not that big a 
deal to read the error message at the end of the failed 'configure', 
sigh, and re-run configure with the --with-jpeg=ifavailable option that 
'configure' suggests.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> That's not true for JPEG, PNG, GIF, TIFF, GnuTLS, and probably other
> libraries.

I can't find any other examples.  We even make things such as ACL
support and SELinux support `ifavailable', even though they add support
for operating system features, without which functions such as copy-file
do not exactly work correctly; without libselinux, for instance, file
contexts will not be preserved during copies.

> I routinely run into the sort of messages that Eli is talking about if
> I run plain 'configure' on some platforms. Solaris 10, for example,
> typically doesn't have a JPEG library installed. It's not that big a
> deal to read the error message at the end of the failed 'configure',
> sigh, and re-run configure with the --with-jpeg=ifavailable option
> that 'configure' suggests.

We're not talking about image libraries here, but something extremely
basic, be it window system support (libXpm) or a C compiler.

Anyway, since this support is going to be made `ifavailable' by default
for Android, what makes other systems which don't use GCC any different?

On newer versions of Android, modules don't work anyway because security
policy prohibits calling dlopen or exec on files in application writable
directories, but that is besides the point.

Re: Merging feature/android

[Paul Eggert]

On 2023-03-07 22:58, Po Lu wrote:

> We're not talking about image libraries here, but something extremely
> basic, be it window system support (libXpm) or a C compiler.

I don't know what heuristic was used to decide that GnuTLS and some 
image libraries are considered important enough to nag the Emacs builder 
about, whereas libraries like libXpm are considered less important. 
Perhaps Eli or Stefan can comment on what the heuristic is.


> Anyway, since this support is going to be made `ifavailable' by default
> for Android, what makes other systems which don't use GCC any different?
> 
> On newer versions of Android, modules don't work anyway because security
> policy prohibits calling dlopen or exec on files in application writable
> directories, but that is besides the point.

I agree with the suggestion of making modules ifavailable by default on 
all platforms, as this is more consistent, and it better reflects the 
fact that anyway Emacs must work without any modules installed.

Re: Merging feature/android

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> I don't know what heuristic was used to decide that GnuTLS and some
> image libraries are considered important enough to nag the Emacs
> builder about, whereas libraries like libXpm are considered less
> important. Perhaps Eli or Stefan can comment on what the heuristic is.

Perhaps that the image libraries and GnuTLS are easy to build, install,
and maintain on any system, and that does not apply to GCC.

For example, on one Solaris system, I counted no less than three
out-of-date installations of GCC scattered around /opt/sfw and some
other directories, none of which are currently working.

`cc' (which is Sun C 5.8) works if you give `--without-modules'.

Re: Merging feature/android

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> `cc' (which is Sun C 5.8) works if you give `--without-modules'.

Sun C 5.10, sorry.  I looked at the wrong `cc', not the one used to
build Emacs.

Re: Merging feature/android

[Eli Zaretskii]

> Date: Tue, 7 Mar 2023 21:38:39 -0800
> Cc: arsen@aarsen.me, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 2023-03-07 15:55, Po Lu wrote:
> > XPM is actually a good example of what I'm talking about: it is used by default only if the system in question typically has libXpm preinstalled.
> 
> That's not true for JPEG, PNG, GIF, TIFF, GnuTLS, and probably other 
> libraries.
> 
> I routinely run into the sort of messages that Eli is talking about if I 
> run plain 'configure' on some platforms. Solaris 10, for example, 
> typically doesn't have a JPEG library installed. It's not that big a 
> deal to read the error message at the end of the failed 'configure', 
> sigh, and re-run configure with the --with-jpeg=ifavailable option that 
> 'configure' suggests.

100% agreement.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>,  arsen@aarsen.me,  emacs-devel@gnu.org
> Date: Wed, 08 Mar 2023 14:58:35 +0800
> 
> Anyway, since this support is going to be made `ifavailable' by default
> for Android

Only because you keep insisting that it's needed.  My preference is to
leave it "default ON" on all platforms.  I see no reason why having
Android users see the error and re-run configure with ifavailable (if
needed) would be such a catastrophe.  We _want_ Emacs to be capable of
loading modules, and we want the user to be aware if the build cannot
do that, so that the user could take the remedial action if possible.

> On newer versions of Android, modules don't work anyway because security
> policy prohibits calling dlopen or exec on files in application writable
> directories, but that is besides the point.

The users who build Emacs on Android aren't necessarily Android
experts.  They could be Emacs experts who just happen to use Android.
Why not make sure they are acutely aware of the issue?

(I don't believe we are still arguing about this nit.  Why on Earth?)

Re: Merging feature/android

[Eli Zaretskii]

> Date: Tue, 7 Mar 2023 23:07:01 -0800
> Cc: Eli Zaretskii <eliz@gnu.org>, arsen@aarsen.me, emacs-devel@gnu.org
> From: Paul Eggert <eggert@cs.ucla.edu>
> 
> On 2023-03-07 22:58, Po Lu wrote:
> 
> > We're not talking about image libraries here, but something extremely
> > basic, be it window system support (libXpm) or a C compiler.
> 
> I don't know what heuristic was used to decide that GnuTLS and some 
> image libraries are considered important enough to nag the Emacs builder 
> about, whereas libraries like libXpm are considered less important. 
> Perhaps Eli or Stefan can comment on what the heuristic is.

GnuTLS is nowadays a must for any networking, that's why.

> I agree with the suggestion of making modules ifavailable by default on 
> all platforms, as this is more consistent, and it better reflects the 
> fact that anyway Emacs must work without any modules installed.

I disagree.  We decided to make modules the default for a reason.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Only because you keep insisting that it's needed.  My preference is to
> leave it "default ON" on all platforms.  I see no reason why having
> Android users see the error and re-run configure with ifavailable (if
> needed) would be such a catastrophe.  We _want_ Emacs to be capable of
> loading modules, and we want the user to be aware if the build cannot
> do that, so that the user could take the remedial action if possible.

Because (on many systems, not just on Android) a working version of GCC
is not a small dependency required for networking, or some
easy-to-install image library, but a massive piece of software that is
difficult to install and keep working.  Emacs currently also doesn't
work on systems where the linker refuses to link with `dlopen' by
default:

  % ./configure CC='gcc445'
  ...
  checking for dlopen... no
  configure: error: Dynamic modules are not supported on your system

and requires tweaking:

  % ./configure CC='gcc445' LDFLAGS='-Wl,-insecure -lc'

dynamic modules, being a niche feature (count the number of packages on
ELPA and NonGNU that provide them), do not deserve such special
treatment, so configure should not insist that they be available, at
least on any system where GCC is not typically available.

How is this change any different from us bumping the minimum required
versions of GTK+ or ImageMagick in the past?  From the POV of users
running the older versions, GTK+ or ImageMagick support silently
disappeared without warning.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: eggert@cs.ucla.edu,  arsen@aarsen.me,  emacs-devel@gnu.org
> Date: Thu, 09 Mar 2023 09:12:56 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Only because you keep insisting that it's needed.  My preference is to
> > leave it "default ON" on all platforms.  I see no reason why having
> > Android users see the error and re-run configure with ifavailable (if
> > needed) would be such a catastrophe.  We _want_ Emacs to be capable of
> > loading modules, and we want the user to be aware if the build cannot
> > do that, so that the user could take the remedial action if possible.
> 
> Because (on many systems, not just on Android) a working version of GCC
> is not a small dependency required for networking, or some
> easy-to-install image library, but a massive piece of software that is
> difficult to install and keep working.  Emacs currently also doesn't
> work on systems where the linker refuses to link with `dlopen' by
> default:
> 
>   % ./configure CC='gcc445'
>   ...
>   checking for dlopen... no
>   configure: error: Dynamic modules are not supported on your system
> 
> and requires tweaking:
> 
>   % ./configure CC='gcc445' LDFLAGS='-Wl,-insecure -lc'
> 
> dynamic modules, being a niche feature (count the number of packages on
> ELPA and NonGNU that provide them), do not deserve such special
> treatment, so configure should not insist that they be available, at
> least on any system where GCC is not typically available.

I disagree with your conclusion, and I still think we should complain
noisily if modules cannot be supported.

I wish you stopped arguing and just did what I asked long ago, even if
you disagree.  This minor issue is not worth all the energy and
bandwidth we wasted already, let alone what we are going to waste if
the argument keeps going on.

Please.

> How is this change any different from us bumping the minimum required
> versions of GTK+ or ImageMagick in the past?

I explained how it's different, more then once: support for modules is
very important for Emacs, so omitting is silently is not TRT.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> I disagree with your conclusion, and I still think we should complain
> noisily if modules cannot be supported.

OK, but I still think this is a very bad idea.  Do you object to
rewriting emacs-module.c to not utilize __attribute__ ((cleanup))?

What was said earlier by some people is wrong: as-is, in Emacs, it makes
no difference whether or not we write:

   foo __attribute__ ((cleanup (module_reset_handlerlist)))...

or:

   foo...
   module_reset_handlerlist (foo);

Not on Emacs 29, of course.  We could even go back to using
__attribute__ ((cleanup)) if it is available, and use the portable
approach otherwise.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: eggert@cs.ucla.edu,  arsen@aarsen.me,  emacs-devel@gnu.org
> Date: Thu, 09 Mar 2023 16:07:04 +0800
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > I disagree with your conclusion, and I still think we should complain
> > noisily if modules cannot be supported.
> 
> OK, but I still think this is a very bad idea.  Do you object to
> rewriting emacs-module.c to not utilize __attribute__ ((cleanup))?
> 
> What was said earlier by some people is wrong: as-is, in Emacs, it makes
> no difference whether or not we write:
> 
>    foo __attribute__ ((cleanup (module_reset_handlerlist)))...
> 
> or:
> 
>    foo...
>    module_reset_handlerlist (foo);

If you want to discuss this, please start a new thread and CC Daniel
Colascione and Philipp Stephani, who I believe wrote that part of
emacs-module.c.  I don't think we should change that without a
thorough discussion, as that code is quite old and I don't think it
caused any trouble whatsoever.

__attribute__ ((cleanup)) and emacs-module.c

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Po Lu <luangruo@yahoo.com>
>> Cc: eggert@cs.ucla.edu,  arsen@aarsen.me,  emacs-devel@gnu.org
>> Date: Thu, 09 Mar 2023 16:07:04 +0800
>> 
>> Eli Zaretskii <eliz@gnu.org> writes:
>> 
>> > I disagree with your conclusion, and I still think we should complain
>> > noisily if modules cannot be supported.
>> 
>> OK, but I still think this is a very bad idea.  Do you object to
>> rewriting emacs-module.c to not utilize __attribute__ ((cleanup))?
>> 
>> What was said earlier by some people is wrong: as-is, in Emacs, it makes
>> no difference whether or not we write:
>> 
>>    foo __attribute__ ((cleanup (module_reset_handlerlist)))...
>> 
>> or:
>> 
>>    foo...
>>    module_reset_handlerlist (foo);
>
> If you want to discuss this, please start a new thread and CC Daniel
> Colascione and Philipp Stephani, who I believe wrote that part of
> emacs-module.c.  I don't think we should change that without a
> thorough discussion, as that code is quite old and I don't think it
> caused any trouble whatsoever.

Ok.

Phillip, Daniel, I'd like to replace the use of the `cleanup' attribute
in emacs-module.c with portable C code.  The reason is that it is
implemented unsatisfactorily or not at all in certain compilers I want
to use.  This includes Sun C 5.10, and various versions of the Android
NDK GCC that crash generating debuginfo.

Since Emacs is not built with C++ exception handling, there should be no
reason to use that attribute.  Any objections?

Thanks in advance.

Re: __attribute__ ((cleanup)) and emacs-module.c

[Philipp Stephani]

On Thu, 9 Mar 2023 at 11:20, Po Lu <luangruo@yahoo.com> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> >> From: Po Lu <luangruo@yahoo.com>
> >> Cc: eggert@cs.ucla.edu,  arsen@aarsen.me,  emacs-devel@gnu.org
> >> Date: Thu, 09 Mar 2023 16:07:04 +0800
> >>
> >> Eli Zaretskii <eliz@gnu.org> writes:
> >>
> >> > I disagree with your conclusion, and I still think we should complain
> >> > noisily if modules cannot be supported.
> >>
> >> OK, but I still think this is a very bad idea.  Do you object to
> >> rewriting emacs-module.c to not utilize __attribute__ ((cleanup))?
> >>
> >> What was said earlier by some people is wrong: as-is, in Emacs, it makes
> >> no difference whether or not we write:
> >>
> >>    foo __attribute__ ((cleanup (module_reset_handlerlist)))...
> >>
> >> or:
> >>
> >>    foo...
> >>    module_reset_handlerlist (foo);
> >
> > If you want to discuss this, please start a new thread and CC Daniel
> > Colascione and Philipp Stephani, who I believe wrote that part of
> > emacs-module.c.  I don't think we should change that without a
> > thorough discussion, as that code is quite old and I don't think it
> > caused any trouble whatsoever.
>
> Ok.
>
> Phillip, Daniel, I'd like to replace the use of the `cleanup' attribute
> in emacs-module.c with portable C code.  The reason is that it is
> implemented unsatisfactorily or not at all in certain compilers I want
> to use.  This includes Sun C 5.10, and various versions of the Android
> NDK GCC that crash generating debuginfo.
>
> Since Emacs is not built with C++ exception handling, there should be no
> reason to use that attribute.  Any objections?

This attribute is necessary at the moment because otherwise the
handlerlist wouldn't get reset when returning from the module
implementation functions. Without it, you'd need to audit every
occurrence of "return" in the code (including the code generated by
the helper macros) and reset the handlerlist manually. This is
unrelated to C++ exception handling. It's not impossible to do this
manually (it's not so different from the unbind_to calls in the Emacs
codebase), but rather error-prone.

Re: __attribute__ ((cleanup)) and emacs-module.c

[Po Lu]

Philipp Stephani <phst@google.com> writes:

> This attribute is necessary at the moment because otherwise the
> handlerlist wouldn't get reset when returning from the module
> implementation functions. Without it, you'd need to audit every
> occurrence of "return" in the code (including the code generated by
> the helper macros) and reset the handlerlist manually. This is
> unrelated to C++ exception handling. It's not impossible to do this
> manually (it's not so different from the unbind_to calls in the Emacs
> codebase), but rather error-prone.

Yes, I know why it's necessary.  I'm up to doing what what it does by
hand in portable C.

Unmatched unbind_to's have never been a significant problem for us, so
this shouldn't either be.  Daniel, any comments?

Thanks.

Re: __attribute__ ((cleanup)) and emacs-module.c

[Paul Eggert]

On 2023-03-09 05:31, Po Lu wrote:
> Philipp Stephani<phst@google.com>  writes:
> 
>> It's not impossible to do this
>> manually (it's not so different from the unbind_to calls in the Emacs
>> codebase), but rather error-prone.
> Yes, I know why it's necessary.  I'm up to doing what what it does by
> hand in portable C.

Thanks for volunteering to take on this portability task.

Re: __attribute__ ((cleanup)) and emacs-module.c

[Po Lu via Emacs development discussions.]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2023-03-09 05:31, Po Lu wrote:
>> Philipp Stephani<phst@google.com>  writes:
>> 
>>> It's not impossible to do this
>>> manually (it's not so different from the unbind_to calls in the Emacs
>>> codebase), but rather error-prone.
>> Yes, I know why it's necessary.  I'm up to doing what what it does by
>> hand in portable C.
>
> Thanks for volunteering to take on this portability task.

Would people please review this change?  I have not yet had a chance to
test it, but if it is good I will install it to feature/android.

Thanks.

diff --git a/src/emacs-module.c b/src/emacs-module.c
index d9e564771d0..4719b15c992 100644
--- a/src/emacs-module.c
+++ b/src/emacs-module.c
@@ -253,10 +253,8 @@ module_decode_utf_8 (const char *str, ptrdiff_t len)
 
 /* It is very important that pushing the handler doesn't itself raise
    a signal.  Install the cleanup only after the handler has been
-   pushed.  Use __attribute__ ((cleanup)) to avoid
-   non-local-exit-prone manual cleanup.  This is an extension provided
-   by GCC and similar compilers; configure prevents module.c from
-   being compiled when it is not present.
+   pushed.  All code following this point should use
+   MODULE_INTERNAL_CLEANUP before each return.
 
    The do-while forces uses of the macro to be followed by a semicolon.
    This macro cannot enclose its entire body inside a do-while, as the
@@ -276,17 +274,20 @@ #define MODULE_HANDLE_NONLOCAL_EXIT(retval)                             \
       return retval;							\
     }									\
   struct handler *internal_cleanup                                      \
-    __attribute__ ((cleanup (module_reset_handlerlist)))                \
     = internal_handler;                                                 \
   if (sys_setjmp (internal_cleanup->jmp))                               \
     {									\
       module_handle_nonlocal_exit (env,                                 \
                                    internal_cleanup->nonlocal_exit,     \
                                    internal_cleanup->val);              \
+      module_reset_handlerlist (&internal_cleanup);			\
       return retval;							\
     }									\
   do { } while (false)
 
+#define MODULE_INTERNAL_CLEANUP			\
+  module_reset_handlerlist (&internal_cleanup)
+
 \f
 /* Implementation of runtime and environment functions.
 
@@ -313,7 +314,10 @@ #define MODULE_HANDLE_NONLOCAL_EXIT(retval)                             \
       Emacs functions, by placing the macro
       MODULE_HANDLE_NONLOCAL_EXIT right after the above 2 tests.
 
-   5. Do NOT use 'eassert' for checking validity of user code in the
+   5. Finally, any code which expands MODULE_HANDLE_NONLOCAL_EXIT
+      should use MODULE_INTERNAL_CLEANUP prior to returning.
+
+   6. Do NOT use 'eassert' for checking validity of user code in the
       module.  Instead, make those checks part of the code, and if the
       check fails, call 'module_non_local_exit_signal_1' or
       'module_non_local_exit_throw_1' to report the error.  This is
@@ -436,6 +440,7 @@ module_make_global_ref (emacs_env *env, emacs_value value)
       bool overflow = INT_ADD_WRAPV (ref->refcount, 1, &ref->refcount);
       if (overflow)
 	overflow_error ();
+      MODULE_INTERNAL_CLEANUP;
       return &ref->value;
     }
   else
@@ -448,6 +453,7 @@ module_make_global_ref (emacs_env *env, emacs_value value)
       Lisp_Object value;
       XSETPSEUDOVECTOR (value, ref, PVEC_OTHER);
       hash_put (h, new_obj, value, hashcode);
+      MODULE_INTERNAL_CLEANUP;
       return &ref->value;
     }
 }
@@ -479,6 +485,8 @@ module_free_global_ref (emacs_env *env, emacs_value global_value)
       if (--ref->refcount == 0)
         hash_remove_from_table (h, obj);
     }
+
+  MODULE_INTERNAL_CLEANUP;
 }
 
 static enum emacs_funcall_exit
@@ -572,6 +580,8 @@ #define XSET_MODULE_FUNCTION(var, ptr) \
 module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
 		      emacs_function func, const char *docstring, void *data)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
 
   if (! (0 <= min_arity
@@ -596,7 +606,9 @@ module_make_function (emacs_env *env, ptrdiff_t min_arity, ptrdiff_t max_arity,
   XSET_MODULE_FUNCTION (result, function);
   eassert (MODULE_FUNCTIONP (result));
 
-  return lisp_to_value (env, result);
+  value = lisp_to_value (env, result);
+  MODULE_INTERNAL_CLEANUP;
+  return value;
 }
 
 static emacs_finalizer
@@ -605,6 +617,7 @@ module_get_function_finalizer (emacs_env *env, emacs_value arg)
   MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_MODULE_FUNCTION (lisp);
+  MODULE_INTERNAL_CLEANUP;
   return XMODULE_FUNCTION (lisp)->finalizer;
 }
 
@@ -616,6 +629,7 @@ module_set_function_finalizer (emacs_env *env, emacs_value arg,
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_MODULE_FUNCTION (lisp);
   XMODULE_FUNCTION (lisp)->finalizer = fin;
+  MODULE_INTERNAL_CLEANUP;
 }
 
 void
@@ -635,6 +649,7 @@ module_make_interactive (emacs_env *env, emacs_value function, emacs_value spec)
   /* Normalize (interactive nil) to (interactive). */
   XMODULE_FUNCTION (lisp_fun)->interactive_form
     = NILP (lisp_spec) ? list1 (Qinteractive) : list2 (Qinteractive, lisp_spec);
+  MODULE_INTERNAL_CLEANUP;
 }
 
 Lisp_Object
@@ -668,21 +683,30 @@ module_funcall (emacs_env *env, emacs_value func, ptrdiff_t nargs,
     newargs[1 + i] = value_to_lisp (args[i]);
   emacs_value result = lisp_to_value (env, Ffuncall (nargs1, newargs));
   SAFE_FREE ();
+  MODULE_INTERNAL_CLEANUP;
   return result;
 }
 
 static emacs_value
 module_intern (emacs_env *env, const char *name)
 {
+  emacs_value tem;
+
   MODULE_FUNCTION_BEGIN (NULL);
-  return lisp_to_value (env, intern (name));
+  tem = lisp_to_value (env, intern (name));
+  MODULE_INTERNAL_CLEANUP;
+  return tem;
 }
 
 static emacs_value
 module_type_of (emacs_env *env, emacs_value arg)
 {
+  emacs_value tem;
+
   MODULE_FUNCTION_BEGIN (NULL);
-  return lisp_to_value (env, Ftype_of (value_to_lisp (arg)));
+  tem = lisp_to_value (env, Ftype_of (value_to_lisp (arg)));
+  MODULE_INTERNAL_CLEANUP;
+  return tem;
 }
 
 static bool
@@ -708,14 +732,20 @@ module_extract_integer (emacs_env *env, emacs_value arg)
   intmax_t i;
   if (! integer_to_intmax (lisp, &i))
     xsignal1 (Qoverflow_error, lisp);
+  MODULE_INTERNAL_CLEANUP;
   return i;
 }
 
 static emacs_value
 module_make_integer (emacs_env *env, intmax_t n)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
-  return lisp_to_value (env, make_int (n));
+  value = lisp_to_value (env, make_int (n));
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 static double
@@ -724,14 +754,21 @@ module_extract_float (emacs_env *env, emacs_value arg)
   MODULE_FUNCTION_BEGIN (0);
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_TYPE (FLOATP (lisp), Qfloatp, lisp);
+  MODULE_INTERNAL_CLEANUP;
+
   return XFLOAT_DATA (lisp);
 }
 
 static emacs_value
 module_make_float (emacs_env *env, double d)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
-  return lisp_to_value (env, make_float (d));
+  value = lisp_to_value (env, make_float (d));
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 static bool
@@ -763,6 +800,7 @@ module_copy_string_contents (emacs_env *env, emacs_value value, char *buf,
   if (buf == NULL)
     {
       *len = required_buf_size;
+      MODULE_INTERNAL_CLEANUP;
       return true;
     }
 
@@ -778,36 +816,51 @@ module_copy_string_contents (emacs_env *env, emacs_value value, char *buf,
   *len = required_buf_size;
   memcpy (buf, SDATA (lisp_str_utf8), raw_size + 1);
 
+  MODULE_INTERNAL_CLEANUP;
   return true;
 }
 
 static emacs_value
 module_make_string (emacs_env *env, const char *str, ptrdiff_t len)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
   if (! (0 <= len && len <= STRING_BYTES_BOUND))
     overflow_error ();
   Lisp_Object lstr
     = len == 0 ? empty_multibyte_string : module_decode_utf_8 (str, len);
-  return lisp_to_value (env, lstr);
+  value = lisp_to_value (env, lstr);
+  MODULE_INTERNAL_CLEANUP;
+  return value;
 }
 
 static emacs_value
 module_make_unibyte_string (emacs_env *env, const char *str, ptrdiff_t length)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
   if (! (0 <= length && length <= STRING_BYTES_BOUND))
     overflow_error ();
   Lisp_Object lstr
     = length == 0 ? empty_unibyte_string : make_unibyte_string (str, length);
-  return lisp_to_value (env, lstr);
+  value = lisp_to_value (env, lstr);
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 static emacs_value
 module_make_user_ptr (emacs_env *env, emacs_finalizer fin, void *ptr)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
-  return lisp_to_value (env, make_user_ptr (fin, ptr));
+  value = lisp_to_value (env, make_user_ptr (fin, ptr));
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 static void *
@@ -816,6 +869,8 @@ module_get_user_ptr (emacs_env *env, emacs_value arg)
   MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_USER_PTR (lisp);
+  MODULE_INTERNAL_CLEANUP;
+
   return XUSER_PTR (lisp)->p;
 }
 
@@ -826,6 +881,7 @@ module_set_user_ptr (emacs_env *env, emacs_value arg, void *ptr)
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_USER_PTR (lisp);
   XUSER_PTR (lisp)->p = ptr;
+  MODULE_INTERNAL_CLEANUP;
 }
 
 static emacs_finalizer
@@ -834,6 +890,7 @@ module_get_user_finalizer (emacs_env *env, emacs_value arg)
   MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_USER_PTR (lisp);
+  MODULE_INTERNAL_CLEANUP;
   return XUSER_PTR (lisp)->finalizer;
 }
 
@@ -845,6 +902,7 @@ module_set_user_finalizer (emacs_env *env, emacs_value arg,
   Lisp_Object lisp = value_to_lisp (arg);
   CHECK_USER_PTR (lisp);
   XUSER_PTR (lisp)->finalizer = fin;
+  MODULE_INTERNAL_CLEANUP;
 }
 
 static void
@@ -864,15 +922,21 @@ module_vec_set (emacs_env *env, emacs_value vector, ptrdiff_t index,
   Lisp_Object lisp = value_to_lisp (vector);
   check_vec_index (lisp, index);
   ASET (lisp, index, value_to_lisp (value));
+  MODULE_INTERNAL_CLEANUP;
 }
 
 static emacs_value
 module_vec_get (emacs_env *env, emacs_value vector, ptrdiff_t index)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
   Lisp_Object lisp = value_to_lisp (vector);
   check_vec_index (lisp, index);
-  return lisp_to_value (env, AREF (lisp, index));
+  value = lisp_to_value (env, AREF (lisp, index));
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 static ptrdiff_t
@@ -881,6 +945,8 @@ module_vec_size (emacs_env *env, emacs_value vector)
   MODULE_FUNCTION_BEGIN (0);
   Lisp_Object lisp = value_to_lisp (vector);
   CHECK_VECTOR (lisp);
+  MODULE_INTERNAL_CLEANUP;
+
   return ASIZE (lisp);
 }
 
@@ -896,23 +962,37 @@ module_should_quit (emacs_env *env)
 static enum emacs_process_input_result
 module_process_input (emacs_env *env)
 {
+  enum emacs_process_input_result rc;
+
   MODULE_FUNCTION_BEGIN (emacs_process_input_quit);
   maybe_quit ();
-  return emacs_process_input_continue;
+  rc = emacs_process_input_continue;
+  MODULE_INTERNAL_CLEANUP;
+  return rc;
 }
 
 static struct timespec
 module_extract_time (emacs_env *env, emacs_value arg)
 {
+  struct timespec value;
+
   MODULE_FUNCTION_BEGIN ((struct timespec) {0});
-  return lisp_time_argument (value_to_lisp (arg));
+  value = lisp_time_argument (value_to_lisp (arg));
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 static emacs_value
 module_make_time (emacs_env *env, struct timespec time)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
-  return lisp_to_value (env, timespec_to_lisp (time));
+  value = lisp_to_value (env, timespec_to_lisp (time));
+  MODULE_INTERNAL_CLEANUP;
+
+  return value;
 }
 
 /*
@@ -989,7 +1069,10 @@ module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
       EMACS_INT x = XFIXNUM (o);
       *sign = (0 < x) - (x < 0);
       if (x == 0 || count == NULL)
-        return true;
+	{
+	  MODULE_INTERNAL_CLEANUP;
+	  return true;
+	}
       /* As a simplification we don't check how many array elements
          are exactly required, but use a reasonable static upper
          bound.  For most architectures exactly one element should
@@ -1000,6 +1083,7 @@ module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
       if (magnitude == NULL)
         {
           *count = required;
+	  MODULE_INTERNAL_CLEANUP;
           return true;
         }
       if (*count < required)
@@ -1018,12 +1102,16 @@ module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
       verify (required * bits < PTRDIFF_MAX);
       for (ptrdiff_t i = 0; i < required; ++i)
         magnitude[i] = (emacs_limb_t) (u >> (i * bits));
+      MODULE_INTERNAL_CLEANUP;
       return true;
     }
   const mpz_t *x = xbignum_val (o);
   *sign = mpz_sgn (*x);
   if (count == NULL)
-    return true;
+    {
+      MODULE_INTERNAL_CLEANUP;
+      return true;
+    }
   size_t required_size = (mpz_sizeinbase (*x, 2) + numb - 1) / numb;
   eassert (required_size <= PTRDIFF_MAX);
   ptrdiff_t required = (ptrdiff_t) required_size;
@@ -1031,6 +1119,7 @@ module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
   if (magnitude == NULL)
     {
       *count = required;
+      MODULE_INTERNAL_CLEANUP;
       return true;
     }
   if (*count < required)
@@ -1043,6 +1132,7 @@ module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
   size_t written;
   mpz_export (magnitude, &written, order, size, endian, nails, *x);
   eassert (written == required_size);
+  MODULE_INTERNAL_CLEANUP;
   return true;
 }
 
@@ -1050,21 +1140,34 @@ module_extract_big_integer (emacs_env *env, emacs_value arg, int *sign,
 module_make_big_integer (emacs_env *env, int sign,
                          ptrdiff_t count, const emacs_limb_t *magnitude)
 {
+  emacs_value value;
+
   MODULE_FUNCTION_BEGIN (NULL);
   if (sign == 0)
-    return lisp_to_value (env, make_fixed_natnum (0));
+    {
+      value = lisp_to_value (env, make_fixed_natnum (0));
+      MODULE_INTERNAL_CLEANUP;
+      return value;
+    }
   enum { order = -1, size = sizeof *magnitude, endian = 0, nails = 0 };
   mpz_import (mpz[0], count, order, size, endian, nails, magnitude);
   if (sign < 0)
     mpz_neg (mpz[0], mpz[0]);
-  return lisp_to_value (env, make_integer_mpz ());
+  value = lisp_to_value (env, make_integer_mpz ());
+  MODULE_INTERNAL_CLEANUP;
+  return value;
 }
 
 static int
 module_open_channel (emacs_env *env, emacs_value pipe_process)
 {
+  int rc;
+
   MODULE_FUNCTION_BEGIN (-1);
-  return open_channel_for_module (value_to_lisp (pipe_process));
+  rc = open_channel_for_module (value_to_lisp (pipe_process));
+  MODULE_INTERNAL_CLEANUP;
+
+  return rc;
 }
 
 \f

Re: __attribute__ ((cleanup)) and emacs-module.c

[Paul Eggert]

On 2023-03-10 17:37, Po Lu wrote:

> +#define MODULE_INTERNAL_CLEANUP			\
> +  module_reset_handlerlist (&internal_cleanup)

No need for the "&" here. Omit it, and change the type of 
module_reset_handlerlist to take 'struct handler *' instead of 'struct 
handler **'.

Also, please make this macro a parameterless function-style macro called 
via 'MODULE_INTERNAL_CLEANUP ()' as it acts like a function.

Also, please define a macro MODULE_FUNCTION_END something like this:

   #define MODULE_FUNCTION_END(retval) \
      (MODULE_INTERNAL_CLEANUP (), retval)

and use 'return MODULE_FUNCTION_END (<simple-expression>);' when the 
cleanup is immediately followed by a return of a value of a simple 
expression. For complex expressions that can throw exceptions, you can 
use something like:

     <type> r = <complex-expression>;
     return MODULE_FUNCTION_END (r);

Re: __attribute__ ((cleanup)) and emacs-module.c

[Po Lu]

Paul Eggert <eggert@cs.ucla.edu> writes:

> On 2023-03-10 17:37, Po Lu wrote:
>
>> +#define MODULE_INTERNAL_CLEANUP			\
>> +  module_reset_handlerlist (&internal_cleanup)
>
> No need for the "&" here. Omit it, and change the type of
> module_reset_handlerlist to take 'struct handler *' instead of 'struct
> handler **'.
>
> Also, please make this macro a parameterless function-style macro
> called via 'MODULE_INTERNAL_CLEANUP ()' as it acts like a function.

OK, will do.

> Also, please define a macro MODULE_FUNCTION_END something like this:
>
>   #define MODULE_FUNCTION_END(retval) \
>      (MODULE_INTERNAL_CLEANUP (), retval)
>
> and use 'return MODULE_FUNCTION_END (<simple-expression>);' when the
> cleanup is immediately followed by a return of a value of a simple
> expression. For complex expressions that can throw exceptions, you can
> use something like:
>
>     <type> r = <complex-expression>;
>     return MODULE_FUNCTION_END (r);

I think this is likely to be confusing, because people won't realize the
situations in which it cannot be used, and we will end up with code
like:

  return MODULE_FUNCTION_END (Fthis_thing_that_calls_xmalloc ())

Re: __attribute__ ((cleanup)) and emacs-module.c

[Paul Eggert]

On 2023-03-11 16:42, Po Lu wrote:
> I think this is likely to be confusing, because people won't realize the
> situations in which it cannot be used, and we will end up with code
> like:
> 
>    return MODULE_FUNCTION_END (Fthis_thing_that_calls_xmalloc ())

Perhaps you're right. It's no big deal either way, I expect.

It's too bad we can't assume GNU (and C23 style) typeof, as that would 
let us evaluate the macro argument safely. But it'll be a while before 
we can assume C23.

Re: Merging feature/android

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Please take a look at the feature/android branch.
>
> I would like to merge it into master in the next couple of days, but
> before I do so I want everyone else to be happy with its contents as
> well.  So would everyone please do the usual with the diff between the
> branch and master?
>
> Thanks.

I think the issues people pointed out have now been resolved.  The
feature/android branch builds under GNU/Linux, Haiku and MS-DOS.

Would someone please test it on Windows as well?

Re: Merging feature/android

[Robert Pluim]

>>>>> On Tue, 14 Mar 2023 15:16:55 +0800, Po Lu <luangruo@yahoo.com> said:

    Po> Po Lu <luangruo@yahoo.com> writes:
    >> Please take a look at the feature/android branch.
    >> 
    >> I would like to merge it into master in the next couple of days, but
    >> before I do so I want everyone else to be happy with its contents as
    >> well.  So would everyone please do the usual with the diff between the
    >> branch and master?
    >> 
    >> Thanks.

    Po> I think the issues people pointed out have now been resolved.  The
    Po> feature/android branch builds under GNU/Linux, Haiku and MS-DOS.

It build on my GNU/Linux box, but 'make check' has some issues

1 files did not finish:
  lisp/emacs-lisp/edebug-tests.log
4 files contained unexpected results:
  src/fileio-tests.log
  lisp/simple-tests.log
  lisp/replace-tests.log
  lisp/kmacro-tests.log

most of which are variants of

Test simple-test-undo-extra-boundary-in-tex backtrace:
  signal(error ("Invalid argument macro in `get-device-terminal'"))
  apply(signal (error ("Invalid argument macro in `get-device-terminal

    Po> Would someone please test it on Windows as well?

I can send you a quote at an extortionate rate for that ;-)

Robert
-- 

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Tue, 14 Mar 2023 15:16:55 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po> Po Lu <luangruo@yahoo.com> writes:
>     >> Please take a look at the feature/android branch.
>     >> 
>     >> I would like to merge it into master in the next couple of days, but
>     >> before I do so I want everyone else to be happy with its contents as
>     >> well.  So would everyone please do the usual with the diff between the
>     >> branch and master?
>     >> 
>     >> Thanks.
>
>     Po> I think the issues people pointed out have now been resolved.  The
>     Po> feature/android branch builds under GNU/Linux, Haiku and MS-DOS.
>
> It build on my GNU/Linux box, but 'make check' has some issues
>
> 1 files did not finish:
>   lisp/emacs-lisp/edebug-tests.log
> 4 files contained unexpected results:
>   src/fileio-tests.log
>   lisp/simple-tests.log
>   lisp/replace-tests.log
>   lisp/kmacro-tests.log
>
> most of which are variants of
>
> Test simple-test-undo-extra-boundary-in-tex backtrace:
>   signal(error ("Invalid argument macro in `get-device-terminal'"))
>   apply(signal (error ("Invalid argument macro in `get-device-terminal

If you could get a backtrace for this, it would be great.  What is
calling frames-on-display-list?

Thanks.

Re: Merging feature/android

[Robert Pluim]

>>>>> On Tue, 14 Mar 2023 18:39:22 +0800, Po Lu <luangruo@yahoo.com> said:

    >> most of which are variants of
    >> 
    >> Test simple-test-undo-extra-boundary-in-tex backtrace:
    >> signal(error ("Invalid argument macro in `get-device-terminal'"))
    >> apply(signal (error ("Invalid argument macro in `get-device-terminal

    Po Lu> If you could get a backtrace for this, it would be great.  What is
    Po Lu> calling frames-on-display-list?

    Po Lu> Thanks.

Debugger entered--Lisp error: (error "Invalid argument macro in ‘get-device-terminal’")
  signal(error ("Invalid argument macro in ‘get-device-terminal’"))
  error("Invalid argument %s in `get-device-terminal'" macro)
  get-device-terminal(macro)
  frames-on-display-list(macro)
  framep-on-display(macro)
  device-class(macro nil)
  minibuffer-setup-on-screen-keyboard()
  read-from-minibuffer("LaTeX block name [enumerate]: " nil (keymap (menu-bar keymap (minibuf "Minibuf" keymap (tab menu-item "Complete" minibuffer-complete :help "Complete as far as possible") (space menu-item "Complete Word" minibuffer-complete-word :help "Complete at most one word") (63 menu-item "List Completions" minibuffer-completion-help :help "Display all possible completions") "Minibuf")) (M-down . minibuffer-next-completion) (M-up . minibuffer-previous-completion) (27 keymap (13 . minibuffer-choose-completion) (103 keymap (27 keymap (99 . switch-to-completions))) (118 . switch-to-completions)) (prior . switch-to-completions) (63 . minibuffer-completion-help) (32 . minibuffer-complete-word) (backtab . minibuffer-complete) (9 . minibuffer-complete) keymap (menu-bar keymap (minibuf "Minibuf" keymap (previous menu-item "Previous History Item" previous-history-element :help "Put previous minibuffer history element in the min...") (next menu-item "Next History Item" next-history-element :help "Put next minibuffer history element in the minibuf...") (isearch-backward menu-item "Isearch History Backward" isearch-backward :help "Incrementally search minibuffer history backward") (isearch-forward menu-item "Isearch History Forward" isearch-forward :help "Incrementally search minibuffer history forward") (return menu-item "Enter" exit-minibuffer :key-sequence "\15" :help "Terminate input and exit minibuffer") (quit menu-item "Quit" abort-recursive-edit :help "Abort input and exit minibuffer") "Minibuf")) (24 keymap (down . minibuffer-complete-defaults) (up . minibuffer-complete-history)) (13 . exit-minibuffer) (10 . exit-minibuffer) (7 . abort-minibuffers) (C-tab . file-cache-minibuffer-complete) (9 . self-insert-command) (XF86Back . previous-history-element) (up . previous-line-or-history-element) (prior . previous-history-element) (XF86Forward . next-history-element) (down . next-line-or-history-element) (next . next-history-element) (27 keymap (60 . minibuffer-beginning-of-buffer) (114 . previous-matching-history-element) (115 . next-matching-history-element) (112 . previous-history-element) (110 . next-history-element))) nil nil "enumerate" nil)
  completing-read-default("LaTeX block name [enumerate]: " #f(compiled-function (string pred action) #<bytecode -0x40de75530394a3a>) nil nil nil nil "enumerate" nil)
  completing-read("LaTeX block name [enumerate]: " #f(compiled-function (string pred action) #<bytecode -0x40de75530394a3a>) nil nil nil nil "enumerate")
  (let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice)
  eval((let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice))
  skeleton-read((let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) nil nil)
  (setq str (skeleton-read '(let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) nil nil))
  eval((setq str (skeleton-read '(let ((choice (completing-read ... ... nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) nil nil)))
  skeleton-internal-1((setq str (skeleton-read '(let ((choice (completing-read ... ... nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) nil nil)) t nil)
  skeleton-internal-1(str nil nil)
  skeleton-internal-list(((let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) n "\\begin{" str "}" (cdr (assoc str latex-block-args-alist)) > n (or (cdr (assoc str latex-block-body-alist)) '(nil > _)) (unless (bolp) 'n) "\\end{" str "}" > n) nil)
  #f(compiled-function () #<bytecode 0x1a7e183eddc6ce30>)()
  funcall(#f(compiled-function () #<bytecode 0x1a7e183eddc6ce30>))
  (let nil (funcall '#f(compiled-function () #<bytecode 0x1a7e183eddc6ce30>)))
  eval((let nil (funcall '#f(compiled-function () #<bytecode 0x1a7e183eddc6ce30>))))
  skeleton-insert(((let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) n "\\begin{" str "}" (cdr (assoc str latex-block-args-alist)) > n (or (cdr (assoc str latex-block-body-alist)) '(nil > _)) (unless (bolp) 'n) "\\end{" str "}" > n) nil nil)
  skeleton-proxy-new(((let ((choice (completing-read (format "LaTeX block name [%s]: " latex-block-default) (latex-complete-envnames) nil nil nil nil latex-block-default))) (setq latex-block-default choice) (unless (or (member choice latex-standard-block-names) (member choice latex-block-names)) (push choice latex-block-names)) choice) n "\\begin{" str "}" (cdr (assoc str latex-block-args-alist)) > n (or (cdr (assoc str latex-block-body-alist)) '(nil > _)) (unless (bolp) 'n) "\\end{" str "}" > n) nil nil)
  latex-insert-block(nil nil)
  funcall-interactively(latex-insert-block nil nil)
  call-interactively(latex-insert-block nil nil)
  command-execute(latex-insert-block)
  execute-kbd-macro([3 15 13 67108911])
  (progn (switch-to-buffer "temp.tex") (latex-mode) (execute-kbd-macro (read-kbd-macro "\nC-c C-o\11\11\11;; latex-insert-block\nRET\11\11\11;; newline\n...")) (buffer-substring-no-properties (point-min) (point-max)))
  (unwind-protect (progn (switch-to-buffer "temp.tex") (latex-mode) (execute-kbd-macro (read-kbd-macro "\nC-c C-o\11\11\11;; latex-insert-block\nRET\11\11\11;; newline\n...")) (buffer-substring-no-properties (point-min) (point-max))) (switch-to-buffer before-buffer))
  (let ((before-buffer (current-buffer))) (unwind-protect (progn (switch-to-buffer "temp.tex") (latex-mode) (execute-kbd-macro (read-kbd-macro "\nC-c C-o\11\11\11;; latex-insert-block\nRET\11\11\11;; newline\n...")) (buffer-substring-no-properties (point-min) (point-max))) (switch-to-buffer before-buffer)))
  (progn (let ((before-buffer (current-buffer))) (unwind-protect (progn (switch-to-buffer "temp.tex") (latex-mode) (execute-kbd-macro (read-kbd-macro "\nC-c C-o\11\11\11;; latex-insert-block\nRET\11\11\11;; newline\n...")) (buffer-substring-no-properties (point-min) (point-max))) (switch-to-buffer before-buffer))))
  eval((progn (let ((before-buffer (current-buffer))) (unwind-protect (progn (switch-to-buffer "temp.tex") (latex-mode) (execute-kbd-macro (read-kbd-macro "\nC-c C-o\11\11\11;; latex-insert-block\nRET\11\11\11;; newline\n...")) (buffer-substring-no-properties (point-min) (point-max))) (switch-to-buffer before-buffer)))) t)
  elisp--eval-last-sexp(nil)
  eval-last-sexp(nil)
  funcall-interactively(eval-last-sexp nil)
  call-interactively(eval-last-sexp nil nil)
  command-execute(eval-last-sexp)

Robert
-- 

Re: Merging feature/android

[Robert Pluim]

>>>>> On Tue, 14 Mar 2023 11:47:36 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Tue, 14 Mar 2023 18:39:22 +0800, Po Lu <luangruo@yahoo.com> said:
    >>> most of which are variants of
    >>> 
    >>> Test simple-test-undo-extra-boundary-in-tex backtrace:
    >>> signal(error ("Invalid argument macro in `get-device-terminal'"))
    >>> apply(signal (error ("Invalid argument macro in `get-device-terminal

    Robert>     Po Lu> If you could get a backtrace for this, it would be great.  What is
    Robert>     Po Lu> calling frames-on-display-list?

    Robert>     Po Lu> Thanks.

This fixes 3 of the 4 test failures:

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ef0eb1ca108..cdff129b7a8 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4598,9 +4598,10 @@ minibuffer-setup-on-screen-keyboard
     (cancel-timer minibuffer-on-screen-keyboard-timer)
     (setq minibuffer-on-screen-keyboard-timer nil))
   (setq minibuffer-on-screen-keyboard-displayed nil)
-  (when (not (memq (device-class last-event-frame
+  (when (and (not (eq last-event-frame 'macro))
+             (not (memq (device-class last-event-frame
                                last-event-device)
-                   '(keyboard core-keyboard)))
+                   '(keyboard core-keyboard))))
     (setq minibuffer-on-screen-keyboard-displayed
           (frame-toggle-on-screen-keyboard (selected-frame) nil))))


Robert
-- 

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Tue, 14 Mar 2023 11:47:36 +0100, Robert Pluim <rpluim@gmail.com> said:
>
>>>>>> On Tue, 14 Mar 2023 18:39:22 +0800, Po Lu <luangruo@yahoo.com> said:
>     >>> most of which are variants of
>     >>> 
>     >>> Test simple-test-undo-extra-boundary-in-tex backtrace:
>     >>> signal(error ("Invalid argument macro in `get-device-terminal'"))
>     >>> apply(signal (error ("Invalid argument macro in `get-device-terminal
>
>     Robert>     Po Lu> If you could get a backtrace for this, it would be great.  What is
>     Robert>     Po Lu> calling frames-on-display-list?
>
>     Robert>     Po Lu> Thanks.
>
> This fixes 3 of the 4 test failures:
>
> diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
> index ef0eb1ca108..cdff129b7a8 100644
> --- a/lisp/minibuffer.el
> +++ b/lisp/minibuffer.el
> @@ -4598,9 +4598,10 @@ minibuffer-setup-on-screen-keyboard
>      (cancel-timer minibuffer-on-screen-keyboard-timer)
>      (setq minibuffer-on-screen-keyboard-timer nil))
>    (setq minibuffer-on-screen-keyboard-displayed nil)
> -  (when (not (memq (device-class last-event-frame
> +  (when (and (not (eq last-event-frame 'macro))
> +             (not (memq (device-class last-event-frame
>                                 last-event-device)
> -                   '(keyboard core-keyboard)))
> +                   '(keyboard core-keyboard))))
>      (setq minibuffer-on-screen-keyboard-displayed
>            (frame-toggle-on-screen-keyboard (selected-frame) nil))))
>
>
> Robert

Does this work too?

Thanks.

diff --git a/lisp/minibuffer.el b/lisp/minibuffer.el
index ef0eb1ca108..b28bbae7c64 100644
--- a/lisp/minibuffer.el
+++ b/lisp/minibuffer.el
@@ -4598,9 +4598,11 @@ minibuffer-setup-on-screen-keyboard
     (cancel-timer minibuffer-on-screen-keyboard-timer)
     (setq minibuffer-on-screen-keyboard-timer nil))
   (setq minibuffer-on-screen-keyboard-displayed nil)
-  (when (not (memq (device-class last-event-frame
-                               last-event-device)
-                   '(keyboard core-keyboard)))
+  (when (and (framep last-event-frame)
+             last-event-device
+             (not (memq (device-class last-event-frame
+                                      last-event-device)
+                        '(keyboard core-keyboard))))
     (setq minibuffer-on-screen-keyboard-displayed
           (frame-toggle-on-screen-keyboard (selected-frame) nil))))
 

Re: Merging feature/android

[Arash Esbati]

Po Lu <luangruo@yahoo.com> writes:

> Would someone please test it on Windows as well?

./autogen.sh and ./configure both work.  Running make says:

  CC       fingerprint.o
  CC       acl_entries.o
  CC       asnprintf.o
  CC       asprintf.o
  CC       frexp.o
asprintf.c:30:1: error: redefinition of 'asprintf'
   30 | asprintf (char **resultp, const char *format, ...)
      | ^~~~~~~~
In file included from Z:/pathto/emacs-android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from asprintf.c:18:
Z:/pathto/msys64/mingw64/include/stdio.h:265:5: note: previous definition of 'asprintf' with type 'int(char **, const char *, ...)'
  265 | int asprintf(char **__ret, const char *__format, ...)
      |     ^~~~~~~~
make[2]: *** [Makefile:102: asprintf.o] Error 1

This is with MSYS2/MinWG64, GCC 12.2.0.  HTH.

Best, Arash

Re: Merging feature/android

[Robert Pluim]

>>>>> On Tue, 14 Mar 2023 19:34:48 +0800, Po Lu <luangruo@yahoo.com> said:


    Po Lu> Does this work too?

Yes.

Robert
-- 

Re: Merging feature/android

[Po Lu]

Arash Esbati <arash@gnu.org> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Would someone please test it on Windows as well?
>
> ./autogen.sh and ./configure both work.  Running make says:
>
>   CC       fingerprint.o
>   CC       acl_entries.o
>   CC       asnprintf.o
>   CC       asprintf.o
>   CC       frexp.o
> asprintf.c:30:1: error: redefinition of 'asprintf'
>    30 | asprintf (char **resultp, const char *format, ...)
>       | ^~~~~~~~
> In file included from Z:/pathto/emacs-android/nt/inc/ms-w32.h:389,
>                  from ../src/conf_post.h:38,
>                  from ../src/config.h:3470,
>                  from asprintf.c:18:
> Z:/pathto/msys64/mingw64/include/stdio.h:265:5: note: previous definition of 'asprintf' with type 'int(char **, const char *, ...)'
>   265 | int asprintf(char **__ret, const char *__format, ...)
>       |     ^~~~~~~~
> make[2]: *** [Makefile:102: asprintf.o] Error 1
>
> This is with MSYS2/MinWG64, GCC 12.2.0.  HTH.

Right.  What if you apply this change?

diff --git a/nt/gnulib-cfg.mk b/nt/gnulib-cfg.mk
index eca3778f203..e107d037153 100644
--- a/nt/gnulib-cfg.mk
+++ b/nt/gnulib-cfg.mk
@@ -71,7 +71,10 @@ OMIT_GNULIB_MODULE_utimens =
 OMIT_GNULIB_MODULE_fchmodat = true
 OMIT_GNULIB_MODULE_lchmod = true
 OMIT_GNULIB_MODULE_futimens = true
+OMIT_GNULIB_MODULE_float = true
 OMIT_GNULIB_MODULE_utimensat = true
 OMIT_GNULIB_MODULE_file-has-acl = true
 OMIT_GNULIB_MODULE_nproc = true
 OMIT_GNULIB_MODULE_nanosleep = true
+OMIT_GNULIB_MODULE_vfprintf-posix = true
+OMIT_GNULIB_MODULE_vasnprintf = true

Re: Merging feature/android

[Arash Esbati]

Po Lu <luangruo@yahoo.com> writes:

> Right.  What if you apply this change?
>
> diff --git a/nt/gnulib-cfg.mk b/nt/gnulib-cfg.mk
> index eca3778f203..e107d037153 100644
> --- a/nt/gnulib-cfg.mk
> +++ b/nt/gnulib-cfg.mk
> @@ -71,7 +71,10 @@ OMIT_GNULIB_MODULE_utimens =
>  OMIT_GNULIB_MODULE_fchmodat = true
>  OMIT_GNULIB_MODULE_lchmod = true
>  OMIT_GNULIB_MODULE_futimens = true
> +OMIT_GNULIB_MODULE_float = true
>  OMIT_GNULIB_MODULE_utimensat = true
>  OMIT_GNULIB_MODULE_file-has-acl = true
>  OMIT_GNULIB_MODULE_nproc = true
>  OMIT_GNULIB_MODULE_nanosleep = true
> +OMIT_GNULIB_MODULE_vfprintf-posix = true
> +OMIT_GNULIB_MODULE_vasnprintf = true

I did 'git clean -fdx' and then applied your patch, but no avail, still
the same error.

Best, Arash

Re: Merging feature/android

[Po Lu]

Arash Esbati <arash@gnu.org> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Right.  What if you apply this change?
>>
>> diff --git a/nt/gnulib-cfg.mk b/nt/gnulib-cfg.mk
>> index eca3778f203..e107d037153 100644
>> --- a/nt/gnulib-cfg.mk
>> +++ b/nt/gnulib-cfg.mk
>> @@ -71,7 +71,10 @@ OMIT_GNULIB_MODULE_utimens =
>>  OMIT_GNULIB_MODULE_fchmodat = true
>>  OMIT_GNULIB_MODULE_lchmod = true
>>  OMIT_GNULIB_MODULE_futimens = true
>> +OMIT_GNULIB_MODULE_float = true
>>  OMIT_GNULIB_MODULE_utimensat = true
>>  OMIT_GNULIB_MODULE_file-has-acl = true
>>  OMIT_GNULIB_MODULE_nproc = true
>>  OMIT_GNULIB_MODULE_nanosleep = true
>> +OMIT_GNULIB_MODULE_vfprintf-posix = true
>> +OMIT_GNULIB_MODULE_vasnprintf = true
>
> I did 'git clean -fdx' and then applied your patch, but no avail, still
> the same error.
>
> Best, Arash

Huh.

Eli, is there anything I have to do to make the Windows NT gnulib stuff
omit a gnulib module, other than putting:

  OMIT_GNULIB_MODULE_xxx

in nt/gnulib-cfg.mk?

Re: Merging feature/android

[Robert Pluim]

>>>>> On Tue, 14 Mar 2023 14:10:44 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Tue, 14 Mar 2023 19:34:48 +0800, Po Lu <luangruo@yahoo.com> said:
    Robert>     Po Lu> Does this work too?

    Robert> Yes.

And for the fileio-tests failure, I think youʼll need to revisit
84d27fe53b28

If I undo part of that commit as follows, the test succeeds:

diff --git a/src/fileio.c b/src/fileio.c
index 99f710ccbf0..978e22f038c 100644
--- a/src/fileio.c
+++ b/src/fileio.c
@@ -5004,7 +5004,7 @@ because (1) it preserves some marker positions (in unchanged portions
   /* Decode file format.  Don't do this if Qformat_decode is not
      bound, which can happen when called early during loadup.  */
 
-  if (inserted > 0 && !NILP (Fboundp (Qformat_decode)))
+  if (inserted > 0)
     {
       /* Don't run point motion or modification hooks when decoding.  */
       specpdl_ref count1 = SPECPDL_INDEX ();


Robert
-- 

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> Date: Tue, 14 Mar 2023 21:49:53 +0800
> 
> Arash Esbati <arash@gnu.org> writes:
> 
> > I did 'git clean -fdx' and then applied your patch, but no avail, still
> > the same error.
> >
> > Best, Arash
> 
> Huh.
> 
> Eli, is there anything I have to do to make the Windows NT gnulib stuff
> omit a gnulib module, other than putting:
> 
>   OMIT_GNULIB_MODULE_xxx
> 
> in nt/gnulib-cfg.mk?

Yes, you need to force regeneration of configure and gnulib.mk.  The
easiest way is to touch configure.ac and gnulib.mk.in, and then run
"make".

In general, I'd suggest to OMIT_* all Gnulib modules you added for
Android, as I see no reason why we'd need to build them on Windows,
and they can only cause trouble.

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Tue, 14 Mar 2023 14:10:44 +0100, Robert Pluim <rpluim@gmail.com> said:
>
>>>>>> On Tue, 14 Mar 2023 19:34:48 +0800, Po Lu <luangruo@yahoo.com> said:
>     Robert>     Po Lu> Does this work too?
>
>     Robert> Yes.
>
> And for the fileio-tests failure, I think youʼll need to revisit
> 84d27fe53b28
>
> If I undo part of that commit as follows, the test succeeds:
>
> diff --git a/src/fileio.c b/src/fileio.c
> index 99f710ccbf0..978e22f038c 100644
> --- a/src/fileio.c
> +++ b/src/fileio.c
> @@ -5004,7 +5004,7 @@ because (1) it preserves some marker positions (in unchanged portions
>    /* Decode file format.  Don't do this if Qformat_decode is not
>       bound, which can happen when called early during loadup.  */
>  
> -  if (inserted > 0 && !NILP (Fboundp (Qformat_decode)))
> +  if (inserted > 0)
>      {
>        /* Don't run point motion or modification hooks when decoding.  */
>        specpdl_ref count1 = SPECPDL_INDEX ();
>
>
> Robert

Why isn't `format-decode' defined already when the test is run?
format-decode.el is indeed preloaded.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> In general, I'd suggest to OMIT_* all Gnulib modules you added for
> Android, as I see no reason why we'd need to build them on Windows,
> and they can only cause trouble.

Thanks, I've now gone ahead and done that.

Re: Merging feature/android

[Corwin Brust]

On Tue, Mar 14, 2023 at 7:21 PM Po Lu <luangruo@yahoo.com> wrote:
>
> Eli Zaretskii <eliz@gnu.org> writes:
>
> > In general, I'd suggest to OMIT_* all Gnulib modules you added for
> > Android, as I see no reason why we'd need to build them on Windows,
> > and they can only cause trouble.
>
> Thanks, I've now gone ahead and done that.

Did you forget to push? (Or, do you plan to tell us when to try
building for Windows again?)  The last I see for feature/android is
a39ca9bf8e34e7cf6760e2fa3b7d644bef09ce91

Re: Merging feature/android

[Po Lu]

Corwin Brust <corwin@bru.st> writes:

> Did you forget to push?

Yes.  I've done that now.

Thanks for testing.

Re: Merging feature/android

[Corwin Brust]

On Tue, Mar 14, 2023 at 8:30 PM Po Lu <luangruo@yahoo.com> wrote:
>
> Thanks for testing.

Of course, thanks for working on the Android port!

No errors now, building feature/android with MSYS2/MINGW64.

Re: Merging feature/android

[Po Lu]

Corwin Brust <corwin@bru.st> writes:

> On Tue, Mar 14, 2023 at 8:30 PM Po Lu <luangruo@yahoo.com> wrote:
>>
>> Thanks for testing.
>
> Of course, thanks for working on the Android port!
>
> No errors now, building feature/android with MSYS2/MINGW64.

Great.  Is it easy for you to test on plain MinGW as well?

Re: Merging feature/android

[Corwin Brust]

On Wed, Mar 15, 2023 at 1:03 AM Corwin Brust <corwin@bru.st> wrote:
>
> No errors now, building feature/android with MSYS2/MINGW64.

I think I "spoke" too soon. I didn't notice these immediately as they
didn't interrupt the build, but I do the below in thee output from
make.  Everything after this looks normal.

  CCLD     runemacs.exe
tmp=etc/emacsclient.tmpdesktop; rm -f ${tmp}; \
client_name=`echo emacsclient | sed 's,x,x,'`.exe; \
sed -e "/^Exec=/
s|emacsclient|/d/emacs-build/install/emacs-feature_android/bin/${client_name}|"
\
  -e "/^Icon=emacs/ s/emacs/`echo emacs | sed 's,x,x,'`/" \
  -e "/^StartupNotify=true$/d" \
  ./etc/emacsclient.desktop > ${tmp}; \
/usr/bin/install -c -m 644 ${tmp}
"/d/emacs-build/install/emacs-feature_android/share/applications/${client_name}.desktop";
\
rm -f ${tmp}
  CC       fingerprint.o
  CC       acl_entries.o
  CC       asnprintf.o
  CC       asprintf.o
  CC       frexp.o
  CC       memmem.o
  CC       mktime.o
  CC       printf.o
  CC       printf-args.o
asprintf.c:30:1: error: redefinition of 'asprintf'
   30 | asprintf (char **resultp, const char *format, ...)
      | ^~~~~~~~
In file included from
C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from asprintf.c:18:
C:/msys2/mingw64/include/stdio.h:265:5: note: previous definition of
'asprintf' with type 'int(char **, const char *, ...)'
  265 | int asprintf(char **__ret, const char *__format, ...)
      |     ^~~~~~~~
  CC       printf-parse.o
  CC       vasnprintf.o
make[1]: *** [Makefile:102: asprintf.o] Error 1
make[1]: *** Waiting for unfinished jobs....
printf.c:30:1: error: redefinition of 'printf'
   30 | printf (const char *format, ...)
      | ^~~~~~
In file included from
C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from printf.c:18:
C:/msys2/mingw64/include/stdio.h:368:5: note: previous definition of
'printf' with type 'int(const char *, ...)'
  368 | int printf (const char *__format, ...)
      |     ^~~~~~
make[1]: *** [Makefile:102: printf.o] Error 1

Re: Merging feature/android

[Corwin Brust]

On Wed, Mar 15, 2023 at 1:15 AM Po Lu <luangruo@yahoo.com> wrote:
>
> Great.  Is it easy for you to test on plain MinGW as well?

I haven't tried before; I'll do that now.

Re: Merging feature/android

[Corwin Brust]

On Wed, Mar 15, 2023 at 1:15 AM Po Lu <luangruo@yahoo.com> wrote:
>
> Great.  Is it easy for you to test on plain MinGW as well?
>

Running under mingw32, I get these errors from make (configure runs fine):

  CC       vasnprintf.o
asprintf.c:30:1: error: redefinition of 'asprintf'
   30 | asprintf (char **resultp, const char *format, ...)
      | ^~~~~~~~
In file included from
C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from asprintf.c:18:
C:/msys2/mingw32/include/stdio.h:265:5: note: previous definition of
'asprintf' with type 'int(char **, const char *, ...)'
  265 | int asprintf(char **__ret, const char *__format, ...)
      |     ^~~~~~~~
make[2]: Entering directory '/c/Users/corwi/emacs-build/git/android/doc/lispref'
  GEN      ../../info/elisp.info
  CC       vasprintf.o
  CC       vfprintf.o
make[2]: Entering directory '/c/Users/corwi/emacs-build/git/android/doc/emacs'
  GEN      ../../info/emacs.info
make[2]: *** [Makefile:102: asprintf.o] Error 1
make[2]: *** Waiting for unfinished jobs....
printf.c:30:1: error: redefinition of 'printf'
   30 | printf (const char *format, ...)
      | ^~~~~~
In file included from
C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from printf.c:18:
C:/msys2/mingw32/include/stdio.h:368:5: note: previous definition of
'printf' with type 'int(const char *, ...)'
  368 | int printf (const char *__format, ...)
      |     ^~~~~~
make[2]: *** [Makefile:102: printf.o] Error 1
vasprintf.c:33:1: error: redefinition of 'vasprintf'
   33 | vasprintf (char **resultp, const char *format, va_list args)
      | ^~~~~~~~~
In file included from
C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from vasprintf.c:17:
C:/msys2/mingw32/include/stdio.h:276:5: note: previous definition of
'vasprintf' with type 'int(char **, const char *, char *)'
  276 | int vasprintf(char **__ret, const char *__format,
__builtin_va_list __local_argv)
      |     ^~~~~~~~~
vfprintf.c:36:1: error: redefinition of 'vfprintf'
   36 | vfprintf (FILE *fp, const char *format, va_list args)
      | ^~~~~~~~
In file included from
C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
                 from ../src/conf_post.h:38,
                 from ../src/config.h:3470,
                 from vfprintf.c:18:
C:/msys2/mingw32/include/stdio.h:409:5: note: previous definition of
'vfprintf' with type 'int(FILE *, const char *, char *)' {aka
'int(struct _iobuf *, const char *, char *)'}
  409 | int vfprintf (FILE *__stream, const char *__format,
__builtin_va_list __local_argv)
      |     ^~~~~~~~
  GEN      info/dir
make[2]: *** [Makefile:102: vfprintf.o] Error 1
make[2]: *** [Makefile:102: vasprintf.o] Error 1
make[2]: Leaving directory '/c/Users/corwi/emacs-build/git/android/lib'
make[1]: *** [Makefile:537: lib] Error 2
make[1]: *** Waiting for unfinished jobs....

Re: Merging feature/android

[Po Lu]

Corwin Brust <corwin@bru.st> writes:

> On Wed, Mar 15, 2023 at 1:03 AM Corwin Brust <corwin@bru.st> wrote:
>>
>> No errors now, building feature/android with MSYS2/MINGW64.
>
> I think I "spoke" too soon. I didn't notice these immediately as they
> didn't interrupt the build, but I do the below in thee output from
> make.  Everything after this looks normal.
>
>   CCLD     runemacs.exe
> tmp=etc/emacsclient.tmpdesktop; rm -f ${tmp}; \
> client_name=`echo emacsclient | sed 's,x,x,'`.exe; \
> sed -e "/^Exec=/
> s|emacsclient|/d/emacs-build/install/emacs-feature_android/bin/${client_name}|"
> \
>   -e "/^Icon=emacs/ s/emacs/`echo emacs | sed 's,x,x,'`/" \
>   -e "/^StartupNotify=true$/d" \
>   ./etc/emacsclient.desktop > ${tmp}; \
> /usr/bin/install -c -m 644 ${tmp}
> "/d/emacs-build/install/emacs-feature_android/share/applications/${client_name}.desktop";
> \
> rm -f ${tmp}
>   CC       fingerprint.o
>   CC       acl_entries.o
>   CC       asnprintf.o
>   CC       asprintf.o
>   CC       frexp.o
>   CC       memmem.o
>   CC       mktime.o
>   CC       printf.o
>   CC       printf-args.o
> asprintf.c:30:1: error: redefinition of 'asprintf'
>    30 | asprintf (char **resultp, const char *format, ...)
>       | ^~~~~~~~
> In file included from
> C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
>                  from ../src/conf_post.h:38,
>                  from ../src/config.h:3470,
>                  from asprintf.c:18:
> C:/msys2/mingw64/include/stdio.h:265:5: note: previous definition of
> 'asprintf' with type 'int(char **, const char *, ...)'
>   265 | int asprintf(char **__ret, const char *__format, ...)
>       |     ^~~~~~~~
>   CC       printf-parse.o
>   CC       vasnprintf.o
> make[1]: *** [Makefile:102: asprintf.o] Error 1
> make[1]: *** Waiting for unfinished jobs....
> printf.c:30:1: error: redefinition of 'printf'
>    30 | printf (const char *format, ...)
>       | ^~~~~~
> In file included from
> C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
>                  from ../src/conf_post.h:38,
>                  from ../src/config.h:3470,
>                  from printf.c:18:
> C:/msys2/mingw64/include/stdio.h:368:5: note: previous definition of
> 'printf' with type 'int(const char *, ...)'
>   368 | int printf (const char *__format, ...)
>       |     ^~~~~~
> make[1]: *** [Makefile:102: printf.o] Error 1

Thanks, but now I don't know why vasnprintf.o and printf-args.o are being
built, given that:

  OMIT_GNULIB_MODULE_vasnprintf

is set to true in nt/gnulib-cfg.mk, and all of the configury and gnulib
stuff has been regenerated.  Any ideas?  Could you perhaps try to
determine that?

Thanks.

Re: Merging feature/android

[Robert Pluim]

>>>>> On Wed, 15 Mar 2023 08:16:26 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Why isn't `format-decode' defined already when the test is run?
    Po Lu> format-decode.el is indeed preloaded.

It is defined, the code just isnʼt checking the right thing.

diff --git i/src/fileio.c w/src/fileio.c
index 99f710ccbf0..5153aeae248 100644
--- i/src/fileio.c
+++ w/src/fileio.c
@@ -5004,7 +5004,7 @@ because (1) it preserves some marker positions (in unchanged portions
   /* Decode file format.  Don't do this if Qformat_decode is not
      bound, which can happen when called early during loadup.  */
 
-  if (inserted > 0 && !NILP (Fboundp (Qformat_decode)))
+  if (inserted > 0 && !NILP (Ffboundp (Qformat_decode)))
     {
       /* Don't run point motion or modification hooks when decoding.  */
       specpdl_ref count1 = SPECPDL_INDEX ();
diff --git i/src/window.c w/src/window.c
index 9a29ecb8807..8c42d3cdd0c 100644
--- i/src/window.c
+++ w/src/window.c
@@ -3516,7 +3516,7 @@ replace_buffer_in_windows (Lisp_Object buffer)
 {
   /* When kill-buffer is called early during loadup, this function is
      undefined.  */
-  if (!NILP (Fboundp (Qreplace_buffer_in_windows)))
+  if (!NILP (Ffboundp (Qreplace_buffer_in_windows)))
     call1 (Qreplace_buffer_in_windows, buffer);
 }
 

Robert
-- 

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Wed, 15 Mar 2023 08:16:26 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po Lu> Why isn't `format-decode' defined already when the test is run?
>     Po Lu> format-decode.el is indeed preloaded.
>
> It is defined, the code just isnʼt checking the right thing.
>
> diff --git i/src/fileio.c w/src/fileio.c
> index 99f710ccbf0..5153aeae248 100644
> --- i/src/fileio.c
> +++ w/src/fileio.c
> @@ -5004,7 +5004,7 @@ because (1) it preserves some marker positions (in unchanged portions
>    /* Decode file format.  Don't do this if Qformat_decode is not
>       bound, which can happen when called early during loadup.  */
>  
> -  if (inserted > 0 && !NILP (Fboundp (Qformat_decode)))
> +  if (inserted > 0 && !NILP (Ffboundp (Qformat_decode)))
>      {
>        /* Don't run point motion or modification hooks when decoding.  */
>        specpdl_ref count1 = SPECPDL_INDEX ();
> diff --git i/src/window.c w/src/window.c
> index 9a29ecb8807..8c42d3cdd0c 100644
> --- i/src/window.c
> +++ w/src/window.c
> @@ -3516,7 +3516,7 @@ replace_buffer_in_windows (Lisp_Object buffer)
>  {
>    /* When kill-buffer is called early during loadup, this function is
>       undefined.  */
> -  if (!NILP (Fboundp (Qreplace_buffer_in_windows)))
> +  if (!NILP (Ffboundp (Qreplace_buffer_in_windows)))
>      call1 (Qreplace_buffer_in_windows, buffer);
>  }
>  
>
> Robert

Oh, thanks!

Re: Merging feature/android

[Robert Pluim]

>>>>> On Wed, 15 Mar 2023 18:25:43 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Oh, thanks!

Weʼre not quite there yet :-)

lisp/emacs-lisp/edebug-tests.log:

  locate-file-internal(nil ("/home/rpluim/repos/emacs-android/lisp" "/
  locate-file(nil ("/home/rpluim/repos/emacs-android/lisp" "/home/rplu
  file-loadhist-lookup(nil)
  file-provides(nil)
  file-dependents(nil)
  unload-feature(edebug-test-code)
  #f(compiled-function () #<bytecode 0x5f78608ef6e90e6>)()
  #f(compiled-function () #<bytecode 0x168e67691f944ca4>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name edebug-tests-trivial-backquote :docum
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/edebug-te
  command-line()
  normal-top-level()
Test edebug-tests-trivial-backquote condition:
    (wrong-type-argument stringp nil)

which looks like an issue with load-history not containing an entry
for the `edebug-test-code' feature? No idea yet for that

And also:

lisp/progmodes/eglot-tests.log:

Test eglot-test-diagnostic-tags-unnecessary-code backtrace:
  signal(ert-test-failed (((should (eq 'eglot-diagnostic-tag-unnecessa
  ert-fail(((should (eq 'eglot-diagnostic-tag-unnecessary-face (face-a
  #f(compiled-function () #<bytecode 0x11898351aa04629a>)()
  eglot--call-with-fixture((("diag-project" ("main.cpp" . "int main(){
  #f(compiled-function () #<bytecode -0x25d7fd8d71f4ef>)()
  ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
  ert-run-test(#s(ert-test :name eglot-test-diagnostic-tags-unnecessar
  ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
  ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
  ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
  ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
  eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
  command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/progmodes/eglot-test
  command-line()
  normal-top-level()
Test eglot-test-diagnostic-tags-unnecessary-code condition:
    (ert-test-failed
     ((should
       (eq 'eglot-diagnostic-tag-unnecessary-face
	    (face-at-point)))
      :form
      (eq eglot-diagnostic-tag-unnecessary-face flymake-warning)
      :value nil))

which I havenʼt investigated yet.

Robert
-- 

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Wed, 15 Mar 2023 18:25:43 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po Lu> Oh, thanks!
>
> Weʼre not quite there yet :-)
>
> lisp/emacs-lisp/edebug-tests.log:
>
>   locate-file-internal(nil ("/home/rpluim/repos/emacs-android/lisp" "/
>   locate-file(nil ("/home/rpluim/repos/emacs-android/lisp" "/home/rplu
>   file-loadhist-lookup(nil)
>   file-provides(nil)
>   file-dependents(nil)
>   unload-feature(edebug-test-code)
>   #f(compiled-function () #<bytecode 0x5f78608ef6e90e6>)()
>   #f(compiled-function () #<bytecode 0x168e67691f944ca4>)()
>   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>   ert-run-test(#s(ert-test :name edebug-tests-trivial-backquote :docum
>   ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
>   ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
>   ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
>   ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
>   eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
>   command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/emacs-lisp/edebug-te
>   command-line()
>   normal-top-level()
> Test edebug-tests-trivial-backquote condition:
>     (wrong-type-argument stringp nil)
>
> which looks like an issue with load-history not containing an entry
> for the `edebug-test-code' feature? No idea yet for that
>
> And also:
>
> lisp/progmodes/eglot-tests.log:
>
> Test eglot-test-diagnostic-tags-unnecessary-code backtrace:
>   signal(ert-test-failed (((should (eq 'eglot-diagnostic-tag-unnecessa
>   ert-fail(((should (eq 'eglot-diagnostic-tag-unnecessary-face (face-a
>   #f(compiled-function () #<bytecode 0x11898351aa04629a>)()
>   eglot--call-with-fixture((("diag-project" ("main.cpp" . "int main(){
>   #f(compiled-function () #<bytecode -0x25d7fd8d71f4ef>)()
>   ert--run-test-internal(#s(ert--test-execution-info :test #s(ert-test
>   ert-run-test(#s(ert-test :name eglot-test-diagnostic-tags-unnecessar
>   ert-run-or-rerun-test(#s(ert--stats :selector ... :tests ... :test-m
>   ert-run-tests((not (or (tag :expensive-test) (tag :unstable) (tag :n
>   ert-run-tests-batch((not (or (tag :expensive-test) (tag :unstable) (
>   ert-run-tests-batch-and-exit((not (or (tag :expensive-test) (tag :un
>   eval((ert-run-tests-batch-and-exit '(not (or (tag :expensive-test) (
>   command-line-1(("-L" ":." "-l" "ert" "-l" "lisp/progmodes/eglot-test
>   command-line()
>   normal-top-level()
> Test eglot-test-diagnostic-tags-unnecessary-code condition:
>     (ert-test-failed
>      ((should
>        (eq 'eglot-diagnostic-tag-unnecessary-face
> 	    (face-at-point)))
>       :form
>       (eq eglot-diagnostic-tag-unnecessary-face flymake-warning)
>       :value nil))
>
> which I havenʼt investigated yet.
>
> Robert

I think I know why this is: I tried to fix a bug in the code which set
Vload_history when an undumped libemacs.so is run.

I'll look into this, thanks.

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Wed, 15 Mar 2023 18:25:43 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po Lu> Oh, thanks!
>
> Weʼre not quite there yet :-)

I think I've just fixed this, thanks.

Re: Merging feature/android

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Corwin Brust <corwin@bru.st> writes:
>
>> On Wed, Mar 15, 2023 at 1:03 AM Corwin Brust <corwin@bru.st> wrote:
>>>
>>> No errors now, building feature/android with MSYS2/MINGW64.
>>
>> I think I "spoke" too soon. I didn't notice these immediately as they
>> didn't interrupt the build, but I do the below in thee output from
>> make.  Everything after this looks normal.
>>
>>   CCLD     runemacs.exe
>> tmp=etc/emacsclient.tmpdesktop; rm -f ${tmp}; \
>> client_name=`echo emacsclient | sed 's,x,x,'`.exe; \
>> sed -e "/^Exec=/
>> s|emacsclient|/d/emacs-build/install/emacs-feature_android/bin/${client_name}|"
>> \
>>   -e "/^Icon=emacs/ s/emacs/`echo emacs | sed 's,x,x,'`/" \
>>   -e "/^StartupNotify=true$/d" \
>>   ./etc/emacsclient.desktop > ${tmp}; \
>> /usr/bin/install -c -m 644 ${tmp}
>> "/d/emacs-build/install/emacs-feature_android/share/applications/${client_name}.desktop";
>> \
>> rm -f ${tmp}
>>   CC       fingerprint.o
>>   CC       acl_entries.o
>>   CC       asnprintf.o
>>   CC       asprintf.o
>>   CC       frexp.o
>>   CC       memmem.o
>>   CC       mktime.o
>>   CC       printf.o
>>   CC       printf-args.o
>> asprintf.c:30:1: error: redefinition of 'asprintf'
>>    30 | asprintf (char **resultp, const char *format, ...)
>>       | ^~~~~~~~
>> In file included from
>> C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
>>                  from ../src/conf_post.h:38,
>>                  from ../src/config.h:3470,
>>                  from asprintf.c:18:
>> C:/msys2/mingw64/include/stdio.h:265:5: note: previous definition of
>> 'asprintf' with type 'int(char **, const char *, ...)'
>>   265 | int asprintf(char **__ret, const char *__format, ...)
>>       |     ^~~~~~~~
>>   CC       printf-parse.o
>>   CC       vasnprintf.o
>> make[1]: *** [Makefile:102: asprintf.o] Error 1
>> make[1]: *** Waiting for unfinished jobs....
>> printf.c:30:1: error: redefinition of 'printf'
>>    30 | printf (const char *format, ...)
>>       | ^~~~~~
>> In file included from
>> C:/Users/corwi/emacs-build/git/android/nt/inc/ms-w32.h:389,
>>                  from ../src/conf_post.h:38,
>>                  from ../src/config.h:3470,
>>                  from printf.c:18:
>> C:/msys2/mingw64/include/stdio.h:368:5: note: previous definition of
>> 'printf' with type 'int(const char *, ...)'
>>   368 | int printf (const char *__format, ...)
>>       |     ^~~~~~
>> make[1]: *** [Makefile:102: printf.o] Error 1
>
> Thanks, but now I don't know why vasnprintf.o and printf-args.o are being
> built, given that:
>
>   OMIT_GNULIB_MODULE_vasnprintf
>
> is set to true in nt/gnulib-cfg.mk, and all of the configury and gnulib
> stuff has been regenerated.  Any ideas?  Could you perhaps try to
> determine that?
>
> Thanks.

Or alternatively please pull and try again, I think I've fixed this now.

Re: Merging feature/android

[Arash Esbati]

Po Lu <luangruo@yahoo.com> writes:

> Or alternatively please pull and try again, I think I've fixed this now.

Yes, it seems to work; I'm actually writing this message with it.

I also see some warnings during make (the ones related to Org excluded):

  In end of data:
  frame.el:2142:8: Warning: the function `android-detect-mouse' is not
  known to be defined.

  In end of data:
  loaddefs.el:3789:55: Warning: the function `c-list-of-strings' is not
  known to be defined.

  In end of data:
  term/android-win.el:54:4: Warning: the function `android-get-connection'
  is not known to be defined.

  In end of data:
  touch-screen.el:113:14: Warning: the function
  `pixel-scroll-precision-scroll-up-page' is not known to be defined.
  touch-screen.el:112:16: Warning: the function
  `pixel-scroll-precision-scroll-down-page' is not known to be defined.

Not sure if 2 and 4 are related to the port though.

Best, Arash

Re: Merging feature/android

[Po Lu]

Arash Esbati <arash@gnu.org> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> Or alternatively please pull and try again, I think I've fixed this now.
>
> Yes, it seems to work; I'm actually writing this message with it.
>
> I also see some warnings during make (the ones related to Org excluded):
>
>   In end of data:
>   frame.el:2142:8: Warning: the function `android-detect-mouse' is not
>   known to be defined.
>
>   In end of data:
>   loaddefs.el:3789:55: Warning: the function `c-list-of-strings' is not
>   known to be defined.
>
>   In end of data:
>   term/android-win.el:54:4: Warning: the function `android-get-connection'
>   is not known to be defined.
>
>   In end of data:
>   touch-screen.el:113:14: Warning: the function
>   `pixel-scroll-precision-scroll-up-page' is not known to be defined.
>   touch-screen.el:112:16: Warning: the function
>   `pixel-scroll-precision-scroll-down-page' is not known to be defined.
>
> Not sure if 2 and 4 are related to the port though.
>
> Best, Arash

Thanks.  I will fix 1 and 3; 2 is a bug in CC Mode and 4 is because
loaddefs.el has not been updated on the branch (as it would cause
annoying merge conflicts.)

Re: Merging feature/android

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: Corwin Brust <corwin@bru.st>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org
> Date: Thu, 16 Mar 2023 09:52:00 +0100
> 
>   In end of data:
>   loaddefs.el:3789:55: Warning: the function `c-list-of-strings' is not
>   known to be defined.

This is unrelated to the feature branch, I see that on master as well,
and asked Alan to look into that.

Re: Merging feature/android

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> This is unrelated to the feature branch, I see that on master as well,
> and asked Alan to look into that.

Thanks.  Another observation: When I run make on master, I see these
warnings:

  CC       atimer.o
  CC       doprnt.o
In file included from process.c:33:
In function 'SDATA',
    inlined from 'SSDATA' at lisp.h:1677:19,
    inlined from 'create_process' at process.c:2254:40,
    inlined from 'Fmake_process' at process.c:2059:7:
lisp.h:1671:31: warning: array subscript 0 is outside array bounds of 'char[]' [-Warray-bounds]
 1671 |   return XSTRING (string)->u.s.data;
      |          ~~~~~~~~~~~~~~~~~~~~~^~~~~
lisp.h:1671:31: warning: null pointer dereference [-Wnull-dereference]
  CC       intervals.o

and

  CC       w32inevt.o
  CC       w32proc.o
w32heap.c: In function 'getrlimit':
w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
  853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
      |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from C:/msys64/mingw64/include/winbase.h:25,
                 from C:/msys64/mingw64/include/windows.h:70,
                 from w32common.h:24,
                 from w32heap.c:54:
C:/msys64/mingw64/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
   45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
      |                            ^~~~~~~~~~~~
w32heap.c:844:34: note: 'm' declared here
  844 |         MEMORY_BASIC_INFORMATION m;
      |                                  ^
  CC       w32image.o

Running make on the feature branch, I don't get them.  Does it mean that
Po Lu has kindly fixed them?

Best, Arash

Re: Merging feature/android

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: luangruo@yahoo.com,  corwin@bru.st,  emacs-devel@gnu.org
> Date: Thu, 16 Mar 2023 12:59:06 +0100
> 
> Thanks.  Another observation: When I run make on master, I see these
> warnings:
> 
>   CC       atimer.o
>   CC       doprnt.o
> In file included from process.c:33:
> In function 'SDATA',
>     inlined from 'SSDATA' at lisp.h:1677:19,
>     inlined from 'create_process' at process.c:2254:40,
>     inlined from 'Fmake_process' at process.c:2059:7:
> lisp.h:1671:31: warning: array subscript 0 is outside array bounds of 'char[]' [-Warray-bounds]
>  1671 |   return XSTRING (string)->u.s.data;
>       |          ~~~~~~~~~~~~~~~~~~~~~^~~~~
> lisp.h:1671:31: warning: null pointer dereference [-Wnull-dereference]
>   CC       intervals.o
> 
> and
> 
>   CC       w32inevt.o
>   CC       w32proc.o
> w32heap.c: In function 'getrlimit':
> w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
>   853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> In file included from C:/msys64/mingw64/include/winbase.h:25,
>                  from C:/msys64/mingw64/include/windows.h:70,
>                  from w32common.h:24,
>                  from w32heap.c:54:
> C:/msys64/mingw64/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
>    45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
>       |                            ^~~~~~~~~~~~
> w32heap.c:844:34: note: 'm' declared here
>   844 |         MEMORY_BASIC_INFORMATION m;
>       |                                  ^
>   CC       w32image.o

Those are bogus warnings (a.k.a. bugs/misfeatures of the GCC version
you are using).  Disregard them.

> Running make on the feature branch, I don't get them.  Does it mean that
> Po Lu has kindly fixed them?

I doubt that.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arash Esbati <arash@gnu.org>
>> Cc: luangruo@yahoo.com,  corwin@bru.st,  emacs-devel@gnu.org
>> Date: Thu, 16 Mar 2023 12:59:06 +0100
>> 
>> Thanks.  Another observation: When I run make on master, I see these
>> warnings:
>> 
>>   CC       atimer.o
>>   CC       doprnt.o
>> In file included from process.c:33:
>> In function 'SDATA',
>>     inlined from 'SSDATA' at lisp.h:1677:19,
>>     inlined from 'create_process' at process.c:2254:40,
>>     inlined from 'Fmake_process' at process.c:2059:7:
>> lisp.h:1671:31: warning: array subscript 0 is outside array bounds of 'char[]' [-Warray-bounds]
>>  1671 |   return XSTRING (string)->u.s.data;
>>       |          ~~~~~~~~~~~~~~~~~~~~~^~~~~
>> lisp.h:1671:31: warning: null pointer dereference [-Wnull-dereference]
>>   CC       intervals.o
>> 
>> and
>> 
>>   CC       w32inevt.o
>>   CC       w32proc.o
>> w32heap.c: In function 'getrlimit':
>> w32heap.c:853:14: warning: 'm' may be used uninitialized [-Wmaybe-uninitialized]
>>   853 |         if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))
>>       |              ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> In file included from C:/msys64/mingw64/include/winbase.h:25,
>>                  from C:/msys64/mingw64/include/windows.h:70,
>>                  from w32common.h:24,
>>                  from w32heap.c:54:
>> C:/msys64/mingw64/include/memoryapi.h:45:28: note: by argument 1 of type 'LPCVOID' {aka 'const void *'} to 'VirtualQuery' declared here
>>    45 |   WINBASEAPI SIZE_T WINAPI VirtualQuery (LPCVOID lpAddress, PMEMORY_BASIC_INFORMATION lpBuffer, SIZE_T dwLength);
>>       |                            ^~~~~~~~~~~~
>> w32heap.c:844:34: note: 'm' declared here
>>   844 |         MEMORY_BASIC_INFORMATION m;
>>       |                                  ^
>>   CC       w32image.o
>
> Those are bogus warnings (a.k.a. bugs/misfeatures of the GCC version
> you are using).  Disregard them.
>
>> Running make on the feature branch, I don't get them.  Does it mean that
>> Po Lu has kindly fixed them?
>
> I doubt that.

I did not.  Perhaps something in gnulib has caused this change?

Re: Merging feature/android

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

>> From: Arash Esbati <arash@gnu.org>
>> Cc: luangruo@yahoo.com,  corwin@bru.st,  emacs-devel@gnu.org
>> Date: Thu, 16 Mar 2023 12:59:06 +0100
>
> Those are bogus warnings (a.k.a. bugs/misfeatures of the GCC version
> you are using).  Disregard them.

Ah, that rings a bell.  I remember that you said that once for the

  if (!VirtualQuery ((LPCVOID) &m, &m, sizeof m))

stuff, I wasn't sure about the other one.

>> Running make on the feature branch, I don't get them.  Does it mean that
>> Po Lu has kindly fixed them?
>
> I doubt that.

So the question is why it happens there and not here?

Best, Arash

Re: Merging feature/android

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: luangruo@yahoo.com,  corwin@bru.st,  emacs-devel@gnu.org
> Date: Thu, 16 Mar 2023 13:13:13 +0100
> 
> >> Running make on the feature branch, I don't get them.  Does it mean that
> >> Po Lu has kindly fixed them?
> >
> > I doubt that.
> 
> So the question is why it happens there and not here?

Are the same compiler warning flags used on both branches?

Re: Merging feature/android

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> Are the same compiler warning flags used on both branches?

I'd say so.  In both cases, I just did:

$ git clean -fdx
$ ./autogen.sh
$ ./configure --without-native-compilation --without-pop
$ make

Best, Arash

Re: Merging feature/android

[Eli Zaretskii]

> From: Arash Esbati <arash@gnu.org>
> Cc: luangruo@yahoo.com,  corwin@bru.st,  emacs-devel@gnu.org
> Date: Thu, 16 Mar 2023 18:09:13 +0100
> 
> Eli Zaretskii <eliz@gnu.org> writes:
> 
> > Are the same compiler warning flags used on both branches?
> 
> I'd say so.  In both cases, I just did:
> 
> $ git clean -fdx
> $ ./autogen.sh
> $ ./configure --without-native-compilation --without-pop
> $ make

That doesn't necessarily prove they are the same.  Please compare the
values of WARN_CFLAGS in src/Makefile between the two branches.

Re: Merging feature/android

[Robert Pluim]

>>>>> On Thu, 16 Mar 2023 09:42:22 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Robert Pluim <rpluim@gmail.com> writes:
    >>>>>>> On Wed, 15 Mar 2023 18:25:43 +0800, Po Lu <luangruo@yahoo.com> said:
    >> 
    >> Po Lu> Oh, thanks!
    >> 
    >> Weʼre not quite there yet :-)

    Po Lu> I think I've just fixed this, thanks.

The edebug tests pass now, but eglot is still failing

This is with

$ clangd --version
Debian clangd version 11.0.1-2

Let me know if I should try with a different LSP server.

Thanks

Robert
-- 

Re: Merging feature/android

[Po Lu]

Robert Pluim <rpluim@gmail.com> writes:

>>>>>> On Thu, 16 Mar 2023 09:42:22 +0800, Po Lu <luangruo@yahoo.com> said:
>
>     Po Lu> Robert Pluim <rpluim@gmail.com> writes:
>     >>>>>>> On Wed, 15 Mar 2023 18:25:43 +0800, Po Lu <luangruo@yahoo.com> said:
>     >> 
>     >> Po Lu> Oh, thanks!
>     >> 
>     >> Weʼre not quite there yet :-)
>
>     Po Lu> I think I've just fixed this, thanks.
>
> The edebug tests pass now, but eglot is still failing
>
> This is with
>
> $ clangd --version
> Debian clangd version 11.0.1-2
>
> Let me know if I should try with a different LSP server.
>
> Thanks
>
> Robert

I can't reproduce this sorry, not on feature/android nor master.  But
that's because:

  ELC      lisp/progmodes/eglot-tests.elc
  GEN      lisp/progmodes/eglot-tests.log
Running 50 tests (2023-03-17 16:18:18+0800, selector `(not (or (tag :unstable) (tag :nativecomp)))')
[eglot-tests] [eglot-test-auto-detect-running-server]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-auto-detect-running-server]: OK
[eglot] Asking EGLOT (project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
../src/emacs: writing to child signal FD: Bad file descriptor
[eglot-tests] Killing (cena.c coiso.c merdix.c), wiping /tmp/eglot--fixtureLcjmzK, restoring nil
   passed   1/50  eglot-test-auto-detect-running-server (0.259419 sec)
[eglot-tests] [eglot-test-auto-reconnect]: test start
make[1]: *** [Makefile:174: lisp/progmodes/eglot-tests.log] Aborted (core dumped)

Re: Merging feature/android

[Arash Esbati]

Eli Zaretskii <eliz@gnu.org> writes:

> That doesn't necessarily prove they are the same.  Please compare the
> values of WARN_CFLAGS in src/Makefile between the two branches.

Thanks.  This is from master (line breaks added manually):

  WARN_CFLAGS = -fno-common -Wall -Warith-conversion -Wdate-time
  -Wdisabled-optimization -Wdouble-promotion -Wduplicated-cond -Wextra
  -Wformat-signedness -Winit-self -Winvalid-pch -Wlogical-op
  -Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes
  -Wnested-externs -Wnull-dereference -Wold-style-definition -Wopenmp-simd
  -Wpacked -Wpointer-arith -Wstrict-prototypes
  -Wsuggest-attribute=noreturn -Wsuggest-final-methods
  -Wsuggest-final-types -Wtrampolines -Wuninitialized -Wunknown-pragmas
  -Wunused-macros -Wvariadic-macros -Wvector-operation-performance
  -Wwrite-strings -Warray-bounds=2 -Wattribute-alias=2 -Wformat=2
  -Wformat-truncation=2 -Wimplicit-fallthrough=5 -Wshift-overflow=2
  -Wuse-after-free=3 -Wvla-larger-than=4031
  -Wno-missing-field-initializers -Wno-override-init -Wno-sign-compare
  -Wno-type-limits -Wno-unused-parameter -Wno-format-nonliteral
  -Wno-bidi-chars -Wno-pointer-sign

This is from feature/android:

  WARN_CFLAGS =

And while looking at the diff:

master uses the -isystem flag, feature branch uses -I, e.g.:

  WEBP_CFLAGS= -isystem C:/msys64/mingw64/include/webp  (master)
  WEBP_CFLAGS= -IC:/msys64/mingw64/include/webp         (feature/android)

Best, Arash

Re: Merging feature/android

[Po Lu]

Arash Esbati <arash@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> That doesn't necessarily prove they are the same.  Please compare the
>> values of WARN_CFLAGS in src/Makefile between the two branches.
>
> Thanks.  This is from master (line breaks added manually):
>
>   WARN_CFLAGS = -fno-common -Wall -Warith-conversion -Wdate-time
>   -Wdisabled-optimization -Wdouble-promotion -Wduplicated-cond -Wextra
>   -Wformat-signedness -Winit-self -Winvalid-pch -Wlogical-op
>   -Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes
>   -Wnested-externs -Wnull-dereference -Wold-style-definition -Wopenmp-simd
>   -Wpacked -Wpointer-arith -Wstrict-prototypes
>   -Wsuggest-attribute=noreturn -Wsuggest-final-methods
>   -Wsuggest-final-types -Wtrampolines -Wuninitialized -Wunknown-pragmas
>   -Wunused-macros -Wvariadic-macros -Wvector-operation-performance
>   -Wwrite-strings -Warray-bounds=2 -Wattribute-alias=2 -Wformat=2
>   -Wformat-truncation=2 -Wimplicit-fallthrough=5 -Wshift-overflow=2
>   -Wuse-after-free=3 -Wvla-larger-than=4031
>   -Wno-missing-field-initializers -Wno-override-init -Wno-sign-compare
>   -Wno-type-limits -Wno-unused-parameter -Wno-format-nonliteral
>   -Wno-bidi-chars -Wno-pointer-sign
>
> This is from feature/android:
>
>   WARN_CFLAGS =

That's odd, I will look into this.  Thanks.

> And while looking at the diff:
>
> master uses the -isystem flag, feature branch uses -I, e.g.:
>
>   WEBP_CFLAGS= -isystem C:/msys64/mingw64/include/webp  (master)
>   WEBP_CFLAGS= -IC:/msys64/mingw64/include/webp         (feature/android)
>
> Best, Arash

I don't understand this change, since the configury used outside Android
didn't change.

Re: Merging feature/android

[Po Lu]

Arash Esbati <arash@gnu.org> writes:

> Eli Zaretskii <eliz@gnu.org> writes:
>
>> That doesn't necessarily prove they are the same.  Please compare the
>> values of WARN_CFLAGS in src/Makefile between the two branches.
>
> Thanks.  This is from master (line breaks added manually):
>
>   WARN_CFLAGS = -fno-common -Wall -Warith-conversion -Wdate-time
>   -Wdisabled-optimization -Wdouble-promotion -Wduplicated-cond -Wextra
>   -Wformat-signedness -Winit-self -Winvalid-pch -Wlogical-op
>   -Wmissing-declarations -Wmissing-include-dirs -Wmissing-prototypes
>   -Wnested-externs -Wnull-dereference -Wold-style-definition -Wopenmp-simd
>   -Wpacked -Wpointer-arith -Wstrict-prototypes
>   -Wsuggest-attribute=noreturn -Wsuggest-final-methods
>   -Wsuggest-final-types -Wtrampolines -Wuninitialized -Wunknown-pragmas
>   -Wunused-macros -Wvariadic-macros -Wvector-operation-performance
>   -Wwrite-strings -Warray-bounds=2 -Wattribute-alias=2 -Wformat=2
>   -Wformat-truncation=2 -Wimplicit-fallthrough=5 -Wshift-overflow=2
>   -Wuse-after-free=3 -Wvla-larger-than=4031
>   -Wno-missing-field-initializers -Wno-override-init -Wno-sign-compare
>   -Wno-type-limits -Wno-unused-parameter -Wno-format-nonliteral
>   -Wno-bidi-chars -Wno-pointer-sign
>
> This is from feature/android:
>
>   WARN_CFLAGS =
>
> And while looking at the diff:
>
> master uses the -isystem flag, feature branch uses -I, e.g.:
>
>   WEBP_CFLAGS= -isystem C:/msys64/mingw64/include/webp  (master)
>   WEBP_CFLAGS= -IC:/msys64/mingw64/include/webp         (feature/android)
>
> Best, Arash

I think I've fixed this now.  Would you please test on MinGW to make
sure?

Re: Merging feature/android

[Arash Esbati]

Po Lu <luangruo@yahoo.com> writes:

> I think I've fixed this now.  Would you please test on MinGW to make
> sure?

Thanks.  Yes, it seems to be fixed, WARN_CFLAGS is not empty now.

OTOH, I see 2 warnings which I don't get when compiling master:

  CC       casetab.o
fileio.c:131: warning: macro "emacs_fd_to_int" is not used [-Wunused-macros]
  131 | #define emacs_fd_to_int(fds)    (fds)
      |
  CC       casefiddle.o

and

  CC       lastfile.o
image.c: In function 'image_create_bitmap_from_data':
image.c:499:1: warning: function might be candidate for attribute 'noreturn' [-Wsuggest-attribute=noreturn]
  499 | image_create_bitmap_from_data (struct frame *f, char *bits,
      | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  CCLD     temacs.exe


HTH.  Best, Arash

Re: Merging feature/android

[Po Lu]

Arash Esbati <arash@gnu.org> writes:

> Po Lu <luangruo@yahoo.com> writes:
>
>> I think I've fixed this now.  Would you please test on MinGW to make
>> sure?
>
> Thanks.  Yes, it seems to be fixed, WARN_CFLAGS is not empty now.
>
> OTOH, I see 2 warnings which I don't get when compiling master:
>
>   CC       casetab.o
> fileio.c:131: warning: macro "emacs_fd_to_int" is not used [-Wunused-macros]
>   131 | #define emacs_fd_to_int(fds)    (fds)
>       |
>   CC       casefiddle.o
>
> and
>
>   CC       lastfile.o
> image.c: In function 'image_create_bitmap_from_data':
> image.c:499:1: warning: function might be candidate for attribute 'noreturn' [-Wsuggest-attribute=noreturn]
>   499 | image_create_bitmap_from_data (struct frame *f, char *bits,
>       | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>   CCLD     temacs.exe
>
>
> HTH.  Best, Arash

Thanks, both ought to be fixed now.

Re: Merging feature/android

[Arash Esbati]

Po Lu <luangruo@yahoo.com> writes:

> Thanks, both ought to be fixed now.

Confirmed; I see only the 2 warnings I also get with master.  Thanks for
your work on this feature.

Best, Arash

Re: Merging feature/android

[Corwin Brust]

On Fri, Mar 17, 2023 at 8:45 AM Arash Esbati <arash@gnu.org> wrote:
>
> Po Lu <luangruo@yahoo.com> writes:
>
> > Thanks, both ought to be fixed now.
>
> Confirmed; I see only the 2 warnings I also get with master.  Thanks for
> your work on this feature.

I'm also able to build feature/android now on MSYS2/MINGW64 without
error.  MINGW32 build is running now; no issues so far ;)

Re: Merging feature/android

[Po Lu]

Po Lu <luangruo@yahoo.com> writes:

> Robert Pluim <rpluim@gmail.com> writes:
>
>>>>>>> On Thu, 16 Mar 2023 09:42:22 +0800, Po Lu <luangruo@yahoo.com> said:
>>
>>     Po Lu> Robert Pluim <rpluim@gmail.com> writes:
>>     >>>>>>> On Wed, 15 Mar 2023 18:25:43 +0800, Po Lu <luangruo@yahoo.com> said:
>>     >> 
>>     >> Po Lu> Oh, thanks!
>>     >> 
>>     >> Weʼre not quite there yet :-)
>>
>>     Po Lu> I think I've just fixed this, thanks.
>>
>> The edebug tests pass now, but eglot is still failing
>>
>> This is with
>>
>> $ clangd --version
>> Debian clangd version 11.0.1-2
>>
>> Let me know if I should try with a different LSP server.
>>
>> Thanks
>>
>> Robert
>
> I can't reproduce this sorry, not on feature/android nor master.  But
> that's because:
>
>   ELC      lisp/progmodes/eglot-tests.elc
>   GEN      lisp/progmodes/eglot-tests.log
> Running 50 tests (2023-03-17 16:18:18+0800, selector `(not (or (tag :unstable) (tag :nativecomp)))')
> [eglot-tests] [eglot-test-auto-detect-running-server]: test start
> [eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
> [eglot-tests] [eglot-test-auto-detect-running-server]: OK
> [eglot] Asking EGLOT (project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
> [jsonrpc] Server exited with status 9
> ../src/emacs: writing to child signal FD: Bad file descriptor
> [eglot-tests] Killing (cena.c coiso.c merdix.c), wiping /tmp/eglot--fixtureLcjmzK, restoring nil
>    passed   1/50  eglot-test-auto-detect-running-server (0.259419 sec)
> [eglot-tests] [eglot-test-auto-reconnect]: test start
> make[1]: *** [Makefile:174: lisp/progmodes/eglot-tests.log] Aborted (core dumped)

Ping!

Do you see the clangd problem on master as well?
I have a feeling this is some problem with clangd, but it reliably
crashes for me while running this test.

This is:

  clangd version 15.0.0 (Fedora 15.0.0-3.fc37)

Re: Merging feature/android

[Robert Pluim]

>>>>> On Mon, 20 Mar 2023 19:09:34 +0800, Po Lu <luangruo@yahoo.com> said:

    Po Lu> Do you see the clangd problem on master as well?

I do now, although I could have sworn it wasnʼt failing earlier.

    Po Lu> I have a feeling this is some problem with clangd, but it reliably
    Po Lu> crashes for me while running this test.

    Po Lu> This is:

    Po Lu>   clangd version 15.0.0 (Fedora 15.0.0-3.fc37)

Debian clangd version 11.0.1-2

here. Looks like itʼs an issue on master, I see if I can bisect.

Robert
-- 

Re: Merging feature/android

[Robert Pluim]

>>>>> On Mon, 20 Mar 2023 12:23:27 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Mon, 20 Mar 2023 19:09:34 +0800, Po Lu <luangruo@yahoo.com> said:
    Robert>     Po Lu> Do you see the clangd problem on master as well?

    Robert> I do now, although I could have sworn it wasnʼt failing earlier.

    Robert>     Po Lu> I have a feeling this is some problem with clangd, but it reliably
    Robert>     Po Lu> crashes for me while running this test.

    Robert>     Po Lu> This is:

    Robert>     Po Lu>   clangd version 15.0.0 (Fedora 15.0.0-3.fc37)

    Robert> Debian clangd version 11.0.1-2

    Robert> here. Looks like itʼs an issue on master, I see if I can bisect.

OK, so on master originally this test used rust, and was skipped on my
system, but now itʼs using clangd, and failing. João, any ideas?
Should we just add a clangd version test to
`eglot-test-diagnostic-tags-unnecessary-code'?

Robert
-- 

Re: Merging feature/android

[João Távora]

On Mon, Mar 20, 2023 at 2:20 PM Robert Pluim <rpluim@gmail.com> wrote:
>
> >>>>> On Mon, 20 Mar 2023 12:23:27 +0100, Robert Pluim <rpluim@gmail.com> said:
>
> >>>>> On Mon, 20 Mar 2023 19:09:34 +0800, Po Lu <luangruo@yahoo.com> said:
>     Robert>     Po Lu> Do you see the clangd problem on master as well?
>
>     Robert> I do now, although I could have sworn it wasnʼt failing earlier.
>
>     Robert>     Po Lu> I have a feeling this is some problem with clangd, but it reliably
>     Robert>     Po Lu> crashes for me while running this test.
>
>     Robert>     Po Lu> This is:
>
>     Robert>     Po Lu>   clangd version 15.0.0 (Fedora 15.0.0-3.fc37)
>
>     Robert> Debian clangd version 11.0.1-2
>
>     Robert> here. Looks like itʼs an issue on master, I see if I can bisect.
>
> OK, so on master originally this test used rust, and was skipped on my
> system, but now itʼs using clangd, and failing. João, any ideas?
> Should we just add a clangd version test to
> `eglot-test-diagnostic-tags-unnecessary-code'?

Hello,

Took me quite a while of rummaging to figure out what you are
talking about here.  Consider changing the subject line or
adding a bit more context next time.

_If_ I understand correctly, Robert, your run of
eglot-test-diagnostic-tags-unnecessary-code is failing
because of a face mismatch, presumably because only
clangd-11 is installed in your system, but if you install
a more recent clangd it goes away?  If so, yes, I think a
version check is appropriate, yes.

João

Re: Merging feature/android

[Robert Pluim]

>>>>> On Mon, 20 Mar 2023 17:34:22 +0000, João Távora <joaotavora@gmail.com> said:

    João> On Mon, Mar 20, 2023 at 2:20 PM Robert Pluim <rpluim@gmail.com> wrote:
    >> 
    >> >>>>> On Mon, 20 Mar 2023 12:23:27 +0100, Robert Pluim <rpluim@gmail.com> said:
    >> 
    >> >>>>> On Mon, 20 Mar 2023 19:09:34 +0800, Po Lu <luangruo@yahoo.com> said:
    Robert> Po Lu> Do you see the clangd problem on master as well?
    >> 
    Robert> I do now, although I could have sworn it wasnʼt failing earlier.
    >> 
    Robert> Po Lu> I have a feeling this is some problem with clangd, but it reliably
    Robert> Po Lu> crashes for me while running this test.
    >> 
    Robert> Po Lu> This is:
    >> 
    Robert> Po Lu>   clangd version 15.0.0 (Fedora 15.0.0-3.fc37)
    >> 
    Robert> Debian clangd version 11.0.1-2
    >> 
    Robert> here. Looks like itʼs an issue on master, I see if I can bisect.
    >> 
    >> OK, so on master originally this test used rust, and was skipped on my
    >> system, but now itʼs using clangd, and failing. João, any ideas?
    >> Should we just add a clangd version test to
    >> `eglot-test-diagnostic-tags-unnecessary-code'?

    João> Hello,

    João> Took me quite a while of rummaging to figure out what you are
    João> talking about here.  Consider changing the subject line or
    João> adding a bit more context next time.

Sounds like you forgot to do (package-install 'telepathy) ;-)

    João> _If_ I understand correctly, Robert, your run of
    João> eglot-test-diagnostic-tags-unnecessary-code is failing
    João> because of a face mismatch, presumably because only
    João> clangd-11 is installed in your system, but if you install
    João> a more recent clangd it goes away?  If so, yes, I think a
    João> version check is appropriate, yes.

Yes. Version 11 fails, version 17 works, I donʼt know where the cutoff
point is. Which version do you use?

Robert
-- 

eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Tue, 21 Mar 2023 08:48:04 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Mon, 20 Mar 2023 17:34:22 +0000, João Távora <joaotavora@gmail.com> said:

    João> _If_ I understand correctly, Robert, your run of
    João> eglot-test-diagnostic-tags-unnecessary-code is failing
    João> because of a face mismatch, presumably because only
    João> clangd-11 is installed in your system, but if you install
    João> a more recent clangd it goes away?  If so, yes, I think a
    João> version check is appropriate, yes.

    Robert> Yes. Version 11 fails, version 17 works, I donʼt know where the cutoff
    Robert> point is. Which version do you use?

So version 11 fails, version 13 fails, version 14 works, so I guess >=
14 should do the trick.

Robert
-- 

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Tue, 21 Mar 2023 14:08:39 +0100, Robert Pluim <rpluim@gmail.com> said:

>>>>> On Tue, 21 Mar 2023 08:48:04 +0100, Robert Pluim <rpluim@gmail.com> said:
>>>>> On Mon, 20 Mar 2023 17:34:22 +0000, João Távora <joaotavora@gmail.com> said:

    João> _If_ I understand correctly, Robert, your run of
    João> eglot-test-diagnostic-tags-unnecessary-code is failing
    João> because of a face mismatch, presumably because only
    João> clangd-11 is installed in your system, but if you install
    João> a more recent clangd it goes away?  If so, yes, I think a
    João> version check is appropriate, yes.

    Robert> Yes. Version 11 fails, version 17 works, I donʼt know where the cutoff
    Robert> point is. Which version do you use?

    Robert> So version 11 fails, version 13 fails, version 14 works, so I guess >=
    Robert> 14 should do the trick.

João, would this be ok for you for master? (this test is currently
failing on EMBA 😀)

diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
index 7ac26732737..dfe969cf9f6 100644
--- a/test/lisp/progmodes/eglot-tests.el
+++ b/test/lisp/progmodes/eglot-tests.el
@@ -452,6 +452,14 @@ eglot-test-basic-diagnostics
 (ert-deftest eglot-test-diagnostic-tags-unnecessary-code ()
   "Test rendering of diagnostics tagged \"unnecessary\"."
   (skip-unless (executable-find "clangd"))
+  (skip-unless (>=
+                (string-to-number
+                 (string-trim
+                  (replace-regexp-in-string
+                  "\\`.*version \\([0-9]+\\).*"
+                  "\\1"
+                  (shell-command-to-string "clangd --version"))))
+                14))
   (eglot--with-fixture
       `(("diag-project" .
          (("main.cpp" . "int main(){float a = 42.2; return 0;}"))))

Robert
-- 

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 1:49 PM Robert Pluim <rpluim@gmail.com> wrote:
>
> >>>>> On Tue, 21 Mar 2023 14:08:39 +0100, Robert Pluim <rpluim@gmail.com> said:
>
> >>>>> On Tue, 21 Mar 2023 08:48:04 +0100, Robert Pluim <rpluim@gmail.com> said:
> >>>>> On Mon, 20 Mar 2023 17:34:22 +0000, João Távora <joaotavora@gmail.com> said:
>
>     João> _If_ I understand correctly, Robert, your run of
>     João> eglot-test-diagnostic-tags-unnecessary-code is failing
>     João> because of a face mismatch, presumably because only
>     João> clangd-11 is installed in your system, but if you install
>     João> a more recent clangd it goes away?  If so, yes, I think a
>     João> version check is appropriate, yes.
>
>     Robert> Yes. Version 11 fails, version 17 works, I donʼt know where the cutoff
>     Robert> point is. Which version do you use?
>
>     Robert> So version 11 fails, version 13 fails, version 14 works, so I guess >=
>     Robert> 14 should do the trick.
>
> João, would this be ok for you for master? (this test is currently
> failing on EMBA 😀)
>
> diff --git a/test/lisp/progmodes/eglot-tests.el b/test/lisp/progmodes/eglot-tests.el
> index 7ac26732737..dfe969cf9f6 100644
> --- a/test/lisp/progmodes/eglot-tests.el
> +++ b/test/lisp/progmodes/eglot-tests.el
> @@ -452,6 +452,14 @@ eglot-test-basic-diagnostics
>  (ert-deftest eglot-test-diagnostic-tags-unnecessary-code ()
>    "Test rendering of diagnostics tagged \"unnecessary\"."
>    (skip-unless (executable-find "clangd"))
> +  (skip-unless (>=
> +                (string-to-number
> +                 (string-trim
> +                  (replace-regexp-in-string
> +                  "\\`.*version \\([0-9]+\\).*"
> +                  "\\1"
> +                  (shell-command-to-string "clangd --version"))))
> +                14))
>    (eglot--with-fixture
>        `(("diag-project" .
>           (("main.cpp" . "int main(){float a = 42.2; return 0;}"))))

Looks very verbose: why can't we use 'version<='?

And I think I'd rather make some eglot--clangd-version helper.

Another option is to skip this on EMBA completely.  I presume
there's an 'EMBA' environment variable there.  Don't use 'CI',
though, as GitHub actions also sets it and the test passes fine in that
CI server.

Another option is to install a newer clangd on EMBA, as 11 is
really quite old when compared to the newer versions.  Michael,
you installed clangd on EMBA.  What are the options to upgrade
an external program there?

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Tue, 21 Mar 2023 14:07:19 +0000, João Távora <joaotavora@gmail.com> said:

    João> Looks very verbose: why can't we use 'version<='?

For some reason I thought the version<= stuff was only for emacs
versions. Dʼoh

    João> And I think I'd rather make some eglot--clangd-version helper.

Sure

    João> Another option is to skip this on EMBA completely.  I presume
    João> there's an 'EMBA' environment variable there.  Don't use 'CI',
    João> though, as GitHub actions also sets it and the test passes fine in that
    João> CI server.

`EMACS_EMBA_CI' is the environment variable

    João> Another option is to install a newer clangd on EMBA, as 11 is
    João> really quite old when compared to the newer versions.  Michael,
    João> you installed clangd on EMBA.  What are the options to upgrade
    João> an external program there?

My vague memory is that itʼs running Debian stable, so clangd 11 is
the latest available version there.

Robert
-- 

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

Hi João,

> Another option is to skip this on EMBA completely.  I presume
> there's an 'EMBA' environment variable there.  Don't use 'CI',
> though, as GitHub actions also sets it and the test passes fine in that
> CI server.

See test/README, it explains it. You can use the $EMACS_EMBA_CI
environment variable. A common pattern in Emacs tests is

--8<---------------cut here---------------start------------->8---
  :tags (and (getenv "EMACS_EMBA_CI") '(:unstable))
--8<---------------cut here---------------end--------------->8---

> Another option is to install a newer clangd on EMBA, as 11 is
> really quite old when compared to the newer versions.  Michael,
> you installed clangd on EMBA.  What are the options to upgrade
> an external program there?

As Robert has said the other message, we run Debian bullseye on
EMBA. The clangd installation is spefified in file
test/infra/Dockerfile.emba:

--8<---------------cut here---------------start------------->8---
FROM debian:bullseye as emacs-base

RUN apt-get update && \
    apt-get install -y --no-install-recommends -o=Dpkg::Use-Pty=0 \
      libc-dev gcc g++ make autoconf automake libncurses-dev gnutls-dev \
      libdbus-1-dev libacl1-dev acl git texinfo gdb \
    && rm -rf /var/lib/apt/lists/*

FROM emacs-base as emacs-inotify

# We install clangd for Eglot tests.
RUN apt-get update && \
    apt-get install -y --no-install-recommends -o=Dpkg::Use-Pty=0 \
      inotify-tools clangd \
    && rm -rf /var/lib/apt/lists/*
...
--8<---------------cut here---------------end--------------->8---

If you have something else for installation of clangd: I'm all ears.

> João

Best regards, Michael.

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 2:56 PM Michael Albinus <michael.albinus@gmx.de> wrote:

>
> If you have something else for installation of clangd: I'm all ears.

I don't but Google does.  I haven't used Debian in a while, but
I remember you can add sources to your /etc/apt/source.list to
install newer versions of some packages.

Here are some examples:

  https://stackoverflow.com/questions/66223241/how-to-install-clang-11-on-debian

Version 14 is available here in "sid"

  https://packages.debian.org/sid/clangd

Finally there is this, which purports to host very new clangd for Debian

    https://apt.llvm.org/

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

Hi João,

>> If you have something else for installation of clangd: I'm all ears.
>
> I don't but Google does.  I haven't used Debian in a while, but
> I remember you can add sources to your /etc/apt/source.list to
> install newer versions of some packages.
>
> Here are some examples:
>
>   https://stackoverflow.com/questions/66223241/how-to-install-clang-11-on-debian
>
> Version 14 is available here in "sid"
>
>   https://packages.debian.org/sid/clangd
>
> Finally there is this, which purports to host very new clangd for Debian
>
>     https://apt.llvm.org/

All these recipes mean that we install clangd + a number of unknown
dependencies, which brings us near to Debian sid. I like to avoid this,
Debian on EMBA shall use a conservative software selection.

Even the recent upgrade from Debian stretch to Debian bullseye on EMBA
has caused trouble, see for example bug#62210 or bug#62211.

So perhaps it is better you mark the failing test as :unstable on EMBA,
until we have upgraded to a suitable Debian release.

> João

Best regards, Michael.

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 3:34 PM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> Hi João,
>
> >> If you have something else for installation of clangd: I'm all ears.
> >
> > I don't but Google does.  I haven't used Debian in a while, but
> > I remember you can add sources to your /etc/apt/source.list to
> > install newer versions of some packages.
> >
> > Here are some examples:
> >
> >   https://stackoverflow.com/questions/66223241/how-to-install-clang-11-on-debian
> >
> > Version 14 is available here in "sid"
> >
> >   https://packages.debian.org/sid/clangd
> >
> > Finally there is this, which purports to host very new clangd for Debian
> >
> >     https://apt.llvm.org/
>
> All these recipes mean that we install clangd + a number of unknown
> dependencies, which brings us near to Debian sid. I like to avoid this,
> Debian on EMBA shall use a conservative software selection.

I would characterize the dependencies installed as "unknown".
Since this a dockerfile change we're talking about, it's should
pretty easy to check what was effectively added and revert
if problems arise.

> Even the recent upgrade from Debian stretch to Debian bullseye on EMBA
> has caused trouble, see for example bug#62210 or bug#62211.

I'm not suggesting a full system upgrade.

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 3:38 PM João Távora <joaotavora@gmail.com> wrote:

> > All these recipes mean that we install clangd + a number of unknown
> > dependencies, which brings us near to Debian sid. I like to avoid this,
> > Debian on EMBA shall use a conservative software selection.
>
> I would characterize the dependencies installed as "unknown".
   ^^^^^^^

* wouldn't *

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

Hi João,

>> Even the recent upgrade from Debian stretch to Debian bullseye on EMBA
>> has caused trouble, see for example bug#62210 or bug#62211.
>
> I'm not suggesting a full system upgrade.

Perhaps we can move eglot-tests to a different (Debian) environment, say
in the platform-images and platforms stages, or so. Don't know, I'm just
thinking randomly :-)

> João

Best regards, Michael.

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 3:55 PM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> Hi João,
>
> >> Even the recent upgrade from Debian stretch to Debian bullseye on EMBA
> >> has caused trouble, see for example bug#62210 or bug#62211.
> >
> > I'm not suggesting a full system upgrade.
>
> Perhaps we can move eglot-tests to a different (Debian) environment, say
> in the platform-images and platforms stages, or so. Don't know, I'm just
> thinking randomly :-)

I don't understand the EMBA structure at all.  I thought it would be
basically  "make check", but there are all kinds of
subjobs/stages/pipelines forming a CI/CD model I'm not familiar with.
I can't really figure out what is passing as it should or
failing as it shouldn't.

I don't think the test should be marked unstable, as it is not
really that: it's pretty stable.  So let's install Robert's
upcoming patch (using version<= and a helper) and considering upgrading
clangd soon via one of those methods.  It's not very useful to have
a version of clangd in EMBA that doesn't represent very well what
people are using.

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Tue, 21 Mar 2023 16:26:14 +0000, João Távora <joaotavora@gmail.com> said:

    João> I don't think the test should be marked unstable, as it is not
    João> really that: it's pretty stable.  So let's install Robert's
    João> upcoming patch (using version<= and a helper)

I wonʼt get to that today.

    João> and considering upgrading
    João> clangd soon via one of those methods.  It's not very useful to have
    João> a version of clangd in EMBA that doesn't represent very well what
    João> people are using.

I run Debian stable because itʼs, well, stable, and I suspect many
others do as well, so having clangd not be the latest and greatest is
not going to be unusual.

Robert
-- 

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 4:34 PM Robert Pluim <rpluim@gmail.com> wrote:
>
> >>>>> On Tue, 21 Mar 2023 16:26:14 +0000, João Távora <joaotavora@gmail.com> said:
>
>     João> I don't think the test should be marked unstable, as it is not
>     João> really that: it's pretty stable.  So let's install Robert's
>     João> upcoming patch (using version<= and a helper)
>
> I wonʼt get to that today.

I can get to that later, but I have no useful way to test.

>     João> and considering upgrading
>     João> clangd soon via one of those methods.  It's not very useful to have
>     João> a version of clangd in EMBA that doesn't represent very well what
>     João> people are using.
>
> I run Debian stable because itʼs, well, stable, and I suspect many
> others do as well, so having clangd not be the latest and greatest is
> not going to be unusual.

I don't think so.  Clangd is not a web server where stability is
paramount, it's a developer's tool,  so I would suppose a newer
version is what's needed.  My experience with Eglot+clangd users
tells me so overwhelmingly.

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[chad]

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

FWIW, Debian stable has clangd-13 as an option, but not v14 or later:

clangd-13/stable 1:13.0.1-6~deb11u1 amd64
>   Language server that provides IDE-like features to editors


~Chad

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

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Tue, Mar 21, 2023 at 6:27 PM chad <yandros@gmail.com> wrote:
>
> FWIW, Debian stable has clangd-13 as an option, but not v14 or later:
>
>> clangd-13/stable 1:13.0.1-6~deb11u1 amd64
>>   Language server that provides IDE-like features to editors

Thanks, that's close.

I really wonder if installing a newer package of that pulls in
that many more dependencies, but I have no way to check.

Anyway, I now pushed a version-checking patch to master.

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Tue, 21 Mar 2023 18:47:46 +0000, João Távora <joaotavora@gmail.com> said:

    João> On Tue, Mar 21, 2023 at 6:27 PM chad <yandros@gmail.com> wrote:
    >> 
    >> FWIW, Debian stable has clangd-13 as an option, but not v14 or later:
    >> 
    >>> clangd-13/stable 1:13.0.1-6~deb11u1 amd64
    >>> Language server that provides IDE-like features to editors

    João> Thanks, that's close.

    João> I really wonder if installing a newer package of that pulls in
    João> that many more dependencies, but I have no way to check.

# apt install clangd-13
Reading package lists... Done
Building dependency tree... Done
Reading state information... Done
The following additional packages will be installed:
  libclang-common-13-dev libclang-cpp13 libllvm13
The following NEW packages will be installed:
  clangd-13 libclang-common-13-dev libclang-cpp13 libllvm13
0 upgraded, 4 newly installed, 0 to remove and 1 not upgraded.
Need to get 39.0 MB of archives.
After this operation, 249 MB of additional disk space will be
used.
Do you want to continue? [Y/n]

So itʼs not that many packages, but the point is moot because clangd
13 also fails.

    João> Anyway, I now pushed a version-checking patch to master.

Thanks, that test is skipped for me now (aside: is there any way to
get ert to not spew the diagnostic output from commands such as clangd
to the terminal?)

Robert
-- 

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

  On Wed, Mar 22, 2023 at 9:36 AM Robert Pluim <rpluim@gmail.com> wrote:
>
> >>>>> On Tue, 21 Mar 2023 18:47:46 +0000, João Távora <joaotavora@gmail.com> said:
>
>     João> On Tue, Mar 21, 2023 at 6:27 PM chad <yandros@gmail.com> wrote:
>     >>
>     >> FWIW, Debian stable has clangd-13 as an option, but not v14 or later:
>     >>
>     >>> clangd-13/stable 1:13.0.1-6~deb11u1 amd64
>     >>> Language server that provides IDE-like features to editors
>
>     João> Thanks, that's close.
>
>     João> I really wonder if installing a newer package of that pulls in
>     João> that many more dependencies, but I have no way to check.
>
> # apt install clangd-13
> Reading package lists... Done
> Building dependency tree... Done
> Reading state information... Done
> The following additional packages will be installed:
>   libclang-common-13-dev libclang-cpp13 libllvm13
> The following NEW packages will be installed:
>   clangd-13 libclang-common-13-dev libclang-cpp13 libllvm13
> 0 upgraded, 4 newly installed, 0 to remove and 1 not upgraded.
> Need to get 39.0 MB of archives.
> After this operation, 249 MB of additional disk space will be
> used.
> Do you want to continue? [Y/n]
>
> So itʼs not that many packages, but the point is moot because clangd
> 13 also fails.

What I meant rather, was trying to install version 14 from one of
those sources I listed earlier.  Perhaps all that we'll see is that
instead of:

  clangd-13 libclang-common-13-dev libclang-cpp13 libllvm13

we'll see

  clangd-14 libclang-common-14-dev libclang-cpp14 libllvm14

If the former is acceptable so should be the latter, especially
if the installation of the support libraries is motivated by
clangd alone.

>     João> Anyway, I now pushed a version-checking patch to master.
>
> Thanks, that test is skipped for me now (aside: is there any way to
> get ert to not spew the diagnostic output from commands such as clangd
> to the terminal?)

As far as I understand the output you mean, when running `make check`
all this output goes to a separate file.  The output is very useful for
debugging test failures.  But maybe I'm not following: what command
are you running exactly?

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Wed, 22 Mar 2023 09:45:26 +0000, João Távora <joaotavora@gmail.com> said:

    João> What I meant rather, was trying to install version 14 from one of
    João> those sources I listed earlier.  Perhaps all that we'll see is that
    João> instead of:

    João>   clangd-13 libclang-common-13-dev libclang-cpp13 libllvm13

    João> we'll see

    João>   clangd-14 libclang-common-14-dev libclang-cpp14 libllvm14

    João> If the former is acceptable so should be the latter, especially
    João> if the installation of the support libraries is motivated by
    João> clangd alone.

Maybe. Itʼs not on my list of things to try :-)

    João> Anyway, I now pushed a version-checking patch to master.
    >> 
    >> Thanks, that test is skipped for me now (aside: is there any way to
    >> get ert to not spew the diagnostic output from commands such as clangd
    >> to the terminal?)

    João> As far as I understand the output you mean, when running `make check`
    João> all this output goes to a separate file.  The output is very useful for
    João> debugging test failures.  But maybe I'm not following: what command
    João> are you running exactly?

The results go to a log file, but some of it also ends up on the
terminal (this is not specific to eglot, it happens with other tests
as well):

$ make -C test eglot-tests
make: Entering directory '/home/rpluim/repos/emacs/test'
make[1]: Entering directory '/home/rpluim/repos/emacs/test'
  GEN      lisp/progmodes/eglot-tests.log
Running 50 tests (2023-03-22 11:15:47+0100, selector `(not (or (tag :unstable) (tag :nativecomp)))')
[eglot-tests] [eglot-test-auto-detect-running-server]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-auto-detect-running-server]: OK
[eglot] Asking EGLOT (project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (cena.c coiso.c merdix.c), wiping /tmp/eglot--fixtureSVO44D, restoring nil
   passed   1/50  eglot-test-auto-detect-running-server (0.161681 sec)
[eglot-tests] [eglot-test-auto-reconnect]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
[jsonrpc] Server exited with status 9
[eglot] (warning) Reconnecting after unexpected server exit.
Warning (eglot): Reconnecting after unexpected server exit.
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
[eglot] Reconnected!
[jsonrpc] Server exited with status 9
[eglot] (warning) Not auto-reconnecting, last one didn't last long.
Warning (eglot): Not auto-reconnecting, last one didn't last long.
[eglot-tests] [eglot-test-auto-reconnect]: OK
[eglot-tests] Killing (thingy.c), wiping /tmp/eglot--fixture0bo9S2, restoring nil
   passed   2/50  eglot-test-auto-reconnect (2.342542 sec)
[eglot-tests] [eglot-test-auto-shutdown]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
[eglot] Asking EGLOT (project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] [eglot-test-auto-shutdown]: OK
[eglot-tests] Killing nil, wiping /tmp/eglot--fixtureFDN5l3, restoring nil
   passed   3/50  eglot-test-auto-shutdown (0.162224 sec)
  skipped   4/50  eglot-test-basic-completions (0.000283 sec)
[eglot-tests] [eglot-test-basic-diagnostics]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `diag-project'.
[eglot-tests] waiting for `(string= method textDocument/publishDiagnostics)'
.
[eglot-tests] detected: textDocument/publishDiagnostics

[eglot-tests] [eglot-test-basic-diagnostics]: OK
[eglot] Asking EGLOT (diag-project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (main.c), wiping /tmp/eglot--fixtureA3MDfA, restoring nil
   passed   5/50  eglot-test-basic-diagnostics (0.339335 sec)
  skipped   6/50  eglot-test-basic-xref (0.001034 sec)
   passed   7/50  eglot-test-capabilities (0.000371 sec)
   passed   8/50  eglot-test-dcase (0.000354 sec)
   passed   9/50  eglot-test-dcase-issue-452 (0.000252 sec)
  skipped  10/50  eglot-test-diagnostic-tags-unnecessary-code (0.023891 sec)
  skipped  11/50  eglot-test-eclipse-connect (0.000277 sec)
  skipped  12/50  eglot-test-eldoc-after-completions (0.000297 sec)
[eglot-tests] [eglot-test-ensure]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-ensure]: OK
[eglot] Asking EGLOT (project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (foo.c bar.c), wiping /tmp/eglot--fixtureUZO630, restoring (c-mode-hook)
   passed  13/50  eglot-test-ensure (0.242147 sec)
   passed  14/50  eglot-test-glob-test (0.084042 sec)
  skipped  15/50  eglot-test-javascript-basic (0.000492 sec)
  skipped  16/50  eglot-test-json-basic (0.000390 sec)
[eglot-tests] [eglot-test-lsp-abiding-column]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode)' buffers in project `project'.
[eglot-tests] waiting for `(should (equal 71 (cadddr (cadadr (aref (cadddr params) 0)))))'
[eglot-tests] detected: textDocument/didChange
[eglot-tests] [eglot-test-lsp-abiding-column]: OK
[eglot] Asking EGLOT (project/(c-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (foo.c), wiping /tmp/eglot--fixturejFxISB, restoring nil
   passed  17/50  eglot-test-lsp-abiding-column (0.181948 sec)
  skipped  18/50  eglot-test-multiline-eldoc (0.000361 sec)
  skipped  19/50  eglot-test-non-unique-completions (0.000314 sec)
  skipped  20/50  eglot-test-path-to-uri-windows (0.000125 sec)
  skipped  21/50  eglot-test-project-wide-diagnostics-rust-analyzer (0.000276 sec)
  skipped  22/50  eglot-test-project-wide-diagnostics-typescript (0.000297 sec)
  skipped  23/50  eglot-test-python-autopep-formatting (0.000262 sec)
  skipped  24/50  eglot-test-python-yapf-formatting (0.000260 sec)
[eglot-tests] [eglot-test-rename-a-symbol]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `rename-project'.
[eglot] applying 2 edits to `main.c'... 
[eglot] applying 2 edits to `main.c'...done
[eglot] Edit successful!
[eglot-tests] [eglot-test-rename-a-symbol]: OK
[eglot] Asking EGLOT (rename-project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (main.c), wiping /tmp/eglot--fixtureVbF4jY, restoring nil
   passed  25/50  eglot-test-rename-a-symbol (0.153973 sec)
  skipped  26/50  eglot-test-rust-analyzer-hover-after-edit (0.000611 sec)
  skipped  27/50  eglot-test-rust-analyzer-watches-files (0.000545 sec)
  skipped  28/50  eglot-test-rust-on-type-formatting (0.000578 sec)
[eglot-tests] [eglot-test-same-server-multi-mode]: test start
[eglot] Connected! Server `clangd' now managing `(c++-mode c-mode c-ts-mode c++-ts-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-same-server-multi-mode]: OK
[eglot] Asking EGLOT (project/(c++-mode c-mode c-ts-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (foo.cpp foolib.h foolib.c), wiping /tmp/eglot--fixture1WKNsB, restoring nil
   passed  29/50  eglot-test-same-server-multi-mode (0.156214 sec)
   passed  30/50  eglot-test-server-programs-class-name-and-contact-spec (0.001711 sec)
   passed  31/50  eglot-test-server-programs-class-name-and-plist (0.000274 sec)
   passed  32/50  eglot-test-server-programs-executable-multiple-major-modes (0.000292 sec)
   passed  33/50  eglot-test-server-programs-executable-with-arg (0.000279 sec)
   passed  34/50  eglot-test-server-programs-executable-with-args-and-autoport (0.000266 sec)
   passed  35/50  eglot-test-server-programs-function (0.000302 sec)
   passed  36/50  eglot-test-server-programs-guess-lang (0.000503 sec)
   passed  37/50  eglot-test-server-programs-host-and-port (0.000304 sec)
   passed  38/50  eglot-test-server-programs-host-and-port-and-tcp-args (0.000265 sec)
   passed  39/50  eglot-test-server-programs-simple-executable (0.000274 sec)
   passed  40/50  eglot-test-server-programs-simple-missing-executable (0.000285 sec)
  skipped  41/50  eglot-test-single-line-eldoc (0.000343 sec)
[eglot-tests] [eglot-test-slow-async-connection]: test start
[eglot] Waiting in background for server `EGLOT (project/(c-mode))'
[eglot] Connected! Server `clangd' now managing `(c-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-slow-async-connection]: OK
[eglot] Asking EGLOT (project/(c-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (something.c), wiping /tmp/eglot--fixtureGOw9Qx, restoring nil
   passed  42/50  eglot-test-slow-async-connection (2.219000 sec)
[eglot-tests] [eglot-test-slow-sync-connection-intime]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-slow-sync-connection-intime]: OK
[eglot] Asking EGLOT (project/(c-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (something.c), wiping /tmp/eglot--fixturedlRt53, restoring nil
   passed  43/50  eglot-test-slow-sync-connection-intime (1.154543 sec)
[eglot-tests] [eglot-test-slow-sync-connection-wait]: test start
[eglot] Connected! Server `clangd' now managing `(c-mode)' buffers in project `project'.
[eglot-tests] [eglot-test-slow-sync-connection-wait]: OK
[eglot] Asking EGLOT (project/(c-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (something.c), wiping /tmp/eglot--fixtureH37RjM, restoring nil
   passed  44/50  eglot-test-slow-sync-connection-wait (1.160179 sec)
[eglot-tests] [eglot-test-slow-sync-timeout]: test start
[jsonrpc] Server exited with status 9
[eglot-tests] [eglot-test-slow-sync-timeout]: OK
[eglot-tests] Killing (something.c), wiping /tmp/eglot--fixturecLAgnU, restoring nil
   passed  45/50  eglot-test-slow-sync-timeout (1.131108 sec)
  skipped  46/50  eglot-test-snippet-completions (0.000943 sec)
  skipped  47/50  eglot-test-snippet-completions-with-company (0.000599 sec)
   passed  48/50  eglot-test-strict-interfaces (0.000569 sec)
[eglot-tests] [eglot-test-tramp-test]: test start


[eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.




[eglot-tests] [eglot-test-tramp-test]: OK
[eglot] Asking EGLOT (project/(c-mode c-ts-mode c++-mode c++-ts-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (cena.c coiso.c merdix.c), wiping /mock:rltb:/tmp/eglot--fixturezOSKmQ, restoring nil
   passed  49/50  eglot-test-tramp-test (0.632109 sec)
[eglot-tests] [eglot-test-tramp-test-2]: test start


[eglot] Connected! Server `clangd' now managing `(c-mode)' buffers in project `project'.
[eglot-tests] waiting for `(should (equal 71 (cadddr (cadadr (aref (cadddr params) 0)))))'
[eglot-tests] detected: textDocument/didChange
[eglot-tests] [eglot-test-tramp-test-2]: OK
[eglot] Asking EGLOT (project/(c-mode)) politely to terminate
[jsonrpc] Server exited with status 9
[eglot-tests] Killing (foo.c), wiping /mock:rltb:/tmp/eglot--fixture8XZVqO, restoring nil
File is missing: /mock:rltb:/tmp/eglot--fixture8XZVqO/project/foo.c~
   passed  50/50  eglot-test-tramp-test-2 (0.412540 sec)

Ran 50 tests, 30 results as expected, 0 unexpected, 20 skipped (2023-03-22 11:15:57+0100, 10.577235 sec)

20 skipped results:
  SKIPPED  eglot-test-basic-completions
  SKIPPED  eglot-test-basic-xref
  SKIPPED  eglot-test-diagnostic-tags-unnecessary-code
  SKIPPED  eglot-test-eclipse-connect
  SKIPPED  eglot-test-eldoc-after-completions
  SKIPPED  eglot-test-javascript-basic
  SKIPPED  eglot-test-json-basic
  SKIPPED  eglot-test-multiline-eldoc
  SKIPPED  eglot-test-non-unique-completions
  SKIPPED  eglot-test-path-to-uri-windows
  SKIPPED  eglot-test-project-wide-diagnostics-rust-analyzer
  SKIPPED  eglot-test-project-wide-diagnostics-typescript
  SKIPPED  eglot-test-python-autopep-formatting
  SKIPPED  eglot-test-python-yapf-formatting
  SKIPPED  eglot-test-rust-analyzer-hover-after-edit
  SKIPPED  eglot-test-rust-analyzer-watches-files
  SKIPPED  eglot-test-rust-on-type-formatting
  SKIPPED  eglot-test-single-line-eldoc
  SKIPPED  eglot-test-snippet-completions
  SKIPPED  eglot-test-snippet-completions-with-company

make[1]: Leaving directory '/home/rpluim/repos/emacs/test'
make: Leaving directory '/home/rpluim/repos/emacs/test'

Robert
-- 

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Michael Albinus]

João Távora <joaotavora@gmail.com> writes:

Hi João,

> What I meant rather, was trying to install version 14 from one of
> those sources I listed earlier.  Perhaps all that we'll see is that
> instead of:
>
>   clangd-13 libclang-common-13-dev libclang-cpp13 libllvm13
>
> we'll see
>
>   clangd-14 libclang-common-14-dev libclang-cpp14 libllvm14
>
> If the former is acceptable so should be the latter, especially
> if the installation of the support libraries is motivated by
> clangd alone.

That would be OK on EMBA. But the recipe you have shown me was to extend
/etc/apt/sources.list, and to run

--8<---------------cut here---------------start------------->8---
sudo apt update && sudo apt upgrade
--8<---------------cut here---------------end--------------->8---

That would likely touch even more packages.

Well, I'll try to add another container on EMBA, which tries this. And
run then eglot tests only in that container. But this will take some
days for me, don't expect it soon.

AFAICS, https://apt.llvm.org/bullseye offers the clangd-15 Debian
package. This shall be good enough.

> Anyway, I now pushed a version-checking patch to master.

And this shall be kept, for test environments somewhere else but EMBA.

> João

Best regards, Michael.

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Wed, Mar 22, 2023 at 11:08 AM Michael Albinus <michael.albinus@gmx.de> wrote:
>
> João Távora <joaotavora@gmail.com> writes:
>
> Hi João,
>
> > What I meant rather, was trying to install version 14 from one of
> > those sources I listed earlier.  Perhaps all that we'll see is that
> > instead of:
> >
> >   clangd-13 libclang-common-13-dev libclang-cpp13 libllvm13
> >
> > we'll see
> >
> >   clangd-14 libclang-common-14-dev libclang-cpp14 libllvm14
> >
> > If the former is acceptable so should be the latter, especially
> > if the installation of the support libraries is motivated by
> > clangd alone.
>
> That would be OK on EMBA. But the recipe you have shown me was to extend
> /etc/apt/sources.list, and to run
>
> --8<---------------cut here---------------start------------->8---
> sudo apt update && sudo apt upgrade
> --8<---------------cut here---------------end--------------->8---


Again, I didn't suggest a full system upgrade.  I just pointed
to places where newer clangd packages for debian can be
obtained. As far as I remember it is possible to upgrade one
specific package.

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Michael Albinus]

Robert Pluim <rpluim@gmail.com> writes:

Hi Robert,

> The results go to a log file, but some of it also ends up on the
> terminal (this is not specific to eglot, it happens with other tests
> as well):
>
> $ make -C test eglot-tests

This will always go to the terminal. If you want to have it in the log
file, call

--8<---------------cut here---------------start------------->8---
# make -C test eglot-tests.log
make: Entering directory '/usr/local/src/emacs/test'
  ELC      lisp/progmodes/eglot-tests.elc
  GEN      lisp/progmodes/eglot-tests.log
make: Leaving directory '/usr/local/src/emacs/test'
--8<---------------cut here---------------end--------------->8---

In case of errors, the log file will be shown on the terminal. Read
test/README for the details.

> Robert

Best regards, Michael.

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[João Távora]

On Wed, Mar 22, 2023 at 10:19 AM Robert Pluim <rpluim@gmail.com> wrote:

> $ make -C test eglot-tests
> make: Entering directory '/home/rpluim/repos/emacs/test'
> make[1]: Entering directory '/home/rpluim/repos/emacs/test'
>   GEN      lisp/progmodes/eglot-tests.log
> Running 50 tests (2023-03-22 11:15:47+0100, selector `(not (or (tag :unstable) (tag :nativecomp)))')
> [eglot-tests] [eglot-test-auto-detect-running-server]: test start
> [eglot] Connected! Server `clangd' now managing `(c-mode c-ts-mode c++-mode c++-ts-mode)' buffers in project `project'.

You're seeing the expected behaviour.  All of this is hidden from
the terminal when running 'make check'.  When specifically asking to
run Eglot tests, like you did, you get output from 'message' and the
sort (and it  This is not clangd diagnostic output, btw. That's only
shown when a test fails.

João

Re: eglot-test-diagnostic-tags-unnecessary-code fails with certain clangd versions

[Robert Pluim]

>>>>> On Wed, 22 Mar 2023 12:15:35 +0100, Michael Albinus <michael.albinus@gmx.de> said:

    Michael> Robert Pluim <rpluim@gmail.com> writes:
    Michael> Hi Robert,

    >> The results go to a log file, but some of it also ends up on the
    >> terminal (this is not specific to eglot, it happens with other tests
    >> as well):
    >> 
    >> $ make -C test eglot-tests

    Michael> This will always go to the terminal. If you want to have it in the log
    Michael> file, call

    Michael> # make -C test eglot-tests.log
    Michael> make: Entering directory '/usr/local/src/emacs/test'
    Michael>   ELC      lisp/progmodes/eglot-tests.elc
    Michael>   GEN      lisp/progmodes/eglot-tests.log
    Michael> make: Leaving directory '/usr/local/src/emacs/test'

Yes, but that puts everything in the log. I like seeing the summary of
which tests failed/passed.

Oh well, I can live with it.

Robert
-- 

Re: Merging feature/android

[Corwin Brust]

On Thu, Mar 30, 2023 at 12:45 AM Po Lu <luangruo@yahoo.com> wrote:
>
> Did the 32-bit MinGW build complete?

Yes, and I was also able to make a new 32-bit build, just now.

Not only does that succeed, I don't see anything especially
frightening when I load it using my full normal config.

My sense is this branch is looking pretty good under windows :)

Re: Merging feature/android

[Eli Zaretskii]

> From: Corwin Brust <corwin@bru.st>
> Date: Sun, 2 Apr 2023 12:22:20 -0500
> Cc: Arash Esbati <arash@gnu.org>, Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> 
> On Thu, Mar 30, 2023 at 12:45 AM Po Lu <luangruo@yahoo.com> wrote:
> >
> > Did the 32-bit MinGW build complete?
> 
> Yes, and I was also able to make a new 32-bit build, just now.
> 
> Not only does that succeed, I don't see anything especially
> frightening when I load it using my full normal config.

I build and use the 32-bit build all the time.

But doing that with MinGW64 is futile, since the MinGW64 runtime no
longer supports Windows versions which were mostly run in 32-bit mode.

Re: Merging feature/android

[Eli Zaretskii]

> Date: Sun, 02 Apr 2023 20:53:41 +0300
> From: Eli Zaretskii <eliz@gnu.org>
> Cc: luangruo@yahoo.com, arash@gnu.org, emacs-devel@gnu.org
> 
> > From: Corwin Brust <corwin@bru.st>
> > Date: Sun, 2 Apr 2023 12:22:20 -0500
> > Cc: Arash Esbati <arash@gnu.org>, Eli Zaretskii <eliz@gnu.org>, emacs-devel@gnu.org
> > 
> > On Thu, Mar 30, 2023 at 12:45 AM Po Lu <luangruo@yahoo.com> wrote:
> > >
> > > Did the 32-bit MinGW build complete?
> > 
> > Yes, and I was also able to make a new 32-bit build, just now.
> > 
> > Not only does that succeed, I don't see anything especially
> > frightening when I load it using my full normal config.
> 
> I build and use the 32-bit build all the time.

Oh, I see you were talk,ing about the android branch.  No, I haven't
built that one.

Re: Merging feature/android

[Po Lu]

Corwin Brust <corwin@bru.st> writes:

> On Fri, Mar 17, 2023 at 8:45 AM Arash Esbati <arash@gnu.org> wrote:
>>
>> Po Lu <luangruo@yahoo.com> writes:
>>
>> > Thanks, both ought to be fixed now.
>>
>> Confirmed; I see only the 2 warnings I also get with master.  Thanks for
>> your work on this feature.
>
> I'm also able to build feature/android now on MSYS2/MINGW64 without
> error.  MINGW32 build is running now; no issues so far ;)

Guys, if there are no further objections or test failures, I'll merge
the branch on 16 April, CST.

Re: Merging feature/android

[Eli Zaretskii]

> From: Po Lu <luangruo@yahoo.com>
> Cc: Arash Esbati <arash@gnu.org>,  Eli Zaretskii <eliz@gnu.org>,
>   emacs-devel@gnu.org
> Date: Thu, 13 Apr 2023 16:25:14 +0800
> 
> Guys, if there are no further objections or test failures, I'll merge
> the branch on 16 April, CST.

Please wait some more weeks, I don't want this calamity on my plate in
parallel with the pretest of Emacs 29.

Thanks.

Re: Merging feature/android

[Po Lu]

Eli Zaretskii <eliz@gnu.org> writes:

> Please wait some more weeks, I don't want this calamity on my plate in
> parallel with the pretest of Emacs 29.

OK, noted.