configure.ac: dpdk: use pkg-config #2400

cpaelzer · 2017-08-16T12:34:58Z

To detect cflags and libs use the sometimes provided pkg-config for
libdpdk. That avoids build errors on systems where special flags are
needed and provided by dpdk via pkg-config, but not yet considered by
the collectd build system.

This closes #2399

Signed-off-by: Christian Ehrhardt christian.ehrhardt@canonical.com

bluca

LGTM

rubenk · 2017-08-16T15:38:45Z

configure.ac

@@ -2350,6 +2350,7 @@ AC_SUBST(BUILD_WITH_LIBDBI_LIBS)
 # --with-libdpdk {{{
 AC_ARG_VAR([LIBDPDK_CPPFLAGS], [Preprocessor flags for libdpdk])
 AC_ARG_VAR([LIBDPDK_LDFLAGS], [Linker flags for libdpdk])
+AC_ARG_VAR([LIBDPDK_LIBS], [Library to link for libdpdk])


Can you make that 'Libraries', plural?

Surely - and in general thanks for your review.
I can make build systems work, but lack the in depth experience in some of them - so thanks for pointing out all the improvement chances - addressing them one by one now.

rubenk · 2017-08-16T15:38:59Z

configure.ac

@@ -2358,8 +2359,16 @@ AC_ARG_WITH([libdpdk],
 )

 if test "x$with_libdpdk" != "xno"; then
