[Buildroot] [PATCH v2 5/7] package: Introduce package-specific BINFMT_FLAT options.

[Arnout Vandecappelle <arnout@mind.be>]

On 07/04/13 23:27, Thomas Petazzoni wrote:
> Dear Sonic Zhang,
>
> On Fri, 29 Mar 2013 17:50:41 +0800, Sonic Zhang wrote:
>
>> diff --git a/package/pkg-autotools.mk b/package/pkg-autotools.mk
>> index 890506b..bdfea20 100644
>> --- a/package/pkg-autotools.mk
>> +++ b/package/pkg-autotools.mk
>> @@ -82,6 +82,11 @@ $(2)_CLEAN_OPT			?= clean
>>   $(2)_UNINSTALL_STAGING_OPT	?= DESTDIR=$$(STAGING_DIR) uninstall
>>   $(2)_UNINSTALL_TARGET_OPT	?= DESTDIR=$$(TARGET_DIR)  uninstall
>>
>> +ifeq ($(BR2_BINFMT_FLAT),y)
>> + ifneq ($$($(2)_FLAT_STACKSIZE),)
>> +  $(2)_CONF_ENV			+= LDFLAGS="$(TARGET_LDFLAGS) -Wl,-elf2flt=-s$$($(2)_FLAT_STACKSIZE)"
>> + endif
>> +endif
>>
>>   #
>>   # Configure step. Only define it if not already defined by the package
>> diff --git a/package/pkg-generic.mk b/package/pkg-generic.mk
>> index 57b0fd0..8ee2b20 100644
>> --- a/package/pkg-generic.mk
>> +++ b/package/pkg-generic.mk
>> @@ -303,6 +303,11 @@ endif
>>
>>   $(2)_REDISTRIBUTE		?= YES
>>
>> +ifeq ($(BR2_BINFMT_FLAT),y)
>> + ifneq ($$($(2)_FLAT_STACKSIZE),)
>> +  $(2)_FLAT_LDFLAGS = -Wl,-elf2flt=-s$$($(2)_FLAT_STACKSIZE)
>> + endif
>> +endif
>>
>>   $(2)_DEPENDENCIES ?= $(filter-out $(1),$(patsubst host-host-%,host-%,$(addprefix host-,$($(3)_DEPENDENCIES))))
>
> I understand the problem but I'm still not really happy with the
> solution here. Maybe Arnout will have some suggestions.

  I had exactly the same thoughts while reading the patch...

> The problem I see with the pkg-autotools change is that some packages
> already override LDFLAGS in their <pkg>_CONF_ENV. For example:
>
> NDISC6_CONF_ENV += LDFLAGS="$(TARGET_LDFLAGS) -lintl"
>
> Since your <pkg>_CONF_ENV += assignment will be done after the ones
> coming from the package .mk file, you're going to remove the -lintl
> that had been added by the package.
>
> For the pkg-generic change, I'm not entirely happy because it means
> each and every package should use <pkg>_FLAT_LDFLAGS, while we have
> told for a long time our packagers to just use $(TARGET_CONFIGURE_OPTS)
> to get all the right compiler/linker flags.
>
> So I think it seems we somehow need to make $(TARGET_LDFLAGS) a
> per-package variable? Arnout what do you think about this? Do you have
> ideas?

  In fact, it can easily be made a per-package variable. In the 
definition of TARGET_LDFLAGS, we can add:

TARGET_LDFLAGS = ... \
	$($(PKG)_LDFLAGS)

  The $(PKG) is only evaluated when the TARGET_LDFLAGS variable is used.

  Outside the scope of the generic rules, $(PKG) evaluates to nothing, 
and _LDFLAGS is not defined, so the $($(PKG)_LDFLAGS) has no effect.

  Inside the scope of the generic rules, $(PKG) is always defined. So we 
just have to add the following to the pkg-generic macro:

ifeq ($(BR2_BINFMT_FLAT),y)
   $(2)_LDFLAGS += -Wl,-elf2flt=-s$$($(2)_FLAT_STACKSIZE)
endif


  As far as I can see, that should work nicely. No need to change 
anything in autotools. Unfortunately, it does require changing the 
packages that have something like:

DOSFSTOOLS_LDFLAGS = $(TARGET_LDFLAGS)

  Also, it is not very intuitive, because in a non-autotools package 
where you can't use the LDFLAGS environment variable, you'll have 
something like:

AXEL_LDFLAGS = -lpthread
define AXEL_BUILD_CMDS
         $(TARGET_CONFIGURE_OPTS) $(MAKE) \
         LFLAGS="$(TARGET_LDFLAGS)" -C $(@D)
endef

i.e. you're defining AXEL_LDFLAGS but using TARGET_LDFLAGS. Not a really 
big issue, I think, but not very nice either.


  For this patch list, however, I think changing the 15 uses of 
FOO_LDFLAGS is going a bit too far. So I propose to make the following 
change instead in package/Makefile.in:

ifeq ($(BR2_BINFMT_FLAT),y)
TARGET_LDFLAGS += $(if $($(PKG)_FLAT_STACKSIZE),\
	-Wl,-elf2flt=-s$($(PKG)_FLAT_STACKSIZE))
endif

  AFAICS, that should work for any package without any further change. 
But needs to be tested, of course...

  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