This is a real small inconvienent since you can't disable x11-autolaunch through configure. Although it's doesn't make much sense if only build with X but not x11-autolaunch. However, since "--enable-x11-autolaunch" is visible to user, so it's reasonable to fix it.
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.
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.