+  PKG_CHECK_MODULES(DPDK, [libdpdk],


Please quote the first argument as well.

rubenk · 2017-08-16T15:39:28Z

configure.ac

@@ -2358,8 +2359,16 @@ AC_ARG_WITH([libdpdk],
 )

 if test "x$with_libdpdk" != "xno"; then
+  PKG_CHECK_MODULES(DPDK, [libdpdk],
+    [LIBDPDK_LIBS="$DPDK_LIBS"],


I haven't checked, but doesn't this override a custom LIBDPKD_LIBS passed in from the command line?

I thought for the new var ther could be no value yet, but you are right.
Even if it would not have overwritten something your question proves it lacks readability.

So I modified it to follow the common pattern of if-not-set -> then if pkg-config use that; otherwise set default. This leaves any value set before in place to be sure (The same way it was done for cflags already).

rubenk · 2017-08-16T15:39:47Z

configure.ac

  if test "x$LIBDPDK_CPPFLAGS" = "x"; then
-    LIBDPDK_CPPFLAGS="-I/usr/include/dpdk"
+    if test "x$DPDK_CFLAGS" != "x"


if and then on the same line please.

rubenk · 2017-08-16T15:40:36Z

configure.ac

-    LIBDPDK_CPPFLAGS="-I/usr/include/dpdk"
+    if test "x$DPDK_CFLAGS" != "x"
+    then
+      LIBDPDK_CPPFLAGS="$DPDK_CFLAGS"


DPDK_CFLAGS can contain compiler flags that don't belong in _CPPFLAGS. Can you introduce an extra variable?

I don't understand that part, what would it contain?
Do you suggest an additional LIBDPDK_CFLAGS to carry those?

rubenk · 2017-08-16T15:45:41Z

configure.ac

@@ -2388,7 +2397,7 @@ fi

 if test "x$with_libdpdk" = "xyes"; then
  SAVE_LDFLAGS="$LDFLAGS"
-  LDFLAGS="$LIBDPDK_LDFLAGS $LDFLAGS"
+  LDFLAGS="$LIBDPDK_LDFLAGS $LIBDPDK_LIBS $LDFLAGS"


LIBS don't belong in LDFLAGS, but unfortunately that's what the pkgconfig macro returns. I don't think setting them like this has the desired effect, AC_CHECK_LIB still checks just libdpdk for rte_eal_init.

Indeed it does check for libdpdk and call rte_eal_init with the action to finally enable dpdk.
Usage of the fixed LIBs "later on" is in the right place in the Makefile replacing the hardcoded -ldpdk (if no pkg-config is found -ldpdk is set) - which in turn is a linker script expanding to the long set.
It is here only set on the LDFLAGS in between being saved and restored in case there are extra args in the provided LIBDPDK_LIBS like the include-dir to make things work.

For the current case we could drop that change and just let the linker script do it's work.
But I'd keep the replacement of the hardcoded -ldpdk in the Makefile to take -ldpdk as default but get overwritten by pkg-config if provided (or a user setting).

cpaelzer · 2017-08-17T09:32:33Z

Adapting all suggested changes as replied and testing the new changes before pushing

cpaelzer · 2017-08-17T09:53:58Z

Tested this which would be close to what we usually have:
git clean -f; git checkout .; libtoolize --force; aclocal; autoheader; automake --force-missing --add-missing; autoconf

./configure --host=i686-linux-gnu --build=i686-linux-gnu --prefix=/usr --mandir=${prefix}/share/man --localstatedir=/var --sysconfdir=/etc --with-perl-bindings=INSTALLDIRS=vendor INSTALL_BASE= --without-libstatgrab --without-included-ltdl --disable-static --disable-silent-rules --enable-all-plugins --disable-apple_sensors --disable-lpar --disable-tape --disable-aquaero --disable-mic --disable-netapp --disable-oracle --disable-routeros --disable-write_mongodb --disable-xmms --disable-grpc --disable-intel_rdt --disable-pf --disable-zone

Checking if it configures DPDK correctly
In the fixed cases to actually work check in config.log the dpdk sepcific CFLAGS (should have the -I), LDFLAGS (should be untouched) and LIBS (should have all including the tail of lz, lm, ...)

Failing on Master for the reported errors (as expected)
Working with what I submitted in v1 (as expected)
Testing the new code after adapting the suggestions

In particular I checked for our discussion around the AC_CHECK_LIB.
In the former content it really checked with "both" so it had all the -l ... -ldpdk.
That was not what was wanted, so yeah for now let it do that via -ldpdk and the linker script it is.

Since the these are more rewrites than follow on fixes I (force) push the new changes over the old one.

To detect cflags and libs use the sometimes provided pkg-config for libdpdk. That avoids build errors on systems where special flags are needed and provided by dpdk via pkg-config, but not yet considered by the collectd build system. This closes collectd#2399 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

cpaelzer · 2017-08-17T09:55:45Z

Ok, repushed and ready for re-review.
Thanks again for your feedback already.

rubenk · 2017-08-17T10:22:14Z

For next time, please don't force push, because now GitHub lost all comments.

rubenk · 2017-08-17T10:23:26Z

configure.ac

  if test "x$LIBDPDK_CPPFLAGS" = "x"; then
-    LIBDPDK_CPPFLAGS="-I/usr/include/dpdk"
+    if test "x$DPDK_CFLAGS" != "x"; then
+      LIBDPDK_CPPFLAGS="$DPDK_CFLAGS"


Because DPDK_CFLAGS can contain flags which are not accepted by the preprocessor, please introduce a LIBDPDK_CFLAGS.

Ok will do so in a follow on commit.

cpaelzer · 2017-08-17T11:21:42Z

Also the checks have found that now (v2) in your env detection fails - I need to check that when providing the follow up fix.
Will ensure a build works on a system without the split headers to so so.

This restores the former behavior of LIBDPDK_CPPFLAGS and now correctly carries CFLAGS read from pkg-config in its own variable. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

cpaelzer · 2017-08-17T11:52:40Z

Next version introducing an explicit LIBDPDK_CFLAGS tested against the new split lib with pkg-config and an old "classic" one both configuring and building for me. Looking forward to see if the checks agree this time.

There is one thing to mention due to the split CFLAGS/CPPFLAGS the check for the header still works in both cases but gets noisier if you run with the CFLAGS to fix the multi-arch include.
On a classic dpdk it looks like:

checking for DPDK... yes
checking rte_config.h usability... yes
checking rte_config.h presence... yes
checking for rte_config.h... yes
checking for rte_eal_init in -ldpdk... yes

While on the new case - that gets CFLAGS from pkg-config but no CPPFLAGS for the correct case that there could be CFLAGS incompatible for the preprocessor - we will see that the preprocessor correctly disagrees.

checking for DPDK... yes
checking rte_config.h usability... yes
checking rte_config.h presence... no
configure: WARNING: rte_config.h: accepted by the compiler, rejected by the preprocessor!
configure: WARNING: rte_config.h: proceeding with the compiler's result
checking for rte_config.h... yes
checking for rte_eal_init in -ldpdk... yes

I wonder if we should adapt configure.ac right away to read LDFLAGS/CPPFLAGS in case they might at some point be available in the pkg-config.

Pushing the follow on fix for your review and the build check to run on it again.

rubenk · 2017-08-17T12:13:19Z

While on the new case - that gets CFLAGS from pkg-config but no CPPFLAGS for the correct case that there could be CFLAGS incompatible for the preprocessor - we will see that the preprocessor correctly disagrees.

Could you share config.log? I'd like to know what the specific issue is.
Those headers should work without any specific preprocessor flags defined, apart from the include path.

I wonder if we should adapt configure.ac right away to read LDFLAGS/CPPFLAGS in case they might at some point be available in the pkg-config.

I don't think the PKG_CHECK_MODULE macro will ever make these available, and even if you get that fixed upstream, it will take years for a newer pkg-config to land in distros.

We could of course just shell out to pkg-config --libs-only-l and pkg-config --cflags-only-I.

Just for reference, can you share the .pc which you are using on Ubuntu?

cpaelzer · 2017-08-17T12:29:11Z

Here the requested files:

An example .pc file on i386 (I include i386 as we have added a compat symlink for x86_64 as the most common platform, so there compile works even without the fix of the proper includes from pkg-config).
Config.log on i386 with libraries split for multi-arch correctness using the pkg-config
Config.log on amd64 with "classic" dpdk headers

rubenk · 2017-08-17T12:40:33Z

Oh wow, that can't be right.

Libs: -L${libdir} -lrte_acl -lrte_bitratestats -lrte_cfgfile -lrte_cmdline -lrte_cryptodev -lrte_distributor -lrte_eal -lrte_efd -lrte_ethdev -lrte_eventdev -lrte_hash -lrte_ip_frag -lrte_jobstats -lrte_kvargs -lrte_latencystats -lrte_lpm -lrte_mbuf -lrte_mempool -lrte_mempool_ring -lrte_mempool_stack -lrte_meter -lrte_metrics -lrte_net -lrte_pdump -lrte_pipeline -lrte_pmd_af_packet -lrte_pmd_ark -lrte_pmd_bnxt -lrte_pmd_bond -lrte_pmd_crypto_scheduler -lrte_pmd_cxgbe -lrte_pmd_e1000 -lrte_pmd_ena -lrte_pmd_enic -lrte_pmd_fm10k -lrte_pmd_i40e -lrte_pmd_ixgbe -lrte_pmd_lio -lrte_pmd_nfp -lrte_pmd_null -lrte_pmd_null_crypto -lrte_pmd_octeontx_ssovf -lrte_pmd_pcap -lrte_pmd_qede -lrte_pmd_ring -lrte_pmd_skeleton_event -lrte_pmd_sw_event -lrte_pmd_tap -lrte_pmd_thunderx_nicvf -lrte_pmd_vhost -lrte_pmd_virtio -lrte_pmd_vmxnet3_uio -lrte_pmd_xenvirt -lrte_port -lrte_power -lrte_reorder -lrte_ring -lrte_sched -lrte_table -lrte_timer -lrte_vhost -ldl -lm -lpthread -lz

I'm not an expert on these things, but is it really needed to link to all these libraries just to get some stats out of DPDK?

I can't imagine the plugin using code from let's say librte_pmde_fm10k.so

Next to that, the main library, libdpkd, seems missing from this line.

bluca · 2017-08-17T12:43:43Z

Yeah that's unfortunately right. There is no main library either, libdpdk.so is just a linker script that links all of those libraries. So it's already doing that with -ldpdk

rubenk · 2017-08-17T12:45:14Z

configure:24124: checking rte_config.h usability
configure:24124: gcc -c -include /usr/include/i386-linux-gnu/dpdk/rte_config.h -I/usr/include/i386-linux-gnu/dpdk -I/usr/include/dpdk -g -O2 -I/usr/include/dpdk  conftest.c >&5
configure:24124: $? = 0
configure:24124: result: yes
configure:24124: checking rte_config.h presence
configure:24124: gcc -E -I/usr/include/dpdk  conftest.c
conftest.c:148:10: fatal error: rte_config.h: No such file or directory
 #include <rte_config.h>
          ^~~~~~~~~~~~~~
compilation terminated.

So if I understand correctly, even that header is arch specific?

bluca · 2017-08-17T12:47:30Z

Yes - different architectures have different configurations

rubenk · 2017-08-17T12:47:52Z

Yeah that's unfortunately right. There is no main library either, libdpdk.so is just a linker script that links all of those libraries. So it's already doing that with -ldpdk

I might misunderstand something, but if -ldpdk already works, why the need for all those libs in .pc?

rubenk · 2017-08-17T12:50:02Z

Yes - different architectures have different configurations

Ok, then I don't see any other solution than adding DPDK_CFLAGS to LIBDPDK_CPPFLAGS, and hoping that dpdk won't introduce any settings that are incompatible with the preprocessor.

bluca · 2017-08-17T12:53:27Z

I might misunderstand something, but if -ldpdk already works, why the need for all those libs in .pc?

I prefer to have an explicit list, rather than an implicit one. Upstream is doing the same in the proposed Meson patchset I linked earlier.

cpaelzer · 2017-08-17T12:57:11Z

Ok, thanks for the discussion - I'll append a commit that adds CFLAGS to CPPFLAGS in this case.

In some way it is bad naming in pkg-config - it is always some sort of conflated ldflags + libs and cppflags + cxxflags + cflags. Due to that to work correctly we append DPDK_CFLAGS (if defined by pkg-config) to LIBDPDK_CPPFLAGS as well. This intentionally does not add those CFLAGS specified by a user to allow full override and fully separated CLFAGS/CPPFLAGS if needed. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

cpaelzer · 2017-08-17T13:07:48Z

With that change (as discussed) old/new style both build correctly for me without the hickup on CFLAGS vs CPPFLAGS.

Ready for re-review

rubenk · 2017-08-17T13:08:06Z

I prefer to have an explicit list, rather than an implicit one. Upstream is doing the same in the proposed Meson patchset I linked earlier.

Ok, that's fine with me. It's not the fact that's it's explicit that I'm worried about, but more that this will probably result in a lot of overlinking. On the other hand, if all these libs are required, upstream DPDK might as well include them all in a single shared library and make everyones lives easier.

bluca · 2017-08-17T13:13:42Z

Ok, that's fine with me. It's not the fact that's it's explicit that I'm worried about, but more that this will probably result in a lot of overlinking. On the other hand, if all these libs are required, upstream DPDK might as well include them all in a single shared library and make everyones lives easier.

Due to the way the PMDs loading works, unfortunately it is required to have them all, that's why the upstream libdpdk.so linker script lists them all too.

It used to be a single shared object, but it was removed and changed to a linker script last year. IIRC the problem was ABI versioning.

Thanks for the review and the merge!

rubenk · 2017-08-17T13:16:13Z

Got it, thanks for the explanation! If you get the chance to talk to them, they should probably drop -ldl -lm -lpthread -lz from their .pc file. The dpdk libs themselves should already be linked to these libs.

Since pull request collectd#2400 pkg-config works great for dpdk, but not if there is no pkg-config around at all. To fix that the PKG_CHECK_MODULES macro needs to define a valid fourth argument, otherwise it will error out. This closes 2404 Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

Backport and adapt patches from upstream to use pkg-config when querying for DPDK: collectd/collectd#2400 collectd/collectd#2405 This allows DPDK in Debian to fix an upstream multi-arch issue, where arch-dependents headers are installed in /usr/include breaking multi-arch co-installability of libdpdk-dev. The arch-dependent DPDK headers will be moved under /usr/include/<arch> to fix the issue, and pkg-config --cflags will return the correct -I values. Closes #872482

bluca approved these changes Aug 16, 2017

View reviewed changes

rubenk reviewed Aug 16, 2017

View reviewed changes

cpaelzer force-pushed the fix-dpdk-multiarch-pkg-config branch from ca6d0b4 to 7968c0d Compare August 17, 2017 09:55

rubenk reviewed Aug 17, 2017

View reviewed changes

configure.ac: dpdk: introduce LIBDPDK_CFLAGS

6ca79b6

This restores the former behavior of LIBDPDK_CPPFLAGS and now correctly carries CFLAGS read from pkg-config in its own variable. Signed-off-by: Christian Ehrhardt <christian.ehrhardt@canonical.com>

rubenk merged commit 929a7f2 into collectd:master Aug 17, 2017

cpaelzer mentioned this pull request Aug 17, 2017

failing on dpdk pkg-config check if no pkg-config is available #2404

Closed

cpaelzer mentioned this pull request Aug 17, 2017

configure.ac: dpdk: tolerate no pkg-config #2405

Merged

This was referenced Sep 1, 2017

Backport patch to build DPDK with pkg-config collectd/pkg-debian#14

Merged

configure.ac: dpdk: use pkg-config - 5.7 #2417

Merged

octo added this to the 5.8 milestone Oct 11, 2017

maryamtahhan mentioned this pull request Oct 12, 2017

Next collectd release #2420

Closed

19 tasks

configure.ac: dpdk: use pkg-config #2400

configure.ac: dpdk: use pkg-config #2400

Conversation

cpaelzer commented Aug 16, 2017

bluca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpaelzer commented Aug 17, 2017

cpaelzer commented Aug 17, 2017 • edited

cpaelzer commented Aug 17, 2017

rubenk commented Aug 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cpaelzer commented Aug 17, 2017

cpaelzer commented Aug 17, 2017

rubenk commented Aug 17, 2017

cpaelzer commented Aug 17, 2017

rubenk commented Aug 17, 2017

bluca commented Aug 17, 2017

rubenk commented Aug 17, 2017

bluca commented Aug 17, 2017

rubenk commented Aug 17, 2017

rubenk commented Aug 17, 2017

bluca commented Aug 17, 2017

cpaelzer commented Aug 17, 2017

cpaelzer commented Aug 17, 2017

rubenk commented Aug 17, 2017

bluca commented Aug 17, 2017

rubenk commented Aug 17, 2017

cpaelzer commented Aug 17, 2017 •

edited