[ovs-dev] acinclude: Also use LIBS from dpkg pkg-config
diff mbox series

Message ID 20190124163813.23928-1-christian.ehrhardt@canonical.com
State Changes Requested
Headers show
Series
  • [ovs-dev] acinclude: Also use LIBS from dpkg pkg-config
Related show

Commit Message

Christian Ehrhardt Jan. 24, 2019, 4:38 p.m. UTC
DPDK 18.11 builds using the more modern meson build system no more
provide the -ldpdk linker script. Instead it is expected to use
pkgconfig for linker options as well.

This change will set DPDK_LIB from pkg-config (if pkg-config was
available) and since that already carries the whole-archive flags around
the PMDs skips the further wrapping in more whole-archive if that is
already part of DPDK_LIB.

Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
---
 acinclude.m4 | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

Comments

Christian Ehrhardt Jan. 24, 2019, 4:46 p.m. UTC | #1
On Thu, Jan 24, 2019 at 5:38 PM Christian Ehrhardt
<christian.ehrhardt@canonical.com> wrote:
>
> DPDK 18.11 builds using the more modern meson build system no more
> provide the -ldpdk linker script. Instead it is expected to use
> pkgconfig for linker options as well.

A single patch didn't seem right for a 0/1 into, but let me add some
further data to help your review in a reply.

Debian/Ubuntu already use the meson build system and therefore will
need this or a similar change.
FYI here an Ubuntu Openvswitch build log of OVS branch-2.11 with dpdk
enabled and this patch applied to get it building.
amd64: https://launchpadlibrarian.net/407992531/buildlog_ubuntu-disco-amd64.openvswitch_2.11.0~git20190121.4e4f80ec2-0ubuntu1~ppa4_BUILDING.txt.gz

> This change will set DPDK_LIB from pkg-config (if pkg-config was
> available) and since that already carries the whole-archive flags around
> the PMDs skips the further wrapping in more whole-archive if that is
> already part of DPDK_LIB.

FYI this is an example content a meson generated pkg-config file for DPDK:

prefix=/usr
libdir=${prefix}/lib/x86_64-linux-gnu
includedir=${prefix}/include/dpdk

Name: DPDK
Description: The Data Plane Development Kit (DPDK).
Note that CFLAGS might contain an -march flag higher than typical baseline.
This is required for a number of static inline functions in the public headers.
Version: 18.11.0
Requires.private: libbsd, zlib, libmnl, libmlx4, libibverbs, libmlx5, libcrypto
Libs: -L${libdir} -lrte_telemetry -lrte_bpf -lrte_flow_classify
-lrte_pipeline -lrte_table -lrte_port -lrte_vhost -lrte_security
-lrte_sched -lrte_reorder -lrte_rawdev -lrte_pdump -lrte_power
-lrte_meter -lrte_member -lrte_lpm -lrte_latencystats -lrte_kni
-lrte_jobstats -
lrte_ip_frag -lrte_gso -lrte_gro -lrte_eventdev -lrte_efd
-lrte_distributor -lrte_cryptodev -lrte_compressdev -lrte_cfgfile
-lrte_bitratestats -lrte_bbdev -lrte_acl -lrte_timer -lrte_hash
-lrte_metrics -lrte_pci -lrte_ethdev -lrte_net -lrte_mbuf
-lrte_mempool -lrte_ring -
lrte_cmdline -lrte_eal -lrte_kvargs
Libs.private: -L${libdir} -lrte_kvargs -lrte_eal -lrte_ring
-lrte_mempool -lrte_mbuf -lrte_pci -lrte_cryptodev -lrte_net
-lrte_cmdline -lrte_ethdev -lrte_hash -lrte_timer -lrte_common_dpaax
-lrte_eventdev -lrte_rawdev -lrte_bus_dpaa -lrte_bus_fslmc
-lrte_bus_pci -lrte_com
mon_octeontx -lrte_bus_vdev -lrte_meter -lrte_sched -lrte_ip_frag
-lrte_mempool_dpaa -lrte_mempool_dpaa2 -lrte_vhost -lrte_security
-lrte_kni -lrte_bus_vmbus -lrte_mempool_octeontx -lpcap -lrte_port
-lrte_lpm -lrte_acl -lrte_table -lrte_pipeline -lrte_gso -lIPSec_MB
-lIPS
ec_MB -lrte_common_cpt -lrte_reorder -lrte_compressdev -lrte_pmd_dpaa
-lrte_pmd_dpaa2 -lrte_pmd_dpaa2_sec -lrte_pmd_octeontx -lrte_bbdev
-lrte_bus_ifpga -Wl,--whole-archive -lrte_mempool_bucket
-lrte_mempool_ring -lrte_mempool_stack -lrte_pmd_af_packet
-lrte_pmd_ark -lrte
_pmd_atlantic -lrte_pmd_avf -lrte_pmd_avp -lrte_pmd_axgbe
-lrte_pmd_bond -lrte_pmd_bnx2x -lrte_pmd_bnxt -lrte_pmd_cxgbe
-lrte_pmd_e1000 -lrte_pmd_ena -lrte_pmd_enetc -lrte_pmd_enic
-lrte_pmd_failsafe -lrte_pmd_fm10k -lrte_pmd_i40e -lrte_pmd_ifc
-lrte_pmd_ixgbe -lrte_pmd_k
ni -lrte_pmd_liquidio -lrte_pmd_mlx4 -lrte_pmd_mlx5 -lrte_pmd_netvsc
-lrte_pmd_nfp -lrte_pmd_null -lrte_pmd_pcap -lrte_pmd_qede
-lrte_pmd_ring -lrte_pmd_sfc -lrte_pmd_softnic -lrte_pmd_tap
-lrte_pmd_thunderx -lrte_pmd_vdev_netvsc -lrte_pmd_vhost
-lrte_pmd_virtio -lrte_pmd
_vmxnet3 -lrte_pmd_aesni_gcm -lrte_pmd_aesni_mb -lrte_pmd_caam_jr
-lrte_pmd_ccp -lrte_pmd_dpaa_sec -lrte_pmd_null_crypto
-lrte_pmd_octeontx_crypto -lrte_pmd_openssl -lrte_pmd_crypto_scheduler
-lrte_pmd_virtio_crypto -lrte_pmd_octeontx_compress -lrte_pmd_qat
-lrte_pmd_zlib
-lrte_pmd_dpaa_event -lrte_pmd_dpaa2_event -lrte_pmd_octeontx_event
-lrte_pmd_opdl_event -lrte_pmd_skeleton_event -lrte_pmd_sw_event
-lrte_pmd_dsw_event -lrte_pmd_bbdev_null -lrte_pmd_skeleton_rawdev
-lrte_pmd_dpaa2_cmdif -lrte_pmd_dpaa2_qdma -lrte_pmd_ifpga_rawdev
-Wl,-
-no-whole-archive -Wl,-Bdynamic -Wl,--no-as-needed -pthread -lm -ldl -lnuma
Cflags: -I${includedir}/../x86_64-linux-gnu/dpdk -I${includedir}
-include rte_config.h -march=corei7



