Makefile: Use pkg-config if available. by ddiproietto · Pull Request #618 · NetSys/bess

ddiproietto · 2017-08-24T19:38:24Z

Debian buster(testing) and presumably ubuntu 17.10 package all the
dependencies required to build BESS (except dpdk).

New versions of GRPC (packaged in debian) require libc-ares and if BESS
is statically linked (by default it is), -lcares must be passed to the
linker.

Instead of forcing a new dependency for all users (even the ones that don't
need it), this commit makes use of pkg-config, which will automatically
include the indirect dependencies (such as libc-ares).

For users that have an old grpc version (such as container_build.py),
pkg-config is still optional. I'd like to make it required, but a new
container image must be generated, and that's a decision we can make
later.

Debian buster(testing) and presumably ubuntu 17.10 package all the dependencies required to build BESS (except dpdk). New versions of GRPC (packaged in debian) require libc-ares and if BESS is statically linked (by default it is), `-lcares` must be passed to the linker. Instead of forcing a new dependency for all users (even the ones that don't need it), this commit makes use of pkg-config, which will automatically include the indirect dependencies (such as libc-ares). For users that have an old grpc version (such as `container_build.py`), pkg-config is still optional. I'd like to make it required, but a new container image must be generated, and that's a decision we can make later.

`-ldl` is used by DPDK on some systems. Therefore it needs to be listed also after it on the LDFLAGS.

chris3torek · 2017-08-25T18:04:57Z

+NO_PKG_CONFIG_LIBS_INDIRECT := -lgrpc -lssl -lcrypto -llzma
+
+ifeq ($(HAS_PKG_CONFIG), yes)
+    PKG_CFLAGS = $(shell $(PKG_CONFIG) --cflags $(PKG_CONFIG_DEPS))


Can this be statically defined (with :=)?

Thanks for the suggestion! I did that here and in the other places I was shelling out to call pkg-config

gflags on ubuntu 16.04 doesn't have a pkg-config file, so we have to list it manually. `-ldl` is used by DPDK and must be listed before and after to properly be dynamically linked.

codecov · 2017-08-26T00:01:42Z

Codecov Report

Merging #618 into master will decrease coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff            @@
##           master    #618      +/-   ##
=========================================
- Coverage   69.33%   69.3%   -0.03%     
=========================================
  Files         204     204              
  Lines       13037   13068      +31     
=========================================
+ Hits         9039    9057      +18     
- Misses       3998    4011      +13

Impacted Files	Coverage Δ
core/drivers/unix_socket.cc	`80.15% <0%> (-4.9%)`	⬇️
core/modules/queue.h	`100% <0%> (ø)`	⬆️
core/modules/vlan_split.h	`0% <0%> (ø)`	⬆️
core/modules/port_inc.h	`100% <0%> (ø)`	⬆️
core/drivers/unix_socket.h	`100% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 425ce73...bbb11c3. Read the comment docs.

sangjinhan · 2017-08-29T23:29:35Z

+# user requests a static build.  These needs to be listed first, so even
+# if they're listed again in the static section, they will be dynamically
+# linked.
+ALWAYS_DYN_LIBS := -lpthread -ldl


Is it necessary to add -lpthread here? The -pthread option already implies this...

I believe it's necessary.

grpc includes -lpthread in its statically required libraries. If we don't specify -lpthread before (in the dynamic section), -lpthread will be linked statically, and that's not well supported apparently

sangjinhan · 2017-08-29T23:30:05Z

+	$(PKG_LIBS) $(ALWAYS_LIBS) \
 	$(LIBS_DL_SHARED) \
-	-ldl
+	$(ALWAYS_DYN_LIBS)


Just checking: Adding $(ALWAYS_DYN_LIBS) twice was intended?

It was intended.

DPDK depends on -ldl, so -ldl must be specified after DPDK.

But then again, we want to dynamically link with -ldl. If one of the other libs depend on -ldl it will be listed in the static section. We must make sure that we include it at the beginning of the cmdline, in the dynamic section

sangjinhan · 2017-08-30T00:30:41Z

Great, thanks!

Codeacious · 2017-08-30T04:43:11Z

This pull request appears to have broken static builds for me on Ubuntu 16.04 LTS (kernel 4.4.0-93-generic):

/usr/local/home/rotor/bess/deps/dpdk-17.05/build/lib/librte_pmd_ark.a(ark_ethdev.o): In function `check_for_ext':
/usr/local/home/rotor/bess/deps/dpdk-17.05/drivers/net/ark/ark_ethdev.c:199: warning: Using 'dlopen' in statically linked applications requires at runtime the shared libraries from the glibc version used for linking
/usr/lib/gcc/x86_64-linux-gnu/5/../../../x86_64-linux-gnu/libdl.a(dlopen.o): In function `dlopen':
(.text+0x5): undefined reference to `__dlopen'
/usr/lib/gcc/x86_64-linux-gnu/5/../../../x86_64-linux-gnu/libdl.a(dlclose.o): In function `dlclose':
(.text+0x1): undefined reference to `__dlclose'
/usr/lib/gcc/x86_64-linux-gnu/5/../../../x86_64-linux-gnu/libdl.a(dlsym.o): In function `dlsym':
(.text+0x5): undefined reference to `__dlsym'
/usr/lib/gcc/x86_64-linux-gnu/5/../../../x86_64-linux-gnu/libdl.a(dlerror.o): In function `dlerror':
(.text+0x1): undefined reference to `__dlerror'
collect2: error: ld returned 1 exit status
Error: port_test
g++ -o port_test port_test.o gtest-all.o gtest_main.o bess.a -rdynamic -L/usr/local/home/rotor/bess/deps/dpdk-17.05/build/lib -Wl,-rpath=/usr/local/home/rotor/bess/deps/dpdk-17.05/build/lib -pthread -static-libstdc++ -lpthread -ldl -Wl,-non_shared -Wl,--whole-archive -ldpdk -Wl,--no-whole-archive -L/usr/local/lib -lglog -lprotobuf -pthread -lpthread -lz -lgrpc++ -lgrpc -lssl -ldl -lcrypto -ldl -lunwind -llzma -lz -lpcap -lgflags -Wl,-call_shared -lpthread -ldl -libverbs
Makefile:394: recipe for target 'port_test' failed

The full output is much longer, but are duplicates of this error on other files.

The issue seems to be that pkg-config --libs --static grpc++ returns -L/usr/local/lib -lgrpc++ -lgrpc -lz -lssl -ldl -lcrypto -ldl, meaning g++ attempts to use symbols from the static library libdl.a instead of libdl.so when linking DPDK. Manually running the linking command without the additional -ldl flags returned by pkg-config solves the issue.

An easy, but hacky, way to do this in the Makefile is to use sed:
PKG_LIBS := $(shell $(PKG_CONFIG) --static --libs $(PKG_CONFIG_DEPS) | sed -e s/-ldl//g)
But there's probably better method to solve this issue.

ddiproietto · 2017-08-30T19:02:01Z

Thanks for reporting this and sorry for the breakage.

I wasn't able to reproduce it on ubuntu 16.04, but your suggestion makes sense to me (I like it better than listing twice the libraries).

I submitted PR #626, I removes -ld from the static libraries, I hope it fixes the problem for you.

Thanks

ddiproietto added the notready label Aug 25, 2017

Makefile: list -ldl also after DPDK.

4067b84

`-ldl` is used by DPDK on some systems. Therefore it needs to be listed also after it on the LDFLAGS.

chris3torek reviewed Aug 25, 2017

View reviewed changes

ddiproietto added 2 commits August 25, 2017 15:57

Makefile: Fix build on container.

c3c0292

gflags on ubuntu 16.04 doesn't have a pkg-config file, so we have to list it manually. `-ldl` is used by DPDK and must be listed before and after to properly be dynamically linked.

Makefile: Use := instead of = when shelling out.

bbb11c3

ddiproietto removed the notready label Aug 28, 2017

sangjinhan approved these changes Aug 29, 2017

View reviewed changes

sangjinhan merged commit 78c6c00 into NetSys:master Aug 30, 2017

ddiproietto deleted the pkg_config branch August 30, 2017 00:36

sangjinhan mentioned this pull request Sep 8, 2017

"undefined reference to '__dlopen'" in [TEST_LD]gate_test of building ''Daemon" #631

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Makefile: Use pkg-config if available.#618

Makefile: Use pkg-config if available.#618
sangjinhan merged 4 commits intoNetSys:masterfrom
ddiproietto:pkg_config

ddiproietto commented Aug 24, 2017

Uh oh!

chris3torek Aug 25, 2017

Uh oh!

ddiproietto Aug 25, 2017

Uh oh!

codecov Bot commented Aug 26, 2017 •

edited

Loading

Uh oh!

sangjinhan Aug 29, 2017

Uh oh!

ddiproietto Aug 29, 2017

Uh oh!

sangjinhan Aug 29, 2017

Uh oh!

ddiproietto Aug 29, 2017

Uh oh!

sangjinhan commented Aug 30, 2017

Uh oh!

Codeacious commented Aug 30, 2017

Uh oh!

ddiproietto commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

ddiproietto commented Aug 24, 2017

Uh oh!

chris3torek Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

ddiproietto Aug 25, 2017

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Aug 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

sangjinhan Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

ddiproietto Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

ddiproietto Aug 29, 2017

Choose a reason for hiding this comment

Uh oh!

sangjinhan commented Aug 30, 2017

Uh oh!

Codeacious commented Aug 30, 2017

Uh oh!

ddiproietto commented Aug 30, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Aug 26, 2017 •

edited

Loading