libevhtp icon indicating copy to clipboard operation
libevhtp copied to clipboard

fix #143 by setting LIB_INSTALL_DIR CMake var

Open schmittlauch opened this issue 6 years ago • 13 comments

This is a simple fix for #143 by reverting parts of 327bf14. I don't know though whether this fits the intended modernized CMake workflow.

schmittlauch avatar Sep 14 '19 19:09 schmittlauch

This is more or less correct.

Currently in OpenWrt, libdir= comes up as blank and includedir=/evhtp , which is totally wrong. This is currently used to fix: https://github.com/openwrt/packages/blob/master/libs/libevhtp/Makefile#L49

In most other pkgconfig files that I've seen, this is the format:

prefix=/usr
exec_prefix=/usr
libdir=${exec_prefix}/lib
includedir=${prefix}/include

which works well in OpenWrt.

neheb avatar Sep 16 '19 18:09 neheb

Please do not hardcode prefixes like /usr but instead set them during the build – or more precisely install – phase! Distributions like NixOS or Guix do not adhere to the FHS but set different prefixes depending on build inputs and versions. Using ${CMAKE_INSTALL_PREFIX} works correctly for us.

schmittlauch avatar Sep 16 '19 19:09 schmittlauch

ping @cotequeiroz

neheb avatar Sep 16 '19 19:09 neheb

Please do not hardcode prefixes like /usr but instead set them during the build – or more precisely install – phase! Distributions like NixOS or Guix do not adhere to the FHS but set different prefixes depending on build inputs and versions. Using ${CMAKE_INSTALL_PREFIX} works correctly for us.

I get what @neheb wants. He does not want to hardcode anything, but to use the prefix and exec_prefix variables, which can be easily overriden, as the first part of libdir and includedir. They are usually set that way by autotools. cmake has the GNUInstallDirs module to help.

So the following patch should do it that way:

--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -14,6 +14,7 @@ include(CheckIncludeFiles)
 include(CheckTypeSize)
 include(CheckCCompilerFlag)
 include(TestBigEndian)
+include(GNUInstallDirs)

 check_function_exists(strndup HAVE_STRNDUP)
 check_function_exists(strnlen HAVE_STRNLEN)
diff --git a/evhtp.pc.in b/evhtp.pc.in
index a7b351f..fbddc51 100644
--- a/evhtp.pc.in
+++ b/evhtp.pc.in
@@ -1,6 +1,7 @@
 prefix=@CMAKE_INSTALL_PREFIX@
-libdir=@LIB_INSTALL_DIR@
-includedir=@INCLUDE_INSTALL_DIR@/evhtp
+exec_prefix=${prefix}
+libdir=${exec_prefix}/@CMAKE_INSTALL_LIBDIR@
+includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@/evhtp

 Name: libevhtp
 Description: A more flexible replacement for libevent's httpd API

Note that this changes behavior. Currently, on gentoo-x86_64, libevht will install libs to /usr/local/lib by default. If you include the GNUInstallDirs module, they will be installed in /usr/local/lib64--which should be right for my system, btw.

Edit: I forgot to show the resulting evhtp.pc file:

$ head -n4 evhtp.pc
prefix=/usr/local
exec_prefix=${prefix}
libdir=${exec_prefix}/lib64
includedir=${prefix}/include/evhtp

cotequeiroz avatar Sep 17 '19 22:09 cotequeiroz

Not quite:

mangix@mangix-pc:~/devstuff/openwrt/staging_dir/target-mips_24kc_musl/usr/lib/pkgconfig$ cat * | grep exec_prefix=\$\{ | wc -l
22
mangix@mangix-pc:~/devstuff/openwrt/staging_dir/target-mips_24kc_musl/usr/lib/pkgconfig$ cat * | grep exec_prefix=/usr | wc -l
166

neheb avatar Sep 17 '19 22:09 neheb

Keep in mind that unlike autotools, cmake does not do prefix/exec_prefix distinction. It will still work the same way. You could also just use @CMAKE_INSTALL_PREFIX@ again.

cotequeiroz avatar Sep 17 '19 22:09 cotequeiroz

Does CMake even use pkgconfig? I see files under /usr/lib/cmake which I'm guessing do what pkgconfig does for autotools. I see

/usr/lib/cmake/libevhtp/libevhtpConfig.cmake
/usr/lib/cmake/libevhtp/libevhtpConfigVersion.cmake
/usr/lib/cmake/libevhtp/libevhtpTargets.cmake
/usr/lib/cmake/libevhtp/libevhtpTargets-release.cmake

As far as pkgconfig files go, autotools should probably be the focus.

neheb avatar Sep 17 '19 22:09 neheb

Yes, they have their own package management tool. Keep in mind that you may want to build non-cmake-aware apps with the library.

cotequeiroz avatar Sep 17 '19 22:09 cotequeiroz

@cotequeiroz Your suggestion works fine for me as well, I just tested it under NixOS.

So how to proceed? Will you open a new PR with your proposed patch or shall I adapt it into this one?

schmittlauch avatar Sep 17 '19 23:09 schmittlauch

You choose. It’s such a small set of changes, that I would prefer not to do it on its own. Besides, we lose the discussion. If you want to give me credit, just write it in the commit message.

cotequeiroz avatar Sep 17 '19 23:09 cotequeiroz

incorporated the proposal of @cotequeiroz

schmittlauch avatar Sep 18 '19 09:09 schmittlauch

So is there any interest in merging this tiny fix?

schmittlauch avatar Jun 03 '20 22:06 schmittlauch

CLA assistant check
All committers have signed the CLA.

CLAassistant avatar Nov 13 '20 13:11 CLAassistant