>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  acinclude.m4 | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index f038fd457..a45411860 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>      case "$with_dpdk" in
>        yes)
>          DPDK_AUTO_DISCOVER="true"
> -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> -                          [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"])
> +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> +                                 [DPDK_INCLUDE="$DPDK_CFLAGS", DPDK_LIB="$DPDK_LIBS"],
> +                                 [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk", DPDK_LIB="-ldpdk"])
>          ;;
>        *)
>          DPDK_AUTO_DISCOVER="false"
> @@ -238,10 +238,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>             DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
>          fi
>          DPDK_LIB_DIR="$with_dpdk/lib"
> +        DPDK_LIB="-ldpdk"
>          ;;
>      esac
>
> -    DPDK_LIB="-ldpdk"
>      DPDK_EXTRA_LIB=""
>
>      ovs_save_CFLAGS="$CFLAGS"
> @@ -346,7 +346,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>      #
>      # These options are specified inside a single -Wl directive to prevent
>      # autotools from reordering them.
> -    DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
> +    #
> +    # OTOH newer versions of dpdk pkg-config (generated with Meson)
> +    # will already have flagged just the right set of libs with
> +    # --whole-archive - in those cases do not wrap it once more.
> +    case "$DPDK_LIB" in
> +      *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
> +      *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
> +    esac
>      AC_SUBST([DPDK_vswitchd_LDFLAGS])
>      AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
>    fi
> --
> 2.17.1
>


--
Christian Ehrhardt
Software Engineer, Ubuntu Server
Canonical Ltd
Luca Boccassi Jan. 24, 2019, 4:57 p.m. UTC | #2
On Thu, 2019-01-24 at 17:38 +0100, Christian Ehrhardt wrote:
> DPDK 18.11 builds using the more modern meson build system no more
> provide the -ldpdk linker script. Instead it is expected to use
> pkgconfig for linker options as well.
> 
> This change will set DPDK_LIB from pkg-config (if pkg-config was
> available) and since that already carries the whole-archive flags
> around
> the PMDs skips the further wrapping in more whole-archive if that is
> already part of DPDK_LIB.
> 
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  acinclude.m4 | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/acinclude.m4 b/acinclude.m4
> index f038fd457..a45411860 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>      case "$with_dpdk" in
>        yes)
>          DPDK_AUTO_DISCOVER="true"
> -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> -                          [DPDK_INCLUDE="-I/usr/local/include/dpdk
> -I/usr/include/dpdk"])
> +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
> DPDK_LIB="$DPDK_LIBS"],
> +                                 [DPDK_INCLUDE="-
> I/usr/local/include/dpdk -I/usr/include/dpdk", DPDK_LIB="-ldpdk"])
>          ;;
>        *)
>          DPDK_AUTO_DISCOVER="false"
> @@ -238,10 +238,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>             DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
>          fi
>          DPDK_LIB_DIR="$with_dpdk/lib"
> +        DPDK_LIB="-ldpdk"
>          ;;
>      esac
>  
> -    DPDK_LIB="-ldpdk"
>      DPDK_EXTRA_LIB=""
>  
>      ovs_save_CFLAGS="$CFLAGS"
> @@ -346,7 +346,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>      #
>      # These options are specified inside a single -Wl directive to
> prevent
>      # autotools from reordering them.
> -    DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-
> archive
> +    #
> +    # OTOH newer versions of dpdk pkg-config (generated with Meson)
> +    # will already have flagged just the right set of libs with
> +    # --whole-archive - in those cases do not wrap it once more.
> +    case "$DPDK_LIB" in
> +      *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
> +      *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-
> whole-archive
> +    esac
>      AC_SUBST([DPDK_vswitchd_LDFLAGS])
>      AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
>    fi

