diff mbox series

[v3] build: add pkg-config validation

Message ID 20201102193426.3295-1-getelson@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series [v3] build: add pkg-config validation |

Checks

Context Check Description
ci/checkpatch warning coding style issues
ci/iol-broadcom-Functional success Functional Testing PASS
ci/iol-broadcom-Performance success Performance Testing PASS
ci/iol-intel-Performance success Performance Testing PASS
ci/iol-intel-Functional success Functional Testing PASS
ci/iol-mellanox-Performance success Performance Testing PASS
ci/iol-testing success Testing PASS
ci/Intel-compilation success Compilation OK

Commit Message

Gregory Etelson Nov. 2, 2020, 7:34 p.m. UTC 
  DPDK relies on pkg-config(1) to provide correct parameters for
compiler and linker used in application build.
Inaccurate build parameters, produced by pkg-config from DPDK .pc
files could fail application build or cause unpredicted results
during application runtime.

This patch validates host pkg-config utility and notifies about
known issues.

Signed-off-by: Gregory Etelson <getelson@nvidia.com>
---
 buildtools/pkg-config/meson.build           | 11 ++++++
 buildtools/pkg-config/pkgconfig-validate.sh | 43 +++++++++++++++++++++
 doc/guides/linux_gsg/sys_reqs.rst           |  5 +++
 3 files changed, 59 insertions(+)
 create mode 100755 buildtools/pkg-config/pkgconfig-validate.sh

Comments

Bruce Richardson Nov. 3, 2020, 10:09 a.m. UTC | #1 
On Mon, Nov 02, 2020 at 09:34:26PM +0200, Gregory Etelson wrote:
> DPDK relies on pkg-config(1) to provide correct parameters for
> compiler and linker used in application build.
> Inaccurate build parameters, produced by pkg-config from DPDK .pc
> files could fail application build or cause unpredicted results
> during application runtime.
> 
> This patch validates host pkg-config utility and notifies about
> known issues.
> 
> Signed-off-by: Gregory Etelson <getelson@nvidia.com>

All looks reasonably ok to me. Some suggestions inline below which might
shorten and simplify the script a bit.

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

> ---
>  buildtools/pkg-config/meson.build           | 11 ++++++
>  buildtools/pkg-config/pkgconfig-validate.sh | 43 +++++++++++++++++++++
>  doc/guides/linux_gsg/sys_reqs.rst           |  5 +++
>  3 files changed, 59 insertions(+)
>  create mode 100755 buildtools/pkg-config/pkgconfig-validate.sh
> 
> diff --git a/buildtools/pkg-config/meson.build b/buildtools/pkg-config/meson.build
> index 5f19304289..4f907d7638 100644
> --- a/buildtools/pkg-config/meson.build
> +++ b/buildtools/pkg-config/meson.build
> @@ -53,3 +53,14 @@ This is required for a number of static inline functions in the public headers.'
>  # For static linking with dependencies as shared libraries,
>  # the internal static libraries must be flagged explicitly.
>  run_command(py3, 'set-static-linker-flags.py', check: true)
> +
> +pkgconf = find_program('pkg-config', 'pkgconf', required: false)
> +if (pkgconf.found())
> +	cmd = run_command('./pkgconfig-validate.sh', pkgconf.path(),
> +			   check:false)
> +	if cmd.returncode() != 0
> +		version = run_command(pkgconf, '--version')
> +		warning('invalid pkg-config version @0@'.format(
> +			version.stdout().strip()))
> +	endif
> +endif
> diff --git a/buildtools/pkg-config/pkgconfig-validate.sh b/buildtools/pkg-config/pkgconfig-validate.sh
> new file mode 100755
> index 0000000000..4b3bd2a9e3
> --- /dev/null
> +++ b/buildtools/pkg-config/pkgconfig-validate.sh
> @@ -0,0 +1,43 @@
> +#! /bin/sh
> +# SPDX-License-Identifier: BSD-3-Clause
> +
> +# Statically linked private DPDK objects of form
> +# -l:file.a must be positionned between --whole-archive … --no-whole-archive
> +# linker parameters.
> +# Old pkg-config versions misplace --no-whole-archive parameter and put it
> +# next to --whole-archive.
> +test1_static_libs_order () {
> +	PKG_CONFIG_PATH="$PKG_CONFIG_PATH" \
> +	"$PKGCONF" --libs --static libdpdk | \
> +	grep -q 'whole-archive.*l:lib.*no-whole-archive'
> +	if test "$?" -ne 0 ; then
> +		echo "WARNING: invalid static libraries order"
> +		ret=1

Why not just set "ret=$?" before the condition check? Save having to
pre-init ret to 0 and having it as a global variable.

Also, since the meson.build file has the error printout, you can consider
dropping the warning text too, in which case you can have the function just
return the return-code from grep itself.

> +	fi
> +	return $ret
> +}
> +
> +if [ "$#" -ne 1 ]; then
> +	echo "$0: no pkg-config parameter"
> +	exit 1
> +fi
> +PKGCONF="$1"
> +
> +$PKGCONF --exists libdpdk
> +if [ $? -ne 0 ]; then
> +	# pkgconf could not locate libdpdk.pc from existing PKG_CONFIG_PATH
> +	# check meson template instead

Why bother checking first? Since all we care about is the pkg-config
behaviour, we can just always add on the path to PKG_CONFIG_PATH and
guarantee that way a dpdk file will be found.

> +	pc_file=$(find "$MESON_BUILD_ROOT" -type f -name 'libdpdk.pc' -quit)
> +	if [ ! -f "$pc_file" ]; then
> +		echo "$0: cannot locate libdpdk.pc"
> +		exit 1
> +	fi
> +	pc_dir=$(dirname "$pc_file")
> +	PKG_CONFIG_PATH="${PKG_CONFIG_PATH}:$pc_dir"
> +fi
> +
> +ret=0
> +test1_static_libs_order
> +if [ $ret -ne 0 ]; then
> +	exit $ret
> +fi

Rather than branching, why not just call "exit $ret"?

Given that the return code from the script will be the result of the last
command run, and if we are ok to dropping the print of the warning, I think
the test function can be dropped and the last line of the script just made
the call to pkg-config and grep.

> diff --git a/doc/guides/linux_gsg/sys_reqs.rst b/doc/guides/linux_gsg/sys_reqs.rst
> index 6ecdc04aa9..b67da05e13 100644
> --- a/doc/guides/linux_gsg/sys_reqs.rst
> +++ b/doc/guides/linux_gsg/sys_reqs.rst
> @@ -60,6 +60,11 @@ Compilation of the DPDK
>  
>  *   Linux kernel headers or sources required to build kernel modules.
>  
> +
> +**Known Issues:**
> +
> +*   pkg-config v0.27 supplied with RHEL-7 does not process correctly libdpdk.pc Libs.private section.
> +
>  .. note::
>  
>     Please ensure that the latest patches are applied to third party libraries
> -- 
> 2.28.0
>
Gregory Etelson Nov. 4, 2020, 8:38 a.m. UTC | #2 
Hello Bruce,

Thank you for the review.
I'll update the script and post a new patch to the mailing list.

