Bug 83583

Comment on attachment 105857 [details] [review]
Fix installation of empty directories for cmake build system

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

Looks reasonable, but it would make sense to skip the vestigial support for the system bus when compiling for Windows.

::: cmake/CMakeLists.txt
@@ +405,2 @@
>    # bus-test expects a non empty string
>  	set (DBUS_SYSTEM_PID_FILE "/dbus-pid")

Does this make any sense? If it's just a dummy path that is not expected to ever exist on Windows, would it be OK to use the same path as on Unix?

::: cmake/bus/CMakeLists.txt
@@ +89,4 @@
>  
>  install_targets(/bin dbus-daemon)
>  install_files(/etc/dbus-1 FILES ${config_DATA})
> +install(DIRECTORY DESTINATION var/run/dbus)

This is only used by the system bus. I don't think it makes sense to install this on Windows, where there is no system bus (D-Bus is not part of the OS). Wrap this in if (!WIN32) perhaps?

@@ +90,5 @@
>  install_targets(/bin dbus-daemon)
>  install_files(/etc/dbus-1 FILES ${config_DATA})
> +install(DIRECTORY DESTINATION var/run/dbus)
> +install(DIRECTORY DESTINATION etc/dbus-1/session.d)
> +install(DIRECTORY DESTINATION etc/dbus-1/system.d)

Likewise for system.d.

@@ +92,5 @@
> +install(DIRECTORY DESTINATION var/run/dbus)
> +install(DIRECTORY DESTINATION etc/dbus-1/session.d)
> +install(DIRECTORY DESTINATION etc/dbus-1/system.d)
> +install(DIRECTORY DESTINATION etc/dbus-1/services)
> +install(DIRECTORY DESTINATION etc/dbus-1/system-services)

Likewise for system-services.

::: cmake/tools/CMakeLists.txt
@@ +46,5 @@
>  target_link_libraries(dbus-monitor ${DBUS_LIBRARIES})
>  install_targets(/bin dbus-monitor )
> +
> +# create the /var/lib/dbus directory for dbus-uuidgen
> +install(DIRECTORY DESTINATION var/lib/dbus)

Does this make sense on Windows either? My guess is "no".

