[3/4] devtools/test-meson-builds: add testing of pkg-config file

Message ID 20190423220644.54589-4-bruce.richardson@intel.com (mailing list archive)
State Superseded, archived
Headers
Series add testing of libdpdk pkg-config file |

Checks

Context Check Description
ci/checkpatch success coding style OK
ci/Intel-compilation success Compilation OK

Commit Message

Bruce Richardson April 23, 2019, 10:06 p.m. UTC
  The pkg-config file generated as part of the build of DPDK should allow
applications to be built with an installed DPDK. We can test this as
part of the build by doing an install of DPDK to a temporary directory
within the build folder, and by then compiling up a few sample apps
using make working off that directory.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 devtools/test-meson-builds.sh | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)
  

Comments

Luca Boccassi April 24, 2019, 9:22 a.m. UTC | #1
On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> The pkg-config file generated as part of the build of DPDK should
> allow
> applications to be built with an installed DPDK. We can test this as
> part of the build by doing an install of DPDK to a temporary
> directory
> within the build folder, and by then compiling up a few sample apps
> using make working off that directory.
> 
> Signed-off-by: Bruce Richardson <
> bruce.richardson@intel.com
> >
> ---
>  devtools/test-meson-builds.sh | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-
> builds.sh
> index 630a1a6fe..dfba2a782 100755
> --- a/devtools/test-meson-builds.sh
> +++ b/devtools/test-meson-builds.sh
> @@ -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then
>  			$use_shared --cross-file $f
>  	done
>  fi
> +
> +##############
> +# Test installation of the x86-default target, to be used for
> checking
> +# the sample apps build using the pkg-config file for cflags and
> libs
> +###############
> +build_path=build-x86-default
> +DESTDIR=`pwd`/$build_path/install-root ; export DESTDIR
> +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ; export
> PKG_CONFIG_PATH
> +$ninja_cmd -C $build_path install
> +
> +# rather than hacking our environment, just edit the .pc file prefix
> value
> +sed -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc

What about just using meson's prefix option instead? Which is how it
would be used in a real use case

> +for example in helloworld l2fwd l3fwd skeleton timer; do
> +	echo "## Building $example"
> +	make -C $DESTDIR/usr/local/share/dpdk/examples/$example
> +done
>
  
Bruce Richardson April 24, 2019, 10:41 a.m. UTC | #2
On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > The pkg-config file generated as part of the build of DPDK should allow
> > applications to be built with an installed DPDK. We can test this as
> > part of the build by doing an install of DPDK to a temporary directory
> > within the build folder, and by then compiling up a few sample apps
> > using make working off that directory.
> > 
> > Signed-off-by: Bruce Richardson < bruce.richardson@intel.com
> > >
> > --- devtools/test-meson-builds.sh | 17 +++++++++++++++++ 1 file
> > changed, 17 insertions(+)
> > 
> > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-
> > builds.sh index 630a1a6fe..dfba2a782 100755 ---
> > a/devtools/test-meson-builds.sh +++ b/devtools/test-meson-builds.sh @@
> > -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then $use_shared
> > --cross-file $f done fi + +############## +# Test installation of the
> > x86-default target, to be used for checking +# the sample apps build
> > using the pkg-config file for cflags and libs +###############
> > +build_path=build-x86-default +DESTDIR=`pwd`/$build_path/install-root ;
> > export DESTDIR +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ;
> > export PKG_CONFIG_PATH +$ninja_cmd -C $build_path install + +# rather
> > than hacking our environment, just edit the .pc file prefix value +sed
> > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> 
> What about just using meson's prefix option instead? Which is how it
> would be used in a real use case
> 
I don't think that would fully work, as my understanding is that the prefix
option would apply only to the /usr/local parts, but not to the kernel
modules which would still try and install in /lib/.

/Bruce
  
Luca Boccassi April 24, 2019, 11:02 a.m. UTC | #3
On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > The pkg-config file generated as part of the build of DPDK should
> > > allow
> > > applications to be built with an installed DPDK. We can test this
> > > as
> > > part of the build by doing an install of DPDK to a temporary
> > > directory
> > > within the build folder, and by then compiling up a few sample
> > > apps
> > > using make working off that directory.
> > > 
> > > Signed-off-by: Bruce Richardson < 
> > > bruce.richardson@intel.com
> > > 
> > > 
> > > --- devtools/test-meson-builds.sh | 17 +++++++++++++++++ 1 file
> > > changed, 17 insertions(+)
> > > 
> > > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-
> > > builds.sh index 630a1a6fe..dfba2a782 100755 ---
> > > a/devtools/test-meson-builds.sh +++ b/devtools/test-meson-
> > > builds.sh @@
> > > -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then
> > > $use_shared
> > > --cross-file $f done fi + +############## +# Test installation of
> > > the
> > > x86-default target, to be used for checking +# the sample apps
> > > build
> > > using the pkg-config file for cflags and libs +###############
> > > +build_path=build-x86-default +DESTDIR=`pwd`/$build_path/install-
> > > root ;
> > > export DESTDIR
> > > +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ;
> > > export PKG_CONFIG_PATH +$ninja_cmd -C $build_path install + +#
> > > rather
> > > than hacking our environment, just edit the .pc file prefix value
> > > +sed
> > > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> > 
> > What about just using meson's prefix option instead? Which is how
> > it
> > would be used in a real use case
> > 
> 
> I don't think that would fully work, as my understanding is that the
> prefix
> option would apply only to the /usr/local parts, but not to the
> kernel
> modules which would still try and install in /lib/.
> 
> /Bruce

What about doing a meson configure -Denable_kmods=false before the
ninja install? The modules are not needed for that test anyway, right?
Alternatively, the kernel src dir could be symlinked in the build path,
and the kernel_dir option could be used

I'm just worried that the test should be as "realistic" as possible, to
avoid missing something
  