Acked-by: Luca Boccassi <bluca@debian.org>
Aaron Conole Jan. 25, 2019, 4:28 p.m. UTC | #3
Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:

> DPDK 18.11 builds using the more modern meson build system no more
> provide the -ldpdk linker script. Instead it is expected to use
> pkgconfig for linker options as well.
>
> This change will set DPDK_LIB from pkg-config (if pkg-config was
> available) and since that already carries the whole-archive flags around
> the PMDs skips the further wrapping in more whole-archive if that is
> already part of DPDK_LIB.
>
> Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>
> ---
>  acinclude.m4 | 17 ++++++++++++-----
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index f038fd457..a45411860 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>      case "$with_dpdk" in
>        yes)
>          DPDK_AUTO_DISCOVER="true"
> -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> -                          [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"])
> +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> +                                 [DPDK_INCLUDE="$DPDK_CFLAGS", DPDK_LIB="$DPDK_LIBS"],
> +
> [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
> DPDK_LIB="-ldpdk"])

Something about this autoconf check caused all of the travis builds to
fail:

  https://travis-ci.org/ovsrobot/ovs/builds/483986081

The PKG_CHECK_MODULES_STATIC seems to have been added in a later version
of autoconf than is shipped in the travis environment, so it might be
something we need to test against and provide an alternative
implementation, ala the following bug report:

  https://bugs.freedesktop.org/show_bug.cgi?id=19541

Thinking about it, is there a reason to change from dynamic linking to
static linking?  Especially since we can achieve something similar via
the `PKG_CONFIG="$PKG_CONFIG --static"` override?

>          ;;
>        *)
>          DPDK_AUTO_DISCOVER="false"
> @@ -238,10 +238,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>             DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
>          fi
>          DPDK_LIB_DIR="$with_dpdk/lib"
> +        DPDK_LIB="-ldpdk"
>          ;;
>      esac
>  
> -    DPDK_LIB="-ldpdk"
>      DPDK_EXTRA_LIB=""
>  
>      ovs_save_CFLAGS="$CFLAGS"
> @@ -346,7 +346,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>      #
>      # These options are specified inside a single -Wl directive to prevent
>      # autotools from reordering them.
> -    DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
> +    #
> +    # OTOH newer versions of dpdk pkg-config (generated with Meson)
> +    # will already have flagged just the right set of libs with
> +    # --whole-archive - in those cases do not wrap it once more.
> +    case "$DPDK_LIB" in
> +      *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
> +      *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
> +    esac
>      AC_SUBST([DPDK_vswitchd_LDFLAGS])
>      AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
>    fi
Luca Boccassi Jan. 25, 2019, 4:39 p.m. UTC | #4
On Fri, 2019-01-25 at 11:28 -0500, Aaron Conole wrote:
> Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:
> 
> > DPDK 18.11 builds using the more modern meson build system no more
> > provide the -ldpdk linker script. Instead it is expected to use
> > pkgconfig for linker options as well.
> > 
> > This change will set DPDK_LIB from pkg-config (if pkg-config was
> > available) and since that already carries the whole-archive flags
> > around
> > the PMDs skips the further wrapping in more whole-archive if that
> > is
> > already part of DPDK_LIB.
> > 
> > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com
> > >
> > ---
> >  acinclude.m4 | 17 ++++++++++++-----
> >  1 file changed, 12 insertions(+), 5 deletions(-)
> > 
> > diff --git a/acinclude.m4 b/acinclude.m4
> > index f038fd457..a45411860 100644
> > --- a/acinclude.m4
> > +++ b/acinclude.m4
> > @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> >      case "$with_dpdk" in
> >        yes)
> >          DPDK_AUTO_DISCOVER="true"
> > -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> > -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> > -                          [DPDK_INCLUDE="-I/usr/local/include/dpdk 
> > -I/usr/include/dpdk"])
> > +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> > +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
> > DPDK_LIB="$DPDK_LIBS"],
> > +
> > [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
> > DPDK_LIB="-ldpdk"])
> 
> Something about this autoconf check caused all of the travis builds
> to
> fail:
> 
>   https://travis-ci.org/ovsrobot/ovs/builds/483986081
> 
> The PKG_CHECK_MODULES_STATIC seems to have been added in a later
> version
> of autoconf than is shipped in the travis environment, so it might be
> something we need to test against and provide an alternative
> implementation, ala the following bug report:
> 
>   https://bugs.freedesktop.org/show_bug.cgi?id=19541