Regards,
Gregory

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Tuesday, November 3, 2020 12:09
> To: Gregory Etelson <getelson@nvidia.com>
> Cc: dev@dpdk.org; bluca@debian.org; christian.ehrhardt@canonical.com;
> david.marchand@redhat.com; ktraynor@redhat.com; Matan Azrad
> <matan@nvidia.com>; Raslan Darawsheh <rasland@nvidia.com>; NBU-Contact-
> Thomas Monjalon <thomas@monjalon.net>
> Subject: Re: [PATCH v3] build: add pkg-config validation
> 
> External email: Use caution opening links or attachments
> 
> 
> On Mon, Nov 02, 2020 at 09:34:26PM +0200, Gregory Etelson wrote:
> > DPDK relies on pkg-config(1) to provide correct parameters for
> > compiler and linker used in application build.
> > Inaccurate build parameters, produced by pkg-config from DPDK .pc
> > files could fail application build or cause unpredicted results
> > during application runtime.
> >
> > This patch validates host pkg-config utility and notifies about
> > known issues.
> >
> > Signed-off-by: Gregory Etelson <getelson@nvidia.com>
> 
> All looks reasonably ok to me. Some suggestions inline below which might
> shorten and simplify the script a bit.
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> 
> > ---
> >  buildtools/pkg-config/meson.build           | 11 ++++++
> >  buildtools/pkg-config/pkgconfig-validate.sh | 43 +++++++++++++++++++++
> >  doc/guides/linux_gsg/sys_reqs.rst           |  5 +++
> >  3 files changed, 59 insertions(+)
> >  create mode 100755 buildtools/pkg-config/pkgconfig-validate.sh
> >
> > diff --git a/buildtools/pkg-config/meson.build b/buildtools/pkg-
> config/meson.build
> > index 5f19304289..4f907d7638 100644
> > --- a/buildtools/pkg-config/meson.build
> > +++ b/buildtools/pkg-config/meson.build
> > @@ -53,3 +53,14 @@ This is required for a number of static inline
> functions in the public headers.'
> >  # For static linking with dependencies as shared libraries,
> >  # the internal static libraries must be flagged explicitly.
> >  run_command(py3, 'set-static-linker-flags.py', check: true)
> > +
> > +pkgconf = find_program('pkg-config', 'pkgconf', required: false)
> > +if (pkgconf.found())
> > +     cmd = run_command('./pkgconfig-validate.sh', pkgconf.path(),
> > +                        check:false)
> > +     if cmd.returncode() != 0
> > +             version = run_command(pkgconf, '--version')
> > +             warning('invalid pkg-config version @0@'.format(
> > +                     version.stdout().strip()))
> > +     endif
> > +endif
> > diff --git a/buildtools/pkg-config/pkgconfig-validate.sh
> b/buildtools/pkg-config/pkgconfig-validate.sh
> > new file mode 100755
> > index 0000000000..4b3bd2a9e3
> > --- /dev/null
> > +++ b/buildtools/pkg-config/pkgconfig-validate.sh
> > @@ -0,0 +1,43 @@
> > +#! /bin/sh
> > +# SPDX-License-Identifier: BSD-3-Clause
> > +
> > +# Statically linked private DPDK objects of form
> > +# -l:file.a must be positionned between --whole-archive … --no-whole-
> archive
> > +# linker parameters.
> > +# Old pkg-config versions misplace --no-whole-archive parameter and put
> it
> > +# next to --whole-archive.
> > +test1_static_libs_order () {
> > +     PKG_CONFIG_PATH="$PKG_CONFIG_PATH" \
> > +     "$PKGCONF" --libs --static libdpdk | \
> > +     grep -q 'whole-archive.*l:lib.*no-whole-archive'
> > +     if test "$?" -ne 0 ; then
> > +             echo "WARNING: invalid static libraries order"
> > +             ret=1
> 
> Why not just set "ret=$?" before the condition check? Save having to
> pre-init ret to 0 and having it as a global variable.
> 
> Also, since the meson.build file has the error printout, you can consider
> dropping the warning text too, in which case you can have the function
> just
> return the return-code from grep itself.
> 
> > +     fi
> > +     return $ret
> > +}
> > +
> > +if [ "$#" -ne 1 ]; then
> > +     echo "$0: no pkg-config parameter"
> > +     exit 1
> > +fi
> > +PKGCONF="$1"
> > +
> > +$PKGCONF --exists libdpdk
> > +if [ $? -ne 0 ]; then
> > +     # pkgconf could not locate libdpdk.pc from existing
> PKG_CONFIG_PATH
> > +     # check meson template instead
> 
> Why bother checking first? Since all we care about is the pkg-config
> behaviour, we can just always add on the path to PKG_CONFIG_PATH and
> guarantee that way a dpdk file will be found.
> 
> > +     pc_file=$(find "$MESON_BUILD_ROOT" -type f -name 'libdpdk.pc' -
> quit)
> > +     if [ ! -f "$pc_file" ]; then
> > +             echo "$0: cannot locate libdpdk.pc"
> > +             exit 1
> > +     fi
> > +     pc_dir=$(dirname "$pc_file")
> > +     PKG_CONFIG_PATH="${PKG_CONFIG_PATH}:$pc_dir"
> > +fi
> > +
> > +ret=0
> > +test1_static_libs_order
> > +if [ $ret -ne 0 ]; then
> > +     exit $ret
> > +fi
> 
> Rather than branching, why not just call "exit $ret"?
> 
> Given that the return code from the script will be the result of the last
> command run, and if we are ok to dropping the print of the warning, I
> think
> the test function can be dropped and the last line of the script just made
> the call to pkg-config and grep.
> 
> > diff --git a/doc/guides/linux_gsg/sys_reqs.rst
> b/doc/guides/linux_gsg/sys_reqs.rst
> > index 6ecdc04aa9..b67da05e13 100644
> > --- a/doc/guides/linux_gsg/sys_reqs.rst
> > +++ b/doc/guides/linux_gsg/sys_reqs.rst
> > @@ -60,6 +60,11 @@ Compilation of the DPDK
> >
> >  *   Linux kernel headers or sources required to build kernel modules.
> >
> > +
> > +**Known Issues:**
> > +
> > +*   pkg-config v0.27 supplied with RHEL-7 does not process correctly
> libdpdk.pc Libs.private section.
> > +
> >  .. note::
> >
> >     Please ensure that the latest patches are applied to third party
> libraries
> > --
> > 2.28.0
> >
diff mbox series

