devtools: fix ninja break under default DESTDIR path
diff mbox series

Message ID 1594277585-15281-1-git-send-email-phil.yang@arm.com
State Superseded
Delegated to: David Marchand
Headers show
Series
  • devtools: fix ninja break under default DESTDIR path
Related show

Checks

Context Check Description
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK
ci/iol-broadcom-Performance success Performance Testing PASS
ci/travis-robot success Travis build: passed
ci/checkpatch success coding style OK

Commit Message

Phil Yang July 9, 2020, 6:53 a.m. UTC
If DPDK_ABI_REF_DIR is not set, the default DESTDIR is a relative path.
This will break ninja in the ABI check test.

Fixes: 777014e56d07 ("devtools: add ABI checks")

Signed-off-by: Phil Yang <phil.yang@arm.com>
---
 devtools/test-meson-builds.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Juraj Linkeš Sept. 17, 2020, 11:08 a.m. UTC | #1
> -----Original Message-----
> From: dev <dev-bounces@dpdk.org> On Behalf Of Phil Yang
> Sent: Thursday, July 9, 2020 8:53 AM
> To: david.marchand@redhat.com; dev@dpdk.org
> Cc: Honnappa.Nagarahalli@arm.com; Ruifeng.Wang@arm.com; nd@arm.com
> Subject: [dpdk-dev] [PATCH] devtools: fix ninja break under default DESTDIR
> path
> 
> If DPDK_ABI_REF_DIR is not set, the default DESTDIR is a relative path.
> This will break ninja in the ABI check test.
> 

The commit message is not very clear. The problem is we're specifying ninja's destination dir in variable DESTDIR when invoking ninja install. If the DESTDIR is not an absolute path, ninja complains (when invoking install). This isn't directly related to DPDK_ABI_REF_DIR, it's more a problem with how we call install_target.

> Fixes: 777014e56d07 ("devtools: add ABI checks")
> 
> Signed-off-by: Phil Yang <phil.yang@arm.com>
> ---
>  devtools/test-meson-builds.sh | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh index
> a87de63..2bfcaca 100755
> --- a/devtools/test-meson-builds.sh
> +++ b/devtools/test-meson-builds.sh
> @@ -143,7 +143,7 @@ build () # <directory> <target compiler | cross file>
> <meson options>
>  	config $srcdir $builds_dir/$targetdir $cross --werror $*
>  	compile $builds_dir/$targetdir
>  	if [ -n "$DPDK_ABI_REF_VERSION" ]; then
> -		abirefdir=${DPDK_ABI_REF_DIR:-
> reference}/$DPDK_ABI_REF_VERSION
> +		abirefdir=${DPDK_ABI_REF_DIR:-
> $(pwd)/reference}/$DPDK_ABI_REF_VERSION
>  		if [ ! -d $abirefdir/$targetdir ]; then
>  			# clone current sources
>  			if [ ! -d $abirefdir/src ]; then
> --
> 2.7.4
> 

install_target on line 160 is called improperly. Let's fix it so that it matches line 169:
install_target $abirefdir/build $(readlink -f $abirefdir/$targetdir)

This way we don't need to add $(pwd).
Phil Yang Sept. 20, 2020, 9:32 a.m. UTC | #2
Juraj Linkeš <juraj.linkes@pantheon.tech> writes:

> > Subject: [dpdk-dev] [PATCH] devtools: fix ninja break under default
> DESTDIR
> > path
> >
> > If DPDK_ABI_REF_DIR is not set, the default DESTDIR is a relative path.
> > This will break ninja in the ABI check test.
> >
> 
> The commit message is not very clear. The problem is we're specifying ninja's
> destination dir in variable DESTDIR when invoking ninja install. If the DESTDIR
> is not an absolute path, ninja complains (when invoking install). This isn't
Yes, this defect is due to 'ninja install' not happy with the relative path. 
Will reword the commit message in the next version.

> directly related to DPDK_ABI_REF_DIR, it's more a problem with how we call
> install_target.
If we have set 'DPDK_ABI_REF_DIR' before the test, 'abiref' should be an absolute path, but the default 'abiref' value is a relative path.
So should we keep them align?

> 
> > Fixes: 777014e56d07 ("devtools: add ABI checks")
> >
> > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > ---
> >  devtools/test-meson-builds.sh | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-
> builds.sh index
> > a87de63..2bfcaca 100755
> > --- a/devtools/test-meson-builds.sh
> > +++ b/devtools/test-meson-builds.sh
> > @@ -143,7 +143,7 @@ build () # <directory> <target compiler | cross file>
> > <meson options>
> >  	config $srcdir $builds_dir/$targetdir $cross --werror $*
> >  	compile $builds_dir/$targetdir
> >  	if [ -n "$DPDK_ABI_REF_VERSION" ]; then
> > -		abirefdir=${DPDK_ABI_REF_DIR:-
> > reference}/$DPDK_ABI_REF_VERSION
> > +		abirefdir=${DPDK_ABI_REF_DIR:-
> > $(pwd)/reference}/$DPDK_ABI_REF_VERSION
> >  		if [ ! -d $abirefdir/$targetdir ]; then
> >  			# clone current sources
> >  			if [ ! -d $abirefdir/src ]; then
> > --
> > 2.7.4
> >
> 
> install_target on line 160 is called improperly. Let's fix it so that it matches line
> 169:
> install_target $abirefdir/build $(readlink -f $abirefdir/$targetdir)