Bruce Richardson April 24, 2019, 12:31 p.m. UTC | #4
On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi wrote:
> On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > > The pkg-config file generated as part of the build of DPDK should
> > > > allow
> > > > applications to be built with an installed DPDK. We can test this
> > > > as
> > > > part of the build by doing an install of DPDK to a temporary
> > > > directory
> > > > within the build folder, and by then compiling up a few sample
> > > > apps
> > > > using make working off that directory.
> > > > 
> > > > Signed-off-by: Bruce Richardson < 
> > > > bruce.richardson@intel.com
> > > > 
> > > > 
> > > > --- devtools/test-meson-builds.sh | 17 +++++++++++++++++ 1 file
> > > > changed, 17 insertions(+)
> > > > 
> > > > diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-
> > > > builds.sh index 630a1a6fe..dfba2a782 100755 ---
> > > > a/devtools/test-meson-builds.sh +++ b/devtools/test-meson-
> > > > builds.sh @@
> > > > -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then
> > > > $use_shared
> > > > --cross-file $f done fi + +############## +# Test installation of
> > > > the
> > > > x86-default target, to be used for checking +# the sample apps
> > > > build
> > > > using the pkg-config file for cflags and libs +###############
> > > > +build_path=build-x86-default +DESTDIR=`pwd`/$build_path/install-
> > > > root ;
> > > > export DESTDIR
> > > > +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ;
> > > > export PKG_CONFIG_PATH +$ninja_cmd -C $build_path install + +#
> > > > rather
> > > > than hacking our environment, just edit the .pc file prefix value
> > > > +sed
> > > > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> > > 
> > > What about just using meson's prefix option instead? Which is how
> > > it
> > > would be used in a real use case
> > > 
> > 
> > I don't think that would fully work, as my understanding is that the
> > prefix
> > option would apply only to the /usr/local parts, but not to the
> > kernel
> > modules which would still try and install in /lib/.
> > 
> > /Bruce
> 
> What about doing a meson configure -Denable_kmods=false before the
> ninja install? The modules are not needed for that test anyway, right?
> Alternatively, the kernel src dir could be symlinked in the build path,
> and the kernel_dir option could be used
> 
> I'm just worried that the test should be as "realistic" as possible, to
> avoid missing something
> 
Yes, I did think of that too, but it does mean doing another configuration
run in meson, and possibly a rebuild too if the rte_build_config.h file
changes. Therefore I decided to use DESTDIR for the sake of speed here. I
assumed there would be a pkg-config variable to adjust the output paths
based on a sysroot, but couldn't find a suitable one.

In any case, I'll see about changing things as you suggest in V2 -
correctness is more important that speed here.

/Bruce
  
Luca Boccassi April 24, 2019, 1:37 p.m. UTC | #5
On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi wrote:
> > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > > > The pkg-config file generated as part of the build of DPDK
> > > > > should
> > > > > allow
> > > > > applications to be built with an installed DPDK. We can test
> > > > > this
> > > > > as
> > > > > part of the build by doing an install of DPDK to a temporary
> > > > > directory
> > > > > within the build folder, and by then compiling up a few
> > > > > sample
> > > > > apps
> > > > > using make working off that directory.
> > > > > 
> > > > > Signed-off-by: Bruce Richardson < 
> > > > > bruce.richardson@intel.com
> > > > > 
> > > > > 
> > > > > 
> > > > > --- devtools/test-meson-builds.sh | 17 +++++++++++++++++ 1
> > > > > file
> > > > > changed, 17 insertions(+)
> > > > > 
> > > > > diff --git a/devtools/test-meson-builds.sh b/devtools/test-
> > > > > meson-
> > > > > builds.sh index 630a1a6fe..dfba2a782 100755 ---
> > > > > a/devtools/test-meson-builds.sh +++ b/devtools/test-meson-
> > > > > builds.sh @@
> > > > > -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then
> > > > > $use_shared
> > > > > --cross-file $f done fi + +############## +# Test
> > > > > installation of
> > > > > the
> > > > > x86-default target, to be used for checking +# the sample
> > > > > apps
> > > > > build
> > > > > using the pkg-config file for cflags and libs
> > > > > +###############
> > > > > +build_path=build-x86-default
> > > > > +DESTDIR=`pwd`/$build_path/install-
> > > > > root ;
> > > > > export DESTDIR
> > > > > +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ;
> > > > > export PKG_CONFIG_PATH +$ninja_cmd -C $build_path install +
> > > > > +#
> > > > > rather
> > > > > than hacking our environment, just edit the .pc file prefix
> > > > > value
> > > > > +sed
> > > > > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> > > > 
> > > > What about just using meson's prefix option instead? Which is
> > > > how
> > > > it
> > > > would be used in a real use case
> > > > 
> > > 
> > > I don't think that would fully work, as my understanding is that
> > > the
> > > prefix
> > > option would apply only to the /usr/local parts, but not to the
> > > kernel
> > > modules which would still try and install in /lib/.
> > > 
> > > /Bruce
> > 
> > What about doing a meson configure -Denable_kmods=false before the
> > ninja install? The modules are not needed for that test anyway,
> > right?
> > Alternatively, the kernel src dir could be symlinked in the build
> > path,
> > and the kernel_dir option could be used
> > 
> > I'm just worried that the test should be as "realistic" as
> > possible, to
> > avoid missing something
> > 
> 
> Yes, I did think of that too, but it does mean doing another
> configuration
> run in meson, and possibly a rebuild too if the rte_build_config.h
> file
> changes. Therefore I decided to use DESTDIR for the sake of speed
> here. I
> assumed there would be a pkg-config variable to adjust the output
> paths
> based on a sysroot, but couldn't find a suitable one.
> 
> In any case, I'll see about changing things as you suggest in V2 -
> correctness is more important that speed here.
> 
> /Bruce

There actually is a pkg-config binary option, I just tried and it works
(it seems to be disabled by default on Debian and derivatives): --
define-prefix
  
