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

[ukyg9e5r6k7gubiekd6]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

On 18/08/14 18: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.
> 
>> -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.

I too saw this under mingw. I came up with some fix but I forget what
I did now. I'll have a look at my diffs.

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

I wouldn't really call it layering since it's a bi-directional
dependency. More like an unhealthy co-dependency of the sort that
psychiatrists warn against :-)

The approach I took in the mingw port, and which I think is superior
to attempting to make the linker do what you consider to be the Right
Thing on all platforms and all weird linkers, was to add some function
pointers with sensible default values into libgpsd_core.c, then add
some entry points into libgpsd that let the calling binary set those
function pointers to those it supplies, if and when it wants to.

This is probably buried in a whole lot of other unmergable/confused
crud in my mingw fork, but I can dig it out into a separate diff if
someone wants.

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

I second the motion to have a header which does nothing but define
HAVE_* constants - does not include any other headers, nor do any
typedefs nor macro definitions nor declarations nor anything else.
Call it gpsd_config.h or anything else. My preference would be for
'config.h', but what's in a name?

I believe there exists a legitimate need to find out what the
underlying system does and doesn't have (ie what Sconstruct found),
without also pulling in random other headers that could cause trouble
or even merely slow compilation.

As a specific example, on mingw, anything that pulls in windows.h
incurs large parse overhead and lots of namespace pollution; and
almost all Windoze headers of interest pull in windows.h sooner or later.
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1

iQEcBAEBAgAGBQJT8c2WAAoJECH/phKUbjXCAK0H+wR1ojy1qKVSvGfjcwcHflcD
HerpJrBZscMbw1Vb7S0rN7PR4R3MTsaBWnZne37xFtQY+K9Kol3JBkeesDwcJ7Kz
Wj3thlm/r6RYa2EnVn6d4vxjWBt97njUWyU2tpzEXEFVQC5vuzeYeklfmHkQv7MN
mvGDfDebiwuGOX26J+mNnhTqMLrQb66jcE8nyQhFWpRIo98Exk3c7Qlj4FoglX1E
p2Xfc/K1FE1izryGTEV+OBtcViWo7lmK2ZTUhrvMnMvi/3rRmaWVuTIarl7v9HAF
LOVFwWJqjRuHTHBNt06D1gmno4HUFwRkU71ooKJlILNUKtrAbXUfjkRk5YeT4jA=
=i81x
-----END PGP SIGNATURE-----