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.