Bruce Richardson April 26, 2019, 2:56 p.m. UTC | #6
On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi wrote:
> > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > > > > The pkg-config file generated as part of the build of DPDK
> > > > > > should
> > > > > > allow
> > > > > > applications to be built with an installed DPDK. We can test
> > > > > > this
> > > > > > as
> > > > > > part of the build by doing an install of DPDK to a temporary
> > > > > > directory
> > > > > > within the build folder, and by then compiling up a few
> > > > > > sample
> > > > > > apps
> > > > > > using make working off that directory.
> > > > > > 
> > > > > > Signed-off-by: Bruce Richardson < 
> > > > > > bruce.richardson@intel.com
> > > > > > 
> > > > > > 
> > > > > > 
> > > > > > --- devtools/test-meson-builds.sh | 17 +++++++++++++++++ 1
> > > > > > file
> > > > > > changed, 17 insertions(+)
> > > > > > 
> > > > > > diff --git a/devtools/test-meson-builds.sh b/devtools/test-
> > > > > > meson-
> > > > > > builds.sh index 630a1a6fe..dfba2a782 100755 ---
> > > > > > a/devtools/test-meson-builds.sh +++ b/devtools/test-meson-
> > > > > > builds.sh @@
> > > > > > -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then
> > > > > > $use_shared
> > > > > > --cross-file $f done fi + +############## +# Test
> > > > > > installation of
> > > > > > the
> > > > > > x86-default target, to be used for checking +# the sample
> > > > > > apps
> > > > > > build
> > > > > > using the pkg-config file for cflags and libs
> > > > > > +###############
> > > > > > +build_path=build-x86-default
> > > > > > +DESTDIR=`pwd`/$build_path/install-
> > > > > > root ;
> > > > > > export DESTDIR
> > > > > > +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ;
> > > > > > export PKG_CONFIG_PATH +$ninja_cmd -C $build_path install +
> > > > > > +#
> > > > > > rather
> > > > > > than hacking our environment, just edit the .pc file prefix
> > > > > > value
> > > > > > +sed
> > > > > > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> > > > > 
> > > > > What about just using meson's prefix option instead? Which is
> > > > > how
> > > > > it
> > > > > would be used in a real use case
> > > > > 
> > > > 
> > > > I don't think that would fully work, as my understanding is that
> > > > the
> > > > prefix
> > > > option would apply only to the /usr/local parts, but not to the
> > > > kernel
> > > > modules which would still try and install in /lib/.
> > > > 
> > > > /Bruce
> > > 
> > > What about doing a meson configure -Denable_kmods=false before the
> > > ninja install? The modules are not needed for that test anyway,
> > > right?
> > > Alternatively, the kernel src dir could be symlinked in the build
> > > path,
> > > and the kernel_dir option could be used
> > > 
> > > I'm just worried that the test should be as "realistic" as
> > > possible, to
> > > avoid missing something
> > > 
> > 
> > Yes, I did think of that too, but it does mean doing another
> > configuration
> > run in meson, and possibly a rebuild too if the rte_build_config.h
> > file
> > changes. Therefore I decided to use DESTDIR for the sake of speed
> > here. I
> > assumed there would be a pkg-config variable to adjust the output
> > paths
> > based on a sysroot, but couldn't find a suitable one.
> > 
> > In any case, I'll see about changing things as you suggest in V2 -
> > correctness is more important that speed here.
> > 
> > /Bruce
> 
> There actually is a pkg-config binary option, I just tried and it works
> (it seems to be disabled by default on Debian and derivatives): --
> define-prefix
> 
Any cmdline options to pkg-config don't solve the problem here as it means
that the makefiles for any app need to be modified to use all those.

Also, I've been looking at the option you suggest of disabling the kernel
modules for the install step - the problem that this brings is that it either:
* disables them permanently for the default build, meaning subsequent runs
  may fail to catch errors
* causes us to constantly reconfigure the build directory with/without
  the kmod setting, causing unnecessary work and slowdown in the script.

A third solution is to use a separate build folder for the pkg-config test
builds, but I think we have enough builds already in the setup without adding
another one.

All-in-all, I feel at this point that the original solution of making a small
change to the pkg-config file manually is the best solution for now. I don't
see it as being terribly fragile, and it should catch 95% of problems with
the pkg-config files. I suggest that any rework be looked at in a later set
to improve things.

Regards,
/Bruce
  