Hi,

The PKG_CHECK* macros come from the pkg-config package, not autoconf,
in version 0.29. The version of pkg-config in Ubuntu 14.04 does not
indeed contain it, but 16.04 does.

If you go down the reimplementation route, I would recommend checking
for the macro version first and add a comment, so that you can then
drop it when compatibility with Ubuntu 14.04 and Debian 8 is no longer
required.

> Thinking about it, is there a reason to change from dynamic linking
> to
> static linking?  Especially since we can achieve something similar
> via
> the `PKG_CONFIG="$PKG_CONFIG --static"` override?

Note that this is not doing static linking - it's just getting the full
list of libs (which includes all the PMDs). The _STATIC macro is
implemented exactly as you suggested above.
Luca Boccassi Jan. 25, 2019, 4:45 p.m. UTC | #5
On Fri, 2019-01-25 at 16:39 +0000, Luca Boccassi wrote:
> On Fri, 2019-01-25 at 11:28 -0500, Aaron Conole wrote:
> > Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:
> > 
> > > DPDK 18.11 builds using the more modern meson build system no
> > > more
> > > provide the -ldpdk linker script. Instead it is expected to use
> > > pkgconfig for linker options as well.
> > > 
> > > This change will set DPDK_LIB from pkg-config (if pkg-config was
> > > available) and since that already carries the whole-archive flags
> > > around
> > > the PMDs skips the further wrapping in more whole-archive if that
> > > is
> > > already part of DPDK_LIB.
> > > 
> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.c
> > > om
> > > > 
> > > 
> > > ---
> > >  acinclude.m4 | 17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/acinclude.m4 b/acinclude.m4
> > > index f038fd457..a45411860 100644
> > > --- a/acinclude.m4
> > > +++ b/acinclude.m4
> > > @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> > >      case "$with_dpdk" in
> > >        yes)
> > >          DPDK_AUTO_DISCOVER="true"
> > > -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> > > -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> > > -                          [DPDK_INCLUDE="-
> > > I/usr/local/include/dpdk 
> > > -I/usr/include/dpdk"])
> > > +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> > > +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
> > > DPDK_LIB="$DPDK_LIBS"],
> > > +
> > > [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
> > > DPDK_LIB="-ldpdk"])
> > 
> > Something about this autoconf check caused all of the travis builds
> > to
> > fail:
> > 
> >   https://travis-ci.org/ovsrobot/ovs/builds/483986081
> > 
> > The PKG_CHECK_MODULES_STATIC seems to have been added in a later
> > version
> > of autoconf than is shipped in the travis environment, so it might
> > be
> > something we need to test against and provide an alternative
> > implementation, ala the following bug report:
> > 
> >   https://bugs.freedesktop.org/show_bug.cgi?id=19541
> 
> Hi,
> 
> The PKG_CHECK* macros come from the pkg-config package, not autoconf,
> in version 0.29. The version of pkg-config in Ubuntu 14.04 does not
> indeed contain it, but 16.04 does.
> 
> If you go down the reimplementation route, I would recommend checking
> for the macro version first and add a comment, so that you can then
> drop it when compatibility with Ubuntu 14.04 and Debian 8 is no
> longer
> required.

Also, these macro files are all versioned - autoconf is smart enough to
check the version if you have it locally, and if the system's is
higher, it will use that one instead, so there's no drawback to
backporting.

In other words, you can drop pkg.m4 version 0.29 in the m4/ directory
in the repository, add "m4_include([m4/pkg.m4])" to configure.ac and
then not worry about it anymore until compatibility with Ubuntu 14.04
and Debian 8 can be dropped.

