[Buildroot] [PATCH v2] package/dvb-app: handle static/shared only build

[Romain Naour <romain.naour@openwide.fr>]

Hi Yann, all

Le 04/01/2015 16:09, Yann E. MORIN a ?crit :
> Romain, All,
> 
> On 2015-01-04 13:45 +0100, Romain Naour spake thusly:
>> Also remove tests since they require static libraries.
>>
>> Signed-off-by: Romain Naour <romain.naour@openwide.fr>
>> ---
>> v2: remove tests (ThomasP)
>>     rework static/shared handling logic
>> ---
>>  .../0003-handle-static-shared-only-build.patch     | 35 ++++++++++++++++++++++
>>  .../dvb-apps/0003-support-static-only-build.patch  | 20 -------------
>>  package/dvb-apps/0004-Makefile-remove-test.patch   | 27 +++++++++++++++++
>>  package/dvb-apps/dvb-apps.mk                       |  4 ++-
>>  4 files changed, 65 insertions(+), 21 deletions(-)
>>  create mode 100644 package/dvb-apps/0003-handle-static-shared-only-build.patch
>>  delete mode 100644 package/dvb-apps/0003-support-static-only-build.patch
>>  create mode 100644 package/dvb-apps/0004-Makefile-remove-test.patch
>>
>> diff --git a/package/dvb-apps/0003-handle-static-shared-only-build.patch b/package/dvb-apps/0003-handle-static-shared-only-build.patch
>> new file mode 100644
>> index 0000000..7a7d59a
>> --- /dev/null
>> +++ b/package/dvb-apps/0003-handle-static-shared-only-build.patch
>> @@ -0,0 +1,35 @@
>> +From f461e831f8c0ee9a59c9f194c0306eb73298396b Mon Sep 17 00:00:00 2001
>> +From: Romain Naour <romain.naour@openwide.fr>
>> +Date: Thu, 25 Dec 2014 19:22:16 +0100
>> +Subject: [PATCH] Make.rules: Handle static/shared only build
>> +
>> +Do not build .a library when disable_static is set
>> +Do not build .so library when disable_shared is set
>> +
>> +Signed-off-by: Romain Naour <romain.naour@openwide.fr>
>> +---
>> + Make.rules | 8 +++++++-
>> + 1 file changed, 7 insertions(+), 1 deletion(-)
>> +
>> +diff --git a/Make.rules b/Make.rules
>> +index 3410d7b..4add272 100644
>> +--- a/Make.rules
>> ++++ b/Make.rules
>> +@@ -9,7 +9,13 @@ ifneq ($(lib_name),)
>> + CFLAGS_LIB ?= -fPIC
>> + CFLAGS += $(CFLAGS_LIB)
>> + 
>> +-libraries = $(lib_name).so $(lib_name).a
>> ++ifeq ($(disable_static),)
>> ++libraries = $(lib_name).a
>> ++endif
>> ++
>> ++ifeq ($(disable_shared),)
>> ++libraries += $(lib_name).so
>> ++endif
> 
> I find it weird that one would need to _disable_stuff. I really prefer
> we use positive logic whenever possible, it is much easier to understand.
> 
> What about:
> 
>     ifneq ($(enable_static),no)
>     libraries += $(lib_name).a
>     endif
> 
>     ifneq ($(enable_shared),no)
>     libraries += $(lib_name).so
>     endif
> 
> Ok, so this is negative logic _in_ the Makefile. It is still positive
> from the caller::
>   - by default both are built
>   - it is possible to selectively disable each by passing enable_foo=no,
>     which is not unlike the traditional way it is handled in autotools

Ok, I'm fine with this change if you prefer.

> 
> [--SNIP--]
>> diff --git a/package/dvb-apps/dvb-apps.mk b/package/dvb-apps/dvb-apps.mk
>> index 892af63..a5037af 100644
>> --- a/package/dvb-apps/dvb-apps.mk
>> +++ b/package/dvb-apps/dvb-apps.mk
>> @@ -16,7 +16,9 @@ DVB_APPS_LDLIBS += -liconv
>>  endif
>>  
>>  ifeq ($(BR2_STATIC_LIBS),y)
>> -DVB_APPS_MAKE_OPTS += static=1
>> +DVB_APPS_MAKE_OPTS += disable_shared=1
>> +else ifeq ($(BR2_SHARED_LIBS),y)
>> +DVB_APPS_MAKE_OPTS += disable_static=1
>>  endif
> 
> So you'd get:
> 
>     ifeq ($(BR2_STATIC_LIBS),y)
>     DVB_APPS_MAKE_OPTS += enable_shared=no
>     endif
>     ifeq ($(BR2_SHARED_LIBS),y)
>     DVB_APPS_MAKE_OPTS += enable_static=no
>     endif
> 
> Granted, it is very, very similar to what you're doing, but I really
> prefer options to have positive logic, rather than negative. Also, I
> believe this might be more easily upstreamable.

I already sent the series upstream:
http://www.mail-archive.com/linux-media at vger.kernel.org/msg83830.html

But Arnout noticed that my SoB is missing on the two first patches.
I'll resend a new version tomorrow.

> 
> Note: I read Thomas' comment on the previous iteration, and it does
> not seem like he said he would want disable_XXX variables instead of
> enable_XXX. Thomas, what's your opinion?

Thanks for the review.

Best regards,
Romain

> 
> Regards,
> Yann E. MORIN.
> 
>>  DVB_APPS_INSTALL_STAGING = YES
>> -- 
>> 1.9.3
>>
>> _______________________________________________
>> buildroot mailing list
>> buildroot at busybox.net
>> http://lists.busybox.net/mailman/listinfo/buildroot
>