Luca Boccassi April 26, 2019, 4:10 p.m. UTC | #7
On Fri, 2019-04-26 at 15:56 +0100, Bruce Richardson wrote:
> On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi wrote:
> > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi
> > > > > wrote:
> > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > > > > > The pkg-config file generated as part of the build of
> > > > > > > DPDK
> > > > > > > should
> > > > > > > allow
> > > > > > > applications to be built with an installed DPDK. We can
> > > > > > > test
> > > > > > > this
> > > > > > > as
> > > > > > > part of the build by doing an install of DPDK to a
> > > > > > > temporary
> > > > > > > directory
> > > > > > > within the build folder, and by then compiling up a few
> > > > > > > sample
> > > > > > > apps
> > > > > > > using make working off that directory.
> > > > > > > 
> > > > > > > Signed-off-by: Bruce Richardson < 
> > > > > > > bruce.richardson@intel.com
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > 
> > > > > > > --- devtools/test-meson-builds.sh | 17 +++++++++++++++++
> > > > > > > 1
> > > > > > > file
> > > > > > > changed, 17 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/devtools/test-meson-builds.sh
> > > > > > > b/devtools/test-
> > > > > > > meson-
> > > > > > > builds.sh index 630a1a6fe..dfba2a782 100755 ---
> > > > > > > a/devtools/test-meson-builds.sh +++ b/devtools/test-
> > > > > > > meson-
> > > > > > > builds.sh @@
> > > > > > > -90,3 +90,20 @@ if command -v $c >/dev/null 2>&1 ; then
> > > > > > > $use_shared
> > > > > > > --cross-file $f done fi + +############## +# Test
> > > > > > > installation of
> > > > > > > the
> > > > > > > x86-default target, to be used for checking +# the sample
> > > > > > > apps
> > > > > > > build
> > > > > > > using the pkg-config file for cflags and libs
> > > > > > > +###############
> > > > > > > +build_path=build-x86-default
> > > > > > > +DESTDIR=`pwd`/$build_path/install-
> > > > > > > root ;
> > > > > > > export DESTDIR
> > > > > > > +PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ;
> > > > > > > export PKG_CONFIG_PATH +$ninja_cmd -C $build_path install
> > > > > > > +
> > > > > > > +#
> > > > > > > rather
> > > > > > > than hacking our environment, just edit the .pc file
> > > > > > > prefix
> > > > > > > value
> > > > > > > +sed
> > > > > > > -i "s|prefix=|prefix=$DESTDIR|"
> > > > > > > $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > 
> > > > > > What about just using meson's prefix option instead? Which
> > > > > > is
> > > > > > how
> > > > > > it
> > > > > > would be used in a real use case
> > > > > > 
> > > > > 
> > > > > I don't think that would fully work, as my understanding is
> > > > > that
> > > > > the
> > > > > prefix
> > > > > option would apply only to the /usr/local parts, but not to
> > > > > the
> > > > > kernel
> > > > > modules which would still try and install in /lib/.
> > > > > 
> > > > > /Bruce
> > > > 
> > > > What about doing a meson configure -Denable_kmods=false before
> > > > the
> > > > ninja install? The modules are not needed for that test anyway,
> > > > right?
> > > > Alternatively, the kernel src dir could be symlinked in the
> > > > build
> > > > path,
> > > > and the kernel_dir option could be used
> > > > 
> > > > I'm just worried that the test should be as "realistic" as
> > > > possible, to
> > > > avoid missing something
> > > > 
> > > 
> > > Yes, I did think of that too, but it does mean doing another
> > > configuration
> > > run in meson, and possibly a rebuild too if the
> > > rte_build_config.h
> > > file
> > > changes. Therefore I decided to use DESTDIR for the sake of speed
> > > here. I
> > > assumed there would be a pkg-config variable to adjust the output
> > > paths
> > > based on a sysroot, but couldn't find a suitable one.
> > > 
> > > In any case, I'll see about changing things as you suggest in V2
> > > -
> > > correctness is more important that speed here.
> > > 
> > > /Bruce
> > 
> > There actually is a pkg-config binary option, I just tried and it
> > works
> > (it seems to be disabled by default on Debian and derivatives): --
> > define-prefix
> > 
> 
> Any cmdline options to pkg-config don't solve the problem here as it
> means
> that the makefiles for any app need to be modified to use all those.
> 
> Also, I've been looking at the option you suggest of disabling the
> kernel
> modules for the install step - the problem that this brings is that
> it either:
> * disables them permanently for the default build, meaning subsequent
> runs
>   may fail to catch errors
> * causes us to constantly reconfigure the build directory
> with/without
>   the kmod setting, causing unnecessary work and slowdown in the
> script.
> 
> A third solution is to use a separate build folder for the pkg-config 
> test
> builds, but I think we have enough builds already in the setup
> without adding
> another one.
> 
> All-in-all, I feel at this point that the original solution of making
> a small
> change to the pkg-config file manually is the best solution for now.
> I don't
> see it as being terribly fragile, and it should catch 95% of problems
> with
> the pkg-config files. I suggest that any rework be looked at in a
> later set
> to improve things.
> 
> Regards,
> /Bruce

Makes sense, I had hoped it would be easier - thanks for giving it a
shot!
  
Thomas Monjalon May 2, 2019, 1:11 p.m. UTC | #8
26/04/2019 16:56, Bruce Richardson:
> On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi wrote:
> > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > > > > > +#
> > > > > > > rather
> > > > > > > than hacking our environment, just edit the .pc file prefix
> > > > > > > value
> > > > > > > +sed
> > > > > > > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > 
> > > > > > What about just using meson's prefix option instead? Which is
> > > > > > how
> > > > > > it
> > > > > > would be used in a real use case
> > > > > 
> > > > > I don't think that would fully work, as my understanding is that
> > > > > the
> > > > > prefix
> > > > > option would apply only to the /usr/local parts, but not to the
> > > > > kernel
> > > > > modules which would still try and install in /lib/.
> > > > 
> > > > What about doing a meson configure -Denable_kmods=false before the
> > > > ninja install? The modules are not needed for that test anyway,
> > > > right?
> > > > Alternatively, the kernel src dir could be symlinked in the build
> > > > path,
> > > > and the kernel_dir option could be used
> > > > 
> > > > I'm just worried that the test should be as "realistic" as
> > > > possible, to
> > > > avoid missing something
> > > 
> > > Yes, I did think of that too, but it does mean doing another
> > > configuration
> > > run in meson, and possibly a rebuild too if the rte_build_config.h
> > > file
> > > changes. Therefore I decided to use DESTDIR for the sake of speed
> > > here. I
> > > assumed there would be a pkg-config variable to adjust the output
> > > paths
> > > based on a sysroot, but couldn't find a suitable one.
[...]
> > 
> > There actually is a pkg-config binary option, I just tried and it works
> > (it seems to be disabled by default on Debian and derivatives): --
> > define-prefix
> > 
> Any cmdline options to pkg-config don't solve the problem here as it means
> that the makefiles for any app need to be modified to use all those.

It looks to be a good solution.
Being able to update the DPDK install directory when building
an application should be a mandatory requirement.
I suggest to wrap the call to pkg-config so we can have this ability.
  
Bruce Richardson May 2, 2019, 1:17 p.m. UTC | #9
On Thu, May 02, 2019 at 03:11:10PM +0200, Thomas Monjalon wrote:
> 26/04/2019 16:56, Bruce Richardson:
> > On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi wrote:
> > > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi wrote:
> > > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson wrote:
> > > > > > > > +#
> > > > > > > > rather
> > > > > > > > than hacking our environment, just edit the .pc file prefix
> > > > > > > > value
> > > > > > > > +sed
> > > > > > > > -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > > 
> > > > > > > What about just using meson's prefix option instead? Which is
> > > > > > > how
> > > > > > > it
> > > > > > > would be used in a real use case
> > > > > > 
> > > > > > I don't think that would fully work, as my understanding is that
> > > > > > the
> > > > > > prefix
> > > > > > option would apply only to the /usr/local parts, but not to the
> > > > > > kernel
> > > > > > modules which would still try and install in /lib/.
> > > > > 
> > > > > What about doing a meson configure -Denable_kmods=false before the
> > > > > ninja install? The modules are not needed for that test anyway,
> > > > > right?
> > > > > Alternatively, the kernel src dir could be symlinked in the build
> > > > > path,
> > > > > and the kernel_dir option could be used
> > > > > 
> > > > > I'm just worried that the test should be as "realistic" as
> > > > > possible, to
> > > > > avoid missing something
> > > > 
> > > > Yes, I did think of that too, but it does mean doing another
> > > > configuration
> > > > run in meson, and possibly a rebuild too if the rte_build_config.h
> > > > file
> > > > changes. Therefore I decided to use DESTDIR for the sake of speed
> > > > here. I
> > > > assumed there would be a pkg-config variable to adjust the output
> > > > paths
> > > > based on a sysroot, but couldn't find a suitable one.
> [...]
> > > 
> > > There actually is a pkg-config binary option, I just tried and it works
> > > (it seems to be disabled by default on Debian and derivatives): --
> > > define-prefix
> > > 
> > Any cmdline options to pkg-config don't solve the problem here as it means
> > that the makefiles for any app need to be modified to use all those.
> 
> It looks to be a good solution.
> Being able to update the DPDK install directory when building
> an application should be a mandatory requirement.
> I suggest to wrap the call to pkg-config so we can have this ability.
> 