If so we need to call readlink at line #161 as well as it does at line #171.

> 
> This way we don't need to add $(pwd).

If you don't prefer $(pwd), we can do it like this:
abirefdir=$(readlink -f ${DPDK_ABI_REF_DIR:- reference}/$DPDK_ABI_REF_VERSION)

Thanks,
Phil
Juraj Linkeš Sept. 21, 2020, 6:19 a.m. UTC | #3
> -----Original Message-----
> From: Phil Yang <Phil.Yang@arm.com>
> Sent: Sunday, September 20, 2020 11:32 AM
> To: Juraj Linkeš <juraj.linkes@pantheon.tech>; david.marchand@redhat.com;
> dev@dpdk.org
> Cc: Honnappa Nagarahalli <Honnappa.Nagarahalli@arm.com>; Ruifeng Wang
> <Ruifeng.Wang@arm.com>; nd <nd@arm.com>; nd <nd@arm.com>
> Subject: RE: [dpdk-dev] [PATCH] devtools: fix ninja break under default DESTDIR
> path
> 
> Juraj Linkeš <juraj.linkes@pantheon.tech> writes:
> 
> > > Subject: [dpdk-dev] [PATCH] devtools: fix ninja break under default
> > DESTDIR
> > > path
> > >
> > > If DPDK_ABI_REF_DIR is not set, the default DESTDIR is a relative path.
> > > This will break ninja in the ABI check test.
> > >
> >
> > The commit message is not very clear. The problem is we're specifying
> > ninja's destination dir in variable DESTDIR when invoking ninja
> > install. If the DESTDIR is not an absolute path, ninja complains (when
> > invoking install). This isn't
> Yes, this defect is due to 'ninja install' not happy with the relative path.
> Will reword the commit message in the next version.
> 
> > directly related to DPDK_ABI_REF_DIR, it's more a problem with how we
> > call install_target.
> If we have set 'DPDK_ABI_REF_DIR' before the test, 'abiref' should be an
> absolute path, but the default 'abiref' value is a relative path.
> So should we keep them align?
> 
> >
> > > Fixes: 777014e56d07 ("devtools: add ABI checks")
> > >
> > > Signed-off-by: Phil Yang <phil.yang@arm.com>
> > > ---
> > >  devtools/test-meson-builds.sh | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-
> > builds.sh index
> > > a87de63..2bfcaca 100755
> > > --- a/devtools/test-meson-builds.sh
> > > +++ b/devtools/test-meson-builds.sh
> > > @@ -143,7 +143,7 @@ build () # <directory> <target compiler | cross
> > > file> <meson options>
> > >  	config $srcdir $builds_dir/$targetdir $cross --werror $*
> > >  	compile $builds_dir/$targetdir
> > >  	if [ -n "$DPDK_ABI_REF_VERSION" ]; then
> > > -		abirefdir=${DPDK_ABI_REF_DIR:-
> > > reference}/$DPDK_ABI_REF_VERSION
> > > +		abirefdir=${DPDK_ABI_REF_DIR:-
> > > $(pwd)/reference}/$DPDK_ABI_REF_VERSION
> > >  		if [ ! -d $abirefdir/$targetdir ]; then
> > >  			# clone current sources
> > >  			if [ ! -d $abirefdir/src ]; then
> > > --
> > > 2.7.4
> > >
> >
> > install_target on line 160 is called improperly. Let's fix it so that
> > it matches line
> > 169:
> > install_target $abirefdir/build $(readlink -f $abirefdir/$targetdir)
> 
> If so we need to call readlink at line #161 as well as it does at line #171.
> 
> >
> > This way we don't need to add $(pwd).
> 
> If you don't prefer $(pwd), we can do it like this:
> abirefdir=$(readlink -f ${DPDK_ABI_REF_DIR:-
> reference}/$DPDK_ABI_REF_VERSION)
> 

I mostly wanted the code to be consistent and since readlink was already in the code I pointed that out. I think your last proposal is the best of the two worlds, but let's change the code everywhere (i.e. lines 170-174 as well).

> Thanks,
> Phil
> 
>

Patch
diff mbox series

diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index a87de63..2bfcaca 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -143,7 +143,7 @@  build () # <directory> <target compiler | cross file> <meson options>
 	config $srcdir $builds_dir/$targetdir $cross --werror $*
 	compile $builds_dir/$targetdir
 	if [ -n "$DPDK_ABI_REF_VERSION" ]; then
-		abirefdir=${DPDK_ABI_REF_DIR:-reference}/$DPDK_ABI_REF_VERSION
+		abirefdir=${DPDK_ABI_REF_DIR:-$(pwd)/reference}/$DPDK_ABI_REF_VERSION
 		if [ ! -d $abirefdir/$targetdir ]; then
 			# clone current sources
 			if [ ! -d $abirefdir/src ]; then