> > Thinking about it, is there a reason to change from dynamic linking
> > to
> > static linking?  Especially since we can achieve something similar
> > via
> > the `PKG_CONFIG="$PKG_CONFIG --static"` override?
> 
> Note that this is not doing static linking - it's just getting the
> full
> list of libs (which includes all the PMDs). The _STATIC macro is
> implemented exactly as you suggested above.
>
Ben Pfaff Jan. 25, 2019, 4:50 p.m. UTC | #6
On Fri, Jan 25, 2019 at 04:39:15PM +0000, Luca Boccassi wrote:
> On Fri, 2019-01-25 at 11:28 -0500, Aaron Conole wrote:
> > Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:
> > 
> > > DPDK 18.11 builds using the more modern meson build system no more
> > > provide the -ldpdk linker script. Instead it is expected to use
> > > pkgconfig for linker options as well.
> > > 
> > > This change will set DPDK_LIB from pkg-config (if pkg-config was
> > > available) and since that already carries the whole-archive flags
> > > around
> > > the PMDs skips the further wrapping in more whole-archive if that
> > > is
> > > already part of DPDK_LIB.
> > > 
> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com
> > > >
> > > ---
> > >  acinclude.m4 | 17 ++++++++++++-----
> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> > > 
> > > diff --git a/acinclude.m4 b/acinclude.m4
> > > index f038fd457..a45411860 100644
> > > --- a/acinclude.m4
> > > +++ b/acinclude.m4
> > > @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> > >      case "$with_dpdk" in
> > >        yes)
> > >          DPDK_AUTO_DISCOVER="true"
> > > -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> > > -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> > > -                          [DPDK_INCLUDE="-I/usr/local/include/dpdk 
> > > -I/usr/include/dpdk"])
> > > +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> > > +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
> > > DPDK_LIB="$DPDK_LIBS"],
> > > +
> > > [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
> > > DPDK_LIB="-ldpdk"])
> > 
> > Something about this autoconf check caused all of the travis builds
> > to
> > fail:
> > 
> >   https://travis-ci.org/ovsrobot/ovs/builds/483986081
> > 
> > The PKG_CHECK_MODULES_STATIC seems to have been added in a later
> > version
> > of autoconf than is shipped in the travis environment, so it might be
> > something we need to test against and provide an alternative
> > implementation, ala the following bug report:
> > 
> >   https://bugs.freedesktop.org/show_bug.cgi?id=19541
> 
> Hi,
> 
> The PKG_CHECK* macros come from the pkg-config package, not autoconf,
> in version 0.29. The version of pkg-config in Ubuntu 14.04 does not
> indeed contain it, but 16.04 does.

Probably one can just copy pkg.m4 from pkg-config into the OVS m4/
directory.
Aaron Conole Jan. 25, 2019, 4:53 p.m. UTC | #7
Luca Boccassi <bluca@debian.org> writes:

> On Fri, 2019-01-25 at 16:39 +0000, Luca Boccassi wrote:
>> On Fri, 2019-01-25 at 11:28 -0500, Aaron Conole wrote:
>> > Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:
>> > 
>> > > DPDK 18.11 builds using the more modern meson build system no
>> > > more
>> > > provide the -ldpdk linker script. Instead it is expected to use
>> > > pkgconfig for linker options as well.
>> > > 
>> > > This change will set DPDK_LIB from pkg-config (if pkg-config was
>> > > available) and since that already carries the whole-archive flags
>> > > around
>> > > the PMDs skips the further wrapping in more whole-archive if that
>> > > is
>> > > already part of DPDK_LIB.
>> > > 
>> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.c
>> > > om
>> > > > 
>> > > 
>> > > ---
>> > >  acinclude.m4 | 17 ++++++++++++-----
>> > >  1 file changed, 12 insertions(+), 5 deletions(-)
>> > > 
>> > > diff --git a/acinclude.m4 b/acinclude.m4
>> > > index f038fd457..a45411860 100644
>> > > --- a/acinclude.m4
>> > > +++ b/acinclude.m4
>> > > @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>> > >      case "$with_dpdk" in
>> > >        yes)
>> > >          DPDK_AUTO_DISCOVER="true"
>> > > -        PKG_CHECK_MODULES([DPDK], [libdpdk],
>> > > -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
>> > > -                          [DPDK_INCLUDE="-
>> > > I/usr/local/include/dpdk 
>> > > -I/usr/include/dpdk"])
>> > > +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
>> > > +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
>> > > DPDK_LIB="$DPDK_LIBS"],
>> > > +
>> > > [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
>> > > DPDK_LIB="-ldpdk"])
>> > 
>> > Something about this autoconf check caused all of the travis builds
>> > to
>> > fail:
>> > 
>> >   https://travis-ci.org/ovsrobot/ovs/builds/483986081
>> > 
>> > The PKG_CHECK_MODULES_STATIC seems to have been added in a later
>> > version
>> > of autoconf than is shipped in the travis environment, so it might
>> > be
>> > something we need to test against and provide an alternative
>> > implementation, ala the following bug report:
>> > 
>> >   https://bugs.freedesktop.org/show_bug.cgi?id=19541
>> 
>> Hi,
>> 
>> The PKG_CHECK* macros come from the pkg-config package, not autoconf,
>> in version 0.29. The version of pkg-config in Ubuntu 14.04 does not
>> indeed contain it, but 16.04 does.
>> 
>> If you go down the reimplementation route, I would recommend checking
>> for the macro version first and add a comment, so that you can then
>> drop it when compatibility with Ubuntu 14.04 and Debian 8 is no
>> longer
>> required.
>
> Also, these macro files are all versioned - autoconf is smart enough to
> check the version if you have it locally, and if the system's is
> higher, it will use that one instead, so there's no drawback to
> backporting.

