From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 7A9C6A0521; Tue, 3 Nov 2020 11:20:35 +0100 (CET) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 2037FC8B6; Tue, 3 Nov 2020 11:09:37 +0100 (CET) Received: from mga01.intel.com (mga01.intel.com [192.55.52.88]) by dpdk.org (Postfix) with ESMTP id 0E41EC884 for ; Tue, 3 Nov 2020 11:09:33 +0100 (CET) IronPort-SDR: 6+bq8xFTxvwxRYrzDBkZVMJQsHspxCPUoU5VUJE048RJvlc/NG2rEeoYZxGPW/aHOombwfseKY 6xU9w5JD9YTg== X-IronPort-AV: E=McAfee;i="6000,8403,9793"; a="186873831" X-IronPort-AV: E=Sophos;i="5.77,447,1596524400"; d="scan'208";a="186873831" X-Amp-Result: SKIPPED(no attachment in message) X-Amp-File-Uploaded: False Received: from orsmga004.jf.intel.com ([10.7.209.38]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 03 Nov 2020 02:09:33 -0800 IronPort-SDR: gODGHsBsVioPvW0iboV7Cb4AYk1jXB2laObFCW7vzPmHuJ3Ttz+Mjv4RsFBdf2fmRJIjug3Rdn aKs/GrPm0atg== X-IronPort-AV: E=Sophos;i="5.77,447,1596524400"; d="scan'208";a="470740511" Received: from bricha3-mobl.ger.corp.intel.com ([10.249.45.202]) by orsmga004-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-SHA; 03 Nov 2020 02:09:29 -0800 Date: Tue, 3 Nov 2020 10:09:22 +0000 From: Bruce Richardson To: Gregory Etelson 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 Message-ID: <20201103100922.GB1132@bricha3-MOBL.ger.corp.intel.com> References: <20201029091638.26646-1-getelson@nvidia.com> <20201102193426.3295-1-getelson@nvidia.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20201102193426.3295-1-getelson@nvidia.com> Subject: Re: [dpdk-dev] [PATCH v3] build: add pkg-config validation X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" 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 All looks reasonably ok to me. Some suggestions inline below which might shorten and simplify the script a bit. Acked-by: Bruce Richardson > --- > 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 >