I would have expected that this issue has already been solved for
packagers. I was surprised that I couldn't find an easy way to do so.
  
Luca Boccassi May 2, 2019, 2:08 p.m. UTC | #10
On Thu, 2019-05-02 at 14:17 +0100, Bruce Richardson wrote:
> On Thu, May 02, 2019 at 03:11:10PM +0200, Thomas Monjalon wrote:
> > 26/04/2019 16:56, Bruce Richardson:
> > > On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > > > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi
> > > > > wrote:
> > > > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi
> > > > > > > wrote:
> > > > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson
> > > > > > > > wrote:
> > > > > > > > > +#
> > > > > > > > > rather
> > > > > > > > > than hacking our environment, just edit the .pc file
> > > > > > > > > prefix
> > > > > > > > > value
> > > > > > > > > +sed
> > > > > > > > > -i "s|prefix=|prefix=$DESTDIR|"
> > > > > > > > > $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > > > 
> > > > > > > > What about just using meson's prefix option instead?
> > > > > > > > Which is
> > > > > > > > how
> > > > > > > > it
> > > > > > > > would be used in a real use case
> > > > > > > 
> > > > > > > I don't think that would fully work, as my understanding
> > > > > > > is that
> > > > > > > the
> > > > > > > prefix
> > > > > > > option would apply only to the /usr/local parts, but not
> > > > > > > to the
> > > > > > > kernel
> > > > > > > modules which would still try and install in /lib/.
> > > > > > 
> > > > > > What about doing a meson configure -Denable_kmods=false
> > > > > > before the
> > > > > > ninja install? The modules are not needed for that test
> > > > > > anyway,
> > > > > > right?
> > > > > > Alternatively, the kernel src dir could be symlinked in the
> > > > > > build
> > > > > > path,
> > > > > > and the kernel_dir option could be used
> > > > > > 
> > > > > > I'm just worried that the test should be as "realistic" as
> > > > > > possible, to
> > > > > > avoid missing something
> > > > > 
> > > > > Yes, I did think of that too, but it does mean doing another
> > > > > configuration
> > > > > run in meson, and possibly a rebuild too if the
> > > > > rte_build_config.h
> > > > > file
> > > > > changes. Therefore I decided to use DESTDIR for the sake of
> > > > > speed
> > > > > here. I
> > > > > assumed there would be a pkg-config variable to adjust the
> > > > > output
> > > > > paths
> > > > > based on a sysroot, but couldn't find a suitable one.
> > 
> > [...]
> > > > There actually is a pkg-config binary option, I just tried and
> > > > it works
> > > > (it seems to be disabled by default on Debian and derivatives):
> > > > --
> > > > define-prefix
> > > > 
> > > 
> > > Any cmdline options to pkg-config don't solve the problem here as
> > > it means
> > > that the makefiles for any app need to be modified to use all
> > > those.
> > 
> > It looks to be a good solution.
> > Being able to update the DPDK install directory when building
> > an application should be a mandatory requirement.
> > I suggest to wrap the call to pkg-config so we can have this
> > ability.
> > 
> 
> I would have expected that this issue has already been solved for
> packagers. I was surprised that I couldn't find an easy way to do so.

Packagers use standard paths - so it never becomes a problem.

If editing the file on the fly is not acceptable for the test script,
then I'd suggest to fall back to the pkg-config option, and just
document the need to use it in the docs for this scenarios.
  
Thomas Monjalon May 2, 2019, 3:11 p.m. UTC | #11
02/05/2019 16:08, Luca Boccassi:
> On Thu, 2019-05-02 at 14:17 +0100, Bruce Richardson wrote:
> > On Thu, May 02, 2019 at 03:11:10PM +0200, Thomas Monjalon wrote:
> > > 26/04/2019 16:56, Bruce Richardson:
> > > > On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > > > > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > > > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi
> > > > > > wrote:
> > > > > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi
> > > > > > > > wrote:
> > > > > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson
> > > > > > > > > wrote:
> > > > > > > > > > +#
> > > > > > > > > > rather
> > > > > > > > > > than hacking our environment, just edit the .pc file
> > > > > > > > > > prefix
> > > > > > > > > > value
> > > > > > > > > > +sed
> > > > > > > > > > -i "s|prefix=|prefix=$DESTDIR|"
> > > > > > > > > > $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > > > > 
> > > > > > > > > What about just using meson's prefix option instead?
> > > > > > > > > Which is
> > > > > > > > > how
> > > > > > > > > it
> > > > > > > > > would be used in a real use case
> > > > > > > > 
> > > > > > > > I don't think that would fully work, as my understanding
> > > > > > > > is that
> > > > > > > > the
> > > > > > > > prefix
> > > > > > > > option would apply only to the /usr/local parts, but not
> > > > > > > > to the
> > > > > > > > kernel
> > > > > > > > modules which would still try and install in /lib/.
> > > > > > > 
> > > > > > > What about doing a meson configure -Denable_kmods=false
> > > > > > > before the
> > > > > > > ninja install? The modules are not needed for that test
> > > > > > > anyway,
> > > > > > > right?
> > > > > > > Alternatively, the kernel src dir could be symlinked in the
> > > > > > > build
> > > > > > > path,
> > > > > > > and the kernel_dir option could be used
> > > > > > > 
> > > > > > > I'm just worried that the test should be as "realistic" as
> > > > > > > possible, to
> > > > > > > avoid missing something
> > > > > > 
> > > > > > Yes, I did think of that too, but it does mean doing another
> > > > > > configuration
> > > > > > run in meson, and possibly a rebuild too if the
> > > > > > rte_build_config.h
> > > > > > file
> > > > > > changes. Therefore I decided to use DESTDIR for the sake of
> > > > > > speed
> > > > > > here. I
> > > > > > assumed there would be a pkg-config variable to adjust the
> > > > > > output
> > > > > > paths
> > > > > > based on a sysroot, but couldn't find a suitable one.
> > > 
> > > [...]
> > > > > There actually is a pkg-config binary option, I just tried and
> > > > > it works
> > > > > (it seems to be disabled by default on Debian and derivatives):
> > > > > --
> > > > > define-prefix
> > > > > 
> > > > 
> > > > Any cmdline options to pkg-config don't solve the problem here as
> > > > it means
> > > > that the makefiles for any app need to be modified to use all
> > > > those.
> > > 
> > > It looks to be a good solution.
> > > Being able to update the DPDK install directory when building
> > > an application should be a mandatory requirement.
> > > I suggest to wrap the call to pkg-config so we can have this
> > > ability.
> > > 
> > 
> > I would have expected that this issue has already been solved for
> > packagers. I was surprised that I couldn't find an easy way to do so.
> 
> Packagers use standard paths - so it never becomes a problem.
> 
> If editing the file on the fly is not acceptable for the test script,
> then I'd suggest to fall back to the pkg-config option, and just
> document the need to use it in the docs for this scenarios.

