Re: [PATCH 1/8] populate_sdk_ext.bbclass: install multilib targets as populate_sdk does

[Paul Eggleton <paul.eggleton@linux.intel.com>]

On Wed, 14 Dec 2016 10:25:36 Robert Yang wrote:
> After I looked into the code again, these changes are related,
> so that I can't split them into 2, please see my comments inline.

I've replied inline.
 
> On 12/13/2016 12:55 PM, Paul Eggleton wrote:
> > There are a bunch of changes in here that don't relate to the multilib fix
> > - details below.
> > 
> > On Wed, 16 Nov 2016 22:19:30 Robert Yang wrote:
> >> Fixed:
> >> MACHINE = "qemux86-64"
> >> require conf/multilib.conf
> >> MULTILIBS = "multilib:lib32"
> >> DEFAULTTUNE_virtclass-multilib-lib32 = "x86"
> >> 
> >> $ bitbake core-image-minimal -cpopulate_sdk_ext
> >> [snip]
> >> Testing
> >> /buildarea/lyang1/test_po/tmp/work/qemux86_64-poky-linux/core-image-minim
> >> al
> >> /1.0-r0/testsdkext//tc/environment-setup-x86-pokymllib32-linux test_cvs
> >> (oeqa.sdk.buildcvs.BuildCvsTest) ... FAIL
> >> [snip]
> >> 
> >> It was failed because no lib32 toolchains.
> >> 
> >> The fixes include:
> >> * Set SDK_TARGETS correctly
> >> * Return multilib depends in get_ext_sdk_depends()
> >> * Write information to all environment-setup-* scripts.
> >> 
> >> [YOCTO #10647]
> >> 
> >> Signed-off-by: Robert Yang <liezhi.yang@windriver.com>
> >> ---
> >> 
> >>  meta/classes/populate_sdk_ext.bbclass | 61
> >> 
> >> ++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 23
> >> deletions(-)
> >> 
> >> diff --git a/meta/classes/populate_sdk_ext.bbclass
> >> b/meta/classes/populate_sdk_ext.bbclass index 26b5ca6..ce9c40a 100644
> >> --- a/meta/classes/populate_sdk_ext.bbclass
> >> +++ b/meta/classes/populate_sdk_ext.bbclass
> >> @@ -39,7 +39,7 @@ SDK_LOCAL_CONF_BLACKLIST ?= "CONF_VERSION \
> >> 
> >>  SDK_INHERIT_BLACKLIST ?= "buildhistory icecc"
> >>  SDK_UPDATE_URL ?= ""
> >> 
> >> -SDK_TARGETS ?= "${PN}"
> >> +SDK_TARGETS ?= "${@multilib_pkg_extend(d, d.getVar('BPN', True))}"
> >> 
> >>  def get_sdk_install_targets(d, images_only=False):
> >>      sdk_install_targets = ''
> >> 
> >> @@ -562,38 +562,52 @@ SDK_PRE_INSTALL_COMMAND_task-populate-sdk-ext =
> >> "${sdk_ext_preinst}" sdk_ext_postinst() {
> >> 
> >>  	printf "\nExtracting buildtools...\n"
> >>  	cd $target_sdk_dir
> >> 
> >> -	env_setup_script="$target_sdk_dir/environment-
> > 
> > setup-${REAL_MULTIMACH_TARGE
> > 
> >> T_SYS}" 
> >> -	printf "buildtools\ny" | ./${SDK_BUILDTOOLS_INSTALLER} >
> >> buildtools.log || { printf 'ERROR: buildtools installation failed:\n' ;
> >> cat buildtools.log ; echo "printf 'ERROR: this SDK was not fully
> >> installed and needs reinstalling\n'" >> $env_setup_script ; exit 1 ; }
> >> +	env_setup_scripts="`ls $target_sdk_dir/environment-setup-*`"
> >> +	./${SDK_BUILDTOOLS_INSTALLER} -d buildtools -y > buildtools.log
> >> +	if [ $? -ne 0 ]; then
> >> +		echo 'ERROR: buildtools installation failed:'
> >> +		cat buildtools.log
> >> +		for e in $env_setup_scripts; do
> >> +			echo "echo 'ERROR: this SDK was not fully installed and needs
> >> reinstalling'" >> $e +		done
> >> +		exit 1
> >> +	fi
> > 
> > This change isn't entirely related to the multilib changes.
> 
> The old code only considers one env_setup_script, but there are multiple
> ones now, so they are related.

Yes, I understand that. What I meant was, you've unrolled the single line 
*and* added handling for multiple env setup scripts - both are good changes 
but I'd like to see them in separate patches so that it's clear what's being 
done.

> >>  		# dash which is /bin/sh on Ubuntu will not preserve the
> >>  		# current working directory when first ran, nor will it set $1
> >>  		when
> >>  		# sourcing a script. That is why this has to look so ugly.
> >>  		LOGFILE="$target_sdk_dir/preparing_build_system.log"
> >> 
> >> -		sh -c ". buildtools/environment-setup* > $LOGFILE && cd
> >> $target_sdk_dir/`dirname ${oe_init_build_env_path}` && set
> >> $target_sdk_dir
> >> && . $target_sdk_dir/${oe_init_build_env_path} $target_sdk_dir >>
> >> $LOGFILE
> >> && python $target_sdk_dir/ext-sdk-prepare.py $LOGFILE
> >> '${SDK_INSTALL_TARGETS}'" || { echo "printf 'ERROR: this SDK was not
> >> fully
> >> installed and needs reinstalling\n'" >> $env_setup_script ; exit 1 ; }
> >> -		rm $target_sdk_dir/ext-sdk-prepare.py
> >> +		sh -c ". buildtools/environment-setup* > $LOGFILE && cd
> >> $target_sdk_dir/`dirname ${oe_init_build_env_path}` && set
> >> $target_sdk_dir
> >> && . $target_sdk_dir/${oe_init_build_env_path} $target_sdk_dir >>
> >> $LOGFILE
> >> && python $target_sdk_dir/ext-sdk-prepare.py $LOGFILE
> >> '${SDK_INSTALL_TARGETS}'" +		if [ $? -ne 0 ]; then
> >> +			for e in $env_setup_scripts; do
> >> +				echo "echo 'ERROR: this SDK was not fully installed and
> >> needs reinstalling'" >> $e +			done
> >> +			exit 1
> >> +		fi
> >> +		rm -f $target_sdk_dir/ext-sdk-prepare.py
> > 
> > That last line is also unrelated.
> 
> The old code only considers one env_setup_script, but there are multiple
> ones now, so they are related.

Specifically in the last line you are doing rm -f. That is a change from rm 
previously. Again, there's nothing wrong with that, but it's not related to 
the multilib handling.

Thanks,
Paul

-- 

Paul Eggleton
Intel Open Source Technology Centre