Skip to content
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

Added pkg-config support #37

Merged
merged 2 commits into from
Oct 21, 2014
Merged

Conversation

fabianfreyer
Copy link
Contributor

This patch adds pkg-config support. This has been tested on OS X and FreeBSD using the expat parser. No tests with the libxml parser have been made.

fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Sep 26, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
@fabianfreyer fabianfreyer mentioned this pull request Sep 26, 2014
@@ -60,9 +60,17 @@ fi

AC_CHECK_HEADERS([arpa/nameser_compat.h])


Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

m4_ifdef([PKG_INSTALLDIR], [PKG_INSTALLDIR],
[AC_ARG_WITH([pkgconfigdir],
[AS_HELP_STRING([--with-pkgconfigdir],
[install directory for openconnect.pc pkg-config file])],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/openconnect.pc/libstrophe.pc/

@fabianfreyer
Copy link
Contributor Author

@pasis, I have made the changes you suggested. At the opportunity, I refactored some of the if/else cruft in configure.ac to use the implicit cases in PKG_CHECK_MODULES. I'm not sure if the behavior has stayed the same, I have to explicitely pass --libdir=${path_to_libxml2_include_folder} because ${libdir}/libxml2 just gets expanded to ${prefix}/include/libxml2 on my system, which is not expanded further. However, libxml/parser.h is present and usable at ${libdir}/libxml2/libxml/parser.h == ${prefix}/include/libxml2/libxml/parser.h. The expat check however works both when expat.pc is present and missing.

@@ -63,9 +86,21 @@ fi

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if I should also add resolv to Requires:.

@pasis
Copy link
Member

pasis commented Oct 7, 2014

@fabianfreyer, I'll take a look. Meanwhile, some changes required due to my mistake. Linking with libstrophe.so doesn't require any dependency. Dynamic library already has it:

$ ldd libstrophe.so
...
    libssl.so.1.0.0 => /usr/lib64/libssl.so.1.0.0 (0x00007f6e64d56000)
    libcrypto.so.1.0.0 => /usr/lib64/libcrypto.so.1.0.0 (0x00007f6e64986000)
    libexpat.so.1 => /usr/lib64/libexpat.so.1 (0x00007f6e6475c000)
    libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f6e64545000)
    libz.so.1 => /lib64/libz.so.1 (0x00007f6e63d87000)
$ gcc -o examples/basic -lstrophe examples/basic.o
$ ldd examples/basic
...
    libstrophe.so.0 => /usr/lib64/libstrophe.so.0 (0x00007f01dd708000)
    libssl.so.1.0.0 => /usr/lib64/libssl.so.1.0.0 (0x00007f01dd0fb000)
    libcrypto.so.1.0.0 => /usr/lib64/libcrypto.so.1.0.0 (0x00007f01dcd2c000)
    libexpat.so.1 => /usr/lib64/libexpat.so.1 (0x00007f01dcb02000)
    libresolv.so.2 => /lib64/libresolv.so.2 (0x00007f01dc8eb000)
    libz.so.1 => /lib64/libz.so.1 (0x00007f01dc4d1000)

But static library is just an archive with object files and it requires all the depenndecies. So, we need to use fields Requires.private and Libs.private (see libxml-2.0.pc as an example):

Requires:
Requires.private: openssl expat
Libs: -L${libdir} -lstrophe
Libs.private: -lresolv

This should work in the following way:

$ pkg-config --libs libstrophe
-L/usr/local/lib -lstrophe
$ pkg-config --static --libs libstrophe
-L/usr/local/lib -lstrophe -lssl -lcrypto -lz -lexpat -lresolv

I'm not sure if I should also add resolv to Requires:.

Yes, but to Libs.private.

@fabianfreyer
Copy link
Contributor Author

This means, technically, this would suffice?

diff --git a/libstrophe.pc.in b/libstrophe.pc.in
index bee8b7b..ba271c2 100644
--- a/libstrophe.pc.in
+++ b/libstrophe.pc.in
@@ -7,6 +7,8 @@ Name: libstrophe
 Description: A simple, lightweight C library for writing XMPP clients
 URL: http://strophe.im/libstrophe/
 Version: @VERSION@
-Requires: @PC_REQUIRES@
-Libs: -L${libdir} -lstrophe @PC_LIBS@
+Requires:
+Requires.private: @PC_REQUIRES@
+Libs: -L${libdir} -lstrophe
+Libs.private: -L${libdir} -lstrophe @PC_LIBS@
 Cflags: -I${includedir} @PC_CFLAGS@

@pasis
Copy link
Member

pasis commented Oct 7, 2014

Yes, but Libs.private shouldn't include Libs:

Libs.private: @PC_LIBS@

@fabianfreyer
Copy link
Contributor Author

@pasis: done. Are there any changes to be made to Makefile.am?

@fabianfreyer
Copy link
Contributor Author

Otherwise, I'd rebase this into a single squash commit.

fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Oct 7, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Oct 7, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Oct 7, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Oct 7, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Oct 7, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
fabianfreyer added a commit to fabianfreyer/libstrophe that referenced this pull request Oct 8, 2014
These are currently pull requests:
strophe#30
strophe#37

Once these are merged, and a new release is made, this commit can be
reverted.
@fabianfreyer fabianfreyer force-pushed the feature/pkg-config branch 2 times, most recently from c96922d to 671ebda Compare October 8, 2014 14:33
@fabianfreyer
Copy link
Contributor Author

I have rebased this into two commits.

@@ -54,8 +77,8 @@ AC_MSG_NOTICE([libstrophe will use the $with_parser XML parser])
AC_SEARCH_LIBS([socket], [socket])

if test "x$PLATFORM" != xfreebsd; then
AC_CHECK_LIB([resolv], [res_query], [],
[AC_CHECK_LIB([resolv], [__res_query], [],
AC_CHECK_LIB([resolv], [res_query], [LIBS="-lresolv $LIBS"; PC_LIBS+=(-lresolv)],
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Move PC_LIBS out of AC_CHECK_LIB() just to avoid duplication:

if test "x$PLATFORM" != xfreebsd; then
        AC_CHECK_LIB()
        PC_LIBS+=(-lresolv)
else

Isn't -lresolv added to LIBS twice in your patch? AC_CHECK_LIB() should add it implicitly.

@pasis
Copy link
Member

pasis commented Oct 8, 2014

I left 1 comment. It looks good, but I still need to check it.

@fabianfreyer
Copy link
Contributor Author

I think -lresolv should be added to Libs, not Libs.private:

$ ldd $/tmp/strophe/lib/libstrophe.so
    linux-vdso.so.1 =>  (0x00007fffddde4000)
    libssl.so.1.0.0 => /usr/lib/x86_64-linux-gnu/libssl.so.1.0.0 (0x00007f5c3921b000)
    libcrypto.so.1.0.0 => /usr/lib/x86_64-linux-gnu/libcrypto.so.1.0.0 (0x00007f5c38e23000)
    libexpat.so.1 => /lib/x86_64-linux-gnu/libexpat.so.1 (0x00007f5c38bf8000)
    libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f5c3886d000)
    libdl.so.2 => /lib/x86_64-linux-gnu/libdl.so.2 (0x00007f5c38669000)
    libz.so.1 => /lib/x86_64-linux-gnu/libz.so.1 (0x00007f5c38451000)
    /lib64/ld-linux-x86-64.so.2 (0x00007f5c3969b000)
$echo "int main() {return 0;}" | gcc -xc $(env PKG_CONFIG_PATH=/tmp/strophe/lib/pkgconfig pkg-config --cflags --libs libstrophe) -
/tmp/strophe/lib/libstrophe.so: undefined reference to `__res_query'
collect2: error: ld returned 1 exit status

This is with the following .pc file:

prefix=/tmp/strophe
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Name: libstrophe
Description: A simple, lightweight C library for writing XMPP clients
URL: http://strophe.im/libstrophe/
Version: 0.8.7
Requires:
Requires.private:
Libs: -L${libdir} -lstrophe
Libs.private: -lssl -lcrypto -lexpat -lresolv
Cflags: -I${includedir}

@pasis
Copy link
Member

pasis commented Oct 14, 2014

@fabianfreyer, could you show files Makefile.am, configure.ac that lead to your problem with libresolv? And please, provide some info:

  • system (is it linux 64-bit?)
  • ls /lib/libresolv*
  • string, how libstrophe.so is linked, e.g.:
libtool: link:  gcc -shared  -fPIC -DPIC  src/.libs/libstrophe_la-auth.o src/.libs/libstrophe_la-conn.o src/.libs/libstrophe_la-ctx.o src/.libs/libstrophe_la-event.o src/.libs/libstrophe_la-handler.o src/.libs/libstrophe_la-hash.o src/.libs/libstrophe_la-jid.o src/.libs/libstrophe_la-md5.o src/.libs/libstrophe_la-sasl.o src/.libs/libstrophe_la-scram.o src/.libs/libstrophe_la-sha1.o src/.libs/libstrophe_la-snprintf.o src/.libs/libstrophe_la-sock.o src/.libs/libstrophe_la-stanza.o src/.libs/libstrophe_la-thread.o src/.libs/libstrophe_la-tls_openssl.o src/.libs/libstrophe_la-util.o src/.libs/libstrophe_la-parser_expat.o   -lssl -lcrypto -lexpat -lresolv  -O2   -Wl,-soname -Wl,libstrophe.so.0 -Wl,-version-script -Wl,.libs/libstrophe.ver -o .libs/libstrophe.so.0.0.0

I can't reproduce this problem on my system with the latest version of your patch. And libstrophe.so is linked with libresolv properly:

$ ldd .libs/libstrophe.so
...
libresolv.so.2 => /lib64/libresolv.so.2 (0x00007fbe09b8b000)

@fabianfreyer
Copy link
Contributor Author

@pasis this is weird, I can't seem to be able to reproduce this either, on the same systems I was observing this earlier.

@pasis pasis merged commit 3231214 into strophe:master Oct 21, 2014
@pasis
Copy link
Member

pasis commented Oct 21, 2014

Merged. I've made a minor change, see 057f906.

@boothj5 boothj5 added this to the 0.8.7 milestone Oct 21, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants