|
Description
Ralf Habacker
2017-02-09 08:58:06 UTC
Created attachment 129424 [details] [review] Add and install find package config support to cmake build system. Comment on attachment 129424 [details] [review] Add and install find package config support to cmake build system. Review of attachment 129424 [details] [review]: ----------------------------------------------------------------- Is there canonical documentation that I can look at on how to write this stuff? This seems not particularly similar to PulseAudioConfig.cmake, which is the only example I have immediately available. ::: cmake/CMakeLists.txt @@ +101,2 @@ > else() > + set(INSTALL_TARGETS_DEFAULT_ARGS EXPORT DBus1Targets RUNTIME DESTINATION "${EXPANDED_BINDIR}" LIBRARY DESTINATION "${EXPANDED_LIBDIR}" ARCHIVE DESTINATION "${EXPANDED_LIBDIR}") This seems wrong. You're changing the RUNTIME DESTINATION for non-Windows builds from ${libdir} to ${bindir}. If RUNTIME DESTINATION is where we put public shared libraries (libdbus-1-3.dll, libdbus-1.so.3) then it's correct that it goes in bin on Windows but in lib everywhere else, because that's how Windows and Unix filesystem layouts conventionally work: Windows: bin\ foo.exe libfoo.dll lib\ libfoo.dll.a libfoo.a Unix: bin/ foo (executable) lib/ libfoo.a libfoo.so (analogous to .dll.a) libfoo.so.0 (analogous to .dll) @@ +644,5 @@ > +configure_file(DBus1ConfigVersion.cmake.in "${CMAKE_BINARY_DIR}/DBus1ConfigVersion.cmake" @ONLY) > +install(FILES > + "${CMAKE_BINARY_DIR}/DBus1Config.cmake" > + "${CMAKE_BINARY_DIR}/DBus1ConfigVersion.cmake" > + DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev Do we want to install (something derived from) these files when building with Autotools too, so that CMake users can consume a D-Bus version that was built using Autotools? @@ +646,5 @@ > + "${CMAKE_BINARY_DIR}/DBus1Config.cmake" > + "${CMAKE_BINARY_DIR}/DBus1ConfigVersion.cmake" > + DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev > +) > +install(EXPORT DBus1Targets DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev) What typically ends up in this file? ::: cmake/DBus1Config.cmake.in @@ +20,5 @@ > +set(DBus1_DEFINITIONS) > +set(DBus1_LIBRARIES dbus-1) > + > +set_property(TARGET dbus-1 PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${DBus1_INCLUDE_DIRS}) > +set_property(TARGET dbus-1 PROPERTY INTERFACE_COMPILE_DEFINITIONS ${DBus1_DEFINITIONS}) I would really prefer this stuff to be picked up from pkg-config, at least on Unix platforms, rather than duplicating it - pkg-config is the canonical "API" for how you link to libdbus. (It would be fine if we don't use pkg-config on Windows, if it's a problem there.) (In reply to Simon McVittie from comment #2) > Comment on attachment 129424 [details] [review] [review] > Add and install find package config support to cmake build system. > > Review of attachment 129424 [details] [review] [review]: > ----------------------------------------------------------------- > > Is there canonical documentation that I can look at on how to write this > stuff? yes, see below. In cmake there are two ways for generating package configuration files, the "module" and the "config" mode. In the "module" mode there are calls to find_path, find_library used to get the related locations at runtime. An example is https://cmake.org/Wiki/CMake:How_To_Find_Libraries#Writing_find_modules. In "config" mode there are specific cmake commands used to let cmake collect all specified executable and library targets and generated "importable" configurations files by default. A tutorial is located at https://cmake.org/Wiki/CMake/Tutorials/How_to_create_a_ProjectConfig.cmake_file > This seems not particularly similar to PulseAudioConfig.cmake, which is the > only example I have immediately available. The appended patch uses the "config" mode, which has the advantage that any newly added targets are exported to client packages by default. > > ::: cmake/CMakeLists.txt > @@ +101,2 @@ > > else() > > + set(INSTALL_TARGETS_DEFAULT_ARGS EXPORT DBus1Targets RUNTIME DESTINATION "${EXPANDED_BINDIR}" LIBRARY DESTINATION "${EXPANDED_LIBDIR}" ARCHIVE DESTINATION "${EXPANDED_LIBDIR}") > > This seems wrong. no > You're changing the RUNTIME DESTINATION for non-Windows builds from ${libdir} to ${bindir}. which fixes an issue installing executables on linux in lib dir instead of the required bin dir. > > If RUNTIME DESTINATION is where we put public shared libraries no, it points to where the executables should be installed > > @@ +644,5 @@ > > +configure_file(DBus1ConfigVersion.cmake.in "${CMAKE_BINARY_DIR}/DBus1ConfigVersion.cmake" @ONLY) > > +install(FILES > > + "${CMAKE_BINARY_DIR}/DBus1Config.cmake" > > + "${CMAKE_BINARY_DIR}/DBus1ConfigVersion.cmake" > > + DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev > > Do we want to install (something derived from) these files when building > with Autotools too, so that CMake users can consume a D-Bus version that was > built using Autotools? I did not thought about that, cmake provides a way to fetch installed package flags from pkgconfig. See the libxml example at https://cmake.org/Wiki/CMake:How_To_Find_Libraries#Writing_find_modules. > > @@ +646,5 @@ > > + "${CMAKE_BINARY_DIR}/DBus1Config.cmake" > > + "${CMAKE_BINARY_DIR}/DBus1ConfigVersion.cmake" > > + DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev > > +) > > +install(EXPORT DBus1Targets DESTINATION "${INSTALL_CMAKE_DIR}" COMPONENT dev) > > What typically ends up in this file? Adding the term "EXPORT DBus1Targets" to any add_executable or add_library target (which is part of this patch) let cmake create files named <PREFIX>Targets*.cmake containing imported targets, which provide cmake targets from the requested package to a client package as if they would created in the client package. See https://cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets for more informations. > ::: cmake/DBus1Config.cmake.in > @@ +20,5 @@ > > +set(DBus1_DEFINITIONS) > > +set(DBus1_LIBRARIES dbus-1) .. > I would really prefer this stuff to be picked up from pkg-config, at least > on Unix platforms, rather than duplicating it The mentioned term "dbus-1" is a cmake target name, not a library name intended to be prefixed with -l. > pkg-config is the canonical "API" for how you link to libdbus. Any idea how to get pkgconfig package informations from a packagesinstalled in a non standard location ? There is PKG_CONFIG_PATH to add special pathes to search for, but I did not find any way to setup a relocatable pc file ? > > +set_property(TARGET dbus-1 PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${DBus1_INCLUDE_DIRS}) > > +set_property(TARGET dbus-1 PROPERTY INTERFACE_COMPILE_DEFINITIONS ${DBus1_DEFINITIONS}) > This lines are to setup the imported cmake target name 'dbus-1' with the grabbed settings. The target name is then to be added to a target with target_link_libraries(<target> dbus-1) > (It would be fine if we don't use pkg-config on Windows, if it's a problem there.) pkgconfig is not supported on windows, therefore does the mentioned libxml2 find package configurations contains additional searches. In the dbus case there would be also a second find_path required to find the directory where dbus-arch-deps.h has been installed. find_path(DBus1_ARCH_INCLUDE_DIR dbus/dbus-arch-deps.h HINTS ${PC_DBUS1_INCLUDE_DIRS} PATH_SUFFIXES dbus-1.0 ) (In reply to Ralf Habacker from comment #3) > > You're changing the RUNTIME DESTINATION for non-Windows builds from ${libdir} to ${bindir}. > which fixes an issue installing executables on linux in lib dir instead of > the required bin dir. Then please do that as its own commit, and say why in the commit message - if it's fixing a pre-existing bug, that's fine, but a future maintainer isn't going to know what the bug was unless the commit log tells them. If fixing that bug is a prerequisite for applying the rest of your find-package support, we can make sure to apply them in that order. No need to open a separate bug report unless you particularly want to, you can attach it to this one - but if you're changing things that the other maintainers don't really understand, which includes the whole CMake build system, it's doubly important for commit messages to be clear about what you're changing and why. > > Do we want to install (something derived from) these files when building > > with Autotools too, so that CMake users can consume a D-Bus version that was > > built using Autotools? > I did not thought about that, cmake provides a way to fetch installed > package flags from pkgconfig. See the libxml example at > https://cmake.org/Wiki/CMake:How_To_Find_Libraries#Writing_find_modules. What I meant here is sort of the other way round: suppose you or your Linux distribution have installed dbus with Autotools, and now you want to use that installation to build some KDE thing with CMake. If it's feasible to make the Autotools build system install a compatible version of DBus1Config.cmake, then the KDE thing won't need to know whether dbus was built using CMake or Autotools, it can just use DBus1Config.cmake either way. (Of course, that relies on the assumption that the expected contents of the installed files is something that Autotools can replicate.) > Adding the term "EXPORT DBus1Targets" to any add_executable or add_library > target (which is part of this patch) let cmake create files named > <PREFIX>Targets*.cmake containing imported targets, which provide cmake > targets from the requested package to a client package as if they would > created in the client package. See > https://cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets for > more informations. That sounds as though it wouldn't necessarily be very feasible to do without running CMake though. But perhaps I'm wrong? If you try a reasonably typical build (--prefix=/usr/local or --prefix=/mingw32 or whatever), what text ends up in those files? I'm not sure that this is necessarily a valuable thing to have for every target. Our only public API is the -ldbus-1 library (and the executables in ${bindir}, but users of those are really meant to arrange for them to be in $PATH or %PATH% as appropriate). Executables that are installed in ${libexecdir}, like dbus-daemon-launch-helper and the tests, are definitely not meant to be "user-facing". > > I would really prefer this stuff to be picked up from pkg-config, at least > > on Unix platforms, rather than duplicating it > > The mentioned term "dbus-1" is a cmake target name, not a library name > intended to be prefixed with -l. Oh, OK. So it's broadly equivalent to dbus-1.pc in the Autotools/pkg-config world - "here is an abstract piece of API you can use". > Any idea how to get pkgconfig package informations from a packagesinstalled > in a non standard location ? There is PKG_CONFIG_PATH to add special pathes > to search for, but I did not find any way to setup a relocatable pc file ? It's possible to define locations in terms of ${pcfiledir}, although for that to work, you have to be able to build a relative path between the pkg-config file directory (which could be either lib/pkgconfig or lib/x86_64/pkgconfig or even something different, depending on build options) and the prefix. Perhaps we could do this a bit like configure.ac's handling of SYSCONFDIR_FROM_PKGDATADIR and DATADIR_FROM_PKGSYSCONFDIR? Here's how I'd spell that in Autotools language: AS_IF([test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib"], [pkgconfig_prefix='${pcfiledir}/../..'], [pkgconfig_prefix="$prefix"]) AC_SUBST([pkgconfig_prefix]) and then use @pkgconfig_prefix@ instead of @prefix@ in the .pc file. This would result in the .pc file starting with the literal text "prefix=${pcfiledir}/../.." in a build with the default --libdir, making it relocatable; but if a distribution like Debian has overridden the --libdir, then it will hard-code a non-relocatable prefix instead. That's fine because distribution builds aren't designed to be relocatable anyway. (As with sysconfdir vs. pkgdatadir, in non-default build configurations I don't want to get into counting how many "/" there are and adding the right number of "../" to compensate - that seems easy to get wrong.) Created attachment 129491 [details] [review] Keep cmake include file installation dir on Windows in sync with Unix. Created attachment 129492 [details] [review] Add find package config support to cmake build system. - rebased (In reply to Simon McVittie from comment #4) > (In reply to Ralf Habacker from comment #3) > > > You're changing the RUNTIME DESTINATION for non-Windows builds from ${libdir} to ${bindir}. > > which fixes an issue installing executables on linux in lib dir instead of > > the required bin dir. > > Then please do that as its own commit, fixed with bug 99752 Created attachment 129521 [details] [review] cmake: Use relative install locations on Unix too Using expanded paths make no sense in install commands because they may be patched by cmake for example by specifying DESTDIR on install. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99752 Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk> Bug: Created attachment 129522 [details] [review] cmake: use default GNU installation layout. cmake provides a macro named GnuInstallDirs to let install locations be compatible with GNU's install location layout on several plattforms. Using that layout makes cmake installs be more compatible to what autotools use and also supports 32 and 64 bit installations out of the box. Created attachment 129523 [details] [review] cmake: create relocatable dbus-1.pc file Relocatable pkgconfig files are required to use installed packages from a non standard location. Created attachment 129524 [details] [review] cmake: Keep include file install locations on Windows in sync with Unix. Created attachment 129525 [details] [review] cmake: Add find package config support - use pkgconfig approach on unix Created attachment 129526 [details] [review] Add cmake find_package support to autotools build system. Install cmake find_package related config files with autotools to let clients using cmake >= 2.6 find dbus library and headers by default. Created attachment 129538 [details] [review] cmake: Add find_package support for windows. pkg-config on windows is not supported so we need to use cmake buildin target export support, which exports all targets into DBus1ConfigTargets*.cmake on build. The files are installed into the related directory where cmake expects find_package related config files. Comment on attachment 129521 [details] [review] cmake: Use relative install locations on Unix too fixed with bug 99752 (In reply to Simon McVittie from comment #4) > (In reply to Ralf Habacker from comment #3) > What I meant here is sort of the other way round: suppose you or your Linux > distribution have installed dbus with Autotools, and now you want to use > that installation to build some KDE thing with CMake. If it's feasible to > make the Autotools build system install a compatible version of > DBus1Config.cmake, then the KDE thing won't need to know whether dbus was > built using CMake or Autotools, it can just use DBus1Config.cmake either way. see patches > (Of course, that relies on the assumption that the expected contents of the > installed files is something that Autotools can replicate.) > > > Adding the term "EXPORT DBus1Targets" to any add_executable or add_library > > target (which is part of this patch) let cmake create files named > > <PREFIX>Targets*.cmake containing imported targets, which provide cmake > > targets from the requested package to a client package as if they would > > created in the client package. See > > https://cmake.org/Wiki/CMake/Tutorials/Exporting_and_Importing_Targets for > > more informations. > > That sounds as though it wouldn't necessarily be very feasible to do without > running CMake though. But perhaps I'm wrong? > If you try a reasonably typical > build (--prefix=/usr/local or --prefix=/mingw32 or whatever), in cmake world this is cmake -DCMAKE_INSTALL_PREFIX=/usr/local/ > what text ends up in those files? something like the following # Create imported target dbus-1 add_library(dbus-1 SHARED IMPORTED) set_property(TARGET dbus-1 APPEND PROPERTY IMPORTED_CONFIGURATIONS NOCONFIG) set_target_properties(dbus-1 PROPERTIES IMPORTED_LINK_INTERFACE_LIBRARIES_NOCONFIG "-lpthread;/usr/lib64/librt.so" IMPORTED_LOCATION_NOCONFIG "${_IMPORT_PREFIX}/lib64/libdbus-1.so.3.16.3" IMPORTED_SONAME_NOCONFIG "libdbus-1.so.3" ) > I'm not sure that this is necessarily a valuable thing to have for every > target. Our only public API is the -ldbus-1 library (and the executables in > ${bindir}, but users of those are really meant to arrange for them to be in > $PATH or %PATH% as appropriate). cmake exports covers that also set_property(TARGET dbus-daemon APPEND PROPERTY IMPORTED_CONFIGURATIONS NOCONFIG) set_target_properties(dbus-daemon PROPERTIES IMPORTED_LOCATION_NOCONFIG "${_IMPORT_PREFIX}/bin/dbus-daemon" ) > Executables that are installed in > ${libexecdir}, like dbus-daemon-launch-helper This is still installed in lib > > > I would really prefer this stuff to be picked up from pkg-config, at least > > > on Unix platforms, rather than duplicating it see patches > AS_IF([test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib"], > [pkgconfig_prefix='${pcfiledir}/../..'], > [pkgconfig_prefix="$prefix"]) > AC_SUBST([pkgconfig_prefix]) > > and then use @pkgconfig_prefix@ instead of @prefix@ in the .pc file. see patches Comment on attachment 129522 [details] [review] cmake: use default GNU installation layout. Review of attachment 129522 [details] [review]: ----------------------------------------------------------------- Great! r+ on the assumption that ${CMAKE_INSTALL_BINDIR} would typically be "bin", and ${CMAKE_INSTALL_FULL_BINDIR} would be "/usr/local/bin" or something. ::: cmake/CMakeLists.txt @@ +66,4 @@ > > set(prefix ${DBUS_INSTALL_DIR}) > set(exec_prefix ${prefix}) > +set(DBUS_MACHINE_UUID_FILE ${DBUS_INSTALL_FULL_LIBDIR}/dbus/machine-id) This seems really strange: it should be in ${DBUS_LOCALSTATEDIR}/dbus. But that isn't a regression, it was in this weird place already, so OK. @@ +68,5 @@ > set(exec_prefix ${prefix}) > +set(DBUS_MACHINE_UUID_FILE ${DBUS_INSTALL_FULL_LIBDIR}/dbus/machine-id) > +set(DBUS_BINDIR ${CMAKE_INSTALL_FULL_BINDIR}) > +set(DBUS_DAEMONDIR ${CMAKE_INSTALL_FULL_BINDIR}) > +set(DBUS_LOCALSTATEDIR ${CMAKE_INSTALL_FULL_LOCALSTATEDIR}) These all start with /usr/local or equivalent, right? (In particular, if we have a DESTDIR it isn't included?) Comment on attachment 129523 [details] [review] cmake: create relocatable dbus-1.pc file Review of attachment 129523 [details] [review]: ----------------------------------------------------------------- You seem to have lost LIBDBUS_LIBS here. Do you need to put that back? ::: cmake/CMakeLists.txt @@ +634,4 @@ > set(PLATFORM_LIBS pthread ${LIBRT}) > include(FindPkgConfig QUIET) > if(PKG_CONFIG_FOUND) > + set(prefix "\${pcfiledir}/../..") As I mentioned before, this is going to be wrong if ${CMAKE_INSTALL_LIBDIR} has more than one path component (it would often be something like lib/x86_64-linux-gnu on Debian). We can generate a relocatable .pc file if ${CMAKE_INSTALL_LIBDIR} is "lib", and a non-relocatable one (${prefix} hard-coded) otherwise. Comment on attachment 129524 [details] [review] cmake: Keep include file install locations on Windows in sync with Unix. Review of attachment 129524 [details] [review]: ----------------------------------------------------------------- Great, I like simplification :-) Comment on attachment 129525 [details] [review] cmake: Add find package config support Review of attachment 129525 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.cmake.in @@ +8,5 @@ > +# [1] This variable is not required if DBus1_LIBRARIES is added > +# to a target with target_link_libraries > + > +# Compute paths > +get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE) This is going to fail whenever ${CMAKE_INSTALL_LIBDIR} contains more than one path component. I'd prefer to compute it in the build system and substitute it into this .in file with @DBus1_INSTALL_DIR@ or similar. It can have logic like this pseudocode: if CMAKE_INSTALL_LIBDIR == 'lib': DBus1_INSTALL_DIR = '${CMAKE_CURRENT_LIST_DIR}/../../../' # need to make sure the $ is literal else: DBus1_INSTALL_DIR = $DBUS_INSTALL_DIR # evaluated at build time @@ +9,5 @@ > +# to a target with target_link_libraries > + > +# Compute paths > +get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE) > +get_filename_component(DBus1_PKGCONFIG_DIR "${CMAKE_CURRENT_LIST_DIR}/../../pkgconfig" ABSOLUTE) I think this one is OK though, because we have a non-configurable layout for pkgconfig relative to cmake. Comment on attachment 129526 [details] [review] Add cmake find_package support to autotools build system. Review of attachment 129526 [details] [review]: ----------------------------------------------------------------- ::: Makefile.am @@ +10,4 @@ > DISTCLEANFILES = \ > + dbus-1.pc \ > + DBus1Config.cmake \ > + DBus1ConfigVersion.cmake Would be slightly better as: DISTCLEANFILES = \ $(cmakeconfig_DATA) \ $(pkgconfig_DATA) \ $(NULL) to reduce duplication ::: cmake/CMakeLists.txt @@ -646,4 @@ > set(PLATFORM_LIBS pthread ${LIBRT}) > include(FindPkgConfig QUIET) > if(PKG_CONFIG_FOUND) > - set(prefix "\${pcfiledir}/../..") (Why in this commit?) ::: dbus-1.pc.in @@ +1,1 @@ > +prefix=${pcfiledir}/../.. As in the CMake commit, this won't work for a non-default ${libdir}; we need to restrict relocatable builds to the situation where ${libdir} evaluates to the same thing as ${prefix}/lib. Comment on attachment 129538 [details] [review] cmake: Add find_package support for windows. Review of attachment 129538 [details] [review]: ----------------------------------------------------------------- Nice! > pkg-config on windows is not supported pkg-config certainly does contain Windows-specific code, so either it's supported on Windows (by someone) or there's a lot of simplification that could be done. I think it would be more accurate to say that we don't want to require CMake users to have pkg-config installed, particularly on Windows. ::: cmake/DBus1Config.cmake.in @@ +11,5 @@ > # Compute paths > get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE) > +# Our library dependencies (contains definitions for IMPORTED targets) > +if(EXISTS "${CMAKE_CURRENT_LIST_DIR}/DBus1Targets.cmake") > + # do not additional search pathes for implicit libraries "search paths" Comment on attachment 129522 [details] [review] cmake: use default GNU installation layout. Review of attachment 129522 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +66,4 @@ > > set(prefix ${DBUS_INSTALL_DIR}) > set(exec_prefix ${prefix}) > +set(DBUS_MACHINE_UUID_FILE ${DBUS_INSTALL_FULL_LIBDIR}/dbus/machine-id) Will be another patch @@ +68,5 @@ > set(exec_prefix ${prefix}) > +set(DBUS_MACHINE_UUID_FILE ${DBUS_INSTALL_FULL_LIBDIR}/dbus/machine-id) > +set(DBUS_BINDIR ${CMAKE_INSTALL_FULL_BINDIR}) > +set(DBUS_DAEMONDIR ${CMAKE_INSTALL_FULL_BINDIR}) > +set(DBUS_LOCALSTATEDIR ${CMAKE_INSTALL_FULL_LOCALSTATEDIR}) yes,_FULL_ variables are based on CMAKE_INSTALL_PREFIX which is the 'prefix' autotools equivalent. See https://cmake.org/cmake/help/v3.0/module/GNUInstallDirs.html for more details. applied fixes to git master Created attachment 129557 [details] [review] cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools. (In reply to Simon McVittie from comment #18) > Comment on attachment 129523 [details] [review] [review] > cmake: create relocatable dbus-1.pc file > > Review of attachment 129523 [details] [review] [review]: > ----------------------------------------------------------------- > > You seem to have lost LIBDBUS_LIBS here. Do you need to put that back? Was irritated by the unsupported libs.private in pkgconfig 0.28, will readd that > ::: cmake/CMakeLists.txt > @@ +634,4 @@ > > set(PLATFORM_LIBS pthread ${LIBRT}) > > include(FindPkgConfig QUIET) > > if(PKG_CONFIG_FOUND) > > + set(prefix "\${pcfiledir}/../..") > > As I mentioned before, this is going to be wrong if ${CMAKE_INSTALL_LIBDIR} > has more than one path component (it would often be something like > lib/x86_64-linux-gnu on Debian). Sorry, oversaw that in this patch set. https://cmake.org/cmake/help/v3.0/command/find_package.html mentiones a different solution for this: "... Paths with lib/<arch> are enabled if the CMAKE_LIBRARY_ARCHITECTURE variable is set.." and at https://cmake.org/cmake/help/v3.0/variable/CMAKE_LIBRARY_ARCHITECTURE.html#variable:CMAKE_LIBRARY_ARCHITECTURE CMAKE_LIBRARY_ARCHITECTURE - Target architecture library directory name, if detected. so it can be if(CMAKE_LIBRARY_ARCHITECTURE) set(prefix "\${pcfiledir}/../../..") else() set(prefix "\${pcfiledir}/../../") endif() (In reply to Simon McVittie from comment #20) > Comment on attachment 129525 [details] [review] [review] > cmake: Add find package config support > > Review of attachment 129525 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/DBus1Config.cmake.in > @@ +8,5 @@ > > +# [1] This variable is not required if DBus1_LIBRARIES is added > > +# to a target with target_link_libraries > > + > > +# Compute paths > > +get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE) > > This is going to fail whenever ${CMAKE_INSTALL_LIBDIR} contains more than > one path component. here the above mentioned solution is also possible if(CMAKE_LIBRARY_ARCHITECTURE) get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../../" else() get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" endif() (In reply to Simon McVittie from comment #21) > Comment on attachment 129526 [details] [review] [review] > Add cmake find_package support to autotools build system. > > Review of attachment 129526 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Makefile.am > DISTCLEANFILES = \ > $(cmakeconfig_DATA) \ > $(pkgconfig_DATA) \ > $(NULL) > > to reduce duplication will fix that > > ::: cmake/CMakeLists.txt > @@ -646,4 @@ > > set(PLATFORM_LIBS pthread ${LIBRT}) > > include(FindPkgConfig QUIET) > > if(PKG_CONFIG_FOUND) > > - set(prefix "\${pcfiledir}/../..") > > (Why in this commit?) obsolate, see below > ::: dbus-1.pc.in > @@ +1,1 @@ > > +prefix=${pcfiledir}/../.. is obsolate now because will be ixed in generating dbus-1.pc code using CMAKE_LIBRARY_ARCHITECTURE. Created attachment 129560 [details] [review] cmake: create relocatable dbus-1.pc file - update Created attachment 129561 [details] [review] cmake: Add find package config support - update Created attachment 129562 [details] [review] Add cmake find_package support to autotools build system. - update Created attachment 129563 [details] [review] cmake: Add find_package support for windows. - update Comment on attachment 129557 [details] [review] cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools. Review of attachment 129557 [details] [review]: ----------------------------------------------------------------- Looks good (assuming CMAKE_INSTALL_FULL_LOCALSTATEDIR defaults to ${prefix}/var like its Autotools equivalent does) Comment on attachment 129560 [details] [review] cmake: create relocatable dbus-1.pc file Review of attachment 129560 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +641,5 @@ > + if(CMAKE_LIBRARY_ARCHITECTURE) > + set(prefix "\${pcfiledir}/../../../") > + else() > + set(prefix "\${pcfiledir}/../../") > + endif() Interesting solution, but I'd still prefer to leave it non-relocatable in the cases where ${CMAKE_INSTALL_LIBDIR} is not what we expect. The reason I say this is that this is correct for the *default* libdir, but if the user overrides CMAKE_INSTALL_LIBDIR on the command-line or by editing the cache file (CMake equivalents of ./configure --libdir=/whatever) and their override has a different number of path segments than the default, it will be wrong. In particular, on a Debian-derived system where CMAKE_LIBRARY_ARCHITECTURE is non-empty, if a user overrides CMAKE_INSTALL_LIBDIR back to "lib" because they want a standalone/portable/relocatable build, I think it's going to break relocation! In practice, I think ${CMAKE_INSTALL_LIBDIR}=lib (and *maybe* lib64, but that's more complexity than it's worth IMO) is the only case where relocatability is useful - the reason you'd override it is to follow some distribution's policy, but distribution policy rarely (if ever) allows for relocation anyway. Comment on attachment 129561 [details] [review] cmake: Add find package config support Review of attachment 129561 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.cmake.in @@ +8,5 @@ > +# [1] This variable is not required if DBus1_LIBRARIES is added > +# to a target with target_link_libraries > + > +# Compute paths > +if(CMAKE_LIBRARY_ARCHITECTURE) I think we need to make this decision at dbus' configure/cmake time (using a @FOO@ substitution), rather than when the dependent software (a KDE application or whatever) is built. I don't think there's any guarantee that the libdir configuration for dbus is the same as the libdir configuration for the dependent software. As with the pkg-config, I'd really prefer this to be relocatable if and only if CMAKE_INSTALL_LIBDIR is exactly "lib" - otherwise I think we're going to fail relocatability (or fail to build dependent software entirely) whenever user configuration doesn't match what we would expect from the presence or absence of CMAKE_LIBRARY_ARCHITECTURE. Comment on attachment 129562 [details] [review] Add cmake find_package support to autotools build system. Review of attachment 129562 [details] [review]: ----------------------------------------------------------------- Looks good, but can't be merged until the rest of this bug is ready. Comment on attachment 129563 [details] [review] cmake: Add find_package support for windows. Review of attachment 129563 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.cmake.in @@ +31,5 @@ > + set(DBus1_LIBRARIES dbus-1) > + > + set_property(TARGET dbus-1 PROPERTY INTERFACE_INCLUDE_DIRECTORIES ${DBus1_INCLUDE_DIRS}) > + set_property(TARGET dbus-1 PROPERTY INTERFACE_COMPILE_DEFINITIONS ${DBus1_DEFINITIONS}) > +else() # installed from autotools Correct me if I'm wrong here, but in this version of the patch I think you only install DBus1Targets.cmake when building for a Windows host *and* using CMake? So this comment should be # installed from Autotools and/or on Unix (In reply to Simon McVittie from comment #32) > Comment on attachment 129557 [details] [review] [review] > cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools. > > Review of attachment 129557 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks good (assuming CMAKE_INSTALL_FULL_LOCALSTATEDIR defaults to > ${prefix}/var like its Autotools equivalent does) it does, see https://cmake.org/cmake/help/v3.0/module/GNUInstallDirs.html LOCALSTATEDIR - modifiable single-machine data (var) (In reply to Simon McVittie from comment #33) > Comment on attachment 129560 [details] [review] [review] > cmake: create relocatable dbus-1.pc file > > Review of attachment 129560 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/CMakeLists.txt > @@ +641,5 @@ > > + if(CMAKE_LIBRARY_ARCHITECTURE) > > + set(prefix "\${pcfiledir}/../../../") > > + else() > > + set(prefix "\${pcfiledir}/../../") > > + endif() > > Interesting solution, but I'd still prefer to leave it non-relocatable in > the cases where ${CMAKE_INSTALL_LIBDIR} is not what we expect. > Many thanks for this detailed information :-) I'm going to change that into for now # no relocatable support for debian multiarch because # CMAKE_INSTALL_LIBDIR may be overriden on command line if(CMAKE_INSTALL_LIBDIR STREQUAL "lib" OR CMAKE_INSTALL_LIBDIR STREQUAL "lib64") set(prefix "\${pcfiledir}/../../") else() set(prefix "${CMAKE_INSTALL_PREFIX}") endif() (In reply to Simon McVittie from comment #34) > Comment on attachment 129561 [details] [review] [review] > cmake: Add find package config support > > Review of attachment 129561 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/DBus1Config.cmake.in > @@ +8,5 @@ > > +# [1] This variable is not required if DBus1_LIBRARIES is added > > +# to a target with target_link_libraries > > + > > +# Compute paths > > +if(CMAKE_LIBRARY_ARCHITECTURE) > > I think we need to make this decision at dbus' configure/cmake time (using a > @FOO@ substitution), rather than when the dependent software (a KDE > application or whatever) is built. I don't think there's any guarantee that > the libdir configuration for dbus is the same as the libdir configuration > for the dependent software. for cmake I'm going to add the following stuff to cmake/CMakeLists.txt # used in DBus1Config.cmake.in if(CMAKE_LIBRARY_ARCHITECTURE) set(DBUS_MULTIARCH_LIBDIR 1) else() set(DBUS_MULTIARCH_LIBDIR 0) endif() configure_file(DBus1Config.cmake.in "${CMAKE_BINARY_DIR}/DBus1Config.cmake" @ONLY) ----------- in DBus1Config.cmake.in ------------ ... # Compute paths if(@DBUS_MULTIARCH_LIBDIR@) get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../../" ABSOLUTE) else() get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}/../../../" ABSOLUTE) endif() How to set DBUS_MULTIARCH_LIBDIR with autotools ? Created attachment 129592 [details] [review] cmake: create relocatable dbus-1.pc file - add multiarch issue hint - do not make package relocatable on multiarch distributions Created attachment 129593 [details] [review] cmake: Add find package config support - fix multi arch issue Created attachment 129594 [details] [review] cmake: Add find_package support for windows. - fix comments (In reply to Ralf Habacker from comment #38) > I'm going to change that into for now > # no relocatable support for debian multiarch because > # CMAKE_INSTALL_LIBDIR may be overriden on command line > if(CMAKE_INSTALL_LIBDIR STREQUAL "lib" OR CMAKE_INSTALL_LIBDIR > STREQUAL "lib64") > set(prefix "\${pcfiledir}/../../") > else() > set(prefix "${CMAKE_INSTALL_PREFIX}") > endif() The logic makes sense, the comment less so. Perhaps this? # For simplicity, we're not relocatable if CMAKE_INSTALL_LIBDIR # is something more complicated (e.g. Debian multiarch); # we don't want to have to compute how many ../ to add Comment on attachment 129592 [details] [review] cmake: create relocatable dbus-1.pc file Review of attachment 129592 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ +615,5 @@ > endif(DBUS_DISABLE_CHECKS) > > +if(CMAKE_LIBRARY_ARCHITECTURE) > + message("WARNING: multi architecture library location detected. The generated package may not be usable from non standard install locations.") > +endif() This would seem more appropriate in the block that sets prefix to an absolute path, so that instead of essentially warning "you're on Debian" even if you have used -DCMAKE_INSTALL_LIBDIR=lib, it is actually warning about the non-relocatable case. Perhaps like this? else() message("Unusual CMAKE_INSTALL_PREFIX: the generated package will not be relocatable.") set(prefix "${CMAKE_INSTALL_PREFIX}") endif() Comment on attachment 129593 [details] [review] cmake: Add find package config support Review of attachment 129593 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.cmake.in @@ +8,5 @@ > +# [1] This variable is not required if DBus1_LIBRARIES is added > +# to a target with target_link_libraries > + > +# Compute paths > +if(@DBUS_MULTIARCH_LIBDIR@) This is still switching on whether the platform we built dbus on appears to be Debian-derived, and not on whether the libdir is actually one of the ones that is too complicated to be relocatable. So it's still going to do the wrong thing if a user builds dbus on Debian (multiarch) with -DCMAKE_INSTALL_LIBDIR="lib", or if a user builds on Red Hat (non-multiarch) but for some reason they choose to use -DCMAKE_INSTALL_LIDBIR="private-libs/64bit" or something. If you swap the direction of this check, and change it to if(@DBUS_RELOCATABLE_LIBDIR@) get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}../../.." ABSOLUTE) else() set(DBus1_INSTALL_DIR "@DBUS_INSTALL_DIR@") endif() and then in our CMake files, use if(${CMAKE_INSTALL_LIBDIR} EQUAL "lib" OR ${CMAKE_INSTALL_LIBDIR} EQUAL "lib64") # we know we can use ../ to get to the prefix set(DBUS_RELOCATABLE_LIBDIR true) else() # sorry, it's too hard to make this particular build relocatable set(DBUS_RELOCATABLE_LIBDIR false) endif() would that work? I might have got the CMake syntax a bit wrong, but hopefully you get the general idea. (Or you could even use a check that CMAKE_INSTALL_LIBDIR does not contain "/", instead of comparing with hard-coded values, but I don't know how to spell that in CMake syntax.) (In reply to Ralf Habacker from comment #39) > How to set DBUS_MULTIARCH_LIBDIR with autotools ? Don't - as I said in reviewing the CMake stuff, it's the wrong check. Instead compare the EXPANDED_LIBDIR with what we would expect it to be (this would have to come after the AS_AC_EXPAND commands that set up EXPANDED_LIBDIR): AS_IF([test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib"], [pkgconfig_prefix='${pcfiledir}/../..'], [pkgconfig_prefix="$prefix"]) AC_SUBST([pkgconfig_prefix]) and change the .pc file to use prefix=@pkgconfig_prefix@ instead of prefix=@prefix@. This would have to go with a corresponding CMake change to change ${prefix} back to ${CMAKE_INSTALL_PREFIX}, and set ${pkgconfig_prefix} to '${pcfiledir}/../..' (suitably escaped so that CMake doesn't try to expand ${pcfiledir}). We can't change the meaning of ${prefix}, because that's a built-in Autotools thing that has other effects. I'd prefer the CMake ${prefix} to either not exist, or mean exactly the same thing it means in Autotools, which I think is ${CMAKE_INSTALL_PREFIX} (a typical value would be /usr or /usr/local). Optionally, you could change the test from test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib" to test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib" || test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib64" but I'm not sure it's really worth it. (In reply to Simon McVittie from comment #46) > AS_IF([test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib" > || test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib64"], > [pkgconfig_prefix='${pcfiledir}/../..'], > [pkgconfig_prefix="$prefix"]) > AC_SUBST([pkgconfig_prefix]) I'm going to add this to configure.ac > and change the .pc file to use prefix=@pkgconfig_prefix@ instead of > prefix=@prefix@. also added > > This would have to go with a corresponding CMake change to change ${prefix} > back to ${CMAKE_INSTALL_PREFIX}, and set ${pkgconfig_prefix} to > '${pcfiledir}/../..' (suitably escaped so that CMake doesn't try to expand > ${pcfiledir}). of course the cmake part generating the pkgconfig file need to be adjusted to use "set(pkgconfig_prefix" instead of "set(prefix" I understand to change set(prefix into set(pkg in CMakeList.txt in the "creating pkgconfig part" if(DBUS_RELOCATABLE_LIBDIR) set(pkgconfig_prefix "\${pcfiledir}/../../") else() message("Unusual CMAKE_INSTALL_PREFIX: the generated package will not be relocatable.") set(pkgconfig_prefix "${CMAKE_INSTALL_PREFIX}") endif() > We can't change the meaning of ${prefix}, because that's a built-in > Autotools thing that has other effects. > I'd prefer the CMake ${prefix} to either not exist, > or mean exactly the same thing it means in Autotools, > which I think is ${CMAKE_INSTALL_PREFIX} (a typical value would be /usr or > /usr/local). this looks to be given if (DBUSDIR) set(DBUS_INSTALL_DIR "${DBUSDIR}") endif (DBUSDIR) if ($ENV{DBUSDIR}) set(DBUS_INSTALL_DIR "$ENV{DBUSDIR}") endif ($ENV{DBUSDIR}) if (DBUS_INSTALL_DIR) set(CMAKE_INSTALL_PREFIX "${DBUS_INSTALL_DIR}" CACHE PATH "install prefix" FORCE) else (DBUS_INSTALL_DIR) set(DBUS_INSTALL_DIR "${CMAKE_INSTALL_PREFIX}") endif (DBUS_INSTALL_DIR) set(DBUS_PREFIX ${DBUS_INSTALL_DIR}) set(prefix ${DBUS_INSTALL_DIR}) set(exec_prefix ${prefix}) Created attachment 129611 [details] [review] cmake: create relocatable dbus-1.pc file - fixup Created attachment 129612 [details] [review] cmake: Add find package config support - fixup Created attachment 129613 [details] [review] cmake: Add find_package support for windows. - fixup Created attachment 129614 [details] [review] Add cmake find_package support to autotools build system. - fixup Created attachment 129615 [details] [review] Let autotools generate a relocatable dbus-1.pc in standard cases. For simplicity, we're not relocatable if libdir is something more complicated (e.g. Debian multiarch); we don't want to have to compute how many ../ to add. Related cmake changes are included. Comment on attachment 129557 [details] [review] cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools. committed to git master (In reply to Ralf Habacker from comment #47) > > This would have to go with a corresponding CMake change to change ${prefix} > > back to ${CMAKE_INSTALL_PREFIX}, and set ${pkgconfig_prefix} to > > '${pcfiledir}/../..' (suitably escaped so that CMake doesn't try to expand > > ${pcfiledir}). > of course the cmake part generating the pkgconfig file need to be adjusted > to use "set(pkgconfig_prefix" instead of "set(prefix" > > I understand to change set(prefix into set(pkg in CMakeList.txt in the > "creating pkgconfig part" > > if(DBUS_RELOCATABLE_LIBDIR) > set(pkgconfig_prefix "\${pcfiledir}/../../") > else() > message("Unusual CMAKE_INSTALL_PREFIX: the generated package > will not be relocatable.") > set(pkgconfig_prefix "${CMAKE_INSTALL_PREFIX}") > endif() Yes, that's what I meant. Sorry if I was unclear, but you seem to have got the right idea anyway :-) > > I'd prefer the CMake ${prefix} to either not exist, > > or mean exactly the same thing it means in Autotools > > this looks to be given ... > set(prefix ${DBUS_INSTALL_DIR}) > set(exec_prefix ${prefix}) Yes, that's how I would prefer it to be. I think at some point in the history of this bug you were setting prefix to "\${pcfiledir}/../../", which would have been unlike Autotools (and hence not how I wanted it to be). Comment on attachment 129611 [details] [review] cmake: create relocatable dbus-1.pc file Review of attachment 129611 [details] [review]: ----------------------------------------------------------------- This sets prefix to a value that doesn't match Autotools, which is not right. But that's fixed in your later patch "Let autotools generate a relocatable dbus-1.pc in standard cases" (Attachment #129615 [details]). Those two patches put together look OK. I'd prefer to combine them into one, so we're going directly from one correct/consistent situation (${prefix} is always absolute, .pc file is never relocatable) to another correct/consistent situation (${prefix is still always absolute, but .pc file is relocatable if the ${libdir} is suitable for that). Comment on attachment 129612 [details] [review] cmake: Add find package config support Review of attachment 129612 [details] [review]: ----------------------------------------------------------------- I think this makes sense now, but I'd like to give it a bit of testing myself before applying. ::: cmake/DBus1Config.cmake.in @@ +8,5 @@ > +# [1] This variable is not required if DBus1_LIBRARIES is added > +# to a target with target_link_libraries > + > +# Compute paths > +if(@DBUS_RELOCATABLE_LIBDIR@) Just to check, will if(1)/else()/endif() and if(0)/else()/endif() do the obvious thing? Comment on attachment 129613 [details] [review] cmake: Add find_package support for windows. Review of attachment 129613 [details] [review]: ----------------------------------------------------------------- This is quite hard to review (and probably quite hard for you to rebase, sorry) because it re-indents most of DBus1Config.cmake.in, so perhaps it would make sense to squash it into the commit that adds that file? Then we'd be going from one consistent situation to another, without passing through an intermediate state where DBus1Config.cmake.in is installed, but doesn't actually work in one supported configuration (CMake+Windows builds with no pkg-config). ::: cmake/DBus1Config.cmake.in @@ +25,5 @@ > + if(NOT TARGET dbus-1) > + include("${CMAKE_CURRENT_LIST_DIR}/DBus1Targets.cmake") > + endif() > + > + set(DBus1_INCLUDE_DIRS "${DBus1_INSTALL_DIR}/include/dbus-1.0" "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_LIBDIR@/dbus-1.0/include") Do we want to use @CMAKE_INSTALL_INCLUDEDIR@ here, similar to the libdir? Comment on attachment 129614 [details] [review] Add cmake find_package support to autotools build system. Review of attachment 129614 [details] [review]: ----------------------------------------------------------------- Looks fine. It might make sense to squash this into the commit that adds DBus1ConfigVersion.cmake.in - then we're adding this functionality to Autotools and CMake simultaneously, and they always have feature parity. I need to get some work stuff dealt with first, but I'll try to do a round of testing on this later today. (In reply to Simon McVittie from comment #56) > ::: cmake/DBus1Config.cmake.in > @@ +8,5 @@ > > +# [1] This variable is not required if DBus1_LIBRARIES is added > > +# to a target with target_link_libraries > > + > > +# Compute paths > > +if(@DBUS_RELOCATABLE_LIBDIR@) > > Just to check, will if(1)/else()/endif() and if(0)/else()/endif() do the > obvious thing? yes (In reply to Simon McVittie from comment #57) > > + if(NOT TARGET dbus-1) > > + include("${CMAKE_CURRENT_LIST_DIR}/DBus1Targets.cmake") > > + endif() > > + > > + set(DBus1_INCLUDE_DIRS "${DBus1_INSTALL_DIR}/include/dbus-1.0" "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_LIBDIR@/dbus-1.0/include") > > Do we want to use @CMAKE_INSTALL_INCLUDEDIR@ here, similar to the libdir? okay (In reply to Simon McVittie from comment #58) > Comment on attachment 129614 [details] [review] [review] > Add cmake find_package support to autotools build system. > > Review of attachment 129614 [details] [review] [review]: > ----------------------------------------------------------------- > > Looks fine. It might make sense to squash this into the commit that adds > DBus1ConfigVersion.cmake.in - then we're adding this functionality to > Autotools and CMake simultaneously, and they always have feature parity. so we have two patches: 1. relocatable pc file for cmake and autotools 2. find_package config support for cmake and autotools. Created attachment 129638 [details] [review] cmake, autotools: create relocatable dbus-1.pc file Relocatable pkgconfig files are required to use installed packages from a non standard location. For simplicity, we're not relocatable if libdir (autotools) or CMAKE_INSTALL_LIBDIR (cmake) is something more complicated (e.g. Debian multiarch); we don't want to have to compute how many ../ to add. Created attachment 129639 [details] [review] cmake, autotools: Add find package config support for cmake clients With this support cmake and autotools generates cmake equivalent of pkgconfig files on configure time named DBus1Config*.cmake. These files are installed into the related directory where cmake expects find_package related config files. For instructions how to use this feature with clients see readme.cmake. With previous DBus versions each cmake client using DBus as dependency needed a related FindDBus*.cmake in its source distribution or in the cmake binary packages. With the 'config' find package style support provided by this patch this requirement has been removed. The generated config file uses pkgconfig on unix or autotools to fetch package build flags, which is the prefered way. On Windows we do not want to require CMake users to have pkg-config installed so it uses cmake buildin target export support for exporting all targets into DBus1ConfigTargets*.cmake. Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99721 Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de> [made with autotools and unix distro related help from smcv] Yes you've combined the patches how I was hoping. Sorry, I need to review and test these, but ran out of time today. Please don't merge right now. (In reply to Simon McVittie from comment #65) > Yes you've combined the patches how I was hoping. > BTW: Because with these patches we are now controlling package finding on all related platforms and major build systems we can get rid of the legacy include/dbus-1.0 include dir location. (In reply to Simon McVittie from comment #65) > Yes you've combined the patches how I was hoping. > > Sorry, I need to review and test these, but ran out of time today. Please > don't merge right now. Any idea how long it will take ? If it helps I can split out the cmake find_package part for windows as base patch and rebase the unix related part on top of this, with which you may have more time to review. (In reply to Ralf Habacker from comment #67) > Any idea how long it will take ? Hopefully today. I like that the version you have now works the same for Autotools and for CMake - I'd prefer to land it all at once rather than splitting it. (In reply to Simon McVittie from comment #4) > AS_IF([test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib"], > [pkgconfig_prefix='${pcfiledir}/../..'], > [pkgconfig_prefix="$prefix"]) > AC_SUBST([pkgconfig_prefix]) Unfortunately we might have to revert this part, or make it conditional on --enable-relocatable or something, because pkg-config doesn't remove redundant components foo/bar/../.. from paths before it filters out standard -I, -L paths; so using prefix=${pcfiledir}/../.. in a file in /usr/lib/pkgconfig would put -L/usr/lib/pkgconfig/../../lib (equivalent to -L/usr/lib) on the compiler command line even though pkg-config normally filters out -L/usr/lib. The reason for needing to filter out standard -I and (particularly) -L flags is not very well documented, but I believe the idea is that if libfoo depends on libbar, libfoo is in /opt/lib, libbar is in /usr/lib, and there is an older libfoo also in /usr/lib, then we do not want `pkg-config --libs foo` to put -L/usr/lib on the compiler command-line explicitly, because that could easily result in the compiler finding the older /usr/lib/libfoo.so instead of the desired /opt/lib/libfoo.so. This happens a lot on OS X, where /usr/lib contains a version of OpenSSL from the stone age, but the OpenSSL that people actually want to link against is a version from Fink or Homebrew in /sw/lib or something. Aha! If you use
pkg-config --define-prefix [...]
instead of plain pkg-config, then it automatically rewrites ${prefix}, based on the assumption that the pkg-config files are in ${prefix}/lib/pkgconfig.
So one possible solution would be to revert the use of @pkgconfig_prefix@, and instead tell people who are compiling a relocatable library stack to use PKG_CONFIG="pkg-config --define-prefix".
In a pkg-config.exe compiled for Windows, --define-prefix is the default anyway.
Comment on attachment 129639 [details] [review] cmake, autotools: Add find package config support for cmake clients Review of attachment 129639 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.cmake.in @@ +8,5 @@ > +# [1] This variable is not required if DBus1_LIBRARIES is added > +# to a target with target_link_libraries > + > +# Compute paths > +if(@DBUS_RELOCATABLE_LIBDIR@) This variable isn't substituted when compiling with Autotools, so it stays set to the literal value @DBUS_RELOCATABLE_LIBDIR@. @@ +11,5 @@ > +# Compute paths > +if(@DBUS_RELOCATABLE_LIBDIR@) > + get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}../../.." ABSOLUTE) > +else() > + set(DBus1_INSTALL_DIR "@DBUS_INSTALL_DIR@") This variable isn't substituted by Autotools. We should use @EXPANDED_PREFIX@ since that already exists. @@ +25,5 @@ > + if(NOT TARGET dbus-1) > + include("${CMAKE_CURRENT_LIST_DIR}/DBus1Targets.cmake") > + endif() > + > + set(DBus1_INCLUDE_DIRS "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_INCLUDEDIR@/dbus-1.0" "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_LIBDIR@/dbus-1.0/include") These CMake variables aren't substituted when compiling with Autotools, although that's probably harmless because we shouldn't be in this code path in that case anyway. (In reply to Simon McVittie from comment #71) > This variable isn't substituted by Autotools. We should use > @EXPANDED_PREFIX@ since that already exists. Or, better, @DBUS_PREFIX@, since both Autotools and CMake set that already. Created attachment 129806 [details] [review] build: Optionally create relocatable dbus-1.pc file Relocatable pkgconfig files are necessary when using packages installed to a location that does not match the location for which they were compiled. However, using ${pcfiledir} is problematic for system installations in standard locations, because it interferes with pkg-config's ability to filter out -I, -L options that are redundant with compiler defaults (which is important if you are trying to use a newer version of a library than the system copy). So make relocation optional, and switch it off by default when using Autotools (as distribution vendors do in practice) and compiling for a Unix platform. For simplicity, we're also not relocatable if libdir (autotools) or CMAKE_INSTALL_LIBDIR (cmake) is something more complicated (e.g. Debian multiarch); we don't want to have to compute how many ../ to add. [smcv: don't be relocatable by default on Unix + Autotools] --- I think I'm happy with this if you are. Created attachment 129807 [details] [review] cmake, autotools: Add find package config support for cmake clients From: Ralf Habacker <ralf.habacker@freenet.de> With this support cmake and autotools generates cmake equivalent of pkgconfig files on configure time named DBus1Config*.cmake. These files are installed into the related directory where cmake expects find_package related config files. For instructions how to use this feature with clients see readme.cmake. With previous DBus versions each cmake client using DBus as dependency needed a related FindDBus*.cmake in its source distribution or in the cmake binary packages. With the 'config' find package style support provided by this patch this requirement has been removed. The generated config file uses pkgconfig on unix or autotools to fetch package build flags, which is the prefered way. On Windows we do not want to require CMake users to have pkg-config installed so it uses cmake buildin target export support for exporting all targets into DBus1ConfigTargets*.cmake. [smcv: make sure variable substitution works in Autotools too] --- Again, I'm happy with this if you are. I renamed DBUS_RELOCATABLE_LIBDIR to just DBUS_RELOCATABLE, AC_SUBST'd it from configure.ac so the Autotools code path works, and used @DBUS_PREFIX@ for the other substitution. This line is still not substituted when using Autotools: set(DBus1_INCLUDE_DIRS "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_INCLUDEDIR@/dbus-1.0" "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_LIBDIR@/dbus-1.0/include") but that's in a code path that won't be activated in this case, so if it doesn't cause any problems I don't mind leaving it as-is. I forgot to mention that Attachment #129806 [details] is also credited as your commit. If you want to make DBUS_RELOCATABLE default to 0 on CMake+Unix too, that would be fine - I don't really mind either way. (In reply to Simon McVittie from comment #71) > This variable isn't substituted by Autotools. We should use > @EXPANDED_PREFIX@ since that already exists. > Thanks for updating the patch > @@ +25,5 @@ > > + if(NOT TARGET dbus-1) > > + include("${CMAKE_CURRENT_LIST_DIR}/DBus1Targets.cmake") > > + endif() > > + > > + set(DBus1_INCLUDE_DIRS "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_INCLUDEDIR@/dbus-1.0" "${DBus1_INSTALL_DIR}/@CMAKE_INSTALL_LIBDIR@/dbus-1.0/include") > > These CMake variables aren't substituted when compiling with Autotools, > although that's probably harmless because we shouldn't be in this code path > in that case anyway. We may split the DBus1Config.cmake.in into two separate pieces. on for windows only and one for unix/autotools e.g. DBus1Config.cmake.in and DBus1Config.pkgconfig.in. The latter should be used on linux and autotools. Unfortunally for autotools I do not know how to generate a config file from an xxx.in file' using a different name. DBus1Config.pkgconfig.in need to be configured into DBus1Config.cmake in build dir. If you have a pointer I can update the patch. With cmake I would use if(WIN32) configure_file(DBus1Config.cmake.in "${CMAKE_BINARY_DIR}/DBus1Config.cmake" @ONLY) else() configure_file(DBus1Config.pkgconfig.in "${CMAKE_BINARY_DIR}/DBus1Config.cmake" @ONLY) endif() Comment on attachment 129807 [details] [review] cmake, autotools: Add find package config support for cmake clients Review of attachment 129807 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.cmake.in @@ +9,5 @@ > +# to a target with target_link_libraries > + > +# Compute paths > +if(@DBUS_RELOCATABLE@) > + get_filename_component(DBus1_INSTALL_DIR "${CMAKE_CURRENT_LIST_DIR}../../.." ABSOLUTE) A final test on windows shows that this cmake command needs to use "${CMAKE_CURRENT_LIST_DIR}../../../.." instead because we are in <root>/lib/cmake/DBus1 and needs to go to <root>/. I will update the patch (In reply to Ralf Habacker from comment #77) > A final test on windows shows that this cmake command needs to use > "${CMAKE_CURRENT_LIST_DIR}../../../.." instead because we are in > <root>/lib/cmake/DBus1 and needs to go to <root>/. I will update the patch Hmm, that doesn't look right. Is this because you missed out a / after the }? Should it be ${CMAKE_CURRENT_LIST_DIR}/../../.. instead? (Start in DBus1, one .. takes you to cmake, a second takes you to lib and a third takes you to the root.) (In reply to Simon McVittie from comment #78) > Hmm, that doesn't look right. Is this because you missed out a / after the > }? Should it be ${CMAKE_CURRENT_LIST_DIR}/../../.. instead? exactly, thanks for this pointer. Created attachment 129837 [details] [review] cmake, autotools: Add find package config support for cmake clients - fix find install root on windows - split out pkgconfig based find_package config file into DBus1Config.pkgconfig.in; tested with autotools and cmake on linux Comment on attachment 129837 [details] [review] cmake, autotools: Add find package config support for cmake clients Review of attachment 129837 [details] [review]: ----------------------------------------------------------------- ::: cmake/DBus1Config.pkgconfig.in @@ +7,5 @@ > + > +# [1] This variable is not required if DBus1_LIBRARIES is added > +# to a target with target_link_libraries > + > +get_filename_component(DBus1_PKGCONFIG_DIR "${CMAKE_CURRENT_LIST_DIR}/../../pkgconfig" ABSOLUTE) I was about to say "doesn't this need to check DBUS_RELOCATABLE?" - but actually it doesn't, because the pkg-config filename and the cmake filename are both in ${libdir}, so it's reasonable to say we don't support changing their relationship to each other. I think this is good now, but I'd like to re-test it before landing it. (In reply to Simon McVittie from comment #81) > I think this is good now, but I'd like to re-test it before landing it. It looks fine locally, including with mingw targeting Windows... but on Travis-CI I get configure: error: Your libdir is too complicated to be relocatable, use ${prefix}/lib instead and I'm not quite sure why. My only vague speculation so far is that we might need to also allow ${prefix}\\lib or something. Trying to debug that now (see the try-master branch in https://github.com/smcv/dbus for work in progress). configure: error: Your libdir is too complicated to be relocatable (prefix=NONE, libdir=${exec_prefix}/lib. Please Use --libdir=${prefix}/lib or --disable-relocation.
Looks like I'll need to put together some workaround for that.
(In reply to Simon McVittie from comment #83) > Looks like I'll need to put together some workaround for that. Anything I can assist to get this solved ? (In reply to Simon McVittie from comment #82) > (In reply to Simon McVittie from comment #81) > > I think this is good now, but I'd like to re-test it before landing it. > > It looks fine locally, including with mingw targeting Windows... but on > Travis-CI I get > > configure: error: Your libdir is too complicated to be relocatable, use > ${prefix}/lib instead > and I'm not quite sure why. My only vague speculation so far is that we > might need to also allow ${prefix}\\lib or something. Trying to debug that > now (see the try-master branch in https://github.com/smcv/dbus for work in > progress). The exact message at https://travis-ci.org/smcv/dbus/jobs/205099926 is configure: error: Your libdir is too complicated to be relocatable (prefix=NONE, libdir=${exec_prefix}/lib. Please Use --libdir=${prefix}/lib or --disable-relocation. the related test is + AS_IF([test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib" || + test "x$EXPANDED_LIBDIR" = "x$EXPANDED_PREFIX/lib64"], It looks that libdir seems not be expanded but set to "${exec_prefix}/lib", soe the test fails. cmake does it the other way around. It tries to detect the multi architecture mode by a regex see https://github.com/Kitware/CMake/blob/master/Modules/CMakeFindPackageMode.cmake#L94 for debian # guess Debian multiarch if it has not been set: if(EXISTS /etc/debian_version) if(NOT CMAKE_${LANGUAGE}_LIBRARY_ARCHITECTURE ) file(GLOB filesInLib RELATIVE /lib /lib/*-linux-gnu* ) foreach(file ${filesInLib}) if("${file}" MATCHES "${CMAKE_LIBRARY_ARCHITECTURE_REGEX}") set(CMAKE_${LANGUAGE}_LIBRARY_ARCHITECTURE ${file}) break() endif() endforeach() endif() if(NOT CMAKE_LIBRARY_ARCHITECTURE) set(CMAKE_LIBRARY_ARCHITECTURE ${CMAKE_${LANGUAGE}_LIBRARY_ARCHITECTURE}) endif() endif() and https://github.com/Kitware/CMake/blob/master/Modules/CMakeDetermineCompilerABI.cmake#L111 The regex is platform specific for example: /usr/share/cmake/Modules/Platform/kFreeBSD.cmake:4: set(CMAKE_LIBRARY_ARCHITECTURE_REGEX "[a-z0-9_]+(-[a-z0-9_]+)?-kfreebsd-gnu[a-z0-9_]*") /usr/share/cmake/Modules/Platform/Linux.cmake:49: set(CMAKE_LIBRARY_ARCHITECTURE_REGEX "[a-z0-9_]+(-[a-z0-9_]+)?-linux-gnu[a-z0-9_]*") /usr/share/cmake/Modules/Platform/QNX.cmake:4: unset(CMAKE_LIBRARY_ARCHITECTURE_REGEX) /usr/share/cmake/Modules/Platform/GNU.cmake:11: set(CMAKE_LIBRARY_ARCHITECTURE_REGEX "[a-z0-9_]+(-[a-z0-9_]+)?-gnu[a-z0-9_]*") so I suggest to use a regex too. (In reply to Ralf Habacker from comment #84) > (In reply to Simon McVittie from comment #83) > > Looks like I'll need to put together some workaround for that. > Anything I can assist to get this solved ? dbus library linking has been non-relocatable for over 10 years; please be patient :-) What I'm going to do for now is to test a version where relocation is optional (default on) with CMake, but always disabled with Autotools. (Thinking about the situation some more, I'm not comfortable with relocation being always-on in CMake, because it does break some setups.) Then we can switch it to "possible to enable" with Autotools, or even have a cleverer default based on ${prefix}, later. If I write "if(ON)" or "if(OFF)" into a generated CMake file, will it act like if(1) or if(0) in C, or do I need to do something more complicated? Test procedure: autoreconf cd ~/tmp mkdir cmake-nonreloc cmake-reloc cmake-odd autotools ( cd cmake-nonreloc && cmake ~/src/dbus/cmake -DDBUS_RELOCATABLE=OFF ) ( cd cmake-reloc && cmake ~/src/dbus/cmake ) ( cd cmake-odd && cmake ~/src/dbus/cmake -DCMAKE_INSTALL_LIBDIR=Libraries ) ( cd autotools && ~/src/dbus/cmake/configure ) for d in cmake-nonreloc cmake-reloc cmake-odd autotools; do make -C "$d" || break; done for d in cmake-nonreloc cmake-reloc cmake-odd autotools; do make -C "$d" install DESTDIR="$(pwd)/$d/DESTDIR" || break; done and then look in */DESTDIR/ at the generated files. Expected results: * cmake-nonreloc/DESTDIR/usr/local/lib/pkgconfig/dbus-1.pc hard-codes /usr/local * cmake-reloc/DESTDIR/usr/local/lib/pkgconfig/dbus-1.pc uses ${pcfiledir}/../../ * cmake-odd/DESTDIR/usr/local/Libraries/pkgconfig/dbus-1.pc hard-codes /usr/local because the CMAKE_INSTALL_LIBDIR is something strange * autotools/ is the same as cmake-nonreloc/ for now (In reply to Ralf Habacker from comment #85) > cmake does it the other way around. It tries to detect the multi > architecture > mode by a regex I don't think that helps here. What we need to know is: is the user's chosen ${libdir} precisely one level of hierarchy below the ${prefix}? If yes, we can use ../ to get to the ${prefix}. If no, we cannot. The thing about Debian-style multiarch that breaks this relocation is not that it isn't lib or lib64, but rather that it contains a number of levels of directory hierarchy that is not 1. Autotools has the additional complications that ${prefix} and ${exec_prefix} are allowed to be different, and that ${prefix} and ${exec_prefix} technically default to "NONE" (and then the real values are substituted later). For the latter, I don't know why but it's quite annoying. Using the pkg-config file in a relocatable mode does not allow for ${prefix} and ${exec_prefix} differing, so I'll probably force it to be non-relocatable if ${exec_prefix} is not '${prefix}', or if ${libdir} is neither '${prefix}/lib' nor '${exec_prefix}/lib' (or maybe lib64, but to be honest I don't think allowing that is actually very important). The EXPANDED_WHATEVER variables are really a bit of a hack, and one day I should remove them (the Correct™ way to do them is to do most of the substitutions in the Makefile.am instead of as AC_CONFIG_FILES)... but not right now. Created attachment 130044 [details] [review] cmake: Optionally create relocatable dbus-1.pc file From: Ralf Habacker <ralf.habacker@freenet.de> Relocatable pkgconfig files are necessary when using packages installed to a location that does not match the location for which they were compiled. However, using ${pcfiledir} is problematic for system installations in standard locations, because it interferes with pkg-config's ability to filter out -I, -L options that are redundant with compiler defaults (which is important if you are trying to use a newer version of a library than the system copy). In practice operating system vendors installing dbus to standard locations use Autotools, so we enable relocatable builds by default when building with CMake. For simplicity, we're also not relocatable if the library directory is something more complicated than lib or lib64 (e.g. under Debian multiarch); we don't want to have to compute how many ../ to add. This is non-trivial to determine in an Autotools build, so for now there is no support for relocation when built with Autotools, even as an opt-in feature. Going via the ${original_prefix} variable is because under Autotools, both ${prefix} and ${exec_prefix} technically default to NONE, with NONE replaced with their real defaults of /usr/local and '${prefix}' (respectively) later on. If we tried to expand ${prefix} at the time that we choose the value of ${pkgconfig_prefix}, that would cause a broken value "prefix=NONE" to be hard-coded. [smcv: no relocation on Autotools, make it optional in CMake, expand commit message] Bug: https://bugs.freedesktop.org/show_bug.cgi?id=99721 Created attachment 130045 [details] [review] cmake, autotools: Add find package config support for cmake clients From: Ralf Habacker <ralf.habacker@freenet.de> With this support cmake and autotools generates cmake equivalent of pkgconfig files on configure time named DBus1Config*.cmake. These files are installed into the related directory where cmake expects find_package related config files. For instructions how to use this feature with clients see readme.cmake. With previous DBus versions each cmake client using DBus as dependency needed a related FindDBus*.cmake in its source distribution or in the cmake binary packages. With the 'config' find package style support provided by this patch this requirement has been removed. The generated config file uses pkgconfig on unix or autotools to fetch package build flags, which is the prefered way. On Windows we do not want to require CMake users to have pkg-config installed so it uses cmake buildin target export support for exporting all targets into DBus1ConfigTargets*.cmake. [smcv: make sure variable substitution works in Autotools too] --- In Autotools-land, we no longer need to substitute or define DBUS_RELOCATABLE, because that's only used in cmake/DBus1Config.cmake in your latest version of this patch. Created attachment 130053 [details] [review] autotools: Allow relocatable pkg-config metadata on an opt-in basis --- It was a bit more complicated than I'd anticipated, so I'm glad I split this out into its own commit, but it seems to work now. Comment on attachment 130044 [details] [review] cmake: Optionally create relocatable dbus-1.pc file Review of attachment 130044 [details] [review]: ----------------------------------------------------------------- Looks good from my side. But because I'm the author and I need a reviewer for this: you are also happy with this patch ? Comment on attachment 130045 [details] [review] cmake, autotools: Add find package config support for cmake clients Review of attachment 130045 [details] [review]: ----------------------------------------------------------------- I'm fine with this patch, you too ? Comment on attachment 130053 [details] [review] autotools: Allow relocatable pkg-config metadata on an opt-in basis Review of attachment 130053 [details] [review]: ----------------------------------------------------------------- Looks good (In reply to Ralf Habacker from comment #90) > Looks good from my side. But because I'm the author and I need a reviewer > for this: you are also happy with this patch ? Sorry for the delay in landing this, I've been busy with non-D-Bus at work. I've put both the patches that were originally yours down as having been reviewed by both of us, since by this point they're clearly a team effort :-) Fixed in git for 1.11.12. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.