New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The pthread linker flag already contains "-l" #1552

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@bjori
Contributor

bjori commented Jun 7, 2017

The pkgconfig created by cmake adds "-l" to the pthread linked flag, producing -l-lpthread.
This comes from ${CURL_LIBS} which retrieves the ${CMAKE_THREAD_LIBS_INIT} value which already contains -l

This is reproducable on Ubuntu like so:

$ rm -rf cbuild && mkdir cbuild && cd cbuild && cmake .. -DENABLE_THREADED_RESOLVER=1 && cat *.pc

Before:

Libs.private:  -lc -ldl -l-lpthread /usr/lib/x86_64-linux-gnu/libssl.so /usr/lib/x86_64-linux-gnu/libcrypto.so /usr/lib/x86_64-linux-gnu/libz.so

After:

Libs.private:  -lc -ldl -lpthread /usr/lib/x86_64-linux-gnu/libssl.so /usr/lib/x86_64-linux-gnu/libcrypto.so /usr/lib/x86_64-linux-gnu/libz.so

Note that with ./configure --enable-threaded-resolver pthread isn't added to the .pc at all, as it is instead added to the CFLAGS as -pthread rather then linker flags as -lpthread

@coveralls

This comment has been minimized.

coveralls commented Jun 7, 2017

Coverage Status

Coverage decreased (-0.01%) to 73.213% when pulling 44f6906 on bjori:master into f7ee701 on curl:master.

@bagder bagder added the cmake label Jun 7, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jun 7, 2017

Yes indeed, -lpthread is not added there by configure. I suppose that tells us a little how rarely that data is actually used, since I believe most linux distros ship with threaded-resolver enabled libcurls these days...

@bagder

This comment has been minimized.

Member

bagder commented Jun 8, 2017

ping @Lekensteyn, this changes code you brought!

@bagder

This comment has been minimized.

Member

bagder commented Jul 6, 2017

No response from any cmake hackers. It seems fine to me so I'll go ahead and merge anyway.

@bagder bagder closed this in 75c3596 Jul 6, 2017

@bagder

This comment has been minimized.

Member

bagder commented Jul 6, 2017

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment