Patchwork [RFCv1,1/4] toolchain-external: split target installation from staging installation

login
register
mail settings
Submitter Thomas Petazzoni
Date June 7, 2014, 9:46 p.m.
Message ID <1402177567-8021-2-git-send-email-thomas.petazzoni@free-electrons.com>
Download mbox | patch
Permalink /patch/357160/
State Deferred
Headers show

Comments

Thomas Petazzoni - June 7, 2014, 9:46 p.m.
Currently, all the installation work of the toolchain-external package
is done during the install-staging step. However, in order to be able
to properly collect the size added by each package to the target
filesystem, we need to make sure that toolchain-external installs its
files to $(TARGET_DIR) during the install-target step.

Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>
---
 toolchain/toolchain-external/toolchain-external.mk | 36 +++++++++++++++++-----
 1 file changed, 29 insertions(+), 7 deletions(-)
Yann E. MORIN - June 9, 2014, 9:49 p.m.
Thomas, All,

On 2014-06-07 23:46 +0200, Thomas Petazzoni spake thusly:
> Currently, all the installation work of the toolchain-external package
> is done during the install-staging step. However, in order to be able
> to properly collect the size added by each package to the target
> filesystem, we need to make sure that toolchain-external installs its
> files to $(TARGET_DIR) during the install-target step.
> 
> Signed-off-by: Thomas Petazzoni <thomas.petazzoni@free-electrons.com>

Tested with a ARM code sourcery toolchain, and a custom toolchain.

Reviewed-by: "Yann E. MORIN" <yann.morin.1998@free.fr>
Tested-by: "Yann E. MORIN" <yann.morin.1998@free.fr>

But see my comment below...

> ---
>  toolchain/toolchain-external/toolchain-external.mk | 36 +++++++++++++++++-----
>  1 file changed, 29 insertions(+), 7 deletions(-)
> 
> diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk
> index c73cc4a..45926cf 100644
> --- a/toolchain/toolchain-external/toolchain-external.mk
> +++ b/toolchain/toolchain-external/toolchain-external.mk
> @@ -551,7 +551,7 @@ endif
>  #                       considered when searching libraries for copy
>  #                       to the target filesystem.
>  
> -define TOOLCHAIN_EXTERNAL_INSTALL_CORE
> +define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS
>  	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
>  	if test -z "$${SYSROOT_DIR}" ; then \
>  		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
> @@ -576,8 +576,6 @@ define TOOLCHAIN_EXTERNAL_INSTALL_CORE
>  			$(call copy_toolchain_lib_root,$${ARCH_SYSROOT_DIR},$${SUPPORT_LIB_DIR},$${ARCH_LIB_DIR},$$libs,/usr/lib); \
>  		done ; \
>  	fi ; \
> -	$(call MESSAGE,"Copying external toolchain sysroot to staging...") ; \
> -	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR}) ; \
>  	if test "$(BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY)" = "y"; then \
>  		$(call MESSAGE,"Copying gdbserver") ; \
>  		gdbserver_found=0 ; \
> @@ -595,6 +593,26 @@ define TOOLCHAIN_EXTERNAL_INSTALL_CORE
>  	fi
>  endef
>  
> +define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS

Maybe the code from here...

> +	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
> +	if test -z "$${SYSROOT_DIR}" ; then \
> +		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
> +		exit 1 ; \
> +	fi ; \
> +	ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> +	ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> +	SUPPORT_LIB_DIR="" ; \
> +	if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \
> +		LIBSTDCPP_A_LOCATION=$$(LANG=C $(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS) -print-file-name=libstdc++.a) ; \
> +		if [ -e "$${LIBSTDCPP_A_LOCATION}" ]; then \
> +			SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \
> +		fi ; \
> +	fi ; \
> +	ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \

... to here could be moved to a common function, so it can be shared
between the staging and target functions?

Otherwise, I'm afraid we might miss changing one or the other if we need
to make some changes.

But granted, that might not be that easy... In which case a big fat
warning above each block, to remind to keep the other in sync, would be
needed.

Regards,
Yann E. MORIN.

> +	$(call MESSAGE,"Copying external toolchain sysroot to staging...") ; \
> +	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR})
> +endef
> +
>  # Special installation target used on the Blackfin architecture when
>  # FDPIC is not the primary binary format being used, but the user has
>  # nonetheless requested the installation of the FDPIC libraries to the
> @@ -685,15 +703,19 @@ define TOOLCHAIN_EXTERNAL_INSTALL_GDBINIT
>  	fi
>  endef
>  
> +define TOOLCHAIN_EXTERNAL_INSTALL_STAGING_CMDS
> +	$(TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS)
> +	$(TOOLCHAIN_EXTERNAL_INSTALL_WRAPPER)
> +	$(TOOLCHAIN_EXTERNAL_INSTALL_GDBINIT)
> +endef
> +
>  # Even though we're installing things in both the staging, the host
>  # and the target directory, we do everything within the
>  # install-staging step, arbitrarily.
> -define TOOLCHAIN_EXTERNAL_INSTALL_STAGING_CMDS
> -	$(TOOLCHAIN_EXTERNAL_INSTALL_CORE)
> +define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_CMDS
> +	$(TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS)
>  	$(TOOLCHAIN_EXTERNAL_INSTALL_BFIN_FDPIC)
>  	$(TOOLCHAIN_EXTERNAL_INSTALL_BFIN_FLAT)
> -	$(TOOLCHAIN_EXTERNAL_INSTALL_WRAPPER)
> -	$(TOOLCHAIN_EXTERNAL_INSTALL_GDBINIT)
>  endef
>  
>  $(eval $(generic-package))
> -- 
> 2.0.0
> 
> _______________________________________________
> buildroot mailing list
> buildroot@busybox.net
> http://lists.busybox.net/mailman/listinfo/buildroot
Thomas Petazzoni - June 10, 2014, 8:04 a.m.
Dear Yann E. MORIN,

On Mon, 9 Jun 2014 23:49:28 +0200, Yann E. MORIN wrote:

> > +define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
> 
> Maybe the code from here...
> 
> > +	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
> > +	if test -z "$${SYSROOT_DIR}" ; then \
> > +		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
> > +		exit 1 ; \
> > +	fi ; \
> > +	ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> > +	ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> > +	SUPPORT_LIB_DIR="" ; \
> > +	if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \
> > +		LIBSTDCPP_A_LOCATION=$$(LANG=C $(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS) -print-file-name=libstdc++.a) ; \
> > +		if [ -e "$${LIBSTDCPP_A_LOCATION}" ]; then \
> > +			SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \
> > +		fi ; \
> > +	fi ; \
> > +	ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \
> 
> ... to here could be moved to a common function, so it can be shared
> between the staging and target functions?

How do you suggest this to be done? The problem is that we need those
variables to be defined within the shell block that follows. I don't
see any easy way to factorize that. Or maybe I should just take this
opportunity, and move some of this crap into a helper shell script,
which will avoid these horrible shell blocks with lots of quoting and
backslashes.

I'll take a look at this.

Best regards,

Thomas
Yann E. MORIN - June 10, 2014, 4:49 p.m.
Thomas, All,

On 2014-06-10 10:04 +0200, Thomas Petazzoni spake thusly:
> On Mon, 9 Jun 2014 23:49:28 +0200, Yann E. MORIN wrote:
> > > +define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
> > 
> > Maybe the code from here...
> > 
> > > +	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
> > > +	if test -z "$${SYSROOT_DIR}" ; then \
> > > +		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
> > > +		exit 1 ; \
> > > +	fi ; \
> > > +	ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> > > +	ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
> > > +	SUPPORT_LIB_DIR="" ; \
> > > +	if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \
> > > +		LIBSTDCPP_A_LOCATION=$$(LANG=C $(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS) -print-file-name=libstdc++.a) ; \
> > > +		if [ -e "$${LIBSTDCPP_A_LOCATION}" ]; then \
> > > +			SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \
> > > +		fi ; \
> > > +	fi ; \
> > > +	ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \
> > 
> > ... to here could be moved to a common function, so it can be shared
> > between the staging and target functions?
> 
> How do you suggest this to be done? The problem is that we need those
> variables to be defined within the shell block that follows. I don't
> see any easy way to factorize that. Or maybe I should just take this
> opportunity, and move some of this crap into a helper shell script,
> which will avoid these horrible shell blocks with lots of quoting and
> backslashes.

Hay, I said: "that might not be that easy..." ;-)

Fact is, I was just pointing out the code duplication, code which is not
trivial, and there is an opportunity for those two part to diverge if we
are not careful.

I don't know how we could do that sanely (without too much of double- or
quadruple-dollar signs all over...) I can have  look at it, if you want.

Maybe a big fat comment is all we really need here.

In the end, this is just an RFC, and there's room for improvements. ;-)
But that sure would be a very nice addition to Buildroot! :-)

Regards,
Yann e. MORIN.

Patch

diff --git a/toolchain/toolchain-external/toolchain-external.mk b/toolchain/toolchain-external/toolchain-external.mk
index c73cc4a..45926cf 100644
--- a/toolchain/toolchain-external/toolchain-external.mk
+++ b/toolchain/toolchain-external/toolchain-external.mk
@@ -551,7 +551,7 @@  endif
 #                       considered when searching libraries for copy
 #                       to the target filesystem.
 
-define TOOLCHAIN_EXTERNAL_INSTALL_CORE
+define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS
 	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
 	if test -z "$${SYSROOT_DIR}" ; then \
 		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
@@ -576,8 +576,6 @@  define TOOLCHAIN_EXTERNAL_INSTALL_CORE
 			$(call copy_toolchain_lib_root,$${ARCH_SYSROOT_DIR},$${SUPPORT_LIB_DIR},$${ARCH_LIB_DIR},$$libs,/usr/lib); \
 		done ; \
 	fi ; \
-	$(call MESSAGE,"Copying external toolchain sysroot to staging...") ; \
-	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR}) ; \
 	if test "$(BR2_TOOLCHAIN_EXTERNAL_GDB_SERVER_COPY)" = "y"; then \
 		$(call MESSAGE,"Copying gdbserver") ; \
 		gdbserver_found=0 ; \
@@ -595,6 +593,26 @@  define TOOLCHAIN_EXTERNAL_INSTALL_CORE
 	fi
 endef
 
+define TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS
+	$(Q)SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC))" ; \
+	if test -z "$${SYSROOT_DIR}" ; then \
+		@echo "External toolchain doesn't support --sysroot. Cannot use." ; \
+		exit 1 ; \
+	fi ; \
+	ARCH_SYSROOT_DIR="$(call toolchain_find_sysroot,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
+	ARCH_LIB_DIR="$(call toolchain_find_libdir,$(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS))" ; \
+	SUPPORT_LIB_DIR="" ; \
+	if test `find $${ARCH_SYSROOT_DIR} -name 'libstdc++.a' | wc -l` -eq 0 ; then \
+		LIBSTDCPP_A_LOCATION=$$(LANG=C $(TOOLCHAIN_EXTERNAL_CC) $(TOOLCHAIN_EXTERNAL_CFLAGS) -print-file-name=libstdc++.a) ; \
+		if [ -e "$${LIBSTDCPP_A_LOCATION}" ]; then \
+			SUPPORT_LIB_DIR=`readlink -f $${LIBSTDCPP_A_LOCATION} | sed -r -e 's:libstdc\+\+\.a::'` ; \
+		fi ; \
+	fi ; \
+	ARCH_SUBDIR=`echo $${ARCH_SYSROOT_DIR} | sed -r -e "s:^$${SYSROOT_DIR}(.*)/$$:\1:"` ; \
+	$(call MESSAGE,"Copying external toolchain sysroot to staging...") ; \
+	$(call copy_toolchain_sysroot,$${SYSROOT_DIR},$${ARCH_SYSROOT_DIR},$${ARCH_SUBDIR},$${ARCH_LIB_DIR},$${SUPPORT_LIB_DIR})
+endef
+
 # Special installation target used on the Blackfin architecture when
 # FDPIC is not the primary binary format being used, but the user has
 # nonetheless requested the installation of the FDPIC libraries to the
@@ -685,15 +703,19 @@  define TOOLCHAIN_EXTERNAL_INSTALL_GDBINIT
 	fi
 endef
 
+define TOOLCHAIN_EXTERNAL_INSTALL_STAGING_CMDS
+	$(TOOLCHAIN_EXTERNAL_INSTALL_SYSROOT_LIBS)
+	$(TOOLCHAIN_EXTERNAL_INSTALL_WRAPPER)
+	$(TOOLCHAIN_EXTERNAL_INSTALL_GDBINIT)
+endef
+
 # Even though we're installing things in both the staging, the host
 # and the target directory, we do everything within the
 # install-staging step, arbitrarily.
-define TOOLCHAIN_EXTERNAL_INSTALL_STAGING_CMDS
-	$(TOOLCHAIN_EXTERNAL_INSTALL_CORE)
+define TOOLCHAIN_EXTERNAL_INSTALL_TARGET_CMDS
+	$(TOOLCHAIN_EXTERNAL_INSTALL_TARGET_LIBS)
 	$(TOOLCHAIN_EXTERNAL_INSTALL_BFIN_FDPIC)
 	$(TOOLCHAIN_EXTERNAL_INSTALL_BFIN_FLAT)
-	$(TOOLCHAIN_EXTERNAL_INSTALL_WRAPPER)
-	$(TOOLCHAIN_EXTERNAL_INSTALL_GDBINIT)
 endef
 
 $(eval $(generic-package))