[GitHub] [zookeeper] Hugmeir opened a new pull request #1547: ZOOKEEPER-4013: C library: provide a libzookeeper.pc for pkg-config

[GitBox <gi...@apache.org>]

Hugmeir opened a new pull request #1547:
URL: https://github.com/apache/zookeeper/pull/1547


   With the commit merged, after `make install`:
   
           ; pkg-config --libs libzookeeper
           -L/usr/local/lib/libzookeeper.a
   
           ; pkg-config --cflags libzookeeper
           -I/usr/local/include/
   
   NOTE: This also includes https://github.com/apache/zookeeper/pull/1546 since there's no reason to do this without first actually installing the libraries & headers >.> So that should be resolved first!


----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org

[GitHub] [zookeeper] ztzg commented on a change in pull request #1547: ZOOKEEPER-4013: C library: provide a libzookeeper.pc for pkg-config

[GitBox <gi...@apache.org>]

ztzg commented on a change in pull request #1547:
URL: https://github.com/apache/zookeeper/pull/1547#discussion_r528622434



##########
File path: zookeeper-client/zookeeper-client-c/CMakeLists.txt
##########
@@ -189,6 +189,7 @@ endif()
 
 if(CYRUS_SASL_FOUND)
   list(APPEND zookeeper_sources src/zk_sasl.c)
+  set(LIBS ${LIBS} -lsasl2)

Review comment:
       This form creates a CMake "list," which ends up as a semi-separated string in the `libzookeeper.pc`.  So you could either continue using a list (in which case I'd suggest `list(APPEND LIBS ...)`), or format a string by quoting as in `set(LIBS "${LIBS} -lsasl2")`.  Yeah…
   
   But in this case I'd suggest not adding those to `pkg-config`'s `Libs:` at all (as you'd then also have to figure out the proper `-L` flags, etc.) but rather as `Requires.private: libsasl2` (and, if OpenSSL is enabled, `Requires.private: [...] libssl`).

##########
File path: zookeeper-client/zookeeper-client-c/CMakeLists.txt
##########
@@ -290,3 +292,31 @@ if(WANT_CPPUNIT)
     "ZKROOT=${CMAKE_CURRENT_SOURCE_DIR}/../.."
     "CLASSPATH=$CLASSPATH:$CLOVER_HOME/lib/clover*.jar")
 endif()
+
+# set up `make install`.
+# headers end up in /usr/local/include/zookeeper/*.h
+# (with /usr/local being overrideable by using --prefix etc)
+INSTALL(FILES
+    include/zookeeper.h
+    include/proto.h
+    include/config.h
+    include/zookeeper_version.h
+    include/winconfig.h
+    include/zookeeper_log.h
+    include/recordio.h
+    generated/zookeeper.jute.h
+  DESTINATION include/zookeeper/)
+# libzookeeper.a ends up in /usr/local/lib/libzookeeper.a
+INSTALL(TARGETS zookeeper
+  ARCHIVE DESTINATION lib)
+
+if (UNIX)
+  INCLUDE(FindPkgConfig QUIET)
+  if(PKG_CONFIG_FOUND)
+    # generate .pc and install
+    CONFIGURE_FILE("libzookeeper.pc.in" "libzookeeper.pc" @ONLY)
+    INSTALL(FILES libzookeeper.pc
+      DESTINATION lib/pkgconfig)
+  endif()

Review comment:
       I would drop the `INCLUDE` and subsequent `PKG_CONFIG_FOUND` conditional, as generating that small ASCII file does not actually require `pkg-config` at compilation time.  But you prefer to keep it, I'd still suggest switching to `find_package(PkgConfig QUIET)`.
   

##########
File path: zookeeper-client/zookeeper-client-c/libzookeeper.pc.in
##########
@@ -0,0 +1,10 @@
+prefix=@CMAKE_INSTALL_PREFIX@
+libdir=${prefix}/lib
+includedir=${prefix}/include/
+
+Name: @CMAKE_PROJECT_NAME@
+Description: Zookeeper C client library
+Version: @CMAKE_PROJECT_VERSION@
+Libs: -L${libdir} -lzookeeper
+Libs.private: @LIBS@ ${libdir}/libzookeeper@CMAKE_STATIC_LIBRARY_SUFFIX@

Review comment:
       Why is `libzookeeper.a` in `Libs.private`?  It seems `-lzookeeper` in `Libs:` should be enough.




----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org