What I mean is that we should be able to compile our apps
after using DESTDIR, not related to the test script.
Can we use an environment variable like RTE_TARGET? DPDK_DIR?
  
Bruce Richardson May 2, 2019, 3:30 p.m. UTC | #12
On Thu, May 02, 2019 at 05:11:30PM +0200, Thomas Monjalon wrote:
> 02/05/2019 16:08, Luca Boccassi:
> > On Thu, 2019-05-02 at 14:17 +0100, Bruce Richardson wrote:
> > > On Thu, May 02, 2019 at 03:11:10PM +0200, Thomas Monjalon wrote:
> > > > 26/04/2019 16:56, Bruce Richardson:
> > > > > On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > > > > > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > > > > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi
> > > > > > > wrote:
> > > > > > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi
> > > > > > > > > wrote:
> > > > > > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson
> > > > > > > > > > wrote:
> > > > > > > > > > > +#
> > > > > > > > > > > rather
> > > > > > > > > > > than hacking our environment, just edit the .pc file
> > > > > > > > > > > prefix
> > > > > > > > > > > value
> > > > > > > > > > > +sed
> > > > > > > > > > > -i "s|prefix=|prefix=$DESTDIR|"
> > > > > > > > > > > $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > > > > > 
> > > > > > > > > > What about just using meson's prefix option instead?
> > > > > > > > > > Which is
> > > > > > > > > > how
> > > > > > > > > > it
> > > > > > > > > > would be used in a real use case
> > > > > > > > > 
> > > > > > > > > I don't think that would fully work, as my understanding
> > > > > > > > > is that
> > > > > > > > > the
> > > > > > > > > prefix
> > > > > > > > > option would apply only to the /usr/local parts, but not
> > > > > > > > > to the
> > > > > > > > > kernel
> > > > > > > > > modules which would still try and install in /lib/.
> > > > > > > > 
> > > > > > > > What about doing a meson configure -Denable_kmods=false
> > > > > > > > before the
> > > > > > > > ninja install? The modules are not needed for that test
> > > > > > > > anyway,
> > > > > > > > right?
> > > > > > > > Alternatively, the kernel src dir could be symlinked in the
> > > > > > > > build
> > > > > > > > path,
> > > > > > > > and the kernel_dir option could be used
> > > > > > > > 
> > > > > > > > I'm just worried that the test should be as "realistic" as
> > > > > > > > possible, to
> > > > > > > > avoid missing something
> > > > > > > 
> > > > > > > Yes, I did think of that too, but it does mean doing another
> > > > > > > configuration
> > > > > > > run in meson, and possibly a rebuild too if the
> > > > > > > rte_build_config.h
> > > > > > > file
> > > > > > > changes. Therefore I decided to use DESTDIR for the sake of
> > > > > > > speed
> > > > > > > here. I
> > > > > > > assumed there would be a pkg-config variable to adjust the
> > > > > > > output
> > > > > > > paths
> > > > > > > based on a sysroot, but couldn't find a suitable one.
> > > > 
> > > > [...]
> > > > > > There actually is a pkg-config binary option, I just tried and
> > > > > > it works
> > > > > > (it seems to be disabled by default on Debian and derivatives):
> > > > > > --
> > > > > > define-prefix
> > > > > > 
> > > > > 
> > > > > Any cmdline options to pkg-config don't solve the problem here as
> > > > > it means
> > > > > that the makefiles for any app need to be modified to use all
> > > > > those.
> > > > 
> > > > It looks to be a good solution.
> > > > Being able to update the DPDK install directory when building
> > > > an application should be a mandatory requirement.
> > > > I suggest to wrap the call to pkg-config so we can have this
> > > > ability.
> > > > 
> > > 
> > > I would have expected that this issue has already been solved for
> > > packagers. I was surprised that I couldn't find an easy way to do so.
> > 
> > Packagers use standard paths - so it never becomes a problem.
> > 
> > If editing the file on the fly is not acceptable for the test script,
> > then I'd suggest to fall back to the pkg-config option, and just
> > document the need to use it in the docs for this scenarios.
> 
> What I mean is that we should be able to compile our apps
> after using DESTDIR, not related to the test script.
> Can we use an environment variable like RTE_TARGET? DPDK_DIR?
> 
We can certainly add one to our example Makefiles, it's just a couple of
lines change to each one to add a prefix to the return value from
pkg-config. I can attempt to do so in a later version of this patch, though
I'd prefer to take more time over it than we have in 19.05.