Cool, I didn't know that!

> In other words, you can drop pkg.m4 version 0.29 in the m4/ directory
> in the repository, add "m4_include([m4/pkg.m4])" to configure.ac and
> then not worry about it anymore until compatibility with Ubuntu 14.04
> and Debian 8 can be dropped.

That sounds like a solution.

>> > Thinking about it, is there a reason to change from dynamic linking
>> > to
>> > static linking?  Especially since we can achieve something similar
>> > via
>> > the `PKG_CONFIG="$PKG_CONFIG --static"` override?
>> 
>> Note that this is not doing static linking - it's just getting the
>> full
>> list of libs (which includes all the PMDs). The _STATIC macro is
>> implemented exactly as you suggested above.
>>
Christian Ehrhardt Jan. 29, 2019, 7:01 a.m. UTC | #8
On Fri, Jan 25, 2019 at 5:53 PM Aaron Conole <aconole@redhat.com> wrote:
>
> Luca Boccassi <bluca@debian.org> writes:
>
> > On Fri, 2019-01-25 at 16:39 +0000, Luca Boccassi wrote:
> >> On Fri, 2019-01-25 at 11:28 -0500, Aaron Conole wrote:
> >> > Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:
> >> >
> >> > > DPDK 18.11 builds using the more modern meson build system no
> >> > > more
> >> > > provide the -ldpdk linker script. Instead it is expected to use
> >> > > pkgconfig for linker options as well.
> >> > >
> >> > > This change will set DPDK_LIB from pkg-config (if pkg-config was
> >> > > available) and since that already carries the whole-archive flags
> >> > > around
> >> > > the PMDs skips the further wrapping in more whole-archive if that
> >> > > is
> >> > > already part of DPDK_LIB.
> >> > >
> >> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.c
> >> > > om
> >> > > >
> >> > >
> >> > > ---
> >> > >  acinclude.m4 | 17 ++++++++++++-----
> >> > >  1 file changed, 12 insertions(+), 5 deletions(-)
> >> > >
> >> > > diff --git a/acinclude.m4 b/acinclude.m4
> >> > > index f038fd457..a45411860 100644
> >> > > --- a/acinclude.m4
> >> > > +++ b/acinclude.m4
> >> > > @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> >> > >      case "$with_dpdk" in
> >> > >        yes)
> >> > >          DPDK_AUTO_DISCOVER="true"
> >> > > -        PKG_CHECK_MODULES([DPDK], [libdpdk],
> >> > > -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
> >> > > -                          [DPDK_INCLUDE="-
> >> > > I/usr/local/include/dpdk
> >> > > -I/usr/include/dpdk"])
> >> > > +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> >> > > +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
> >> > > DPDK_LIB="$DPDK_LIBS"],
> >> > > +
> >> > > [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
> >> > > DPDK_LIB="-ldpdk"])
> >> >
> >> > Something about this autoconf check caused all of the travis builds
> >> > to
> >> > fail:
> >> >
> >> >   https://travis-ci.org/ovsrobot/ovs/builds/483986081
> >> >
> >> > The PKG_CHECK_MODULES_STATIC seems to have been added in a later
> >> > version
> >> > of autoconf than is shipped in the travis environment, so it might
> >> > be
> >> > something we need to test against and provide an alternative
> >> > implementation, ala the following bug report:
> >> >
> >> >   https://bugs.freedesktop.org/show_bug.cgi?id=19541
> >>
> >> Hi,
> >>
> >> The PKG_CHECK* macros come from the pkg-config package, not autoconf,
> >> in version 0.29. The version of pkg-config in Ubuntu 14.04 does not
> >> indeed contain it, but 16.04 does.
> >>
> >> If you go down the reimplementation route, I would recommend checking
> >> for the macro version first and add a comment, so that you can then
> >> drop it when compatibility with Ubuntu 14.04 and Debian 8 is no
> >> longer
> >> required.
> >
> > Also, these macro files are all versioned - autoconf is smart enough to
> > check the version if you have it locally, and if the system's is
> > higher, it will use that one instead, so there's no drawback to
> > backporting.
>
> Cool, I didn't know that!
>
> > In other words, you can drop pkg.m4 version 0.29 in the m4/ directory
> > in the repository, add "m4_include([m4/pkg.m4])" to configure.ac and
> > then not worry about it anymore until compatibility with Ubuntu 14.04
> > and Debian 8 can be dropped.
>
> That sounds like a solution.

