I'm not really convinced by cmake as a build system, but Windows people apparently like it, and I said I'd review patches, so... Initially looking at: https://github.com/choeger/dbus-python-cmake
Created attachment 63818 [details] [review] [from choeger] Start cmake integration
Created attachment 63819 [details] [review] [from choeger] Made tests work
Created attachment 63820 [details] [review] [from choeger] Fix Python 3 incompatibilities
Created attachment 63821 [details] [review] [from choeger] MSVC does not support inline, so just #def it
Created attachment 63822 [details] [review] [from choeger] Use malloc instead of variable sized array This is not supported by Microsoft
Created attachment 63823 [details] [review] [from choeger] Find DbusBasicValue Emulate autoconf check
Created attachment 63824 [details] [review] [from choeger] fix check size includes
Created attachment 63825 [details] [review] [from choeger] Use malloc instead of dynamic size array
Created attachment 63826 [details] [review] [from choeger] Add dbus GLIB dependency
Comment on attachment 63821 [details] [review] [from choeger] MSVC does not support inline, so just #def it Review of attachment 63821 [details] [review]: ----------------------------------------------------------------- In general, please be careful with line endings. dbus-python consistently uses Unix (\n) line endings. When you change a file from Unix to Windows line endings, it makes the diff unreadable, because it changes every line in the file: e.g. see <https://github.com/choeger/dbus-python-cmake/commit/9f86d49a11160819c52e13f137f35b3062768458>. For the patches attached here I've used --ignore-space-change to avoid that. ::: _dbus_bindings/dbus_bindings-internal.h @@ +42,5 @@ > #endif > > +#ifdef _MSC_VER > +# define inline __inline > +#endif The Autotools build system automatically[1] defines inline to the right thing for the compiler (inline, _inline, __inline, __inline__ or nothing) in config.h. To give config.h similar functionality in Autotools and CMake, I would prefer this to appear in config.h.cmake - that's what we do in dbus. [1] if you use AC_C_INLINE; I've just committed a patch to do that
Comment on attachment 63822 [details] [review] [from choeger] Use malloc instead of variable sized array Review of attachment 63822 [details] [review]: ----------------------------------------------------------------- I continue to be sad that Microsoft compilers still haven't caught up with C99, a standard 13 years old. That's probably older than some of their users... ::: _dbus_bindings/message-append.c @@ +934,4 @@ > dbus_signature_iter_init(&obj_sig_iter, obj_sig_str); > > { /* scope for variant_iters */ > + DBusMessageIter* variant_iters = malloc(sizeof(DBusMessageIter) * variant_level); If malloc returns NULL (out of memory), you crash shortly after this point. I'll commit a version of this patch that throws an exception with PyErr_NoMemory() instead. @@ +984,1 @@ > goto out; This leaks variant_iters in a couple of error conditions. I'll commit a version that doesn't.
Comment on attachment 63821 [details] [review] [from choeger] MSVC does not support inline, so just #def it Review of attachment 63821 [details] [review]: ----------------------------------------------------------------- ::: CMakeLists.txt @@ +33,5 @@ > > +find_file(stdint stdint.h) > +if(NOT stdint) > + message(SEND_ERROR "You need a C99 compliant stdint.h for windows, use e.g. http://msinttypes.googlecode.com/svn/trunk/stdint.h") > +endif(NOT stdint) I've removed the uses of stdint.h - it turns out none of them were necessary anyway - so this part can go away.
Comment on attachment 63825 [details] [review] [from choeger] Use malloc instead of dynamic size array Review of attachment 63825 [details] [review]: ----------------------------------------------------------------- ::: _dbus_bindings/server.c @@ +99,4 @@ > > /* scope for list */ > { > + char *list = malloc(sizeof(char) * (length + 1)); Here you're replacing a const char *[] (array of constant strings) with a single non-constant string. Inside the loop, that will assign (char) (some string) to each element in the string, then pass the wrong type to dbus_server_set_auth_mechanisms(). I'm surprised that doesn't produce a compiler error, or at least a warning. As with the other patch similar to this, you leak list on certain errors. I've commited a version with those fixed.
Comment on attachment 63822 [details] [review] [from choeger] Use malloc instead of variable sized array I committed something similar, thanks. Should be superseded by this commit: http://cgit.freedesktop.org/dbus/dbus-python/commit/?id=5b79604a6d1eb11268293342d19da633e5eedaa4 Please comment if you spot mistakes in that commit.
Comment on attachment 63825 [details] [review] [from choeger] Use malloc instead of dynamic size array Also replaced by 5b79604
Created attachment 63827 [details] [review] cumulative diff for choeger's branch I think I've reviewed everything that touches existing files, so it's easier to review the cmake stuff all in one go.
Comment on attachment 63827 [details] [review] cumulative diff for choeger's branch Review of attachment 63827 [details] [review]: ----------------------------------------------------------------- Find*.cmake could do with a copyright notice and license. Did you write them yourself? If not, where did you get them? ::: CMakeLists.txt @@ +1,1 @@ > +project(dbus_python CXX C) I don't think we have any C++? @@ +6,5 @@ > +set(PACKAGE "dbus-python") > +set(PACKAGE_BUGREPORT "http://bugs.freedesktop.org/enter_bug.cgi?product=dbus&component=python") > +set(PACKAGE_NAME "dbus-python") > +set(PACKAGE_URL "") > +set(PACKAGE_VERSION "1.1.0") This is going to get out-of-sync with configure.ac. I'm not sure how best to solve that. @@ +13,5 @@ > + > +include (CheckTypeSize) > + > +#DBUS > +find_package(DBUS REQUIRED) Is there no way to search for a minimum version? dbus-python requires dbus 1.4 or better (and will require 1.6 or better at some point). @@ +38,5 @@ > + > +find_file(stdint stdint.h) > +if(NOT stdint) > + message(SEND_ERROR "You need a C99 compliant stdint.h for windows, use e.g. http://msinttypes.googlecode.com/svn/trunk/stdint.h") > +endif(NOT stdint) Shouldn't be necessary since commit f3199102 ::: cmake/FindDBUS.cmake @@ +1,1 @@ > +FIND_PATH( DBUS_INCLUDE_DIR dbus/dbus.h PATHS ${CMAKE_INCLUDE_PATH}/dbus-1.0 /usr/include/dbus-1.0) One of the things I really don't like about cmake is that you seem to have to write a new FindFoo.cmake (or cargo-cult one from another project) for *every* *single* *library*. In the bad old days, autotools had the same problem - every library had its own whatever.m4 - but pkg-config solved that. Is there no way to tell cmake "this is a perfectly normal library using pkg-config, it's called dbus-1, do the work for me"? If that's not possible, please use pkg-config in this file, at least on Unix (to be honest I'd prefer pkg-config to be the first option on Windows too, only falling back to library-specific knowledge if pkg-config isn't found). I believe telepathy-qt has Find*.cmake for dbus and dbus-glib that understand pkg-config and try that first. @@ +1,2 @@ > +FIND_PATH( DBUS_INCLUDE_DIR dbus/dbus.h PATHS ${CMAKE_INCLUDE_PATH}/dbus-1.0 /usr/include/dbus-1.0) > +FIND_PATH( DBUS_INCLUDE_LIB_DIR dbus/dbus-arch-deps.h PATHS ${CMAKE_LIBRARY_PATH}/dbus-1.0/include /usr/lib/dbus-1.0/include /usr/lib64/dbus-1.0/include) This will fail on Debian-style multiarch systems (Debian 7 and recent Ubuntu), I think - on those systems, D-Bus is in /usr/lib/x86_64-linux-gnu/dbus-1.0/include or something. At the risk of sounding like a broken record, using pkg-config would solve this problem. @@ +12,5 @@ > +IF(NOT DBUS_GLIB_LIBRARY) > + MESSAGE(ERROR "Could not find lib dbus-glib-1") > +ENDIF(NOT DBUS_GLIB_LIBRARY) > + > +IF( DBUS_INCLUDE_DIR AND DBUS_INCLUDE_LIB_DIR AND DBUS_GLIB_INCLUDE_DIR AND DBUS_LIBRARY AND DBUS_GLIB_LIBRARY ) ANOTHER THING I DON'T LIKE ABOUT CMAKE IS HOW EVERYTHING IS IN UNREADABLE CAPITAL LETTERS :-) ::: cmake/FindGLIB.cmake @@ +20,5 @@ > + /usr/local/lib > + ) > + > +else (GLIB_PKG_FOUND) > + # Find Glib even if pkg-config is not working (eg. cross compiling to Windows) pkg-config can work perfectly well when cross-compiling to Windows, you just set PKG_CONFIG_LIBDIR suitably. Here is my cross-compiler setup for dbus targeting Windows: ./configure \ --enable-maintainer-mode --enable-developer \ --build=x86_64-pc-linux-gnu --host=i686-w64-mingw32 \ build_alias=x86_64-pc-linux-gnu host_alias=i686-w64-mingw32 \ --prefix=/opt/i686-w64-mingw32 \ LDFLAGS=-L/opt/i686-w64-mingw32/lib \ CPPFLAGS=-I/opt/i686-w64-mingw32/include \ PKG_CONFIG_LIBDIR=/opt/i686-w64-mingw32/lib/pkgconfig \ PKG_CONFIG_PATH= ::: config.h.cmake @@ +1,1 @@ > +/* config.h.in. Generated from configure.ac by autoheader. */ This line is no longer true. @@ +9,5 @@ > +/* dbus-python minor version */ > +#cmakedefine DBUS_PYTHON_MINOR_VERSION > + > +/* Define to 1 if you have the <dlfcn.h> header file. */ > +#cmakedefine HAVE_DLFCN_H Does cmake automatically turn things like this into a configure-like check, or did you just copy this from config.h.in? There's no point in having macros in config.h.cmake if they're never actually used in the source code, particularly if cmake doesn't do the appropriate checks to set their values correctly. I suspect you only actually need 5-10 #cmakedefines. config.h.in is (as you may have seen from the first line) an auto-generated file. @@ +73,5 @@ > +/* Version number of package */ > +#cmakedefine VERSION "@VERSION@" > + > +/* Do we have DBusBasicValue? */ > +#cmakedefine HAVE_DBUSBASICVALUE Is this actually checked, or does it just always get undefined? ::: test/run-test.sh @@ +91,2 @@ > > +#${MAKE:-make} -s cross-test-server > "$DBUS_TOP_BUILDDIR"/test/cross-server.log& If you're going to delete a line, please delete it, don't just comment it out. But yes, this looks valid; I'll apply this bit.
(In reply to comment #17) > Comment on attachment 63827 [details] [review] [review] > cumulative diff for choeger's branch > > @@ +6,5 @@ > > +set(PACKAGE "dbus-python") > > +set(PACKAGE_BUGREPORT "http://bugs.freedesktop.org/enter_bug.cgi?product=dbus&component=python") > > +set(PACKAGE_NAME "dbus-python") > > +set(PACKAGE_URL "") > > +set(PACKAGE_VERSION "1.1.0") > > This is going to get out-of-sync with configure.ac. I'm not sure how best to > solve that. at least for the version part there is already a cmake makro in dbus git repo which extracts version information from autoconf config file and set related cmake variables see http://cgit.freedesktop.org/dbus/dbus/tree/cmake/modules/MacrosAutotools.cmake
-- GitLab Migration Automatic Message -- This bug has been migrated to freedesktop.org's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.freedesktop.org/dbus/dbus-python/issues/14.
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.