Re: [gpsd-dev] [PATCH] Various fixes to build for android

[Eric S. Raymond]

Samuel CUELLA <address@hidden>:
> I had to build gpsd for an android device that didn't come with default
> gps support. In the process, I've found various issues which the
> following patch aims to fix. Here is a summary of what it does:

Most of this patch is good.  Some of it doesn't follow local conventions.
Other parts of it make me a little nervous.  I've partially merged it,
and have some comments on what I haven't merged.  Once you've digested
these please submit another patch.  After a couple of rounds of this
I have no doubt we'll have the Android port in good shape.
 
> -Feature selection
> --SHM_EXPORT_ENABLE guard was placed *after* the inclusion of sys/shm in
> shmexport.c and libgps_shm.c. That made the build to fail even if shm
> was disabled. The patch moves the guard upwards to exclude the whole
> content if shm support is disabled.

I implemented this slightly differently, moving #include <string> into the
generated part at the end of gpsd_config.h.  We might split out that part
in another round; see below.

> -Undefined Symbols
> --in_port_t and SUN_LEN are not defined on Android. Patch adds checks to
> SConstruct, and fallbacks to generated gpsd_config.h

I took this as is.  

> --getsid syscall wrapped is not included in some versions of bionic as
> well. Patch also adds a check and a fallback declaration modeled on
> fallbacks for strl* functions [1]. An implementation is provided in
> missing-support.h [2]

I changed the name of missing_support.c to getsid.c, otherrwise took this.

What is "bionic"?  I'd like to document better why this patch is required.

> --fd_set is defined by sys/select.h. Reported undefined during the
> build. Moved include from gpsd.c to gpsd.h-head to fix the problem.

This works but I find it unesthetic.  I prefer only simple,
well-standardized headers to be included that early.  Can you identify
the module or modules in which the referencee to fd_set failed? I'd
like to just put the include in the right C modules.

> -Library symbols and linking
> --By default, gpsd doesn't explicitly links libgps (Tried imloads with
> no luck as well).  This results in unix_to_iso8601 being UND. Android
> Linker fails to link gpsd binary because of this. Patched SContruct to
> explicitly link libgpsd against libgps.

This makes me nervous.  I want to look for a less intrusive way to
fix the problem you're seeing.

> --Moved gpsd_write and gpsd_report from gpsd.c to libgpsd_core.c. The
> library libgpsd uses these symbols from gpsd whereas it should be the
> other way around if libgpsd was to be used by another binary. These
> symbols are marked UND which makes the android linker to fail loading
> gpsd binary.

Alas, this patch is flat wrong; it violates the way the code is layered.
These functions are callouts from the library that are *intentionally* 
defined different ways by different binaries.  We need to find a way to
beat your linker into doing the right thing here.

> [1] I would also suggest to add a dependency on libstrl
> (http://ohnopub.net/~ohnobinki/libstrl/).

This seems unnecessary; IMO the implementations of these functions are
too simple to justify adding another external build dependency.

> [2] I would also suggest to remove the conditionals declarations of
> strl* from the generated gpsd_config.h, and have an separate include
> which would be guarded by HAVE_STRL*s. Theses declartions use size_t
> which is not declared, therefore including gpsd_config.h without
> including stdlib.h beforehand can lead to compile errors if any of the
> HAVE_STRL* is false. Same  problem for the getsid declaration I've added
> (uses pid_t). I'd suggest to have only HAVE_* in gpsd_config.h and
> various support includes guarded by these macros. Those includes would
> also in turn all other needed includes. I'll happily take care of that
> if you agree on it.

I am considering a slightly different path.  It turns out only one of
these functions is used anywhere outsuide libgpsd.  It may work better
to move that section from gpsd_config.h to the end of gpsd.h.
-- 
                <a href="http://www.catb.org/~esr/";>Eric S. Raymond</a>