[v10,6/6] devtools: expand meson cross compiling test coverage

  The default test script covers only default host cc compiler, either gcc or
clang, the fix is to cover both, gcc and clang. And also the build dirs are
changed to *-host-$c, indicating the difference of cc used.

Fixes: a55277a788 ("devtools: add test script for meson builds")
Cc: stable@dpdk.org

Signed-off-by: Gavin Hu <gavin.hu@arm.com>
Reviewed-by: Phil Yang <phil.yang@arm.com>
Reviewed-by: Song Zhu <song.zhu@arm.com>
---
 devtools/test-meson-builds.sh | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)
  

On Fri, Jun 15, 2018 at 06:21:43PM +0800, Gavin Hu wrote:
> The default test script covers only default host cc compiler, either gcc or
> clang, the fix is to cover both, gcc and clang. And also the build dirs are
> changed to *-host-$c, indicating the difference of cc used.
> 
> Fixes: a55277a788 ("devtools: add test script for meson builds")
> Cc: stable@dpdk.org
> 
> Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> Reviewed-by: Phil Yang <phil.yang@arm.com>
> Reviewed-by: Song Zhu <song.zhu@arm.com>
> ---

Apologies, some late comments inline below that I missed on previous
reviews.

>  devtools/test-meson-builds.sh | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
> index 9bb5b93bd..f1553b7bd 100755
> --- a/devtools/test-meson-builds.sh
> +++ b/devtools/test-meson-builds.sh
> @@ -44,11 +44,20 @@ done
>  # test compilation with minimal x86 instruction set
>  build build-x86-default -Dmachine=nehalem
>  
> +# compile the general v8a also for clang to increase coverage
> +f=config/arm/arm64_armv8_linuxapp_gcc
> +export CC="ccache clang"
> +build build-$(basename $f | tr '_' '-' | cut -d'-' -f-2)-host-clang \
> +	--cross-file $f
> +

This might be better just as e.g.

+export CC="ccache clang"
+build build-arm64-host-clang --cross-file config/arm/arm64_armv8_linuxapp_gcc

to be shorter and more readable. You don't really need the armv8 part in
the directory name, as it's primarily a test of the clang host build, so
you can omit it to shorten the command to a single line with all hardcoded
values, which is more readable than a version with tr filename mangling.

>  # enable cross compilation if gcc cross-compiler is found
>  for f in config/arm/arm*gcc ; do
>  	c=aarch64-linux-gnu-gcc
>  	if ! command -v $c >/dev/null 2>&1 ; then
> -		continue
> +		echo "## ERROR: $c is missing..."
> +		exit 1

I think this if check should be moved out of the block, and be before the
test with clang, since that clang test will also fail.

>  	fi
> -	build build-$(basename $f | tr '_' '-' | cut -d'-' -f-2) --cross-file $f
> +	export CC="ccache gcc"
> +	build build-$(basename $f | tr '_' '-' | cut -d'-' -f-2)-host-gcc \
> +		--cross-file $f
>  done
> -- 
> 2.11.0
>
  

Hi Bruce,

I submitted v11, please have a look, thanks for your review.

Best Regards,
Gavin

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Friday, June 15, 2018 10:15 PM
> To: Gavin Hu <Gavin.Hu@arm.com>
> Cc: dev@dpdk.org; nd <nd@arm.com>; stable@dpdk.org
> Subject: Re: [dpdk-dev] [PATCH v10 6/6] devtools: expand meson cross
> compiling test coverage
> 
> On Fri, Jun 15, 2018 at 06:21:43PM +0800, Gavin Hu wrote:
> > The default test script covers only default host cc compiler, either
> > gcc or clang, the fix is to cover both, gcc and clang. And also the
> > build dirs are changed to *-host-$c, indicating the difference of cc used.
> >
> > Fixes: a55277a788 ("devtools: add test script for meson builds")
> > Cc: stable@dpdk.org
> >
> > Signed-off-by: Gavin Hu <gavin.hu@arm.com>
> > Reviewed-by: Phil Yang <phil.yang@arm.com>
> > Reviewed-by: Song Zhu <song.zhu@arm.com>
> > ---
> 
> Apologies, some late comments inline below that I missed on previous
> reviews.
> 
> >  devtools/test-meson-builds.sh | 13 +++++++++++--
> >  1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/devtools/test-meson-builds.sh
> > b/devtools/test-meson-builds.sh index 9bb5b93bd..f1553b7bd 100755
> > --- a/devtools/test-meson-builds.sh
> > +++ b/devtools/test-meson-builds.sh
> > @@ -44,11 +44,20 @@ done
> >  # test compilation with minimal x86 instruction set  build
> > build-x86-default -Dmachine=nehalem
> >
> > +# compile the general v8a also for clang to increase coverage
> > +f=config/arm/arm64_armv8_linuxapp_gcc
> > +export CC="ccache clang"
> > +build build-$(basename $f | tr '_' '-' | cut -d'-' -f-2)-host-clang \
> > +	--cross-file $f
> > +
> 
> This might be better just as e.g.
> 
> +export CC="ccache clang"
> +build build-arm64-host-clang --cross-file
> +config/arm/arm64_armv8_linuxapp_gcc
> 
> to be shorter and more readable. You don't really need the armv8 part in the
> directory name, as it's primarily a test of the clang host build, so you can omit
> it to shorten the command to a single line with all hardcoded values, which is
> more readable than a version with tr filename mangling.
> 
> >  # enable cross compilation if gcc cross-compiler is found  for f in
> > config/arm/arm*gcc ; do
> >  	c=aarch64-linux-gnu-gcc
> >  	if ! command -v $c >/dev/null 2>&1 ; then
> > -		continue
> > +		echo "## ERROR: $c is missing..."
> > +		exit 1
> 
> I think this if check should be moved out of the block, and be before the test
> with clang, since that clang test will also fail.
> 
> >  	fi
> > -	build build-$(basename $f | tr '_' '-' | cut -d'-' -f-2) --cross-file $f
> > +	export CC="ccache gcc"
> > +	build build-$(basename $f | tr '_' '-' | cut -d'-' -f-2)-host-gcc \
> > +		--cross-file $f
> >  done
> > --
> > 2.11.0
> >