[Buildroot] [PATCH v4 3/3] package/radlib: reworked autotools with pkg-config

[Arnout Vandecappelle <arnout@mind.be>]

On 04/07/16 14:03, Kinsella, Ray wrote:
> On Thu, 2016-04-07 at 00:36 +0200, Arnout Vandecappelle wrote:
>> Do this in three separate patches: one that sets subdir-objects, one
>> that adds
>> pkg-config, and one that adds mysql-config.
>
> Ok then, use a hook for the rename.
>
> Might make more sense to do one patch for pkg-config with subdir
> -objects, because these are linked - which subdir objects you get
> depends on pkg-config?

  Er, subdir-objects is just an option that is needed for recent autoconf when 
you have directories without Makefile.am. Doesn't have anything to do with 
pkg-config AFAIK.

>
> And then another patch for mysql-config. So two patches?
>
>>
>>> +-AC_ARG_ENABLE(mysql,
>>> +-[  --enable-mysql                 include radlib MySQL database
>>> support],
>>
>>    Why would you remove this? An enable option is as good as a with
>> option.
>
> The original feedback I got was that the autotools implementation for
> radlib was hopelessly screwed up. I was trying to strike a balance
> between limiting changes but improving the overall flows.

  From who did you get that feedback? Thomas wrote that the Makefile.am 
contained some weird stuff, but that's a bit separate.

>
> I used with 'with' instead of 'enable' because of the semantics ...
>
> If a package _must_ be available on the platform for it available to
> choose then 'with' makes more sense than 'enable'. Radlib supports more
> than one back end so selecting a backend by indicating 'with' made more
> sense to me. 'enable' is typically for features whereas 'with', is
> typically used to indicate building 'with' a given package.

  Well, your Config.in didn't express that at least one database must be 
selected, and also your original replies to Thomas didn't say that, so I assumed 
that the databases were indeed optional. But even if they are not optional, I 
wouldn't bother changing the enable in a with.

>
> That said I am agnoistic, I am happy to revert to AC_ARG_ENABLE to
> reduce the patch.
>
[snip]
>>> +-AM_CONDITIONAL(MYSQL, test x$mysql = xtrue)
>>
>>    Keep this, there is no reason to replace it by HAVE_MYSQL.
>
> Ok - I am pretty sure I got feedback to change these conditionals to
> HAVE_, but no problem I can fix.

  I looked back at Thomas's original comments and couldn't find anything 
suggesting that.

>
>
>>> + # define include directories
>>> +-INCLUDES = \
>>> ++AM_CFLAGS = \
>>
>>    Why does this need to be changed?
>
> I will investigate ...
> The original INCLUDES was pulling in host directories.
> Not sure why the switch to AM_CFLAGS
>
>>
>>> + 		-I$(top_srcdir)/h \
>>> + 		-D_GNU_SOURCE
>>> +
>>
>>    Did you forget to add $(SQLITE3_CFLAGS) etc?
>
> nope, no special include is needed for SQLITE.

  Well, that's because in buildroot we install everything in /usr/include, so no 
-I option is needed. But you can't assume that in general, so you will need 
SQLITE3_CFLAGS for upstream.

>
>
>>    Removing the CROSSCOMPILE part should be a separate patch.
>
> ok

  Just to be clear: I mean a separate .patch file, not a separate commit in 
buildroot.git.


  Regards,
  Arnout

[snip]
-- 
Arnout Vandecappelle                          arnout at mind be
Senior Embedded Software Architect            +32-16-286500
Essensium/Mind                                http://www.mind.be
G.Geenslaan 9, 3001 Leuven, Belgium           BE 872 984 063 RPR Leuven
LinkedIn profile: http://www.linkedin.com/in/arnoutvandecappelle
GPG fingerprint:  7493 020B C7E3 8618 8DEC 222C 82EB F404 F9AC 0DDF