Skip to content

Conversation

@Hugmeir
Copy link

@Hugmeir Hugmeir commented Nov 21, 2020

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 #1546 since there's no reason to do this without first actually installing the libraries & headers >.> So that should be resolved first!

Previously, did this:

    ; cmake .
    ; make
    ; make install

ended up doing nothing, because CMakeLists.txt didn't include
INSTALL() directives.  With this commit applied, the last
command outputs:

    ; make install
    [ 18%] Built target hashtable
    [ 75%] Built target zookeeper
    [ 87%] Built target load_gen
    [100%] Built target cli
    Install the project...
    -- Install configuration: ""
    -- Installing: /usr/local/include/zookeeper/zookeeper.h
    -- Installing: /usr/local/include/zookeeper/proto.h
    -- Installing: /usr/local/include/zookeeper/config.h
    -- Installing: /usr/local/include/zookeeper/zookeeper_version.h
    -- Installing: /usr/local/include/zookeeper/winconfig.h
    -- Installing: /usr/local/include/zookeeper/zookeeper_log.h
    -- Installing: /usr/local/include/zookeeper/recordio.h
    -- Installing: /usr/local/lib/libzookeeper.a
Allows more easily integrating with other toolchains:

    ; pkg-config --cflags libzookeeper
    -I/usr/local/include/

    ; pkg-config --libs libzookeeper
    -L/usr/local/lib -lzookeeper

    ; pkg-config --libs --static libzookeeper
    -L/usr/local/lib -lzookeeper /usr/local/lib/libzookeeper.a
@Hugmeir Hugmeir force-pushed the zookeeper-client-c-pkg-config branch from 8c7d506 to 8ef4d69 Compare November 22, 2020 02:53
Copy link
Contributor

@ztzg ztzg left a comment

Choose a reason for hiding this comment

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

Good idea, in general. But unless I am missing something, the generated .pc file currently is garbled, with a bunch of semicolons in the way.

Some of my comments on #1546 also are relevant (notably if we introduce shared objects/DLLs).

Cc: @symat.


if(CYRUS_SASL_FOUND)
list(APPEND zookeeper_sources src/zk_sasl.c)
set(LIBS ${LIBS} -lsasl2)
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Comment on lines +314 to +320
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()
Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Description: Zookeeper C client library
Version: @CMAKE_PROJECT_VERSION@
Libs: -L${libdir} -lzookeeper
Libs.private: @LIBS@ ${libdir}/libzookeeper@CMAKE_STATIC_LIBRARY_SUFFIX@
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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.

2 participants