Bug 99721 - Add and install find package config support to build system.
Summary: Add and install find package config support to build system.
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: All All
: medium normal
Assignee: D-Bus Maintainers
QA Contact: D-Bus Maintainers
URL:
Whiteboard: review?
Keywords: patch
Depends on: 99752
Blocks: 99834
  Show dependency treegraph
 
Reported: 2017-02-09 08:58 UTC by Ralf Habacker
Modified: 2017-03-20 14:51 UTC (History)
1 user (show)

See Also:
i915 platform:
i915 features:


Attachments
Add and install find package config support to cmake build system. (5.78 KB, patch)
2017-02-09 08:58 UTC, Ralf Habacker
Details | Splinter Review
Keep cmake include file installation dir on Windows in sync with Unix. (1.24 KB, patch)
2017-02-10 22:09 UTC, Ralf Habacker
Details | Splinter Review
Add find package config support to cmake build system. (5.06 KB, patch)
2017-02-10 22:10 UTC, Ralf Habacker
Details | Splinter Review
cmake: Use relative install locations on Unix too (1.32 KB, patch)
2017-02-12 14:08 UTC, Ralf Habacker
Details | Splinter Review
cmake: use default GNU installation layout. (6.00 KB, patch)
2017-02-12 14:09 UTC, Ralf Habacker
Details | Splinter Review
cmake: create relocatable dbus-1.pc file (1.95 KB, patch)
2017-02-12 14:09 UTC, Ralf Habacker
Details | Splinter Review
cmake: Keep include file install locations on Windows in sync with Unix. (1.33 KB, patch)
2017-02-12 14:09 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find package config support (6.00 KB, patch)
2017-02-12 14:10 UTC, Ralf Habacker
Details | Splinter Review
Add cmake find_package support to autotools build system. (2.57 KB, patch)
2017-02-12 14:10 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find_package support for windows. (6.25 KB, patch)
2017-02-12 19:01 UTC, Ralf Habacker
Details | Splinter Review
cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools. (1.00 KB, patch)
2017-02-13 19:33 UTC, Ralf Habacker
Details | Splinter Review
cmake: create relocatable dbus-1.pc file (2.14 KB, patch)
2017-02-13 20:46 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find package config support (6.14 KB, patch)
2017-02-13 20:46 UTC, Ralf Habacker
Details | Splinter Review
Add cmake find_package support to autotools build system. (1.81 KB, patch)
2017-02-13 20:47 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find_package support for windows. (6.31 KB, patch)
2017-02-13 20:47 UTC, Ralf Habacker
Details | Splinter Review
cmake: create relocatable dbus-1.pc file (2.62 KB, patch)
2017-02-14 13:05 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find package config support (6.29 KB, patch)
2017-02-14 13:06 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find_package support for windows. (6.35 KB, patch)
2017-02-14 13:06 UTC, Ralf Habacker
Details | Splinter Review
cmake: create relocatable dbus-1.pc file (2.78 KB, patch)
2017-02-14 20:04 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find package config support (6.09 KB, patch)
2017-02-14 20:05 UTC, Ralf Habacker
Details | Splinter Review
cmake: Add find_package support for windows. (6.31 KB, patch)
2017-02-14 20:05 UTC, Ralf Habacker
Details | Splinter Review
Add cmake find_package support to autotools build system. (1.81 KB, patch)
2017-02-14 20:05 UTC, Ralf Habacker
Details | Splinter Review
Let autotools generate a relocatable dbus-1.pc in standard cases. (2.21 KB, patch)
2017-02-14 20:06 UTC, Ralf Habacker
Details | Splinter Review
cmake, autotools: create relocatable dbus-1.pc file (3.93 KB, patch)
2017-02-15 15:58 UTC, Ralf Habacker
Details | Splinter Review
cmake, autotools: Add find package config support for cmake clients (9.42 KB, patch)
2017-02-15 15:58 UTC, Ralf Habacker
Details | Splinter Review
build: Optionally create relocatable dbus-1.pc file (4.86 KB, patch)
2017-02-21 20:03 UTC, Simon McVittie
Details | Splinter Review
cmake, autotools: Add find package config support for cmake clients (9.86 KB, patch)
2017-02-21 20:07 UTC, Simon McVittie
Details | Splinter Review
cmake, autotools: Add find package config support for cmake clients (10.64 KB, patch)
2017-02-22 18:48 UTC, Ralf Habacker
Details | Splinter Review
cmake: Optionally create relocatable dbus-1.pc file (5.08 KB, patch)
2017-03-03 12:13 UTC, Simon McVittie
Details | Splinter Review
cmake, autotools: Add find package config support for cmake clients (10.17 KB, patch)
2017-03-03 12:17 UTC, Simon McVittie
Details | Splinter Review
autotools: Allow relocatable pkg-config metadata on an opt-in basis (2.69 KB, patch)
2017-03-03 19:19 UTC, Simon McVittie
Details | Splinter Review

