From: Bruce Richardson <bruce.richardson@intel.com>
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@nvidia.com,
rasland@nvidia.com, thomas@monjalon.net
Subject: Re: [dpdk-dev] [PATCH v3] build: add pkg-config validation
Date: Tue, 3 Nov 2020 10:09:22 +0000 [thread overview]
Message-ID: <20201103100922.GB1132@bricha3-MOBL.ger.corp.intel.com> (raw)
In-Reply-To: <20201102193426.3295-1-getelson@nvidia.com>
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
>
next prev parent reply other threads:[~2020-11-03 10:20 UTC|newest]
Thread overview: 21+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-29 9:16 [dpdk-dev] [PATCH] " Gregory Etelson
2020-11-01 10:01 ` Thomas Monjalon
2020-11-01 12:06 ` Gregory Etelson
2020-11-02 6:45 ` [dpdk-dev] [PATCH v2] " Gregory Etelson
2020-11-02 12:11 ` Bruce Richardson
2020-11-02 19:39 ` Gregory Etelson
2020-11-02 19:34 ` [dpdk-dev] [PATCH v3] " Gregory Etelson
2020-11-03 10:09 ` Bruce Richardson [this message]
2020-11-04 8:38 ` Gregory Etelson
2020-11-05 12:37 ` [dpdk-dev] [PATCH v4] " Gregory Etelson
2020-11-05 13:17 ` Bruce Richardson
2020-11-13 13:38 ` David Marchand
2020-11-13 15:16 ` Gregory Etelson
2020-11-13 15:32 ` David Marchand
2020-11-17 18:17 ` [dpdk-dev] [PATCH] doc: notify bug in pkg-config v0.27 Gregory Etelson
2020-11-26 15:42 ` [dpdk-dev] [PATCH v2 1/1] doc: add pkg-config requirement for applications Thomas Monjalon
2020-11-26 16:24 ` Bruce Richardson
2020-11-26 16:38 ` Thomas Monjalon
2020-11-26 16:43 ` [dpdk-dev] [PATCH v3 " Thomas Monjalon
2020-11-26 16:46 ` Bruce Richardson
2020-11-27 0:59 ` Thomas Monjalon
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201103100922.GB1132@bricha3-MOBL.ger.corp.intel.com \
--to=bruce.richardson@intel.com \
--cc=bluca@debian.org \
--cc=christian.ehrhardt@canonical.com \
--cc=david.marchand@redhat.com \
--cc=dev@dpdk.org \
--cc=getelson@nvidia.com \
--cc=ktraynor@redhat.com \
--cc=matan@nvidia.com \
--cc=rasland@nvidia.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).