[GitHub] [zookeeper] ztzg commented on a change in pull request #1546: ZOOKEEPER-4012: zookeeper-client-c: add an `install` target to CMakeLists

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

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



##########
File path: zookeeper-client/zookeeper-client-c/CMakeLists.txt
##########
@@ -290,3 +290,21 @@ 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

Review comment:
       Note `$C_CLIENT/include/config.h` only exists if one builds in-place.  This, which is valid (and recommended) in CMake:
   
   ```sh
   mkdir build \
       && cd build \
       && cmake -DCMAKE_INSTALL_PREFIX=/tmp/zkc -DCMAKE_VERBOSE_MAKEFILE=YES .. \
       && cmake --build . \
       && cmake --install .
   ```
   
   fails with "no such file," as `config.h` ends up in `$C_CLIENT/build/include`.
   
   But `config.h` should not be installed, anyway: its only purpose is to drive the compilation of the library.  Each program which is compiled and linked against the ZooKeeper client should have their own `config.h`, and the binary interface to `libzookeeper` should be independent from it.
   
   (I know that `zookeeper.h` includes some `HAVE_*`/`THREADED` preprocessor defines, but that is unfortunate and historical.  And I have also seen your [ZOOKEEPER-4013](https://issues.apache.org/jira/browse/ZOOKEEPER-4013), "pkg-config file for C library," and agree that this is how client program compilation should be driven.)
   
   So: would you mind removing that line?

##########
File path: zookeeper-client/zookeeper-client-c/CMakeLists.txt
##########
@@ -290,3 +290,21 @@ 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)
+

Review comment:
       I have seen your [ZOOKEEPER-4014](https://issues.apache.org/jira/browse/ZOOKEEPER-4014), "libzookeeper.a not properly linking to libhashtable.a," but I believe there is some confusion due to the way `.a` archives ("static libraries") work.
   
   `.a` archives cannot be "linked" against each other; they're just "ZIP archives of objects" of sorts.  The fact that the objects contained in `x.a` require more objects from `y.a` is, at best, encoding in a `pkg-config` or `libtool` metadata file.
   
   In the current state of affairs, the "least wrong" thing to do probably is to install both archives to the destination:
   
   ```
   INSTALL(TARGETS zookeeper hashtable
      ARCHIVE DESTINATION lib)
   ```
   
   and to link any client program against both; i.e., something like:
   
   ```
   $LINK […] -L/path/to/client/lib \
       -lzookeeper -lhashtable \
       -lcrypto -lssl -lsasl2
   ```
   
   But: it is indeed unclear why/if `libhashtable.a` is still needed if we go the CMake route.
   
   The Autoconf path produces a `libhashtable.a`, but it is systematically used with the internal `libzk{st,mt}.a`; as far as I can tell, its only purpose is to allow its thread-agnostic source files to only be compiled once. And as opposed to `libzk{st,mt}.a`, the installed `libzookeeper_{st,mt}.a` archives include the `hashtable` objects.
   
   (`.so` shared objects—or DLLs—also include the `hashtable` code, but that is different as these are "hard-linked" rather than being collections of loose objects.)




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