Alternatively, we can defer the last couple of patches in this set to
19.08, though again I'd prefer to have even this level of minimal testing
of pkg-config into 19.05.

Let me know what you think is best?

/Bruce
  
Thomas Monjalon May 2, 2019, 3:38 p.m. UTC | #13
02/05/2019 17:30, Bruce Richardson:
> On Thu, May 02, 2019 at 05:11:30PM +0200, Thomas Monjalon wrote:
> > 02/05/2019 16:08, Luca Boccassi:
> > > On Thu, 2019-05-02 at 14:17 +0100, Bruce Richardson wrote:
> > > > On Thu, May 02, 2019 at 03:11:10PM +0200, Thomas Monjalon wrote:
> > > > > 26/04/2019 16:56, Bruce Richardson:
> > > > > > On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > > > > > > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > > > > > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi
> > > > > > > > wrote:
> > > > > > > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > > > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi
> > > > > > > > > > wrote:
> > > > > > > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson
> > > > > > > > > > > wrote:
> > > > > > > > > > > > +#
> > > > > > > > > > > > rather
> > > > > > > > > > > > than hacking our environment, just edit the .pc file
> > > > > > > > > > > > prefix
> > > > > > > > > > > > value
> > > > > > > > > > > > +sed
> > > > > > > > > > > > -i "s|prefix=|prefix=$DESTDIR|"
> > > > > > > > > > > > $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > > > > > > 
> > > > > > > > > > > What about just using meson's prefix option instead?
> > > > > > > > > > > Which is
> > > > > > > > > > > how
> > > > > > > > > > > it
> > > > > > > > > > > would be used in a real use case
> > > > > > > > > > 
> > > > > > > > > > I don't think that would fully work, as my understanding
> > > > > > > > > > is that
> > > > > > > > > > the
> > > > > > > > > > prefix
> > > > > > > > > > option would apply only to the /usr/local parts, but not
> > > > > > > > > > to the
> > > > > > > > > > kernel
> > > > > > > > > > modules which would still try and install in /lib/.
> > > > > > > > > 
> > > > > > > > > What about doing a meson configure -Denable_kmods=false
> > > > > > > > > before the
> > > > > > > > > ninja install? The modules are not needed for that test
> > > > > > > > > anyway,
> > > > > > > > > right?
> > > > > > > > > Alternatively, the kernel src dir could be symlinked in the
> > > > > > > > > build
> > > > > > > > > path,
> > > > > > > > > and the kernel_dir option could be used
> > > > > > > > > 
> > > > > > > > > I'm just worried that the test should be as "realistic" as
> > > > > > > > > possible, to
> > > > > > > > > avoid missing something
> > > > > > > > 
> > > > > > > > Yes, I did think of that too, but it does mean doing another
> > > > > > > > configuration
> > > > > > > > run in meson, and possibly a rebuild too if the
> > > > > > > > rte_build_config.h
> > > > > > > > file
> > > > > > > > changes. Therefore I decided to use DESTDIR for the sake of
> > > > > > > > speed
> > > > > > > > here. I
> > > > > > > > assumed there would be a pkg-config variable to adjust the
> > > > > > > > output
> > > > > > > > paths
> > > > > > > > based on a sysroot, but couldn't find a suitable one.
> > > > > 
> > > > > [...]
> > > > > > > There actually is a pkg-config binary option, I just tried and
> > > > > > > it works
> > > > > > > (it seems to be disabled by default on Debian and derivatives):
> > > > > > > --
> > > > > > > define-prefix
> > > > > > > 
> > > > > > 
> > > > > > Any cmdline options to pkg-config don't solve the problem here as
> > > > > > it means
> > > > > > that the makefiles for any app need to be modified to use all
> > > > > > those.
> > > > > 
> > > > > It looks to be a good solution.
> > > > > Being able to update the DPDK install directory when building
> > > > > an application should be a mandatory requirement.
> > > > > I suggest to wrap the call to pkg-config so we can have this
> > > > > ability.
> > > > > 
> > > > 
> > > > I would have expected that this issue has already been solved for
> > > > packagers. I was surprised that I couldn't find an easy way to do so.
> > > 
> > > Packagers use standard paths - so it never becomes a problem.
> > > 
> > > If editing the file on the fly is not acceptable for the test script,
> > > then I'd suggest to fall back to the pkg-config option, and just
> > > document the need to use it in the docs for this scenarios.
> > 
> > What I mean is that we should be able to compile our apps
> > after using DESTDIR, not related to the test script.
> > Can we use an environment variable like RTE_TARGET? DPDK_DIR?
> > 
> We can certainly add one to our example Makefiles, it's just a couple of
> lines change to each one to add a prefix to the return value from
> pkg-config. I can attempt to do so in a later version of this patch, though
> I'd prefer to take more time over it than we have in 19.05.
> 
> Alternatively, we can defer the last couple of patches in this set to
> 19.08, though again I'd prefer to have even this level of minimal testing
> of pkg-config into 19.05.
> 
> Let me know what you think is best?

I think it's best to merge only the fixes at this stage.
  