(In reply to comment #1)
> Comment on attachment 105857 [details] [review] [review]
> Fix installation of empty directories for cmake build system
> 
> Review of attachment 105857 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> Looks reasonable, but it would make sense to skip the vestigial support for
> the system bus when compiling for Windows.

Please note that the cross compiled mingw32 package uses the autotools build system, see https://build.opensuse.org/package/view_file/windows:mingw:win32/mingw32-dbus-1/mingw32-dbus-1.spec?expand=1, so any requested changes affects also the windows build part of autotools if keeping build systems in sync has high priority.

(In reply to comment #2)
> so any requested changes affects also
> the windows build part of autotools

A fair point. I'll do a patch for the Autotools side.

Created attachment 106149 [details] [review]
Make various system-bus-related things Unix-only

There is no system bus on Windows, and there won't be until/unless
it can be secure.

(In reply to comment #1)
> > +# create the /var/lib/dbus directory for dbus-uuidgen
> > +install(DIRECTORY DESTINATION var/lib/dbus)
> 
> Does this make sense on Windows either? My guess is "no".

I haven't removed this from the Autotools build; thinking about it, it does make some sense for this to exist on Windows.

Created attachment 106186 [details] [review]
Fix installation of empty directories for cmake build system.

From: Ralf Habacker <ralf.habacker@freenet.de>

The differences has been found out by comparing with the cross compiled
mingw..-dbus packages.

[exclude system bus support bits on Windows -smcv]

---

How's this, as a version of your Attachment #105857 [details] to be applied after Attachment #106149 [details]?

(In reply to comment #6)
> Created attachment 106186 [details] [review] [review]
> Fix installation of empty directories for cmake build system.
> 
> From: Ralf Habacker <ralf.habacker@freenet.de>
> 
> The differences has been found out by comparing with the cross compiled
> mingw..-dbus packages.
> 
> [exclude system bus support bits on Windows -smcv]
> 
> ---
> 
> How's this, as a version of your Attachment #105857 [details] to be applied
> after Attachment #106149 [details]?

After applying the mentioned patches, cmake installs the services and system-services in etc/dbus-1 instead of share/dbus-1 as done with autotools

Created attachment 106257 [details]
Fix installation of empty directories for cmake build system (update 3)

- disable installation of system.conf on windows
- fix install location of dbus-1/services dir

Created attachment 106259 [details] [review]
Fix installation of empty directories for cmake build system (update 4)

keep cmake install location of dbusarch headers in sync with autotools

While comparing how headers are installed I wonder why a directory include/dbus-1.0 is used by autotools. Taking a look of all other version related install dirs like config and services it should be dbus-1. 
git history also gave me no real reason why dbus-1.0 has been choosed. With recent version it should be dbus-1.8 or to be minor version independent dbus-1. 
I suggest to finish this bug with recent patches and fix this dir issue with bug #73636.

Created attachment 106261 [details] [review]
Fix installation of empty directories for cmake build system (update 5)

do not cover include header location install with this bug. include header location is handled with bug #73636

(In reply to comment #9)
> While comparing how headers are installed I wonder why a directory
> include/dbus-1.0 is used by autotools. Taking a look of all other version
> related install dirs like config and services it should be dbus-1. 

It's the "API version" of libdbus. 1 would have been a better choice than 1.0, but now that it's been "wrong" for several years, changing it would be more disruptive than not changing it.

Comment on attachment 106261 [details] [review]
Fix installation of empty directories for cmake build system (update 5)

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

If what I say below is correct, then this looks good. If what I say below is wrong, it might need another look.

::: cmake/CMakeLists.txt
@@ +73,4 @@
>  set(DBUS_MACHINE_UUID_FILE   ${DBUS_INSTALL_DIR}/lib/dbus/machine-id)
>  set(DBUS_BINDIR              ${EXPANDED_BINDIR})
>  set(DBUS_DAEMONDIR           ${EXPANDED_BINDIR})
> +set(DBUS_LOCALSTATEDIR       ${DBUS_INSTALL_DIR}/var)

Just to check: ${DBUS_INSTALL_DIR} is something like /usr/local, like Autotools' ${prefix}, and does not include the equivalent of Autotools' ${DESTDIR}. Am I correct?

(In reply to comment #12)
> Comment on attachment 106261 [details] [review] [review]
> Fix installation of empty directories for cmake build system (update 5)
> 
> Review of attachment 106261 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> If what I say below is correct, then this looks good. If what I say below is
> wrong, it might need another look.
> 
> ::: cmake/CMakeLists.txt
> @@ +73,4 @@
> >  set(DBUS_MACHINE_UUID_FILE   ${DBUS_INSTALL_DIR}/lib/dbus/machine-id)
> >  set(DBUS_BINDIR              ${EXPANDED_BINDIR})
> >  set(DBUS_DAEMONDIR           ${EXPANDED_BINDIR})
> > +set(DBUS_LOCALSTATEDIR       ${DBUS_INSTALL_DIR}/var)
> 
> Just to check: ${DBUS_INSTALL_DIR} is something like /usr/local, like
> Autotools' ${prefix}

yes, here are some explanations: 
normally the install prefix of cmake package are defined with -DCMAKE_INSTALL_PREFIX=... which is the same as Autotools --prefix command line options. 

In dbus cmake buildsystem there are some convenient settings to be able to set CMAKE_INSTALL_PREFIX from 

- cmake configure command line -DDBUSDIR or
- from the environment variable DBUSDIR

DBUS_INSTALL_DIR is used in the cmake buildsystem and contains the same value as CMAKE_INSTALL_PREFIX. 


> and does not include the equivalent of Autotools' ${DESTDIR}. Am I correct?

yes, with cmake you can also run "make install DESTDIR=<some-location>" which will install dbus generated files into <some-location>${DBUS_INSTALL_DIR}/...

OK, then your patch looks good for master. Thanks for the clarification.

Does Attachment #106149 [details] also look good to you?

Comment on attachment 106149 [details] [review]
Make various system-bus-related things Unix-only

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

looks good. 

off-topic note: generating test-bus-system is not included in cmake build system.

Comment on attachment 106149 [details] [review]
Make various system-bus-related things Unix-only

committed to master

Comment on attachment 106261 [details] [review]
Fix installation of empty directories for cmake build system (update 5)

committed to master

(In reply to comment #15)
> off-topic note: generating test-bus-system is not included in cmake build
> system.

It would be reasonable to compile that on Unix if you want to provide a patch, but I don't think it's very important: as far as I'm concerned, the cmake build system on Unix is mostly there so maintainers who don't use Windows can verify that they haven't broken it too badly. We use Autotools for releases.

I think this is all fixed now. Thanks!

Created attachment 106415 [details] [review]
Build test-bus-system with cmake on non win32 platforms

(In reply to comment #18)
> (In reply to comment #15)
> > off-topic note: generating test-bus-system is not included in cmake build
> > system.
> 
> It would be reasonable to compile that on Unix if you want to provide a
> patch, but I don't think it's very important: as far as I'm concerned, the
> cmake build system on Unix is mostly there so maintainers who don't use
> Windows can verify that they haven't broken it too badly. We use Autotools
> for releases.
here is the related cmake patch to be in sync with autotools

Comment on attachment 106415 [details] [review]
Build test-bus-system with cmake on non win32 platforms

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

Looks good to me

Comment on attachment 106415 [details] [review]
Build test-bus-system with cmake on non win32 platforms

committed to master