Patchwork buildroot: Makefile: Allow target board makefile fill in default target_skeleton path.

login
register
mail settings
Submitter Sonic Zhang
Date Aug. 8, 2012, 6:57 a.m.
Message ID <1344409039-32071-1-git-send-email-sonic.adi@gmail.com>
Download mbox | patch
Permalink /patch/175863/
State Rejected
Headers show

Comments

Sonic Zhang - Aug. 8, 2012, 6:57 a.m.
From: Sonic Zhang <sonic.zhang@analog.com>

If the target skeleton path is empty in config, don't change variable
TARGET_SKELETON in buildroot Makefile. Allow the target board makefile
fill in a default path.


Signed-off-by: Sonic Zhang <sonic.zhang@analog.com>
---
 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)
Thomas Petazzoni - Aug. 9, 2012, 8:41 a.m.
Le Wed, 8 Aug 2012 14:57:19 +0800,
Sonic Zhang <sonic.adi@gmail.com> a écrit :

> If the target skeleton path is empty in config, don't change variable
> TARGET_SKELETON in buildroot Makefile. Allow the target board makefile
> fill in a default path.

The concept of "target board makefile" does not exist. It used to exist
in earlier versions of Buildroot, but it no longer exists in newer
versions. Thus, I have the feeling that rather than fixing this, it is
the way your boards are supported that should be modified according to
the newer Buildroot good practices.

The practices in terms of board support are:

 * One defconfig file per board in configs/, generally named
   <vendor>_<boardname>_defconfig. Those files must be generated with
   'make savedefconfig'

 * Utility files (kernel patches, kernel configuration files and other
   stuff), should be stored in board/<vendor>/<boardname>/. Kernel
   configuration files must be generated with 'make savedefconfig' as
   well.

 * Using a different skeleton from the default one provided by
   Buildroot is really *not* recommended. Instead, if anything needs to
   be tweaked in the filesystem, use the post-build script for that
   effect. This allows to avoid duplicating the skeleton for each and
   every board, causing a maintenance nightmare when we want to change
   something in this skeleton.

Best regards,

Thomas
Sonic Zhang - Aug. 9, 2012, 10:26 a.m.
Hi Thomas,

On Thu, Aug 9, 2012 at 4:41 PM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Le Wed, 8 Aug 2012 14:57:19 +0800,
> Sonic Zhang <sonic.adi@gmail.com> a écrit :
>
>> If the target skeleton path is empty in config, don't change variable
>> TARGET_SKELETON in buildroot Makefile. Allow the target board makefile
>> fill in a default path.
>
> The concept of "target board makefile" does not exist. It used to exist
> in earlier versions of Buildroot, but it no longer exists in newer
> versions. Thus, I have the feeling that rather than fixing this, it is
> the way your boards are supported that should be modified according to
> the newer Buildroot good practices.

If "target board makefile" doesn't exit, where should we add arch
specific makefile targets and compile/link flags? Where should arch
specific Config options be put? Take blackfin specific makefile as an
example bellow.

Makefile.in
----------------
TARGET_CFLAGS += -D__NOMMU__ -D__uClinux__ -D_GNU_SOURCE
ifeq ($(BR2_ABI_FLAT),y)
TARGET_LDFLAGS += -Wl,-elf2flt
endif
ifeq ($(BR2_BFIN_SHARED_FLAT), y)
TARGET_LDFLAGS += -mid-shared-library -mshared-library-id=0
TARGET_CFLAGS += -mid-shared-library -mshared-library-id=0
TARGET_CXXFLAGS += -mid-shared-library -mshared-library-id=0
endif

CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-
romfs.shared.libs.elf:
        set -e; \
        t=`$(CROSS_COMPILE_SHARED_ELF)gcc $(CPUFLAGS)
-print-file-name=libc.a`; \
        t=`dirname $$t`/../..; \
        for i in $$t/lib/*so*; do \
                i=`readlink -f "$$i"`; \
                soname=`$(CROSS_COMPILE_SHARED_ELF)readelf -d "$$i" |
sed -n '/(SONAME)/s:.*[[]\(.*\)[]].*:\1:p'`; \
                $(INSTALL) -D $$i $(TARGET_DIR)/lib/$$soname; \
        done

CROSS_COMPILE_SHARED_FLAT ?= bfin-uclinux-
romfs.shared.libs.flat:
        set -e; \
        t=`$(CROSS_COMPILE_SHARED_FLAT)gcc $(CPUFLAGS)
-mid-shared-library -print-file-name=libc`; \
        if [ -f $$t -a ! -h $$t ] ; then \
                $(INSTALL) -D $$t $(TARGET_DIR)/lib/lib1.so; \
        fi


Config.in
----------------------------------------------------
config BR2_TARGET_ANALOGDEVICES_INSTALL_ELF_SHARED
        bool "Install ELF shared libraries" if !BR2_BFIN_FDPIC
        default y

config BR2_TARGET_ANALOGDEVICES_INSTALL_FLAT_SHARED
        bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
        default y


>
> The practices in terms of board support are:
>
>  * One defconfig file per board in configs/, generally named
>    <vendor>_<boardname>_defconfig. Those files must be generated with
>    'make savedefconfig'
>
>  * Utility files (kernel patches, kernel configuration files and other
>    stuff), should be stored in board/<vendor>/<boardname>/. Kernel
>    configuration files must be generated with 'make savedefconfig' as
>    well.
>
>  * Using a different skeleton from the default one provided by
>    Buildroot is really *not* recommended. Instead, if anything needs to
>    be tweaked in the filesystem, use the post-build script for that
>    effect. This allows to avoid duplicating the skeleton for each and
>    every board, causing a maintenance nightmare when we want to change
>    something in this skeleton.
>

Blackfin Linux distribution has a different file system layout from
current default one in buildroot. And we have no plan to deviate from
current one used for years. Anyway without this patch, we can still
set a custom skeleton for blackfin.

Regards,

Sonic
Thomas Petazzoni - Aug. 9, 2012, 8:44 p.m.
Hello,

Le Thu, 9 Aug 2012 18:26:54 +0800,
Sonic Zhang <sonic.adi@gmail.com> a écrit :

> If "target board makefile" doesn't exit, where should we add arch
> specific makefile targets and compile/link flags? Where should arch
> specific Config options be put? Take blackfin specific makefile as an
> example bellow.

There is no general rule for this, it depends on each thing you want to
add. For things that must be added globally, package/Makefile.in is
usually the current place (though this file would probably need some
cleanup, but this is a different topic).

> Makefile.in
> ----------------
> TARGET_CFLAGS += -D__NOMMU__ -D__uClinux__ -D_GNU_SOURCE

I don't think the -D_GNU_SOURCE should be passed globally to all
packages. Each package should define it if it is needed to build this
package.

Also, before globally defining __NOMMU__ and __uClinux__, I'd like to
understand why they are needed. I think __NOMMU__ is generally used to
test whether fork() or vfork() should be used, but this should probably
be tested using autoconf tests when possible.

That said, if such flags are indeed necessary, package/Makefile.in is a
good place for that.

> ifeq ($(BR2_ABI_FLAT),y)
> TARGET_LDFLAGS += -Wl,-elf2flt
> endif
> ifeq ($(BR2_BFIN_SHARED_FLAT), y)
> TARGET_LDFLAGS += -mid-shared-library -mshared-library-id=0
> TARGET_CFLAGS += -mid-shared-library -mshared-library-id=0
> TARGET_CXXFLAGS += -mid-shared-library -mshared-library-id=0
> endif

Same thing for these flags, package/Makefile.in.

> CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-

No, this should use the existing variables to get the cross-compiler
location.

> romfs.shared.libs.elf:
>         set -e; \
>         t=`$(CROSS_COMPILE_SHARED_ELF)gcc $(CPUFLAGS)
> -print-file-name=libc.a`; \
>         t=`dirname $$t`/../..; \
>         for i in $$t/lib/*so*; do \
>                 i=`readlink -f "$$i"`; \
>                 soname=`$(CROSS_COMPILE_SHARED_ELF)readelf -d "$$i" |
> sed -n '/(SONAME)/s:.*[[]\(.*\)[]].*:\1:p'`; \
>                 $(INSTALL) -D $$i $(TARGET_DIR)/lib/$$soname; \
>         done

Why is this needed? How is this romfs.shared.libs.elf target being used?

> CROSS_COMPILE_SHARED_FLAT ?= bfin-uclinux-
> romfs.shared.libs.flat:
>         set -e; \
>         t=`$(CROSS_COMPILE_SHARED_FLAT)gcc $(CPUFLAGS)
> -mid-shared-library -print-file-name=libc`; \
>         if [ -f $$t -a ! -h $$t ] ; then \
>                 $(INSTALL) -D $$t $(TARGET_DIR)/lib/lib1.so; \
>         fi

Ditto.

> Config.in
> ----------------------------------------------------
> config BR2_TARGET_ANALOGDEVICES_INSTALL_ELF_SHARED
>         bool "Install ELF shared libraries" if !BR2_BFIN_FDPIC
>         default y
> 
> config BR2_TARGET_ANALOGDEVICES_INSTALL_FLAT_SHARED
>         bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
>         default y

Any reason to have these in addition to the existing selection of ABI?
What are the use cases for these?

> Blackfin Linux distribution has a different file system layout from
> current default one in buildroot. And we have no plan to deviate from
> current one used for years. Anyway without this patch, we can still
> set a custom skeleton for blackfin.

How does the filesystem layout differs? Depending on how it differs,
having a separate skeleton may or may not be the right solution.

Best regards,

Thomas
Sonic Zhang - Aug. 13, 2012, 5:04 a.m.
On Fri, Aug 10, 2012 at 4:44 AM, Thomas Petazzoni
<thomas.petazzoni@free-electrons.com> wrote:
> Hello,
>
> Le Thu, 9 Aug 2012 18:26:54 +0800,
> Sonic Zhang <sonic.adi@gmail.com> a écrit :
>
>> If "target board makefile" doesn't exit, where should we add arch
>> specific makefile targets and compile/link flags? Where should arch
>> specific Config options be put? Take blackfin specific makefile as an
>> example bellow.
>
> There is no general rule for this, it depends on each thing you want to
> add. For things that must be added globally, package/Makefile.in is
> usually the current place (though this file would probably need some
> cleanup, but this is a different topic).
>
>> Makefile.in
>> ----------------
>> TARGET_CFLAGS += -D__NOMMU__ -D__uClinux__ -D_GNU_SOURCE
>
> I don't think the -D_GNU_SOURCE should be passed globally to all
> packages. Each package should define it if it is needed to build this
> package.
>

Yes, it is better to put flag GNU_SOURCE to packages.

> Also, before globally defining __NOMMU__ and __uClinux__, I'd like to
> understand why they are needed. I think __NOMMU__ is generally used to
> test whether fork() or vfork() should be used, but this should probably
> be tested using autoconf tests when possible.
>
> That said, if such flags are indeed necessary, package/Makefile.in is a
> good place for that.

Yes, this NOMMU flag is necessary to make application built for NOMMU
architecture. It is not only for fork system call, but also some
memory management logic. I will move it to package/Makefile.in.

>
>> ifeq ($(BR2_ABI_FLAT),y)
>> TARGET_LDFLAGS += -Wl,-elf2flt
>> endif
>> ifeq ($(BR2_BFIN_SHARED_FLAT), y)
>> TARGET_LDFLAGS += -mid-shared-library -mshared-library-id=0
>> TARGET_CFLAGS += -mid-shared-library -mshared-library-id=0
>> TARGET_CXXFLAGS += -mid-shared-library -mshared-library-id=0
>> endif
>
> Same thing for these flags, package/Makefile.in.
>

OK.

>> CROSS_COMPILE_SHARED_ELF ?= bfin-linux-uclibc-
>
> No, this should use the existing variables to get the cross-compiler
> location.

CROSS_COMPILE_SHARED_ELF is not  the existing variables of current
cross-compiler location.
Blackfin has different cross-compiler for different ABI format.
bfin-linux-uclibc- compiler here is used to install the shared elf
libraries only when current cross-compiler is bfin-uclinux- and
current ABI is FLAT.
I prefer to move it to target/Config.arch.in

>
>> romfs.shared.libs.elf:
>>         set -e; \
>>         t=`$(CROSS_COMPILE_SHARED_ELF)gcc $(CPUFLAGS)
>> -print-file-name=libc.a`; \
>>         t=`dirname $$t`/../..; \
>>         for i in $$t/lib/*so*; do \
>>                 i=`readlink -f "$$i"`; \
>>                 soname=`$(CROSS_COMPILE_SHARED_ELF)readelf -d "$$i" |
>> sed -n '/(SONAME)/s:.*[[]\(.*\)[]].*:\1:p'`; \
>>                 $(INSTALL) -D $$i $(TARGET_DIR)/lib/$$soname; \
>>         done
>
> Why is this needed? How is this romfs.shared.libs.elf target being used?

The Blackfin Linux kernel supports to load both shared FLAT and shared
ELF libraries by applications from the same rootfs. Customers are
allowed to install ELF shared libraries into the rootfs which is built
with FLAT ABI flags. Vice versa.

I prefer to move the makefile targets to target/Makefile.in and move
the config options to target/Config.in.

>
>> CROSS_COMPILE_SHARED_FLAT ?= bfin-uclinux-
>> romfs.shared.libs.flat:
>>         set -e; \
>>         t=`$(CROSS_COMPILE_SHARED_FLAT)gcc $(CPUFLAGS)
>> -mid-shared-library -print-file-name=libc`; \
>>         if [ -f $$t -a ! -h $$t ] ; then \
>>                 $(INSTALL) -D $$t $(TARGET_DIR)/lib/lib1.so; \
>>         fi
>
> Ditto.

Ditto.

>
>> Config.in
>> ----------------------------------------------------
>> config BR2_TARGET_ANALOGDEVICES_INSTALL_ELF_SHARED
>>         bool "Install ELF shared libraries" if !BR2_BFIN_FDPIC
>>         default y
>>
>> config BR2_TARGET_ANALOGDEVICES_INSTALL_FLAT_SHARED
>>         bool "Install FLAT shared libraries" if !BR2_BFIN_SHARED_FLAT
>>         default y
>
> Any reason to have these in addition to the existing selection of ABI?
> What are the use cases for these?
>

Ditto

>> Blackfin Linux distribution has a different file system layout from
>> current default one in buildroot. And we have no plan to deviate from
>> current one used for years. Anyway without this patch, we can still
>> set a custom skeleton for blackfin.
>
> How does the filesystem layout differs? Depending on how it differs,
> having a separate skeleton may or may not be the right solution.
>

We want to have the roofs in buildroot for BF60x consist with the
rootfs in uClinux-dist for BF5xx used by customers for years. So, the
default skeleton is not a smart choice for us by now.


Regards,

Sonic Zhang

Patch

diff --git a/Makefile b/Makefile
index 079af65..ce81045 100644
--- a/Makefile
+++ b/Makefile
@@ -399,8 +399,10 @@  $(STAGING_DIR):
 	@ln -snf $(STAGING_DIR) $(BASE_DIR)/staging
 
 ifeq ($(BR2_ROOTFS_SKELETON_CUSTOM),y)
+ifneq ($(BR2_ROOTFS_SKELETON_CUSTOM_PATH),"")
 TARGET_SKELETON=$(BR2_ROOTFS_SKELETON_CUSTOM_PATH)
 endif
+endif
 
 $(BUILD_DIR)/.root:
 	mkdir -p $(TARGET_DIR)