Bug 99752

Summary: Missing pkgconfig support for cmake builds
Product: dbus Reporter: Ralf Habacker <ralf.habacker>
Component: coreAssignee: D-Bus Maintainers <dbus>
Status: RESOLVED FIXED QA Contact: D-Bus Maintainers <dbus>
Severity: normal    
Priority: medium CC: ralf.habacker
Version: git masterKeywords: patch
Hardware: Other   
OS: All   
Whiteboard: review+, possibly with minor changes
i915 platform: i915 features:
Bug Depends on:    
Bug Blocks: 99721    
Attachments: Add pkgconfig file generating support on unix os to cmake build system.
Add pkgconfig file generating support on unix os to cmake build system
Let cmake install arch depending headers on linux in the same location as autotools.
Add pkgconfig file generating support on unix os to cmake build system.
Let cmake install arch depending headers on linux in the same location as autotools.
Use relative install locations on installing targets with cmake on unix too.
Let cmake install arch depending headers on Unix in the same location as autotools.
Let cmake install executables on Unix in bin subdirectory not in lib.
Use relative install locations on installing targets with cmake on Unix too.
Use relative install locations on installing targets with cmake on Unix too.

Description Ralf Habacker 2017-02-10 08:33:54 UTC
cmake builds on unix os currently do not install a pkgconfig package file.
Comment 1 Ralf Habacker 2017-02-10 08:35:34 UTC
Created attachment 129467 [details] [review]
Add pkgconfig file generating support on unix os to cmake build system.
Comment 2 Philip Withnall 2017-02-10 10:18:55 UTC
Comment on attachment 129467 [details] [review]
Add pkgconfig file generating support on unix os to cmake build system.

Review of attachment 129467 [details] [review]:
-----------------------------------------------------------------

::: cmake/dbus/CMakeLists.txt
@@ +310,5 @@
> +    install(FILES ${dbusinclude_HEADERS} DESTINATION include/dbus-1.0/dbus)
> +    install(FILES ${dbusinclude_ARCH_HEADERS} DESTINATION ${EXPANDED_LIBDIR}/dbus-1.0/include/dbus)
> +else()
> +    install(FILES ${dbusinclude_HEADERS} ${dbusinclude_ARCH_HEADERS} DESTINATION include/dbus)
> +endif()

