Bug 66453

And in cmake, no such option yet.

Created attachment 81800 [details] [review]
[PATCH 1/2] Build: fix build option for x11 autolaunch support

Created attachment 81801 [details] [review]
[PATCH 2/2] README.cmake: update a bit

Comment on attachment 81800 [details] [review]
[PATCH 1/2] Build: fix build option for x11 autolaunch support

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

::: cmake/CMakeLists.txt
@@ +334,4 @@
>  set (DBUS_HAVE_ATOMIC_INT ${atomic_int} CACHE STRING "Some atomic integer implementation present")
>  set (DBUS_USE_ATOMIC_INT_486 ${atomic_int_486} CACHE STRING "Use atomic integer implementation for 486")
>  
> +option (DBUS_ENABLE_X11_AUTOLAUNCH "Build with X11 autolaunch support" ON)

Surely this should default to ON if X11_FOUND, or OFF otherwise?

It definitely needs to default to OFF on Windows, otherwise Windows builds won't work.

I don't think having this option in the CMake build is worth the extra complexity. People who care about minor Unix-specific details like this should be using Autotools.

::: configure.ac
@@ +1266,4 @@
>  
>  AC_ARG_ENABLE([x11-autolaunch],
>    AS_HELP_STRING([--enable-x11-autolaunch], [build with X11 auto-launch support]),
> +  [enable_x11_autolaunch=$enableval], [enable_x11_autolaunch=auto])

This is unnecessary. If you use AC_ARG_ENABLE([foo-bar], ...) in configure.ac, and the user specifies --enable-foo-bar=baz, then  configure will always set "enable_foo_bar=baz". --enable-foo-bar is exactly equivalent to --enable-foo-bar=yes, and --disable-foo-bar is exactly equivalent to --enable-foo-bar=no.

If you look at the generated code for this in configure, you can see that it checks whether $enable_x11_autolaunch has been set in order to decide whether to use the fourth argument (enable_x11_autolaunch=auto).

(In reply to comment #4)
> Comment on attachment 81800 [details] [review] [review]
> [PATCH 1/2] Build: fix build option for x11 autolaunch support
> 
> Review of attachment 81800 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: cmake/CMakeLists.txt
> @@ +334,4 @@
> >  set (DBUS_HAVE_ATOMIC_INT ${atomic_int} CACHE STRING "Some atomic integer implementation present")
> >  set (DBUS_USE_ATOMIC_INT_486 ${atomic_int_486} CACHE STRING "Use atomic integer implementation for 486")
> >  
> > +option (DBUS_ENABLE_X11_AUTOLAUNCH "Build with X11 autolaunch support" ON)
> 
> Surely this should default to ON if X11_FOUND, or OFF otherwise?
> 
> It definitely needs to default to OFF on Windows, otherwise Windows builds
> won't work.
> 
> I don't think having this option in the CMake build is worth the extra
> complexity. People who care about minor Unix-specific details like this
> should be using Autotools.
> 
> ::: configure.ac
> @@ +1266,4 @@
> >  
> >  AC_ARG_ENABLE([x11-autolaunch],
> >    AS_HELP_STRING([--enable-x11-autolaunch], [build with X11 auto-launch support]),
> > +  [enable_x11_autolaunch=$enableval], [enable_x11_autolaunch=auto])
> 
> This is unnecessary. If you use AC_ARG_ENABLE([foo-bar], ...) in
> configure.ac, and the user specifies --enable-foo-bar=baz, then  configure
> will always set "enable_foo_bar=baz". --enable-foo-bar is exactly equivalent
> to --enable-foo-bar=yes, and --disable-foo-bar is exactly equivalent to
> --enable-foo-bar=no.
> 
> If you look at the generated code for this in configure, you can see that it
> checks whether $enable_x11_autolaunch has been set in order to decide
> whether to use the fourth argument (enable_x11_autolaunch=auto).

Fully agree! I'll drop this patch, btw, upload a new one to fix coding style in CMakeLists.txt.

Created attachment 81839 [details] [review]
[PATCH v2 1/2] cmake: fix code style

Created attachment 81840 [details] [review]
[PATCH v2 2/2] cmake: update README.cmake a bit

Ralf, you know more about cmake (and your intended coding style for cmake) than I do?

Comment on attachment 81839 [details] [review]
[PATCH v2 1/2] cmake: fix code style

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

looks good

Comment on attachment 81840 [details] [review]
[PATCH v2 2/2] cmake: update README.cmake a bit

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

looks good

(In reply to comment #8)
> Ralf, you know more about cmake (and your intended coding style for cmake)
> than I do?

I'd hoped you were going to test and merge them if you liked them, but OK, whatever. Fixed in git for 1.7.10.