Rework #14
Rework #14
Conversation
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
Signed-off-by: Nicolas Chauvet <kwizart@gmail.com>
| libdir=@CMAKE_INSTALL_LIBDIR@ | ||
| includedir=@CMAKE_INSTALL_INCLUDEDIR@ | ||
| AcesContainer_includedir=@CMAKE_INSTALL_INCLUDEDIR@/aces |
There was a problem hiding this comment.
These should be ${prefix}/@CMAKE_INSTALL_<dir>@ otherwise you get only "lib" or "include": https://people.freedesktop.org/~dbn/pkg-config-guide.html
There was a problem hiding this comment.
Thanks for review.
Sure, I can add a prefix.
There was a problem hiding this comment.
Well, I might have miss-understood. (there is already a prefix ).
Point is cmake doesn't pass variable relative to other variable. So if one ever one choose to use a custom layout, it should still work.
There was a problem hiding this comment.
Hm, not sure there is...
The current template libdir=@CMAKE_INSTALL_LIBDIR@ will evaluate to just libdir=lib, no?
It needs to be libdir=${exec_prefix}/lib (and same for the others), as shown in the pkgconf docs linked.
So the template should be
prefix=@CMAKE_INSTALL_PREFIX@
exec_prefix=${prefix}
includedir=${prefix}/@CMAKE_INSTALL_INCLUDEDIR@
libdir=${exec_prefix}/@CMAKE_INSTALL_LIBDIR@
I also suggest getting rid of AcesContainer_includedir and just use ${includedir}/aces directly in Cflags just like the example, it's more "traditional"...
| find_package( PkgConfig ) | ||
| if ( PKG_CONFIG_FOUND ) | ||
| configure_file(config/AcesContainer.pc.in "${PROJECT_BINARY_DIR}/AcesContainer.pc" @ONLY) | ||
| install( FILES "${PROJECT_BINARY_DIR}/AcesContainer.pc" DESTINATION lib/pkgconfig COMPONENT dev ) | ||
| install( FILES "${PROJECT_BINARY_DIR}/AcesContainer.pc" DESTINATION ${CMAKE_INSTALL_LIBDIR}/pkgconfig COMPONENT dev ) | ||
| endif() |
There was a problem hiding this comment.
Btw, no need to depend on PkgConfig here - just doing string substitution w/ CMake built-in configure_file() command, no pkgconf commands are actually used...
There was a problem hiding this comment.
If PkgConfig isn't found, then the $_libdir/pkgconfig "system directory" will likely not exist (when installed as system lib).
I don't think cmake will error on this.
This effectively makes pkgconf optional, which I think it is.
There was a problem hiding this comment.
I don't think anything prevents you from installing the $_libdir/pkgconfig/.pc files even if the directory doesn't exist... (The only exception might be if you're building the library w/ and for MSVC; all other platforms can use it.)
One is providing this file as convenience for clients of this library at all times - pkgconf is indeed optional for them (they choose what build system and library dependency handling they use), but nothing stops you providing .pc files (and equivalent .cmake config files for that matter) at all times. Most distro packages do, without checking for pkgconf (or cmake).
There was a problem hiding this comment.
Actually, it makes sense to just install always, even MSVC (i.e. vcpkg) supports it.
| VERSION ${AcesContainer_VERSION} | ||
| SOVERSION ${AcesContainer_MAJOR_VERSION}) | ||
|
|
||
| install (TARGETS AcesContainer EXPORT AcesContainerTargets DESTINATION ${CMAKE_INSTALL_LIBDIR}) |
There was a problem hiding this comment.
On Windows, need to make sure binary artifacts (DLL) is installed into bin while import library in lib. This is done by defining RUNTIME, ARCHIVE, and LIBRARY destinations: https://cmake.org/cmake/help/latest/command/install.html#installing-targets
There was a problem hiding this comment.
I Can change that, but I won't be able to test.
As I undertstood, I need to have:
install (TARGETS AcesContainer EXPORT AcesContainerTargets
RUNTIME DESTINATION ${CMAKE_INSTALL_BINDIR}
LIBRARY DESTINATION ${CMAKE_INSTALL_LIBDIR}
ARCHIVE DESTINATION ${CMAKE_INSTALL_LIBDIR}
)
There was a problem hiding this comment.
Yes! I plan to test your changes for a MINGW build and will let you know how it goes.
There was a problem hiding this comment.
OK, I have a flight tonight so after Easter I will try to add commit to fix theses issue. Hopefully next week.
This is an (old) rework of the CMakeLists.txt
Basically it uses standard installation variables instead of creating new ones.
Please test and report any issue.