[Buildroot] [PATCH 5/5] thrift: new package

[Arnout Vandecappelle <arnout@mind.be>]

On 26/10/13 11:48, Gustavo Zacarias wrote:
> On 10/26/2013 12:12 AM, Ryan Barnett wrote:
>> Could you move the license to right below the site? It doesn't really
>> matter but I guess I like to see version, site, license as the first
>> things in a package. I guess there isn't any official standard to the way
>> a package makefile should be in but I guess that is just my person
>> preference.
>
> Huh, that's very OCD :)

  But not as much as requiring 80 hashes in the header :-)

> There are several packages that don't keep them ordered, specially those

  It's not because it wasn't done right in the past that it shouldn't be 
done right now :-)


  I agree with Ryan that it would be a good idea to standardize on some 
order of the variable definitions. But since there is not standard at the 
moment, we have to wait for an edict from Peter before we can enforce it.


[snip]
>>> +ifeq ($(BR2_PREFER_STATIC_LIB),y)
>>> +# openssl uses zlib, so we need to explicitly link with it when static
>>> +THRIFT_CONF_ENV += LIBS=-lz
>>
>> Shouldn't you add a dependancy on libz then?
>>
>> THRIFT_DEPENDCIES += zlib
>
> It's implicit from openssl, we don't build openssl without zlib (it's
> possible to do so, but not really that useful and would complicate other
> packages).

  That it's implicit from openssl is not really relevant - if the 
dependency is ever removed from openssl (OK, this will not happen, but 
just imagine) then we'd like to keep the correct dependency here.

>
>> I haven't tested this so I don't fully understand what exactly what you
>> are doing here. I see that this is an autotools package but it doesn't
>> appear to be building or installing using the autotools infrastructure.
>> Could you please elaborate a little bit more of why exactly you can't use
>> autotoools to build for this? I believe I understand what you are doing
>> here but I guess I would like a few more details from you.
>>
>> Also it looks like thrift is a compiler, so wouldn't we want to have this
>> available as a host tool because presumably there would be a need to to
>> add custom user packages that utilize Thrift to compile their application.
>> Again I haven't had time to read further what exactly thrift does but
>> these are just the few things that are coming to my mind tonight.
>>
>> I will try to investigate this a little bit more in the coming days (next
>> week) but for now this are my initial reaction comments.
>
> A quick overlook then.
> Thrift is basically a compiler and runtime libs.
> The compiler component is only useful for the host because we don't do
> development on the target any more, that would be host-boost, which IS
> actually using autotools infra (no defines overriding HOST_THRIFT_*_CMDS).
> For the target the INSTALL phase has an override to just install the
> runtime libraries and avoid the compiler, tests and examples.
> The target BUILD override patches the thrift makefiles to use $(THRIFT)
> rather than hardcode it (the for-SED loop), and builds passing $(THRIFT)
> to the appropiate host-compiled one (since it's hell bent on using it's
> own built one which would be cross and wouldn't run).
> This is a double catch though, i wanted to build examples as a sane way
> to test it that things are ok but not install them.

  It would be nicer though if that could be done with an upstreamable 
patch, but I think you said before that it wasn't going to happen.

  I do think that your sed magic should be done in POST_PATCH_HOOKS on 
the Makefile.am, though. Patching up the generated Makefiles just feels 
icky to me.

  Regards,
  Arnout


-- 
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:  7CB5 E4CC 6C2E EFD4 6E3D A754 F963 ECAB 2450 2F1F