[v4] build: add pkg-config validation

Message ID 20201105123704.15791-1-getelson@nvidia.com (mailing list archive)
State Superseded, archived
Delegated to: David Marchand
Headers
Series [v4] build: add pkg-config validation |

Checks

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

Commit Message

Gregory Etelson Nov. 5, 2020, 12:37 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 | 29 +++++++++++++++++++++
 doc/guides/linux_gsg/sys_reqs.rst           |  5 ++++
 3 files changed, 45 insertions(+)
 create mode 100755 buildtools/pkg-config/pkgconfig-validate.sh
  

Comments

Bruce Richardson Nov. 5, 2020, 1:17 p.m. UTC | #1
On Thu, Nov 05, 2020 at 02:37:03PM +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>
> ---

You can keep my ack from previous version.

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

One small note is that I don't believe the 'exit $?' line at the end of the
script is necessary, that is default shell behaviour. However, it probably
doesn't hurt to have it explicitly there.

>  buildtools/pkg-config/meson.build           | 11 ++++++++
>  buildtools/pkg-config/pkgconfig-validate.sh | 29 +++++++++++++++++++++
>  doc/guides/linux_gsg/sys_reqs.rst           |  5 ++++
>  3 files changed, 45 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..f5479f999f
> --- /dev/null
> +++ b/buildtools/pkg-config/pkgconfig-validate.sh
> @@ -0,0 +1,29 @@
> +#! /bin/sh
> +# SPDX-License-Identifier: BSD-3-Clause
> +
> +if [ "$#" -ne 1 ]; then
> +	echo "$0: no pkg-config parameter"
> +	exit 1
> +fi
> +PKGCONF="$1"
> +
> +# if pkgconf could not locate libdpdk.pc from existing PKG_CONFIG_PATH
> +# check meson template instead
> +# take the first located file
> +pc_file=$(find "$MESON_BUILD_ROOT" -type f -name 'libdpdk.pc' -print -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"
> +
> +# Statically linked private DPDK objects of form
> +# -l:file.a must be positioned between --whole-archive … --no-whole-archive
> +# linker parameters.
> +# Old pkg-config versions misplace --no-whole-archive parameter and put it
> +# next to --whole-archive.
> +PKG_CONFIG_PATH="$PKG_CONFIG_PATH" \
> +"$PKGCONF" --libs --static libdpdk | \
> +grep -q 'whole-archive.*l:lib.*no-whole-archive'
> +exit "$?"
> 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
>
  
David Marchand Nov. 13, 2020, 1:38 p.m. UTC | #2
On Thu, Nov 5, 2020 at 1:37 PM Gregory Etelson <getelson@nvidia.com> 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.

I am skeptical about this patch.
You want to inform application developers that linking against this
dpdk will fail?
The warning is displayed at configure time of the dpdk.
  
Gregory Etelson Nov. 13, 2020, 3:16 p.m. UTC | #3
Hello David

> -----Original Message-----
> > 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.
> 
> I am skeptical about this patch.
> You want to inform application developers that linking against this dpdk
> will fail?
> The warning is displayed at configure time of the dpdk.
> 
> 
> --
> David Marchand

The patch notifies about invalid pkg-config. Application that uses such
pkg-config will receive wrong linker parameters. These parameters will not
fail link procedure, but may produce invalid output and as the result,
application will have unpredicted behavior.
Optimal notification would be during application build procedure, but
I cannot see how it's possible to interact with an external build.

Regards,
Gregory
  
David Marchand Nov. 13, 2020, 3:32 p.m. UTC | #4
On Fri, Nov 13, 2020 at 4:16 PM Gregory Etelson <getelson@nvidia.com> wrote:
> The patch notifies about invalid pkg-config. Application that uses such
> pkg-config will receive wrong linker parameters. These parameters will not
> fail link procedure, but may produce invalid output and as the result,
> application will have unpredicted behavior.
> Optimal notification would be during application build procedure, but
> I cannot see how it's possible to interact with an external build.

Me neither, so I don't see the point in adding a warning the final
user won't catch.
Documenting this known issue seems enough.
  

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..f5479f999f
--- /dev/null
+++ b/buildtools/pkg-config/pkgconfig-validate.sh
@@ -0,0 +1,29 @@ 
+#! /bin/sh
+# SPDX-License-Identifier: BSD-3-Clause
+
+if [ "$#" -ne 1 ]; then
+	echo "$0: no pkg-config parameter"
+	exit 1
+fi
+PKGCONF="$1"
+
+# if pkgconf could not locate libdpdk.pc from existing PKG_CONFIG_PATH
+# check meson template instead
+# take the first located file
+pc_file=$(find "$MESON_BUILD_ROOT" -type f -name 'libdpdk.pc' -print -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"
+
+# Statically linked private DPDK objects of form
+# -l:file.a must be positioned between --whole-archive … --no-whole-archive
+# linker parameters.
+# Old pkg-config versions misplace --no-whole-archive parameter and put it
+# next to --whole-archive.
+PKG_CONFIG_PATH="$PKG_CONFIG_PATH" \
+"$PKGCONF" --libs --static libdpdk | \
+grep -q 'whole-archive.*l:lib.*no-whole-archive'
+exit "$?"
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