pkgconfig: Fix support for absolute lib and include dirs

[akien-mga]

Using GNUInstallDirs to always have an absolute path regardless of the
user input: https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html

---

Fixes issue mentioned in EasyRPG/Player#1753 (comment)
CMAKE_INSTALL_{LIB,INCLUDE}DIR can be used both with a relative path (lib64, include) or absolute (/usr/lib64, /usr/local/include, etc.), and in the latter case the previous logic is broken.

One drawback of this change is that it no longer makes use expanding ${exec_prefix}, but to be honest I've never seen any real-life use case for that in .pc files. There might be on other platforms than Linux/*BSD though.

---

Not directly related, but since you put liblcf headers in a liblcf folder, I would advise for using #include <liblcf/data.h> instead of #include "data.h" in easyrpg-player, to avoid including the wrong file by mistake. (And I would have understood the issue in EasyRPG/Player#1753 (comment) without having to bother @fdelapena if liblcf was part of the include path.)

[fdelapena]

Thank you!

Not directly related, but since you put liblcf headers in a liblcf folder, I would advise for using #include <liblcf/data.h> instead of #include "data.h" in easyrpg-player, to avoid including the wrong file by mistake.

Yeah, another proposed approach while the API is not stable yet was renaming headers to make them more consistent (lcf_* prefix), as part of other improvements (stop exporting private headers as include headers, etc.).

[Ghabry]

The problem with CMake is always that there are too many ways to solve something and then you start copy-pasting bad code :/.

exec_prefix appears indeed unused, meson doesn't even support it: mesonbuild/meson#364

Yeah the issue with liblcf headers is that initially it was combined with Player code, then we split it. Someday, when the API is stable we should really move the headers around to fix this.

[Ghabry]

[mateofio]

We totally need to put all liblcf headers in a liblcf directory. #include "data.h" is terrible. It should be #include "liblcf/data.h"

Also, we should namespace everything in lcf::

Whenever the list of player PR's becomes small we can get this in. It's a huge change that will cause rebase hell for outstanding PRs.

That's the only reason I haven't done it yet.

[akien-mga]

We totally need to put all liblcf headers in a liblcf directory. #include "data.h" is terrible. It should be #include "liblcf/data.h"

Note that the headers are already in a liblcf directory. So you can do this change today a priori, independently of liblcf itself :)

[Ghabry]

To sum this up:

Autotools does not accept relativ prefixes and libdir and includedir are always relativ to (exec)prefix:

./configure --prefix "$PWD/aaa_installdir"

prefix=/tmp/easyrpg-liblcf/aaa_installdir
exec_prefix=${prefix}
libdir=${exec_prefix}/lib
includedir=${prefix}/include

Because CMake allows overwriting Libdir and includedir this breaks when not using relativ paths.

This works:

cmake . -DCMAKE_INSTALL_PREFIX=aaa_installdir
 -DCMAKE_INSTALL_LIBDIR=ilib -DCMAKE_INSTALL_INCLUDEDIR=iinclude
cmake . -DCMAKE_INSTALL_PREFIX=$PWD/aaa_installdir
 -DCMAKE_INSTALL_LIBDIR=ilib -DCMAKE_INSTALL_INCLUDEDIR=iinclude

prefix=/tmp/easyrpg-liblcf/aaa_installdir
exec_prefix=${prefix}
libdir=${exec_prefix}/ilib
includedir=${prefix}/iinclude

This breaks:

cmake . -DCMAKE_INSTALL_PREFIX=aaa_installdir 
 -DCMAKE_INSTALL_LIBDIR=$PWD/aaa_installdir/ilib 
 -DCMAKE_INSTALL_INCLUDEDIR=$PWD/aaa_installdir/ilib

libdir=${exec_prefix}//tmp/easyrpg-liblcf/aaa_installdir/ilib
includedir=${prefix}//tmp/easyrpg-liblcf/aaa_installdir/ilib

Conclusion:
Based on the autotools version I would consider include and libdirs outside of the install-prefix as invalid and paths that point into prefix could be rewritten by transforming into a relative path.

---

With a "convert to relative" (see patch below) conversion added:

The working from above: Still work and give same result.

Works with absolute:

cmake . -DCMAKE_INSTALL_PREFIX=aaa_installdir 
 -DCMAKE_INSTALL_LIBDIR=$PWD/aaa_installdir/ilib 
 -DCMAKE_INSTALL_INCLUDEDIR=$PWD/aaa_installdir/iinclude

