[dpdk-dev] [PATCH v3] build: add pkg-config validation

[Bruce Richardson]

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 at 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 at 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
>