Bug 73636

Created attachment 92099 [details] [review]
Use cmake install command instead of deprecated install_files

Created attachment 92100 [details] [review]
Keep cmake install locations in sync with autotools

Created attachment 92104 [details] [review]
Fix of cmake service install location

install service files into share/dbus-1/services (with notepad service)

(In reply to comment #0)
> While comparing install locations of various distributions where dbus is
> build for I recognized some differences:
> 
> source-build (autotools)
> config:   /etc/dbus-1    
> doc:      /usr/share/doc/dbus               
> include:  /usr/include/dbus-1/dbus
> services: /share/dbus-1/services
>           /share/dbus-1/system-services

This depends on configure command-line arguments. With no arguments, it should actually be /usr/local/etc, /usr/local/share, /usr/local/bin and /usr/local/include (although distributors are expected to override that).

You're either using some very odd command-line arguments or mis-transcribing the directories, because dbus/Makefile.am has:

dbusincludedir=$(includedir)/dbus-1.0/dbus
dbusarchincludedir=$(libdir)/dbus-1.0/include/dbus

(note dbus-1.0, not dbus-1)

Strictly speaking, it doesn't actually matter where the headers go, as long as it matches what we say in the installed dbus-1.pc.

> source-build (cmake)
...
> include:  /usr/include/dbus/dbus
> services: n.a.

That does seem like a bug... although D-Bus doesn't actually install any .service files, so...

> opensuse (autotools):      
> config:   /etc/dbus-1    
> doc:      /usr/share/doc/packages/dbus-1
> include:  /usr/include/dbus-1.0/dbus
>           /usr/lib64/dbus-1.0/include/dbus
> services: /lib/dbus-1/system-services
>           /lib/systemd/system

If distributors like OpenSUSE want to redirect installation directories, that's their decision. They're responsible for their own installation choices, and if they break it as a result (particularly if it's via patches), they get to keep both pieces.

${docdir} is commonly switched to be somewhere else by distro policy, and doesn't matter from a functional point of view.

/lib/dbus-1/system-services is on the search path as requested in Bug #35229. If distributions want to standardize on installing there instead of /usr/share/dbus-1/system-services, again, that's their decision (and they can live with the consequences, if bad).

/lib/systemd/system is correct for systemd on a system with split /usr.

Setting ${libdir} to ${prefix}/lib64 (SuSE, Red Hat) or ${prefix}/lib/${multiarch} (Debian, Ubuntu) is a common distributor thing. Again, if distributors do this, they get to deal with any breakage that it might cause.

> mingw32-dbus (autotools cross build)
> config:   <mingw-root>/etc/dbus-1    
> doc:      <mingw-root>/share/doc/dbus
> include:  <mingw-root>/include/dbus-1/dbus
>           <mingw-root>/lib/dbus-1.0/include/dbus
> services: /share/dbus-1/services

The defaults should be in terms of ${prefix} just like a native build. If distributors want to override them, again, that's the distributor's call.

(In reply to comment #0)
> include:  /usr/include/dbus-1[.0]/dbus

There are basically two policies you can have about header files. (Assume that you are meant to #include <foo/bar.h> - that's <dbus/dbus.h> for us.)

----

Traditional policy:

* Install it in ${includedir}/foo/bar.h and hope that ${includedir} is on the compiler's default search path, so that no special -I options are needed to make #include <foo/bar.h> work.

* Advantage: no extra -I options needed, unless it's installed to a non-standard ${includedir}

* Disadvantage: you can't install Foo 1.0 and Foo 2.0 in parallel

----

Parallel-installable policy <http://ometer.com/parallel.html>:

* Install it in ${includedir}/something-versioned/foo/bar.h

* Provide a pkg-config .pc file (or a script like sdl-config) which adds -I${includedir}/something-versioned to CFLAGS, and expect library users to use pkg-config (or the script, but pkg-config is better)

* Advantage: you can parallel-install things

* Disadvantage: you have to use pkg-config, even if it was installed in /usr or /usr/local

----

D-Bus' recommended Autotools build system has always followed the parallel-installable policy - you can't include the D-Bus headers without adding -I options to the compiler command line. This is in fact the case *anyway*, even if ${includedir} is on the default include path, because we install the one architecture-dependent header elsewhere.

I would suggest doing the same for CMake, but it obviously depends on which you consider to be more important: being compatible with older CMake installations, or being compatible with Autotools.

Comment on attachment 92099 [details] [review]
Use cmake install command instead of deprecated install_files

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

Looks reasonable, although I don't know CMake in much detail.

::: cmake/bus/CMakeLists.txt
@@ +4,5 @@
>  SET(BUS_DIR ${CMAKE_SOURCE_DIR}/../bus)
>  
>  set (config_DATA
> +	${CMAKE_CURRENT_BINARY_DIR}/session.conf
> +	${CMAKE_CURRENT_BINARY_DIR}/system.conf

What does CMAKE_CURRENT_BINARY_DIR typically expand to?

Does install_files automatically take it into account whereas install doesn't, or something?

Comment on attachment 92100 [details] [review]
Keep cmake install locations in sync with autotools

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

::: cmake/dbus/CMakeLists.txt
@@ +272,4 @@
>  endif(WIN32)
>  
>  install(TARGETS dbus-1 ${INSTALL_TARGETS_DEFAULT_ARGS})
> +install(FILES ${dbusinclude_HEADERS} DESTINATION include/dbus-1/dbus)

Shouldn't this be include/dbus-1.0/dbus if you want it to be the same as Autotools?

(I'm assuming that saying include/dbus-1/dbus here is equivalent to what ${DESTDIR}${prefix}/include/dbus-1/dbus would be in Autotools.)

::: cmake/doc/CMakeLists.txt
@@ +81,4 @@
>  	  if (${_format} STREQUAL "man")
>  		  install(FILES ${_outfile} DESTINATION share/man/man1)
>  	  else ()
> +		  install(FILES ${_outfile} DESTINATION share/doc/dbus-1)

I'm pretty sure this actually defaults to ${prefix}/share/doc/dbus in Autotools, because dbus is the package name at the beginning of AC_INIT.

If distributors choose to override the documentation directory via "./configure --docdir=.../dbus-1", that's their choice.

(Same for the other two patch-bands in this file.)

Comment on attachment 92104 [details] [review]
Fix of cmake service install location

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

> Fix of cmake service install location.

That's not what this patch does, is it? Previously, we didn't install any service files at all. With this patch, we install a non-functional service file, to the correct location.

I don't think installing an example service file is something we should be doing, particularly one that uses someone else's namespace and doesn't work.

::: cmake/notepad.service
@@ +1,2 @@
> +[D-BUS Service]
> +Name=com.microsoft.notepad

No, don't do this. You don't own microsoft.com.

@@ +1,3 @@
> +[D-BUS Service]
> +Name=com.microsoft.notepad
> +Exec=notepad.exe

I'm pretty sure notepad.exe doesn't claim the bus name com.microsoft.notepad when it starts :-) so it isn't suitable for launching as a D-Bus service.

(Equally, /usr/bin/xterm is a perfectly good executable, but is not a D-Bus service.)

Created attachment 95262 [details] [review]
Header install changes

This is what i'm using:

Install archdep files into archdep directory
Install headers into /include/dbus-1 instead of /include

Created attachment 95263 [details] [review]
Documentation installation fixes

As for docdir, i assumed that "share/doc/dbus" was some kind of sanctioned change, so i didn't touch that. I've made -DDOCDIR actually work again instead, and used it to override the docdir in my packaging script.

* Install documentation into DBUS_DOCDIR instead of share/doc/dbus

Also, it's safe to install manpages on non-unix systems, they are not going to hurt you.

Comment on attachment 95262 [details] [review]
Header install changes

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

::: cmake/dbus/CMakeLists.txt
@@ +22,5 @@
>  	${DBUS_DIR}/dbus-syntax.h
>  	${DBUS_DIR}/dbus-threads.h
>  	${DBUS_DIR}/dbus-types.h
> +)
> +set (dbuslibinclude_HEADERS

In practice, would you ever mix different architectures' D-Bus installations in the same ${prefix} on Windows?

As far as I'm concerned, the priorities for the CMake build system are:

* be able to build D-Bus for a Windows host-architecture with
  MSVC, which you can't (yet) do with Autotools

* be able to sanity-check the CMake build system on a Linux
  build-architecture so I don't accidentally break it

The Autotools build system is what I would recommend for every other purpose, including cross-compiling and the sort of OS-level integration where you need to care about lib vs. lib64 (Red Hat, SuSE and similar) or lib/i386-linux-gnu vs. lib/x86_64-linux-gnu (Debian, Ubuntu and similar).

Comment on attachment 95263 [details] [review]
Documentation installation fixes

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

::: cmake/CMakeLists.txt
@@ +59,5 @@
>      SET(DOCDIR ${DATAROOTDIR}/doc/dbus)
>  endif()
>  
> +if (NOT DBUS_DOCDIR)
> +    SET(DBUS_DOCDIR ${DBUS_INSTALL_DIR}/${DOCDIR})

I'd rather not have a mixture of "user-settable" variables that do and don't include the CMake equivalent of $DESTDIR.

For people who want to move the documentation around, what's wrong with "cmake -DDOCDIR=help/devel/dbus-1.9.0" as a direct equivalent of "./configure --docdir=/help/devel/dbus-1.9.0"? (Or whatever path you want.)

::: cmake/doc/CMakeLists.txt
@@ +87,4 @@
>  	  if (${_format} STREQUAL "man")
>  		  install(FILES ${_outfile} DESTINATION share/man/man1)
>  	  else ()
> +		  install(FILES ${_outfile} DESTINATION ${DBUS_DOCDIR})

DESTINATION ${DOCDIR} seems more appropriate? ${DOCDIR} indirectly defaults to share/doc/dbus anyway (it directly mirrors Automake's ${docdir}).

@@ +135,4 @@
>  DOCBOOK(${CMAKE_BINARY_DIR}/doc/dbus-monitor.1.xml html-nochunks)
>  DOCBOOK(${CMAKE_BINARY_DIR}/doc/dbus-send.1.xml html-nochunks)
>  DOCBOOK(${CMAKE_BINARY_DIR}/doc/dbus-uuidgen.1.xml html-nochunks)
> +if (UNIX OR MSYS)

MSYS doesn't appear to be defined anywhere. If you're using -DMSYS manually, I don't think that's a good "user interface" - something explicit like -DENABLE_MAN_PAGES=[0|1] would be better

If the DOCBOOK() macro can produce man pages on all platforms, the man pages might as well be unconditional; binary distributions can always do the equivalent of "rm -r share/man" if they don't want to ship them.

@@ +147,4 @@
>  # handle html index file
>  #
>  configure_file(${CMAKE_CURRENT_SOURCE_DIR}/index.html.cmake ${CMAKE_CURRENT_BINARY_DIR}/index.html )
> +install(FILES ${CMAKE_CURRENT_BINARY_DIR}/index.html DESTINATION ${DBUS_DOCDIR})

Same here

@@ +155,4 @@
>  	${CMAKE_SOURCE_DIR}/../doc/introspect.xsl
>  )
>  
> +install(FILES ${EXTRA_DIST} DESTINATION ${DBUS_DOCDIR})

and here

(In reply to comment #12)
> Comment on attachment 95263 [details] [review] [review]
> Documentation installation fixes
> 
> Review of attachment 95263 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cmake/CMakeLists.txt
> @@ +59,5 @@
> >      SET(DOCDIR ${DATAROOTDIR}/doc/dbus)
> >  endif()
> >  
> > +if (NOT DBUS_DOCDIR)
> > +    SET(DBUS_DOCDIR ${DBUS_INSTALL_DIR}/${DOCDIR})
> 
> I'd rather not have a mixture of "user-settable" variables that do and don't
> include the CMake equivalent of $DESTDIR.
> 
> For people who want to move the documentation around, what's wrong with
> "cmake -DDOCDIR=help/devel/dbus-1.9.0" as a direct equivalent of
> "./configure --docdir=/help/devel/dbus-1.9.0"? (Or whatever path you want.)

I build with
-DDOCDIR=share/doc/dbus-1/1.8.0
, but it needs to be installed into
${DBUS_INSTALL_DIR}/${DOCDIR}
, AFAIU. Hence the separate variable.

> 
> @@ +135,4 @@
> >  DOCBOOK(${CMAKE_BINARY_DIR}/doc/dbus-monitor.1.xml html-nochunks)
> >  DOCBOOK(${CMAKE_BINARY_DIR}/doc/dbus-send.1.xml html-nochunks)
> >  DOCBOOK(${CMAKE_BINARY_DIR}/doc/dbus-uuidgen.1.xml html-nochunks)
> > +if (UNIX OR MSYS)
> 
> MSYS doesn't appear to be defined anywhere. If you're using -DMSYS manually,
> I don't think that's a good "user interface" - something explicit like
> -DENABLE_MAN_PAGES=[0|1] would be better

I assumed it's a CMake's internal variable, like UNIX - http://www.cmake.org/Wiki/CMake_Useful_Variables
This may actually become a problem later though, if CMake does MSYS detection in a wrong way...But if that happens, i'll fix that in CMake.

Tried to change DBUS_DOCDIR in my patch to just DOCDIR and remove DBUS_DOCDIR setting. Surprisingly, it worked.

Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk>

(In reply to Ralf Habacker from comment #15)
> Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk
test message generated by using 'git bz edit 73636', please ignore. I tried to find a git bz function to add Reviewed-by: ... line after running 'git bz apply 73636'

(In reply to Ralf Habacker from comment #16)
> (In reply to Ralf Habacker from comment #15)
> > Reviewed-by: Simon McVittie <simon.mcvittie@collabora.co.uk
> test message generated by using 'git bz edit 73636', please ignore. I tried
> to find a git bz function to add Reviewed-by: ... line after running 'git bz
> apply 73636'

adding reviewers seems to be a special feature of git-bz-moz, a bit-bz fork https://github.com/mozilla/git-bz-moz/blob/master/git-bz.txt, which is not merged into git-bz.

(In reply to Ralf Habacker from comment #17)
> adding reviewers seems to be a special feature of git-bz-moz, a bit-bz fork
> https://github.com/mozilla/git-bz-moz/blob/master/git-bz.txt, which is not
> merged into 

Did not work with freedesktop bugzilla. After fetching an apikey I get: 

 The requested URL /rest/bug/61922 was not found on this server

(In reply to Ralf Habacker from comment #18)
> (In reply to Ralf Habacker from comment #17)
> > adding reviewers seems to be a special feature of git-bz-moz, a bit-bz fork
> > https://github.com/mozilla/git-bz-moz/blob/master/git-bz.txt, which is not
> > merged into 
> 
> Did not work with freedesktop bugzilla. After fetching an apikey I get: 
> 
>  The requested URL /rest/bug/61922 was not found on this server

wrong bug - this comment should have been added to https://bugs.freedesktop.org/show_bug.cgi?id=93283 or a new bug relating to git bz issues. Any preference ?

(In reply to Ralf Habacker from comment #19)
> wrong bug - this comment should have been added to
> https://bugs.freedesktop.org/show_bug.cgi?id=93283 or a new bug relating to
> git bz issues. Any preference ?

Please don't open dbus bugs that can't be resolved by a change to dbus. If git bz is not working optimally for you, either open git bz bugs or feature requests (assuming it has some sort of bug tracking), or don't use git bz.

-- GitLab Migration Automatic Message --

This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus/issues/93.