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

[Samuel CUELLA]

On 08/18/14 10:30, Eric S. Raymond wrote:
> 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.

Ok. I'll review the "offending" parts and send out another patch to fix
the issues, if they are still there in last "git head".

>  
>> -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.
> 
Bionic is just the name of the (crippled) C library used on Android.
Google took parts of FreeBSD to create it. They intentionaly disabled
some features they do not want.


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

fd_set was needed in gpsd.h for gpsd_await_data. Including <unistd.h>
fixes the problem. In fact the bug is still present but shadowed by
another part of the patch you've merged: In SConstruct, HAVE_GETSID not
defined adds "#include <unistd.h>" along with getsid prototype in gpsd.h.

I'll add a fix for that in the next round.

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

Ok, I'll look into that more deeply to see if there is another solution.

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

It seems that I just didn't got the intent.You're using symbols in the
library that the "client" executable *must* provide and you rely on the
linker to do the library used -> executable defined linking.

Is that documented somewhere? Would you agree on using a different
approach, like defining function pointers to be filled-in by the client
code if needed (and maybe a default implementation)?

P.S: I've seen comments and a patch from Matt about that too. I'll try
it on Android to see if that works (guess it will). Do you agree with
that direction ?

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

Best,
Samuel