prefix=/tmp/easyrpg-liblcf/aaa_installdir
exec_prefix=${prefix}
libdir=${exec_prefix}/ilib
includedir=${prefix}/iinclude

Works with mixed absolute/relative:

cmake . -DCMAKE_INSTALL_PREFIX=$PWD/aaa_installdir
  -DCMAKE_INSTALL_LIBDIR=ilib
  -DCMAKE_INSTALL_INCLUDEDIR=$PWD/aaa_installdir/iinclude

prefix=/tmp/easyrpg-liblcf/aaa_installdir
exec_prefix=${prefix}
libdir=${exec_prefix}/ilib
includedir=${prefix}/iinclude

Looks funny and works when one of the paths is outside of prefix:

cmake . -DCMAKE_INSTALL_PREFIX=$PWD/aaa_installdir
  -DCMAKE_INSTALL_LIBDIR=/outside/ilib
  -DCMAKE_INSTALL_INCLUDEDIR=$PWD/aaa_installdir/iinclude

prefix=/tmp/easyrpg-liblcf/aaa_installdir
exec_prefix=${prefix}
libdir=${exec_prefix}/../../../outside/ilib
includedir=${prefix}/iinclude

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 3809723..7d82bcf 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -275,12 +275,23 @@ endif()
 set_property(TARGET lcf PROPERTY SOVERSION 0)
 
 # pkg-config file generation
+include(GNUInstallDirs)
+
+set(LCF_LIBDIR ${CMAKE_INSTALL_LIBDIR})
+if(IS_ABSOLUTE ${LCF_LIBDIR})
+	file(RELATIVE_PATH LCF_LIBDIR ${CMAKE_INSTALL_PREFIX} ${LCF_LIBDIR})
+endif()
+set(LCF_INCLUDEDIR ${CMAKE_INSTALL_INCLUDEDIR})
+if(IS_ABSOLUTE ${LCF_INCLUDEDIR})
+	file(RELATIVE_PATH LCF_INCLUDEDIR ${CMAKE_INSTALL_PREFIX} ${LCF_INCLUDEDIR})
+endif()
+
 set(PACKAGE_TARNAME ${PROJECT_NAME})
 set(PACKAGE_VERSION ${PROJECT_VERSION})
 set(prefix "${CMAKE_INSTALL_PREFIX}")
 set(exec_prefix "\${prefix}")
-set(libdir "\${exec_prefix}/${CMAKE_INSTALL_LIBDIR}")
-set(includedir "\${prefix}/${CMAKE_INSTALL_INCLUDEDIR}")
+set(libdir "\${exec_prefix}/${LCF_LIBDIR}")
+set(includedir "\${prefix}/${LCF_INCLUDEDIR}")
 string(REPLACE ";" " " AX_PACKAGE_REQUIRES_PRIVATE "${LIBLCF_DEPS}")
 configure_file(builds/${PROJECT_NAME}.pc.in builds/${PROJECT_NAME}.pc @ONLY)
 
@@ -288,8 +299,6 @@ configure_file(builds/${PROJECT_NAME}.pc.in builds/${PROJECT_NAME}.pc @ONLY)
 configure_file(builds/${PROJECT_NAME}-config.cmake.in builds/${PROJECT_NAME}-config.cmake @ONLY)
 
 # installation
-include(GNUInstallDirs)
-
 install(
 	TARGETS lcf
 	ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}

[Ghabry]

As I consider this important enough for 0.6.2: Which solution do we want to accept?

[fdelapena]

Adding your approach as a 2nd commit to the PR looks good to me.

[fdelapena]

Not directly related, but since you put liblcf headers in a liblcf folder, I would advise for using #include <liblcf/data.h> instead of #include "data.h" (...)

This part is now handled at #361 and EasyRPG/Player#2099

[carstene1ns]

Looks good to me for now.

[Ghabry]

Not directly related, but since you put liblcf headers in a liblcf folder, I would advise for using #include <liblcf/data.h> instead of #include "data.h" (...)

This part is now handled at #361 and EasyRPG/Player#2099

Yeah we want to combine this liblcf subfolder moving with some other restructurings and doing this all in one step is easier to handle. Will be part of the release after 0.6.2.