Bug 83583 - cmake fixes - install empty directories
Summary: cmake fixes - install empty directories
Status: RESOLVED FIXED
Alias: None
Product: dbus
Classification: Unclassified
Component: core (show other bugs)
Version: unspecified
Hardware: Other All
: medium normal
Assignee: Havoc Pennington
QA Contact:
URL:
Whiteboard: review?
Keywords: patch
Depends on:
Blocks:
 
Reported: 2014-09-07 09:48 UTC by Ralf Habacker
Modified: 2014-09-18 00:42 UTC (History)
0 users

See Also:
i915 platform:
i915 features:


Attachments
Fix installation of empty directories for cmake build system (3.85 KB, patch)
2014-09-07 09:48 UTC, Ralf Habacker
Details | Splinter Review
Make various system-bus-related things Unix-only (3.10 KB, patch)
2014-09-11 16:38 UTC, Simon McVittie
Details | Splinter Review
Fix installation of empty directories for cmake build system. (3.95 KB, patch)
2014-09-12 14:21 UTC, Simon McVittie
Details | Splinter Review
Fix installation of empty directories for cmake build system (update 3) (5.06 KB, text/plain)
2014-09-14 12:37 UTC, Ralf Habacker
Details
Fix installation of empty directories for cmake build system (update 4) (5.06 KB, patch)
2014-09-14 12:57 UTC, Ralf Habacker
Details | Splinter Review
Fix installation of empty directories for cmake build system (update 5) (4.30 KB, patch)
2014-09-14 13:17 UTC, Ralf Habacker
Details | Splinter Review
Build test-bus-system with cmake on non win32 platforms (1.50 KB, patch)
2014-09-17 08:11 UTC, Ralf Habacker
Details | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Ralf Habacker 2014-09-07 09:48:52 UTC
Created attachment 105857 [details] [review]
Fix installation of empty directories for cmake build system

I compared the installed files using the cmake build system with the files installed by the cross compiled mingw32 packages and found several missing directories, which are fixed by the appended patch.
Comment 1 Simon McVittie 2014-09-08 11:12:57 UTC
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".
Comment 2 Ralf Habacker 2014-09-09 05:56:40 UTC
(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.
Comment 3 Simon McVittie 2014-09-11 16:38:27 UTC
(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.
Comment 4 Simon McVittie 2014-09-11 16:38:50 UTC
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.
Comment 5 Simon McVittie 2014-09-11 16:39:57 UTC
(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.
Comment 6 Simon McVittie 2014-09-12 14:21:01 UTC
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]?
Comment 7 Ralf Habacker 2014-09-14 12:26:23 UTC
(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
Comment 8 Ralf Habacker 2014-09-14 12:37:15 UTC
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
Comment 9 Ralf Habacker 2014-09-14 12:57:46 UTC
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.
Comment 10 Ralf Habacker 2014-09-14 13:17:31 UTC
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
Comment 11 Simon McVittie 2014-09-15 10:31:56 UTC
(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 12 Simon McVittie 2014-09-15 10:35:02 UTC
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?
Comment 13 Ralf Habacker 2014-09-15 11:14:50 UTC
(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}/...
Comment 14 Simon McVittie 2014-09-15 11:18:00 UTC
OK, then your patch looks good for master. Thanks for the clarification.

Does Attachment #106149 [details] also look good to you?
Comment 15 Ralf Habacker 2014-09-15 11:29:04 UTC
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 16 Ralf Habacker 2014-09-15 11:44:20 UTC
Comment on attachment 106149 [details] [review]
Make various system-bus-related things Unix-only

committed to master
Comment 17 Ralf Habacker 2014-09-15 11:44:29 UTC
Comment on attachment 106261 [details] [review]
Fix installation of empty directories for cmake build system (update 5)

committed to master
Comment 18 Simon McVittie 2014-09-15 11:45:32 UTC
(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.
Comment 19 Simon McVittie 2014-09-15 12:02:04 UTC
I think this is all fixed now. Thanks!
Comment 20 Ralf Habacker 2014-09-17 08:11:56 UTC
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 21 Simon McVittie 2014-09-17 11:44:25 UTC
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 22 Ralf Habacker 2014-09-18 00:42:07 UTC
Comment on attachment 106415 [details] [review]
Build test-bus-system with cmake on non win32 platforms

committed to master


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.