I just created several patches. removed unused definitions, cleanup libxml2 clue in cmake, align cmake with autotools about the dir-watch backend and another bug fix for cmake on BSD build failure.
Created attachment 81562 [details] [review] [PATCH 1/4] Build: remove unused definitions
Created attachment 81563 [details] [review] [PATCH 2/4] Clean up libxml2 clue and mark kqueue as BSD only
Created attachment 81564 [details] [review] [PATCH 3/4] cmake: align dir watch backend detection with autotools
Created attachment 81565 [details] [review] [PATCH 4/4] cmake: fix build failure on FreeBSD
Comment on attachment 81562 [details] [review] [PATCH 1/4] Build: remove unused definitions Review of attachment 81562 [details] [review]: ----------------------------------------------------------------- ::: cmake/CMakeLists.txt @@ -233,4 @@ > option (DBUS_BUILD_TESTS "enable unit test code" ON) > > if(DBUS_BUILD_TESTS) > - add_definitions(-DDBUS_BUILD_TESTS -DDBUS_ENABLE_EMBEDDED_TESTS) I'd prefer it if you don't. The idea was that DBUS_BUILD_TESTS still works (as a way to reduce diff), but DBUS_ENABLE_EMBEDDED_TESTS is better for new stuff. I'd accept patches that move from checking DBUS_BUILD_TESTS to checking DBUS_ENABLE_EMBEDDED_TESTS (but only on master, and only a few related files at a time please - not all at once :-) Defining DBUS_ENABLE_EMBEDDED_TESTS in the actual build system, and defining DBUS_BUILD_TESTS (in terms of it) in config.h.cmake/config.h, would also work. Context: the tests used to all be what I called "embedded tests" which add extra stuff (potentially including new security risks!) to the library and binaries, so they should be turned off in production builds. When I introduced the separate "modular tests", which can be compiled all the time, I also added the "embedded tests" term. The name DBUS_BUILD_TESTS is misleading now, because enabling "modular tests" doesn't set it. ::: configure.ac @@ -236,5 @@ > fi > -if test "x$enable_modular_tests" != xno; then > - AC_DEFINE([DBUS_ENABLE_MODULAR_TESTS], [1], > - [Define to build independent test binaries]) > -fi Not strictly necessary but I don't think deleting it is particularly worthwhile. @@ -243,5 @@ > > -if test "x$with_glib" != xno; then > - AC_DEFINE([DBUS_WITH_GLIB], [1], > - [Define if GLib, GObject, GIO are available]) > -fi Not strictly necessary but I don't think deleting it is particularly worthwhile. @@ +1042,5 @@ > dnl check if inotify backend is enabled > + AC_CHECK_HEADERS(sys/inotify.h, > + [ > + AC_CHECK_FUNC(inotify_init1, have_inotify=yes, have_inotify=no) > + ], have_inotify=no) Getting rid of the AC_DEFINE for inotify/kqueue: ... if you insist, but I don't think there's that much value in doing so.
Comment on attachment 81563 [details] [review] [PATCH 2/4] Clean up libxml2 clue and mark kqueue as BSD only Review of attachment 81563 [details] [review]: ----------------------------------------------------------------- When you say "clue" do you mean "glue"? ::: cmake/CMakeLists.txt @@ +284,4 @@ > option (DBUS_BUS_ENABLE_DNOTIFY_ON_LINUX "build with dnotify support (linux only)" ON) # add a check ! > endif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") > > +#AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue support (BSD only)]),enable_kqueue=$enableval,enable_kqueue=auto) Do other BSDs have kqueue? Not sure how much point there is in keeping these commented-out versions of the AC_ARG_ENABLE() from configure.ac, any more. @@ +297,5 @@ > endif("${sysname}" MATCHES ".*SOLARIS.*") > > +if(NOT EXPAT_FOUND) > + message(FATAL "expat not found!") > +endif(NOT EXPAT_FOUND) This bit looks fine. @@ -566,4 @@ > message(" Building Doxygen docs: ${DBUS_ENABLE_DOXYGEN_DOCS} ") > message(" Building XML docs: ${DBUS_ENABLE_XML_DOCS} ") > #message(" Gettext libs (empty OK): ${INTLLIBS} ") > -message(" Using XML parser: ${XML_LIB} ") This bit is fine. ::: configure.ac @@ +152,4 @@ > AC_ARG_ENABLE(libaudit,AS_HELP_STRING([--enable-libaudit],[build audit daemon support for SELinux]),enable_libaudit=$enableval,enable_libaudit=auto) > AC_ARG_ENABLE(dnotify, AS_HELP_STRING([--enable-dnotify],[build with dnotify support (linux only)]),enable_dnotify=$enableval,enable_dnotify=auto) > AC_ARG_ENABLE(inotify, AS_HELP_STRING([--enable-inotify],[build with inotify support (linux only)]),enable_inotify=$enableval,enable_inotify=auto) > +AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue support (BSD only)]),enable_kqueue=$enableval,enable_kqueue=auto) Do other BSDs have kqueue?
Comment on attachment 81564 [details] [review] [PATCH 3/4] cmake: align dir watch backend detection with autotools Review of attachment 81564 [details] [review]: ----------------------------------------------------------------- Please defer this until we've got rid of dnotify, at which point there won't be as much code. ::: cmake/CMakeLists.txt @@ +316,4 @@ > endif("${sysname}" MATCHES ".*SOLARIS.*") > > if(NOT EXPAT_FOUND) > + message(FATAL_ERROR "expat not found!") This looks as though it should have been part of a different commit? @@ +403,4 @@ > else (NOT $ENV{TMP} STREQUAL "") > if (WIN32) > #Should never happen, both TMP and TEMP seem always set on Windows > + message(FATAL_ERROR "Could not determine a usable temporary directory") What's the difference between FATAL and FATAL_ERROR?
Comment on attachment 81565 [details] [review] [PATCH 4/4] cmake: fix build failure on FreeBSD Review of attachment 81565 [details] [review]: ----------------------------------------------------------------- It doesn't really matter whether the CMake build system works on non-Linux Unix, or produces "ideal" binaries (with inotify, etc.). It's mostly there for Windows/MSVC users; it's valuable for it to work in a basic way on Linux (so CMake changes can be smoke-tested without using Windows), but the Autotools build system is the recommended one. In Autotools, AC_SYSTEM_EXTENSIONS encapsulates all this rubbish; if there isn't an equally easy solution in CMake, I'm not going to put time or effort into it. Having said that: does replacing PF_UNIX with AF_UNIX (which is numerically the same) work any better? If it does, I'd accept that patch.
(In reply to comment #5) > Comment on attachment 81562 [details] [review] [review] > [PATCH 1/4] Build: remove unused definitions > > Review of attachment 81562 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: cmake/CMakeLists.txt > @@ -233,4 @@ > > option (DBUS_BUILD_TESTS "enable unit test code" ON) > > > > if(DBUS_BUILD_TESTS) > > - add_definitions(-DDBUS_BUILD_TESTS -DDBUS_ENABLE_EMBEDDED_TESTS) > > I'd prefer it if you don't. The idea was that DBUS_BUILD_TESTS still works > (as a way to reduce diff), but DBUS_ENABLE_EMBEDDED_TESTS is better for new > stuff. > > I'd accept patches that move from checking DBUS_BUILD_TESTS to checking > DBUS_ENABLE_EMBEDDED_TESTS (but only on master, and only a few related files > at a time please - not all at once :-) Sure, I'll do that, #bug66291 is now created to track these patches. > > Defining DBUS_ENABLE_EMBEDDED_TESTS in the actual build system, and defining > DBUS_BUILD_TESTS (in terms of it) in config.h.cmake/config.h, would also > work. > > Context: the tests used to all be what I called "embedded tests" which add > extra stuff (potentially including new security risks!) to the library and > binaries, so they should be turned off in production builds. When I > introduced the separate "modular tests", which can be compiled all the time, > I also added the "embedded tests" term. The name DBUS_BUILD_TESTS is > misleading now, because enabling "modular tests" doesn't set it. > > ::: configure.ac > @@ -236,5 @@ > > fi > > -if test "x$enable_modular_tests" != xno; then > > - AC_DEFINE([DBUS_ENABLE_MODULAR_TESTS], [1], > > - [Define to build independent test binaries]) > > -fi > > Not strictly necessary but I don't think deleting it is particularly > worthwhile. > > @@ -243,5 @@ > > > > -if test "x$with_glib" != xno; then > > - AC_DEFINE([DBUS_WITH_GLIB], [1], > > - [Define if GLib, GObject, GIO are available]) > > -fi > > Not strictly necessary but I don't think deleting it is particularly > worthwhile. > > @@ +1042,5 @@ > > dnl check if inotify backend is enabled > > + AC_CHECK_HEADERS(sys/inotify.h, > > + [ > > + AC_CHECK_FUNC(inotify_init1, have_inotify=yes, have_inotify=no) > > + ], have_inotify=no) > > Getting rid of the AC_DEFINE for inotify/kqueue: ... if you insist, but I > don't think there's that much value in doing so. OK, I'll keep them.
(In reply to comment #6) > Comment on attachment 81563 [details] [review] [review] > [PATCH 2/4] Clean up libxml2 clue and mark kqueue as BSD only > > Review of attachment 81563 [details] [review] [review]: > ----------------------------------------------------------------- > > When you say "clue" do you mean "glue"? You always know me. :-). > > ::: cmake/CMakeLists.txt > @@ +284,4 @@ > > option (DBUS_BUS_ENABLE_DNOTIFY_ON_LINUX "build with dnotify support (linux only)" ON) # add a check ! > > endif("${CMAKE_SYSTEM_NAME}" STREQUAL "Linux") > > > > +#AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue support (BSD only)]),enable_kqueue=$enableval,enable_kqueue=auto) > > Do other BSDs have kqueue? According to wikepedia " Kqueue is a scalable event notification interface introduced in FreeBSD 4.1,[1] also supported in NetBSD, OpenBSD, DragonflyBSD, and Mac OS X. " However, haven't verified, I only have one FreeBSD 9.1 instance so far. > > Not sure how much point there is in keeping these commented-out versions of > the AC_ARG_ENABLE() from configure.ac, any more. Yes, there are much more, so it's time to remove them? I'll do that. > > @@ +297,5 @@ > > endif("${sysname}" MATCHES ".*SOLARIS.*") > > > > +if(NOT EXPAT_FOUND) > > + message(FATAL "expat not found!") > > +endif(NOT EXPAT_FOUND) > > This bit looks fine. > > @@ -566,4 @@ > > message(" Building Doxygen docs: ${DBUS_ENABLE_DOXYGEN_DOCS} ") > > message(" Building XML docs: ${DBUS_ENABLE_XML_DOCS} ") > > #message(" Gettext libs (empty OK): ${INTLLIBS} ") > > -message(" Using XML parser: ${XML_LIB} ") > > This bit is fine. > > ::: configure.ac > @@ +152,4 @@ > > AC_ARG_ENABLE(libaudit,AS_HELP_STRING([--enable-libaudit],[build audit daemon support for SELinux]),enable_libaudit=$enableval,enable_libaudit=auto) > > AC_ARG_ENABLE(dnotify, AS_HELP_STRING([--enable-dnotify],[build with dnotify support (linux only)]),enable_dnotify=$enableval,enable_dnotify=auto) > > AC_ARG_ENABLE(inotify, AS_HELP_STRING([--enable-inotify],[build with inotify support (linux only)]),enable_inotify=$enableval,enable_inotify=auto) > > +AC_ARG_ENABLE(kqueue, AS_HELP_STRING([--enable-kqueue],[build with kqueue support (BSD only)]),enable_kqueue=$enableval,enable_kqueue=auto) > > Do other BSDs have kqueue?
(In reply to comment #7) > Comment on attachment 81564 [details] [review] [review] > [PATCH 3/4] cmake: align dir watch backend detection with autotools > > Review of attachment 81564 [details] [review] [review]: > ----------------------------------------------------------------- > > Please defer this until we've got rid of dnotify, at which point there won't > be as much code. Sure, the patch to git rid of dnotify uploaded to #bug33001. > > ::: cmake/CMakeLists.txt > @@ +316,4 @@ > > endif("${sysname}" MATCHES ".*SOLARIS.*") > > > > if(NOT EXPAT_FOUND) > > + message(FATAL_ERROR "expat not found!") > > This looks as though it should have been part of a different commit? Sure, change FATAL to FATAL_ERROR worth a separate commit. > > @@ +403,4 @@ > > else (NOT $ENV{TMP} STREQUAL "") > > if (WIN32) > > #Should never happen, both TMP and TEMP seem always set on Windows > > + message(FATAL_ERROR "Could not determine a usable temporary directory") > > What's the difference between FATAL and FATAL_ERROR? Refer to http://www.cmake.org/cmake/help/v2.8.8/cmake.html#command%3amessage, FATAL_ERROR can stop cmake to generate makefiles, while FATAL doesn't. In fact, FATAL isn't a valid key to message() according to the above manual.
(In reply to comment #8) > Comment on attachment 81565 [details] [review] [review] > [PATCH 4/4] cmake: fix build failure on FreeBSD > > Review of attachment 81565 [details] [review] [review]: > ----------------------------------------------------------------- > > It doesn't really matter whether the CMake build system works on non-Linux > Unix, or produces "ideal" binaries (with inotify, etc.). It's mostly there OK, got your word, I was wondering this are several gaps between cmake and autotools. > for Windows/MSVC users; it's valuable for it to work in a basic way on Linux > (so CMake changes can be smoke-tested without using Windows), but the > Autotools build system is the recommended one. > > In Autotools, AC_SYSTEM_EXTENSIONS encapsulates all this rubbish; if there > isn't an equally easy solution in CMake, I'm not going to put time or effort > into it. > > Having said that: does replacing PF_UNIX with AF_UNIX (which is numerically > the same) work any better? If it does, I'd accept that patch. I'll check it, if this is the only issue.
(In reply to comment #12) > (In reply to comment #8) > > Comment on attachment 81565 [details] [review] [review] [review] > > [PATCH 4/4] cmake: fix build failure on FreeBSD > > > > Review of attachment 81565 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > It doesn't really matter whether the CMake build system works on non-Linux > > Unix, or produces "ideal" binaries (with inotify, etc.). It's mostly there > > OK, got your word, I was wondering this are several gaps between cmake and > autotools. > > > for Windows/MSVC users; it's valuable for it to work in a basic way on Linux > > (so CMake changes can be smoke-tested without using Windows), but the > > Autotools build system is the recommended one. > > > > In Autotools, AC_SYSTEM_EXTENSIONS encapsulates all this rubbish; if there > > isn't an equally easy solution in CMake, I'm not going to put time or effort > > into it. > > > > Having said that: does replacing PF_UNIX with AF_UNIX (which is numerically > > the same) work any better? If it does, I'd accept that patch. > > I'll check it, if this is the only issue. According to http://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Posix-Variants.html, AC_USE_SYSTEM_EXTENSIONS doesn't ask use an old POSIX c standard. In addition, verified that on FreeBSD, if I build CFLAGS="-D__POSIX_C_SOURCE=199309L" explicitly, gmake fail with a lot of errors like below. gmake[3]: Entering directory `/usr/home/chengwei/dbus.git/dbus' CC libdbus_1_la-dbus-sysdeps-unix.lo dbus-sysdeps-unix.c: In function '_dbus_read_socket_with_unix_fds': dbus-sysdeps-unix.c:317: warning: implicit declaration of function 'CMSG_SPACE' dbus-sysdeps-unix.c:317: warning: nested extern declaration of 'CMSG_SPACE' dbus-sysdeps-unix.c:321: warning: implicit declaration of function 'alloca' dbus-sysdeps-unix.c:321: warning: incompatible implicit declaration of built-in function 'alloca' dbus-sysdeps-unix.c:365: warning: implicit declaration of function 'CMSG_LEN' dbus-sysdeps-unix.c:365: warning: nested extern declaration of 'CMSG_LEN' dbus-sysdeps-unix.c: In function '_dbus_write_socket_with_unix_fds_two': dbus-sysdeps-unix.c:466: warning: incompatible implicit declaration of built-in function 'alloca' dbus-sysdeps-unix.c: In function '_dbus_listen_unix_socket': dbus-sysdeps-unix.c:1085: warning: implicit declaration of function 'S_ISSOCK' dbus-sysdeps-unix.c:1085: warning: nested extern declaration of 'S_ISSOCK' dbus-sysdeps-unix.c: In function '_dbus_read_credentials_socket': dbus-sysdeps-unix.c:1808: warning: implicit declaration of function 'getpeereid' dbus-sysdeps-unix.c:1808: warning: nested extern declaration of 'getpeereid' dbus-sysdeps-unix.c: In function 'fill_user_info': dbus-sysdeps-unix.c:2072: error: '_SC_GETPW_R_SIZE_MAX' undeclared (first use in this function) dbus-sysdeps-unix.c:2072: error: (Each undeclared identifier is reported only once dbus-sysdeps-unix.c:2072: error: for each function it appears in.) dbus-sysdeps-unix.c:2094: warning: implicit declaration of function 'getpwuid_r' dbus-sysdeps-unix.c:2094: warning: nested extern declaration of 'getpwuid_r' dbus-sysdeps-unix.c:2097: warning: implicit declaration of function 'getpwnam_r' dbus-sysdeps-unix.c:2097: warning: nested extern declaration of 'getpwnam_r' dbus-sysdeps-unix.c:2183: warning: implicit declaration of function 'getgrouplist' dbus-sysdeps-unix.c:2183: warning: nested extern declaration of 'getgrouplist' dbus-sysdeps-unix.c: In function '_dbus_get_monotonic_time': dbus-sysdeps-unix.c:2636: warning: implicit declaration of function 'gettimeofday' dbus-sysdeps-unix.c:2636: warning: nested extern declaration of 'gettimeofday' dbus-sysdeps-unix.c: In function '_dbus_printf_string_upper_bound': dbus-sysdeps-unix.c:3142: warning: implicit declaration of function 'vsnprintf' dbus-sysdeps-unix.c: In function '_dbus_check_setuid': dbus-sysdeps-unix.c:4160: warning: implicit declaration of function 'issetugid' dbus-sysdeps-unix.c:4160: warning: nested extern declaration of 'issetugid' dbus-sysdeps-unix.c: In function '_dbus_append_address_from_socket': dbus-sysdeps-unix.c:4207: error: field 'ipv6' has incomplete type gmake[3]: *** [libdbus_1_la-dbus-sysdeps-unix.lo] Error 1 gmake[3]: Leaving directory `/usr/home/chengwei/dbus.git/dbus' gmake[2]: *** [all] Error 2 gmake[2]: Leaving directory `/usr/home/chengwei/dbus.git/dbus' gmake[1]: *** [all-recursive] Error 1 gmake[1]: Leaving directory `/usr/home/chengwei/dbus.git' gmake: *** [all] Error 2 So I think it's reasonable to remove the POSIX_C_SOURCE definition here, am I missed anything?
(In reply to comment #13) > (In reply to comment #12) > > (In reply to comment #8) > > > Comment on attachment 81565 [details] [review] [review] [review] [review] > > > [PATCH 4/4] cmake: fix build failure on FreeBSD > > > > > > Review of attachment 81565 [details] [review] [review] [review] [review]: > > > ----------------------------------------------------------------- > > > > > > It doesn't really matter whether the CMake build system works on non-Linux > > > Unix, or produces "ideal" binaries (with inotify, etc.). It's mostly there > > > > OK, got your word, I was wondering this are several gaps between cmake and > > autotools. > > > > > for Windows/MSVC users; it's valuable for it to work in a basic way on Linux > > > (so CMake changes can be smoke-tested without using Windows), but the > > > Autotools build system is the recommended one. > > > > > > In Autotools, AC_SYSTEM_EXTENSIONS encapsulates all this rubbish; if there > > > isn't an equally easy solution in CMake, I'm not going to put time or effort > > > into it. > > > > > > Having said that: does replacing PF_UNIX with AF_UNIX (which is numerically > > > the same) work any better? If it does, I'd accept that patch. > > > > I'll check it, if this is the only issue. > > According to > http://www.gnu.org/software/autoconf/manual/autoconf-2.67/html_node/Posix- > Variants.html, AC_USE_SYSTEM_EXTENSIONS doesn't ask use an old POSIX c > standard. > > In addition, verified that on FreeBSD, if I build > CFLAGS="-D__POSIX_C_SOURCE=199309L" explicitly, gmake fail with a lot of > errors like below. NOTE: I use autotools here, not cmake. > > gmake[3]: Entering directory `/usr/home/chengwei/dbus.git/dbus' > CC libdbus_1_la-dbus-sysdeps-unix.lo > dbus-sysdeps-unix.c: In function '_dbus_read_socket_with_unix_fds': > dbus-sysdeps-unix.c:317: warning: implicit declaration of function > 'CMSG_SPACE' > dbus-sysdeps-unix.c:317: warning: nested extern declaration of 'CMSG_SPACE' > dbus-sysdeps-unix.c:321: warning: implicit declaration of function 'alloca' > dbus-sysdeps-unix.c:321: warning: incompatible implicit declaration of > built-in function 'alloca' > dbus-sysdeps-unix.c:365: warning: implicit declaration of function 'CMSG_LEN' > dbus-sysdeps-unix.c:365: warning: nested extern declaration of 'CMSG_LEN' > dbus-sysdeps-unix.c: In function '_dbus_write_socket_with_unix_fds_two': > dbus-sysdeps-unix.c:466: warning: incompatible implicit declaration of > built-in function 'alloca' > dbus-sysdeps-unix.c: In function '_dbus_listen_unix_socket': > dbus-sysdeps-unix.c:1085: warning: implicit declaration of function > 'S_ISSOCK' > dbus-sysdeps-unix.c:1085: warning: nested extern declaration of 'S_ISSOCK' > dbus-sysdeps-unix.c: In function '_dbus_read_credentials_socket': > dbus-sysdeps-unix.c:1808: warning: implicit declaration of function > 'getpeereid' > dbus-sysdeps-unix.c:1808: warning: nested extern declaration of 'getpeereid' > dbus-sysdeps-unix.c: In function 'fill_user_info': > dbus-sysdeps-unix.c:2072: error: '_SC_GETPW_R_SIZE_MAX' undeclared (first > use in this function) > dbus-sysdeps-unix.c:2072: error: (Each undeclared identifier is reported > only once > dbus-sysdeps-unix.c:2072: error: for each function it appears in.) > dbus-sysdeps-unix.c:2094: warning: implicit declaration of function > 'getpwuid_r' > dbus-sysdeps-unix.c:2094: warning: nested extern declaration of 'getpwuid_r' > dbus-sysdeps-unix.c:2097: warning: implicit declaration of function > 'getpwnam_r' > dbus-sysdeps-unix.c:2097: warning: nested extern declaration of 'getpwnam_r' > dbus-sysdeps-unix.c:2183: warning: implicit declaration of function > 'getgrouplist' > dbus-sysdeps-unix.c:2183: warning: nested extern declaration of > 'getgrouplist' > dbus-sysdeps-unix.c: In function '_dbus_get_monotonic_time': > dbus-sysdeps-unix.c:2636: warning: implicit declaration of function > 'gettimeofday' > dbus-sysdeps-unix.c:2636: warning: nested extern declaration of > 'gettimeofday' > dbus-sysdeps-unix.c: In function '_dbus_printf_string_upper_bound': > dbus-sysdeps-unix.c:3142: warning: implicit declaration of function > 'vsnprintf' > dbus-sysdeps-unix.c: In function '_dbus_check_setuid': > dbus-sysdeps-unix.c:4160: warning: implicit declaration of function > 'issetugid' > dbus-sysdeps-unix.c:4160: warning: nested extern declaration of 'issetugid' > dbus-sysdeps-unix.c: In function '_dbus_append_address_from_socket': > dbus-sysdeps-unix.c:4207: error: field 'ipv6' has incomplete type > gmake[3]: *** [libdbus_1_la-dbus-sysdeps-unix.lo] Error 1 > gmake[3]: Leaving directory `/usr/home/chengwei/dbus.git/dbus' > gmake[2]: *** [all] Error 2 > gmake[2]: Leaving directory `/usr/home/chengwei/dbus.git/dbus' > gmake[1]: *** [all-recursive] Error 1 > gmake[1]: Leaving directory `/usr/home/chengwei/dbus.git' > gmake: *** [all] Error 2 > > So I think it's reasonable to remove the POSIX_C_SOURCE definition here, am > I missed anything?
Created attachment 81604 [details] [review] [PATCH v2 1/5] cmake: clean up libxml2 glue
Created attachment 81605 [details] [review] [PATCH v2 2/5] cmake: get rid of useless commented out code
Created attachment 81606 [details] [review] [PATCH v2 3/5] cmake: terminate to generate makefiles due to fatal error
Created attachment 81607 [details] [review] [PATCH v2 4/5] cmake: align dir watch backend detection with autotools
Created attachment 81608 [details] [review] [PATCH v2 5/5] cmake: fix build failure on FreeBSD
(In reply to comment #15) > [PATCH v2 1/5] cmake: clean up libxml2 glue (In reply to comment #16) > [PATCH v2 2/5] cmake: get rid of useless commented out code (In reply to comment #17) > [PATCH v2 3/5] cmake: terminate to generate makefiles due to fatal error (In reply to comment #18) > [PATCH v2 4/5] cmake: align dir watch backend detection with autotools Applied, 1.7.6.
Comment on attachment 81608 [details] [review] [PATCH v2 5/5] cmake: fix build failure on FreeBSD Review of attachment 81608 [details] [review]: ----------------------------------------------------------------- > First, PF_UNIX will not defined if _POSIX_C_SOURCE defined Use AF_UNIX, then? > Second, ipv6 header netinet6/in6.h will not be included if > _POSIX_C_SOURCE < 200112 Either set _POSIX_C_SOURCE to 200112 or don't set it at all, then? Setting _POSIX_C_SOURCE to an obsolete version, only on non-*BSD, doesn't seem right: surely we should either always define it (to a useful version), or never define it?
(In reply to comment #21) > Comment on attachment 81608 [details] [review] [review] > [PATCH v2 5/5] cmake: fix build failure on FreeBSD > > Review of attachment 81608 [details] [review] [review]: > ----------------------------------------------------------------- > > > First, PF_UNIX will not defined if _POSIX_C_SOURCE defined > > Use AF_UNIX, then? Yes, after replace PF_UNIX with AF_UNIX in source code, we encounter the second issue. > > > Second, ipv6 header netinet6/in6.h will not be included if > > _POSIX_C_SOURCE < 200112 > > Either set _POSIX_C_SOURCE to 200112 or don't set it at all, then? I think it's better to us don't set it at all, it's hard to say which version will OK. > > Setting _POSIX_C_SOURCE to an obsolete version, only on non-*BSD, doesn't > seem right: surely we should either always define it (to a useful version), > or never define it? As previous, seems the obsolete _POSIX_C_SOURCE works fine on Linux and Windows/mingw, so this change try to fix it on FreeBSD, it's relative a conservative safe change. I think we need more info to ensure if it's safe to remove it, especially for windows port.
(In reply to comment #22) > (In reply to comment #21) > > Comment on attachment 81608 [details] [review] [review] [review] > > [PATCH v2 5/5] cmake: fix build failure on FreeBSD > > > > Review of attachment 81608 [details] [review] [review] [review]: > > ----------------------------------------------------------------- > > > > > First, PF_UNIX will not defined if _POSIX_C_SOURCE defined > > > > Use AF_UNIX, then? > > Yes, after replace PF_UNIX with AF_UNIX in source code, we encounter the > second issue. > > > > > > Second, ipv6 header netinet6/in6.h will not be included if > > > _POSIX_C_SOURCE < 200112 > > > > Either set _POSIX_C_SOURCE to 200112 or don't set it at all, then? > > I think it's better to us don't set it at all, it's hard to say which > version will OK. Ignore this line. :-) > > > > > Setting _POSIX_C_SOURCE to an obsolete version, only on non-*BSD, doesn't > > seem right: surely we should either always define it (to a useful version), > > or never define it? > > As previous, seems the obsolete _POSIX_C_SOURCE works fine on Linux and > Windows/mingw, so this change try to fix it on FreeBSD, it's relative a > conservative safe change. > > I think we need more info to ensure if it's safe to remove it, especially > for windows port.
(In reply to comment #22) > As previous, seems the obsolete _POSIX_C_SOURCE works fine on Linux and > Windows/mingw, so this change try to fix it on FreeBSD, it's relative a > conservative safe change. Windows isn't a POSIX platform, so I would be really surprised if _POSIX_C_SOURCE does anything there. The Autotools build system is the reference one on Linux, and that only uses -D_POSIX_C_SOURCE if you --enable-ansi (which might be broken anyway, to be honest - I don't know why we have that option, it should either be on all the time, or not be an option).
(In reply to comment #24) > (In reply to comment #22) > > As previous, seems the obsolete _POSIX_C_SOURCE works fine on Linux and > > Windows/mingw, so this change try to fix it on FreeBSD, it's relative a > > conservative safe change. > > Windows isn't a POSIX platform, so I would be really surprised if > _POSIX_C_SOURCE does anything there. Good to know, so I think it's saft to drop the _POSIX_C_SOURCE define there? > > The Autotools build system is the reference one on Linux, and that only uses > -D_POSIX_C_SOURCE if you --enable-ansi (which might be broken anyway, to be Yep, I see that, and yes it can't build if we toggle on ansi. > honest - I don't know why we have that option, it should either be on all > the time, or not be an option).
Created attachment 81635 [details] [review] [PATCH v3] cmake: do not bind to any POSIX C standard
Created attachment 81639 [details] [review] [PATCH v4] cmake: do not bind to any POSIX C standard add bug link to commit msg from v3.
OK, finally I googled a way to mark patch obsolete without do that when uploading a new one.
(In reply to comment #27) > [PATCH v4] cmake: do not bind to any POSIX C standard I applied this, with a somewhat shorter commit message. I don't think there's any need to quote pieces of the FreeBSD headers, we can just mention that it's a problem and leave it at that: cmake: do not bind to any particular POSIX C standard This caused build failures on FreeBSD. Defining _POSIX_C_SOURCE to a particular version will disable common non-POSIX extensions like PF_UNIX, and on some systems will also disable features of later POSIX versions, like IPv6. If we don't ask for a specific version, we'll get some sort of sensible default. Fixed in git for 1.7.6, thanks.
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.