Description Ralf Habacker 2017-02-09 08:58:06 UTC
Signed-off-by: Ralf Habacker <ralf.habacker@freenet.de>
Comment 1 Ralf Habacker 2017-02-09 08:58:08 UTC
Created attachment 129424 [details] [review]
Add and install find package config support to cmake build system.
Comment 2 Simon McVittie 2017-02-09 11:33:28 UTC
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.)
Comment 3 Ralf Habacker 2017-02-09 21:12:07 UTC
(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 )
Comment 4 Simon McVittie 2017-02-10 11:43:03 UTC
(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.)
Comment 5 Ralf Habacker 2017-02-10 22:09:18 UTC
Created attachment 129491 [details] [review]
Keep cmake include file installation dir on Windows in sync with Unix.
Comment 6 Ralf Habacker 2017-02-10 22:10:28 UTC
Created attachment 129492 [details] [review]
Add find package config support to cmake build system.

- rebased
Comment 7 Ralf Habacker 2017-02-10 23:07:42 UTC
(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
Comment 8 Ralf Habacker 2017-02-12 14:08:17 UTC
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:
Comment 9 Ralf Habacker 2017-02-12 14:09:02 UTC
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.
Comment 10 Ralf Habacker 2017-02-12 14:09:18 UTC
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.
Comment 11 Ralf Habacker 2017-02-12 14:09:34 UTC
Created attachment 129524 [details] [review]
cmake: Keep include file install locations on Windows in sync with Unix.
Comment 12 Ralf Habacker 2017-02-12 14:10:20 UTC
Created attachment 129525 [details] [review]
cmake: Add find package config support

- use pkgconfig approach on unix
Comment 13 Ralf Habacker 2017-02-12 14:10:34 UTC
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.
Comment 14 Ralf Habacker 2017-02-12 19:01:19 UTC
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 15 Ralf Habacker 2017-02-13 08:16:56 UTC
Comment on attachment 129521 [details] [review]
cmake: Use relative install locations on Unix too

fixed with bug 99752
Comment 16 Ralf Habacker 2017-02-13 09:01:39 UTC
(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 17 Simon McVittie 2017-02-13 12:03:05 UTC
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 18 Simon McVittie 2017-02-13 12:05:41 UTC
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 19 Simon McVittie 2017-02-13 12:06:09 UTC
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 20 Simon McVittie 2017-02-13 12:12:31 UTC
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 21 Simon McVittie 2017-02-13 12:16:02 UTC
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 22 Simon McVittie 2017-02-13 12:20:37 UTC
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 23 Ralf Habacker 2017-02-13 14:23:58 UTC
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.
Comment 24 Ralf Habacker 2017-02-13 19:23:33 UTC
applied fixes to git master
Comment 25 Ralf Habacker 2017-02-13 19:33:50 UTC
Created attachment 129557 [details] [review]
cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools.
Comment 26 Ralf Habacker 2017-02-13 19:59:06 UTC
(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()
Comment 27 Ralf Habacker 2017-02-13 20:13:18 UTC
(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.
Comment 28 Ralf Habacker 2017-02-13 20:46:43 UTC
Created attachment 129560 [details] [review]
cmake: create relocatable dbus-1.pc file

- update
Comment 29 Ralf Habacker 2017-02-13 20:46:58 UTC
Created attachment 129561 [details] [review]
cmake: Add find package config support

- update
Comment 30 Ralf Habacker 2017-02-13 20:47:09 UTC
Created attachment 129562 [details] [review]
Add cmake find_package support to autotools build system.

- update
Comment 31 Ralf Habacker 2017-02-13 20:47:40 UTC
Created attachment 129563 [details] [review]
cmake: Add find_package support for windows.

- update
Comment 32 Simon McVittie 2017-02-14 10:27:43 UTC
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 33 Simon McVittie 2017-02-14 10:34:17 UTC
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 34 Simon McVittie 2017-02-14 10:41:53 UTC
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 35 Simon McVittie 2017-02-14 10:42:47 UTC
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 36 Simon McVittie 2017-02-14 10:46:44 UTC
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
Comment 37 Ralf Habacker 2017-02-14 10:51:16 UTC
(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)
Comment 38 Ralf Habacker 2017-02-14 12:36:05 UTC
(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()
Comment 39 Ralf Habacker 2017-02-14 12:45:25 UTC
(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 ?
Comment 40 Ralf Habacker 2017-02-14 13:05:11 UTC
Created attachment 129592 [details] [review]
cmake: create relocatable dbus-1.pc file

- add multiarch issue hint
- do not make package relocatable on multiarch distributions
Comment 41 Ralf Habacker 2017-02-14 13:06:00 UTC
Created attachment 129593 [details] [review]
cmake: Add find package config support

- fix multi arch issue
Comment 42 Ralf Habacker 2017-02-14 13:06:32 UTC
Created attachment 129594 [details] [review]
cmake: Add find_package support for windows.

- fix comments
Comment 43 Simon McVittie 2017-02-14 15:48:27 UTC
(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 44 Simon McVittie 2017-02-14 15:51:07 UTC
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 45 Simon McVittie 2017-02-14 16:00:04 UTC
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.)
Comment 46 Simon McVittie 2017-02-14 16:06:41 UTC
(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.
Comment 47 Ralf Habacker 2017-02-14 19:53:46 UTC
(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})
Comment 48 Ralf Habacker 2017-02-14 20:04:53 UTC
Created attachment 129611 [details] [review]
cmake: create relocatable dbus-1.pc file

- fixup
Comment 49 Ralf Habacker 2017-02-14 20:05:11 UTC
Created attachment 129612 [details] [review]
cmake: Add find package config support

- fixup
Comment 50 Ralf Habacker 2017-02-14 20:05:27 UTC
Created attachment 129613 [details] [review]
cmake: Add find_package support for windows.

- fixup
Comment 51 Ralf Habacker 2017-02-14 20:05:42 UTC
Created attachment 129614 [details] [review]
Add cmake find_package support to autotools build system.

- fixup
Comment 52 Ralf Habacker 2017-02-14 20:06:07 UTC
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 53 Ralf Habacker 2017-02-14 20:07:58 UTC
Comment on attachment 129557 [details] [review]
cmake: keep variable DBUS_MACHINE_UUID_FILE in sync with autotools.

committed to git master
Comment 54 Simon McVittie 2017-02-15 11:54:07 UTC
(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 55 Simon McVittie 2017-02-15 11:58:46 UTC
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 56 Simon McVittie 2017-02-15 12:01:32 UTC
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 57 Simon McVittie 2017-02-15 12:04:34 UTC
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 58 Simon McVittie 2017-02-15 12:05:30 UTC
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.
Comment 59 Simon McVittie 2017-02-15 12:17:50 UTC
I need to get some work stuff dealt with first, but I'll try to do a round of testing on this later today.
Comment 60 Ralf Habacker 2017-02-15 15:27:09 UTC
(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
Comment 61 Ralf Habacker 2017-02-15 15:29:30 UTC
(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
Comment 62 Ralf Habacker 2017-02-15 15:31:27 UTC
(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.
Comment 63 Ralf Habacker 2017-02-15 15:58:21 UTC
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.
Comment 64 Ralf Habacker 2017-02-15 15:58:46 UTC
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]
Comment 65 Simon McVittie 2017-02-15 20:03:33 UTC
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.
Comment 66 Ralf Habacker 2017-02-16 08:43:01 UTC
(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.
Comment 67 Ralf Habacker 2017-02-20 18:09:47 UTC
(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.
Comment 68 Simon McVittie 2017-02-21 14:34:28 UTC
(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.
Comment 69 Simon McVittie 2017-02-21 19:12:05 UTC
(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.
Comment 70 Simon McVittie 2017-02-21 19:26:56 UTC
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 71 Simon McVittie 2017-02-21 19:31:09 UTC
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.
Comment 72 Simon McVittie 2017-02-21 19:33:33 UTC
(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.
Comment 73 Simon McVittie 2017-02-21 20:03:34 UTC
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.
Comment 74 Simon McVittie 2017-02-21 20:07:38 UTC
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.
Comment 75 Simon McVittie 2017-02-21 20:09:14 UTC
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.
Comment 76 Ralf Habacker 2017-02-22 12:39:41 UTC
(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 77 Ralf Habacker 2017-02-22 12:42:21 UTC
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
Comment 78 Simon McVittie 2017-02-22 15:10:49 UTC
(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.)
Comment 79 Ralf Habacker 2017-02-22 17:04:52 UTC
(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.
Comment 80 Ralf Habacker 2017-02-22 18:48:05 UTC
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 81 Simon McVittie 2017-02-23 13:11:12 UTC
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.
Comment 82 Simon McVittie 2017-02-24 20:26:28 UTC
(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).
Comment 83 Simon McVittie 2017-02-24 20:30:46 UTC
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.
Comment 84 Ralf Habacker 2017-03-01 22:41:26 UTC
(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 ?
Comment 85 Ralf Habacker 2017-03-02 22:07:30 UTC
(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.
Comment 86 Simon McVittie 2017-03-03 12:05:03 UTC
(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.
Comment 87 Simon McVittie 2017-03-03 12:13:49 UTC
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
Comment 88 Simon McVittie 2017-03-03 12:17:41 UTC
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.
Comment 89 Simon McVittie 2017-03-03 19:19:14 UTC
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 90 Ralf Habacker 2017-03-06 18:50:22 UTC
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 91 Ralf Habacker 2017-03-06 18:52:19 UTC
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 92 Ralf Habacker 2017-03-06 20:03:00 UTC
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
Comment 93 Simon McVittie 2017-03-20 14:51:52 UTC
(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.