Ben/Aaron, thanks for sorting out that aspect of it!

That said, does it mean that the Travis failures now can be considered passing?

And if so what does it mean for the patch itself, do you consider it ok and
would merge it or are there other challenges that we need to tackle?

> >> > Thinking about it, is there a reason to change from dynamic linking
> >> > to
> >> > static linking?  Especially since we can achieve something similar
> >> > via
> >> > the `PKG_CONFIG="$PKG_CONFIG --static"` override?
> >>
> >> Note that this is not doing static linking - it's just getting the
> >> full
> >> list of libs (which includes all the PMDs). The _STATIC macro is
> >> implemented exactly as you suggested above.
> >>
Aaron Conole Jan. 29, 2019, 1:41 p.m. UTC | #9
Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:

> On Fri, Jan 25, 2019 at 5:53 PM Aaron Conole <aconole@redhat.com> wrote:
>>
>> Luca Boccassi <bluca@debian.org> writes:
>>
>> > On Fri, 2019-01-25 at 16:39 +0000, Luca Boccassi wrote:
>> >> On Fri, 2019-01-25 at 11:28 -0500, Aaron Conole wrote:
>> >> > Christian Ehrhardt <christian.ehrhardt@canonical.com> writes:
>> >> >
>> >> > > DPDK 18.11 builds using the more modern meson build system no
>> >> > > more
>> >> > > provide the -ldpdk linker script. Instead it is expected to use
>> >> > > pkgconfig for linker options as well.
>> >> > >
>> >> > > This change will set DPDK_LIB from pkg-config (if pkg-config was
>> >> > > available) and since that already carries the whole-archive flags
>> >> > > around
>> >> > > the PMDs skips the further wrapping in more whole-archive if that
>> >> > > is
>> >> > > already part of DPDK_LIB.
>> >> > >
>> >> > > Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.c
>> >> > > om
>> >> > > >
>> >> > >
>> >> > > ---
>> >> > >  acinclude.m4 | 17 ++++++++++++-----
>> >> > >  1 file changed, 12 insertions(+), 5 deletions(-)
>> >> > >
>> >> > > diff --git a/acinclude.m4 b/acinclude.m4
>> >> > > index f038fd457..a45411860 100644
>> >> > > --- a/acinclude.m4
>> >> > > +++ b/acinclude.m4
>> >> > > @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>> >> > >      case "$with_dpdk" in
>> >> > >        yes)
>> >> > >          DPDK_AUTO_DISCOVER="true"
>> >> > > -        PKG_CHECK_MODULES([DPDK], [libdpdk],
>> >> > > -                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
>> >> > > -                          [DPDK_INCLUDE="-
>> >> > > I/usr/local/include/dpdk
>> >> > > -I/usr/include/dpdk"])
>> >> > > +        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
>> >> > > +                                 [DPDK_INCLUDE="$DPDK_CFLAGS",
>> >> > > DPDK_LIB="$DPDK_LIBS"],
>> >> > > +
>> >> > > [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk",
>> >> > > DPDK_LIB="-ldpdk"])
>> >> >
>> >> > Something about this autoconf check caused all of the travis builds
>> >> > to
>> >> > fail:
>> >> >
>> >> >   https://travis-ci.org/ovsrobot/ovs/builds/483986081
>> >> >
>> >> > The PKG_CHECK_MODULES_STATIC seems to have been added in a later
>> >> > version
>> >> > of autoconf than is shipped in the travis environment, so it might
>> >> > be
>> >> > something we need to test against and provide an alternative
>> >> > implementation, ala the following bug report:
>> >> >
>> >> >   https://bugs.freedesktop.org/show_bug.cgi?id=19541
>> >>
>> >> Hi,
>> >>
>> >> The PKG_CHECK* macros come from the pkg-config package, not autoconf,
>> >> in version 0.29. The version of pkg-config in Ubuntu 14.04 does not
>> >> indeed contain it, but 16.04 does.
>> >>
>> >> If you go down the reimplementation route, I would recommend checking
>> >> for the macro version first and add a comment, so that you can then
>> >> drop it when compatibility with Ubuntu 14.04 and Debian 8 is no
>> >> longer
>> >> required.
>> >
>> > Also, these macro files are all versioned - autoconf is smart enough to
>> > check the version if you have it locally, and if the system's is
>> > higher, it will use that one instead, so there's no drawback to
>> > backporting.
>>
>> Cool, I didn't know that!
>>
>> > In other words, you can drop pkg.m4 version 0.29 in the m4/ directory
>> > in the repository, add "m4_include([m4/pkg.m4])" to configure.ac and
>> > then not worry about it anymore until compatibility with Ubuntu 14.04
>> > and Debian 8 can be dropped.
>>
>> That sounds like a solution.
>
> Ben/Aaron, thanks for sorting out that aspect of it!
>
> That said, does it mean that the Travis failures now can be considered passing?

Not quite yet.

> And if so what does it mean for the patch itself, do you consider it ok and
> would merge it or are there other challenges that we need to tackle?

I think if you made the adjustments suggested by Luca (copy pkg.m4 into
the m4/ directory in the ovs tree, add the m4_include([m4/pkg.m4])
directive to configure.ac) and then post a v2, it would be acceptable.

>> >> > Thinking about it, is there a reason to change from dynamic linking
>> >> > to
>> >> > static linking?  Especially since we can achieve something similar
>> >> > via
>> >> > the `PKG_CONFIG="$PKG_CONFIG --static"` override?
>> >>
>> >> Note that this is not doing static linking - it's just getting the
>> >> full
>> >> list of libs (which includes all the PMDs). The _STATIC macro is
>> >> implemented exactly as you suggested above.
>> >>
Luca Boccassi Jan. 30, 2019, 10:13 a.m. UTC | #10
On Wed, 2019-01-30 at 11:08 +0100, Christian Ehrhardt wrote:
> DPDK 18.11 can be built using the more modern meson build system.
> In that case it no more provides the -ldpdk linker script. Instead
> it is expected to use pkgconfig for linker options as well.
> 
> FYI here a build log on Ubuntu 19.04 with the three patches applied:
> https://launchpadlibrarian.net/409021278/buildlog_ubuntu-disco-amd64.
> openvswitch_2.11.0~git20190121.4e4f80ec2-
> 0ubuntu1~ppa5_BUILDING.txt.gz
> 
> Updates from v1:
> - add updated pkg.m4 to support PKG_CHECK_MODULES_STATIC
> - include local pkg.m4 in configure.ac
> 
> Christian Ehrhardt (3):
>   acinclude: Also use LIBS from dpkg pkg-config
>   m4: update pkg.m4 to pkg-config 0.29.1.
>   configure.ac: use the locally provided pkg.m4
> 
>  acinclude.m4 |  17 ++--
>  configure.ac |   2 +
>  m4/pkg.m4    | 217 +++++++++++++++++++++++++++++++++--------------
> ----
>  3 files changed, 153 insertions(+), 83 deletions(-)

Series-acked-by: Luca Boccassi <bluca@debian.org>

Patch
diff mbox series

diff --git a/acinclude.m4 b/acinclude.m4
index f038fd457..a45411860 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -223,9 +223,9 @@  AC_DEFUN([OVS_CHECK_DPDK], [
     case "$with_dpdk" in
       yes)
         DPDK_AUTO_DISCOVER="true"
-        PKG_CHECK_MODULES([DPDK], [libdpdk],
-                          [DPDK_INCLUDE="$DPDK_CFLAGS"],
-                          [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk"])
+        PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
+                                 [DPDK_INCLUDE="$DPDK_CFLAGS", DPDK_LIB="$DPDK_LIBS"],
+                                 [DPDK_INCLUDE="-I/usr/local/include/dpdk -I/usr/include/dpdk", DPDK_LIB="-ldpdk"])
         ;;
       *)
         DPDK_AUTO_DISCOVER="false"
@@ -238,10 +238,10 @@  AC_DEFUN([OVS_CHECK_DPDK], [
            DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
         fi
         DPDK_LIB_DIR="$with_dpdk/lib"
+        DPDK_LIB="-ldpdk"
         ;;
     esac
 
-    DPDK_LIB="-ldpdk"
     DPDK_EXTRA_LIB=""
 
     ovs_save_CFLAGS="$CFLAGS"
@@ -346,7 +346,14 @@  AC_DEFUN([OVS_CHECK_DPDK], [
     #
     # These options are specified inside a single -Wl directive to prevent
     # autotools from reordering them.
-    DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
+    #
+    # OTOH newer versions of dpdk pkg-config (generated with Meson)
+    # will already have flagged just the right set of libs with
+    # --whole-archive - in those cases do not wrap it once more.
+    case "$DPDK_LIB" in
+      *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
+      *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
+    esac
     AC_SUBST([DPDK_vswitchd_LDFLAGS])
     AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
   fi