[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [gpsd-dev] [PATCH] Various fixes to build for android
From: |
Eric S. Raymond |
Subject: |
Re: [gpsd-dev] [PATCH] Various fixes to build for android |
Date: |
Mon, 18 Aug 2014 04:30:44 -0400 |
User-agent: |
Mutt/1.5.21 (2010-09-15) |
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>
- Re: [gpsd-dev] [PATCH] Various fixes to build for android,
Eric S. Raymond <=