|
Description
Ralf Habacker
2017-02-10 08:33:54 UTC
Created attachment 129467 [details] [review] Add pkgconfig file generating support on unix os to cmake build system. 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? (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@ (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 Created attachment 129477 [details] [review] Add pkgconfig file generating support on unix os to cmake build system - excluded install header location Created attachment 129478 [details] [review] Let cmake install arch depending headers on linux in the same location as autotools. 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 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 Created attachment 129482 [details] [review] Add pkgconfig file generating support on unix os to cmake build system. - do not use expanded install locationh 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 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. Created attachment 129485 [details] [review] Let cmake install arch depending headers on Unix in the same location as autotools. - commit message wording fix Created attachment 129486 [details] [review] Let cmake install executables on Unix in bin subdirectory not in lib. Created attachment 129487 [details] [review] Use relative install locations on installing targets with cmake on Unix too. - rebased Created attachment 129488 [details] [review] Use relative install locations on installing targets with cmake on Unix too. - rebased 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 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 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). 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. (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.