These changes look unrelated to pkg-config files?
Comment 3 Ralf Habacker 2017-02-10 11:28:06 UTC
(In reply to Philip Withnall from comment #2)

> ::: cmake/dbus/CMakeLists.txt
> @@ +310,5 @@
> > +    install(FILES ${dbusinclude_HEADERS} DESTINATION include/dbus-1.0/dbus)
> > +    install(FILES ${dbusinclude_ARCH_HEADERS} DESTINATION ${EXPANDED_LIBDIR}/dbus-1.0/include/dbus)
> > +else()
> > +    install(FILES ${dbusinclude_HEADERS} ${dbusinclude_ARCH_HEADERS} DESTINATION include/dbus)
> > +endif()
> 
> These changes look unrelated to pkg-config files?

This change is required to install files into the location dbus-1.pc.in requests.

grep include dbus-1.pc.in

Cflags: -I${includedir}/dbus-1.0 -I${libdir}/dbus-1.0/include @DBUS_STATIC_BUILD_CPPFLAGS@
Comment 4 Simon McVittie 2017-02-10 11:58:01 UTC
(In reply to Ralf Habacker from comment #3)
> (In reply to Philip Withnall from comment #2)
> > These changes look unrelated to pkg-config files?
> 
> This change is required to install files into the location dbus-1.pc.in
> requests.

So do a commit that makes this change, explaining why in the commit message, and then a subsequent commit that does the rest of this change?

> grep include dbus-1.pc.in
> 
> Cflags: -I${includedir}/dbus-1.0 -I${libdir}/dbus-1.0/include
> @DBUS_STATIC_BUILD_CPPFLAGS@

I think it's actually harmless for the .pc file to specify include directories that aren't going to exist in some build configurations - but it's no bad thing to make the installed layout more consistent between Autotools and CMake.

For your information, the reason we separate out the architecture-specific header is that ${includedir} is often shared between multiple ABIs, whereas ${libdir} is specific to a single ABI. For instance on 64-bit Red Hat derivatives with -m32 support, you might have:

     /usr/include/dbus-1.0/dbus/dbus-connection.h (etc.) - shared
     /usr/lib64/dbus-1.0/include/dbus/dbus-arch-deps.h - 64-bit
     /usr/lib/dbus-1.0/include/dbus/dbus-arch-deps.h - 32-bit

or on Debian derivatives, if you are on a 64-bit PC with support for compiling 32-bit binaries (-m32) and also an ARM cross-compiler, you might even have:

     /usr/include/dbus-1.0/dbus/dbus-connection.h (etc.) - shared
     /usr/lib/x86_64-linux-gnu/dbus-1.0/include/dbus/dbus-arch-deps.h - 64-bit
     /usr/lib/i386-linux-gnu/dbus-1.0/include/dbus/dbus-arch-deps.h - 32-bit
     /usr/lib/arm-linux-gnueabihf/dbus-1.0/include/dbus/dbus-arch-deps.h - ARM
Comment 5 Ralf Habacker 2017-02-10 13:18:36 UTC
Created attachment 129477 [details] [review]
Add pkgconfig file generating support on unix os to cmake build system

- excluded install header location
Comment 6 Ralf Habacker 2017-02-10 13:18:50 UTC
Created attachment 129478 [details] [review]
Let cmake install arch depending headers on linux in the same location as autotools.
Comment 7 Simon McVittie 2017-02-10 13:28:51 UTC
Comment on attachment 129478 [details] [review]
Let cmake install arch depending headers on linux in the same location as autotools.

Review of attachment 129478 [details] [review]:
-----------------------------------------------------------------

The commit message should really say "on Unix" - this isn't Linux-specific.

::: cmake/dbus/CMakeLists.txt
@@ +307,5 @@
>  
>  install(TARGETS dbus-1 ${INSTALL_TARGETS_DEFAULT_ARGS})
> +if(UNIX)
> +    install(FILES ${dbusinclude_HEADERS} DESTINATION include/dbus-1.0/dbus)
> +    install(FILES ${dbusinclude_ARCH_HEADERS} DESTINATION ${EXPANDED_LIBDIR}/dbus-1.0/include/dbus)

This is an absolute path starting with the value of ${prefix}, whereas the other installations of headers are to paths relative to the ${prefix}. Is that OK?

If absolute paths are not a problem here, you can apply this as-is.

If it is meant to be a relative path, use "lib" instead of "${EXPANDED_LIBDIR}". The CMake build system doesn't seem to support reconfiguring the library subdirectory to any other value, so there's no need to allow that here either.
Comment 8 Simon McVittie 2017-02-10 13:30:30 UTC
Comment on attachment 129477 [details] [review]
Add pkgconfig file generating support on unix os to cmake build system

Review of attachment 129477 [details] [review]:
-----------------------------------------------------------------

Looks fine
Comment 9 Ralf Habacker 2017-02-10 17:27:55 UTC
Created attachment 129482 [details] [review]
Add pkgconfig file generating support on unix os to cmake build system.

- do not use expanded install locationh
Comment 10 Ralf Habacker 2017-02-10 17:28:26 UTC
Created attachment 129483 [details] [review]
Let cmake install arch depending headers on linux in the same location as autotools.

- do not use expanded install location
Comment 11 Ralf Habacker 2017-02-10 17:28:46 UTC
Created attachment 129484 [details] [review]
Use relative install locations on installing targets with cmake on unix too.

Using expanded pathes make no sense in install commands because they
may be patched by cmake for example by specifing DESTDIR on install.
Using relative install locations also reduces the difference between
supported platforms.
Comment 12 Ralf Habacker 2017-02-10 17:31:09 UTC
Created attachment 129485 [details] [review]
Let cmake install arch depending headers on Unix in the same location as autotools.

- commit message wording fix
Comment 13 Ralf Habacker 2017-02-10 17:44:40 UTC
Created attachment 129486 [details] [review]
Let cmake install executables on Unix in bin subdirectory not in lib.
Comment 14 Ralf Habacker 2017-02-10 17:46:23 UTC
Created attachment 129487 [details] [review]
Use relative install locations on installing targets with cmake on Unix too.

- rebased
Comment 15 Ralf Habacker 2017-02-10 17:48:22 UTC
Created attachment 129488 [details] [review]
Use relative install locations on installing targets with cmake on Unix too.

- rebased
Comment 16 Simon McVittie 2017-02-10 18:54:55 UTC
Comment on attachment 129485 [details] [review]
Let cmake install arch depending headers on Unix in the same location as autotools.

Review of attachment 129485 [details] [review]:
-----------------------------------------------------------------

Great
Comment 17 Simon McVittie 2017-02-10 18:58:00 UTC
Comment on attachment 129486 [details] [review]
Let cmake install executables on Unix in bin subdirectory not in lib.

Review of attachment 129486 [details] [review]:
-----------------------------------------------------------------

The subject line's a bit long. Perhaps something like this

    cmake: Install executables in bin/ on Unix, not lib/

    Previously, executables like dbus-daemon were installed to
    the lib subdirectory, but this was unintended. RUNTIME DESTINATION
    is the equivalent of Autotools ${bindir}.

if that's true.
Comment 18 Simon McVittie 2017-02-10 19:00:29 UTC
Comment on attachment 129488 [details] [review]
Use relative install locations on installing targets with cmake on Unix too.

Review of attachment 129488 [details] [review]:
-----------------------------------------------------------------

If this is the case, then please do it :-)

The subject line is a bit long, perhaps

    cmake: Use relative install locations on Unix too

Good use of an extended commit message to justify why a change is right. Spelling: "pathes" should be "paths", "specifing" should be "specifying".

Fine to apply immediately with those fixes (no need to wait for me to review again).
Comment 19 Ralf Habacker 2017-02-10 22:04:42 UTC
Attachment 129482 [details] pushed as d89d6b4 - Add pkgconfig file generating support on unix os to cmake build system.
Attachment 129485 [details] pushed as 59174d8 - Let cmake install arch depending headers on Unix in the same location as autotools.
Comment 20 Ralf Habacker 2017-02-10 22:05:24 UTC
(In reply to Ralf Habacker from comment #19)
> Attachment 129482 [details] pushed as d89d6b4 - Add pkgconfig file
> generating support on unix os to cmake build system.
> Attachment 129485 [details] pushed as 59174d8 - Let cmake install arch
> depending headers on Unix in the same location as autotools.

committed to git master with mentioned changes applied.

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.