Re: Merging feature/android

[Eli Zaretskii <eliz@gnu.org>]

> 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.