Patch

diff --git a/buildtools/pkg-config/meson.build b/buildtools/pkg-config/meson.build
index 5f19304289..4f907d7638 100644
--- a/buildtools/pkg-config/meson.build
+++ b/buildtools/pkg-config/meson.build
@@ -53,3 +53,14 @@  This is required for a number of static inline functions in the public headers.'
 # For static linking with dependencies as shared libraries,
 # the internal static libraries must be flagged explicitly.
 run_command(py3, 'set-static-linker-flags.py', check: true)
+
+pkgconf = find_program('pkg-config', 'pkgconf', required: false)
+if (pkgconf.found())
+	cmd = run_command('./pkgconfig-validate.sh', pkgconf.path(),
+			   check:false)
+	if cmd.returncode() != 0
+		version = run_command(pkgconf, '--version')
+		warning('invalid pkg-config version @0@'.format(
+			version.stdout().strip()))
+	endif
+endif
diff --git a/buildtools/pkg-config/pkgconfig-validate.sh b/buildtools/pkg-config/pkgconfig-validate.sh
new file mode 100755
index 0000000000..4b3bd2a9e3
--- /dev/null
+++ b/buildtools/pkg-config/pkgconfig-validate.sh
@@ -0,0 +1,43 @@ 
+#! /bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+
+# Statically linked private DPDK objects of form
+# -l:file.a must be positionned between --whole-archive … --no-whole-archive
+# linker parameters.
+# Old pkg-config versions misplace --no-whole-archive parameter and put it
+# next to --whole-archive.
+test1_static_libs_order () {
+	PKG_CONFIG_PATH="$PKG_CONFIG_PATH" \
+	"$PKGCONF" --libs --static libdpdk | \
+	grep -q 'whole-archive.*l:lib.*no-whole-archive'
+	if test "$?" -ne 0 ; then
+		echo "WARNING: invalid static libraries order"
+		ret=1
+	fi
+	return $ret
+}
+
+if [ "$#" -ne 1 ]; then
+	echo "$0: no pkg-config parameter"
+	exit 1
+fi
+PKGCONF="$1"
+
+$PKGCONF --exists libdpdk
+if [ $? -ne 0 ]; then
+	# pkgconf could not locate libdpdk.pc from existing PKG_CONFIG_PATH
+	# check meson template instead
+	pc_file=$(find "$MESON_BUILD_ROOT" -type f -name 'libdpdk.pc' -quit)
+	if [ ! -f "$pc_file" ]; then
+		echo "$0: cannot locate libdpdk.pc"
+		exit 1
+	fi
+	pc_dir=$(dirname "$pc_file")
+	PKG_CONFIG_PATH="${PKG_CONFIG_PATH}:$pc_dir"
+fi
+
+ret=0
+test1_static_libs_order
+if [ $ret -ne 0 ]; then
+	exit $ret
+fi
diff --git a/doc/guides/linux_gsg/sys_reqs.rst b/doc/guides/linux_gsg/sys_reqs.rst
index 6ecdc04aa9..b67da05e13 100644
--- a/doc/guides/linux_gsg/sys_reqs.rst
+++ b/doc/guides/linux_gsg/sys_reqs.rst
@@ -60,6 +60,11 @@  Compilation of the DPDK
 
 *   Linux kernel headers or sources required to build kernel modules.
 
+
+**Known Issues:**
+
+*   pkg-config v0.27 supplied with RHEL-7 does not process correctly libdpdk.pc Libs.private section.
+
 .. note::
 
    Please ensure that the latest patches are applied to third party libraries