Bruce Richardson May 2, 2019, 3:41 p.m. UTC | #14
On Thu, May 02, 2019 at 05:38:17PM +0200, Thomas Monjalon wrote:
> 02/05/2019 17:30, Bruce Richardson:
> > On Thu, May 02, 2019 at 05:11:30PM +0200, Thomas Monjalon wrote:
> > > 02/05/2019 16:08, Luca Boccassi:
> > > > On Thu, 2019-05-02 at 14:17 +0100, Bruce Richardson wrote:
> > > > > On Thu, May 02, 2019 at 03:11:10PM +0200, Thomas Monjalon wrote:
> > > > > > 26/04/2019 16:56, Bruce Richardson:
> > > > > > > On Wed, Apr 24, 2019 at 02:37:58PM +0100, Luca Boccassi wrote:
> > > > > > > > On Wed, 2019-04-24 at 13:31 +0100, Bruce Richardson wrote:
> > > > > > > > > On Wed, Apr 24, 2019 at 12:02:24PM +0100, Luca Boccassi
> > > > > > > > > wrote:
> > > > > > > > > > On Wed, 2019-04-24 at 11:41 +0100, Bruce Richardson wrote:
> > > > > > > > > > > On Wed, Apr 24, 2019 at 10:22:04AM +0100, Luca Boccassi
> > > > > > > > > > > wrote:
> > > > > > > > > > > > On Tue, 2019-04-23 at 23:06 +0100, Bruce Richardson
> > > > > > > > > > > > wrote:
> > > > > > > > > > > > > +#
> > > > > > > > > > > > > rather
> > > > > > > > > > > > > than hacking our environment, just edit the .pc file
> > > > > > > > > > > > > prefix
> > > > > > > > > > > > > value
> > > > > > > > > > > > > +sed
> > > > > > > > > > > > > -i "s|prefix=|prefix=$DESTDIR|"
> > > > > > > > > > > > > $PKG_CONFIG_PATH/libdpdk.pc
> > > > > > > > > > > > 
> > > > > > > > > > > > What about just using meson's prefix option instead?
> > > > > > > > > > > > Which is
> > > > > > > > > > > > how
> > > > > > > > > > > > it
> > > > > > > > > > > > would be used in a real use case
> > > > > > > > > > > 
> > > > > > > > > > > I don't think that would fully work, as my understanding
> > > > > > > > > > > is that
> > > > > > > > > > > the
> > > > > > > > > > > prefix
> > > > > > > > > > > option would apply only to the /usr/local parts, but not
> > > > > > > > > > > to the
> > > > > > > > > > > kernel
> > > > > > > > > > > modules which would still try and install in /lib/.
> > > > > > > > > > 
> > > > > > > > > > What about doing a meson configure -Denable_kmods=false
> > > > > > > > > > before the
> > > > > > > > > > ninja install? The modules are not needed for that test
> > > > > > > > > > anyway,
> > > > > > > > > > right?
> > > > > > > > > > Alternatively, the kernel src dir could be symlinked in the
> > > > > > > > > > build
> > > > > > > > > > path,
> > > > > > > > > > and the kernel_dir option could be used
> > > > > > > > > > 
> > > > > > > > > > I'm just worried that the test should be as "realistic" as
> > > > > > > > > > possible, to
> > > > > > > > > > avoid missing something
> > > > > > > > > 
> > > > > > > > > Yes, I did think of that too, but it does mean doing another
> > > > > > > > > configuration
> > > > > > > > > run in meson, and possibly a rebuild too if the
> > > > > > > > > rte_build_config.h
> > > > > > > > > file
> > > > > > > > > changes. Therefore I decided to use DESTDIR for the sake of
> > > > > > > > > speed
> > > > > > > > > here. I
> > > > > > > > > assumed there would be a pkg-config variable to adjust the
> > > > > > > > > output
> > > > > > > > > paths
> > > > > > > > > based on a sysroot, but couldn't find a suitable one.
> > > > > > 
> > > > > > [...]
> > > > > > > > There actually is a pkg-config binary option, I just tried and
> > > > > > > > it works
> > > > > > > > (it seems to be disabled by default on Debian and derivatives):
> > > > > > > > --
> > > > > > > > define-prefix
> > > > > > > > 
> > > > > > > 
> > > > > > > Any cmdline options to pkg-config don't solve the problem here as
> > > > > > > it means
> > > > > > > that the makefiles for any app need to be modified to use all
> > > > > > > those.
> > > > > > 
> > > > > > It looks to be a good solution.
> > > > > > Being able to update the DPDK install directory when building
> > > > > > an application should be a mandatory requirement.
> > > > > > I suggest to wrap the call to pkg-config so we can have this
> > > > > > ability.
> > > > > > 
> > > > > 
> > > > > I would have expected that this issue has already been solved for
> > > > > packagers. I was surprised that I couldn't find an easy way to do so.
> > > > 
> > > > Packagers use standard paths - so it never becomes a problem.
> > > > 
> > > > If editing the file on the fly is not acceptable for the test script,
> > > > then I'd suggest to fall back to the pkg-config option, and just
> > > > document the need to use it in the docs for this scenarios.
> > > 
> > > What I mean is that we should be able to compile our apps
> > > after using DESTDIR, not related to the test script.
> > > Can we use an environment variable like RTE_TARGET? DPDK_DIR?
> > > 
> > We can certainly add one to our example Makefiles, it's just a couple of
> > lines change to each one to add a prefix to the return value from
> > pkg-config. I can attempt to do so in a later version of this patch, though
> > I'd prefer to take more time over it than we have in 19.05.
> > 
> > Alternatively, we can defer the last couple of patches in this set to
> > 19.08, though again I'd prefer to have even this level of minimal testing
> > of pkg-config into 19.05.
> > 
> > Let me know what you think is best?
> 
> I think it's best to merge only the fixes at this stage.
> 
> 
Ok, can patches 1-3 be merged, and I'll do a new patch for the -lbsd fix in
patch 5.

Thanks,
/Bruce
  

Patch

diff --git a/devtools/test-meson-builds.sh b/devtools/test-meson-builds.sh
index 630a1a6fe..dfba2a782 100755
--- a/devtools/test-meson-builds.sh
+++ b/devtools/test-meson-builds.sh
@@ -90,3 +90,20 @@  if command -v $c >/dev/null 2>&1 ; then
 			$use_shared --cross-file $f
 	done
 fi
+
+##############
+# Test installation of the x86-default target, to be used for checking
+# the sample apps build using the pkg-config file for cflags and libs
+###############
+build_path=build-x86-default
+DESTDIR=`pwd`/$build_path/install-root ; export DESTDIR
+PKG_CONFIG_PATH=$DESTDIR/usr/local/lib64/pkgconfig ; export PKG_CONFIG_PATH
+$ninja_cmd -C $build_path install
+
+# rather than hacking our environment, just edit the .pc file prefix value
+sed -i "s|prefix=|prefix=$DESTDIR|" $PKG_CONFIG_PATH/libdpdk.pc
+
+for example in helloworld l2fwd l3fwd skeleton timer; do
+	echo "## Building $example"
+	make -C $DESTDIR/usr/local/share/dpdk/examples/$example
+done