Created attachment 45234 [details] Patch loading xml config files with QtXML I wrote a patch to let QtXML parse the config files. (See attached patch). On windows it's big pain to have to many dependencies so this patch reduces dependencies.
How is adding a dependency reducing a dependency? We had issues in the past with supporting two XML libraries and settled on expat because it is small and runs everywhere. If anything I would say just embed expat. For the small amount of code that needs an XML parser it isn't worth having multiple code paths that can cause unseen pain for the maintainers.
(In reply to comment #1) > We had issues in the past with supporting two XML libraries and settled on > expat because it is small and runs everywhere. Does this mean that libxml2 support is obsolate and could be dropped from the code ? I'm asking because i wrote an expat based client config parser http://lists.freedesktop.org/archives/dbus/2010-June/012914.html and do not like to implement libxml2 too. Ralf
(In reply to comment #1) > If anything I would say just embed expat. Embedding expat is a good idea, too. What do the others think? I'd like to provide a patch for it... Is it worth the time?
I no longer represent the D-Bus project so that is something you should ask the maintainers directly. I say - yes go ahead and embed expat but the maintainer needs to be active about any critical exploits that might get fixed upstream. Given expat's codebase, I don't think that is a huge issue. The reason we left libxml in there is because a small number of people complained so we took the position that they could go and fix the code but we are only supporting expat. I don't think anyone fixed the code. I don't think it even compiles but then again it has been awhile since I have compiled D-Bus myself.
BTW. Ralf's example is the prime reason you want to choose one parser. It sucks to have to add code in more than one place to simply add an option to the configuration.
Created attachment 47111 [details] [review] added library expat as an embeded library to remove the dependency Well the task wasn't that big. So here is the patch to embed libexpat.... It compiles pretty well on windows. Could someone test this on other OSes?
Review of attachment 47111 [details] [review]: There should still be a way to switch between embedded and system Expat when running cmake/configure: on Unix, using the system-wide shared library for things like Expat is strongly preferred for security reasons. (With that setup, Linux distribution packagers would usually use the system Expat, Windows or embedded-system packagers would probably usually use the embedded copy, and everyone else could go either way.) ::: cmake/bus/CMakeLists.txt @@ +35,3 @@ ${BUS_DIR}/activation.c ${BUS_DIR}/activation.h + ${BUS_DIR}/bus.c Please don't randomly change whitespace in lines that you haven't otherwise changed, it makes it harder to see the real patch. ::: cmake/expat/CMakeLists.txt @@ +4,3 @@ + +include(CheckFunctionExists) +CHECK_FUNCTION_EXISTS(memmoven HAVE_MEMMOVE) memmove, surely? @@ +9,3 @@ +add_definitions(-DHAVE_EXPAT_CONFIG_H=1) +add_definitions(-DXML_BUILDING_EXPAT=1) +#add_definitions(-DXML_STATIC=1) If we're building an embedded libexpat, it should be built statically and linked into libdbus; it only makes sense to use a shared library if it's the system-wide copy. @@ +44,3 @@ + +install_targets(/lib expat ) +install_files(/include/expat FILES ${EXPAT_LIB_HEADERS}) It doesn't make sense to install libraries or development headers for an embedded copy of Expat. If it's embedded, it should just be an implementation detail of libdbus. ::: expat/expat.h @@ +1,2 @@ +/* Copyright (c) 1998, 1999, 2000 Thai Open Source Software Center Ltd + See the file COPYING for copying permission. You need to include expat's copyright license (perhaps in expat/COPYING), otherwise it's somewhere between inadvisable and illegal. Preferably, include everything from an expat tarball release in the expat directory.
Created attachment 47428 [details] [review] added library expat as an embeded library to remove the dependency Thank you for your review. I implemented your suggestions: * included everything from an expat tarball release in the expat directory * no "beautification" in cmake/bus/CMakeLists.txt * expat is a static library * the embedded expat is only used if no system expat is found via FindLibExpat.cmake remark: The latest change from 6d2b67fc03a2a0de744bc343791d0b13a1cbf301 did't compile for me. So I reverted it partially. +++ b/cmake/CMakeLists.txt @@ -126,7 +126,7 @@ Fix: - set(CMAKE_CXX_WARNING_LEVEL 4 CACHE STRING FORCE) + set(CMAKE_CXX_WARNING_LEVEL 4 CACHE TYPE STRING FORCE) and later - set(CMAKE_CXX_WARNING_LEVEL 3 CACHE STRING FORCE) + set(CMAKE_CXX_WARNING_LEVEL 3 CACHE TYPE STRING FORCE) As this patch exceeds the max attachment size I split it up into two. Part two contains the less important files of the expat tarball
Created attachment 47429 [details] [review] The part 2. containing the less important files of the expat tarball
Created attachment 47436 [details] [review] added library expat as an embeded library to remove the dependency I just tried to compile with VC 2010 and found a few bugs. So here is the version that should work with VC2010 as well.
(In reply to comment #8) > remark: The latest change from 6d2b67fc03a2a0de744bc343791d0b13a1cbf301 did't > compile for me. So I reverted it partially. > > +++ b/cmake/CMakeLists.txt > @@ -126,7 +126,7 @@ > Fix: > - set(CMAKE_CXX_WARNING_LEVEL 4 CACHE STRING FORCE) > + set(CMAKE_CXX_WARNING_LEVEL 4 CACHE TYPE STRING FORCE) > and later > - set(CMAKE_CXX_WARNING_LEVEL 3 CACHE STRING FORCE) > + set(CMAKE_CXX_WARNING_LEVEL 3 CACHE TYPE STRING FORCE) I have no idea how to drive CMake, but omitting TYPE is what's described in the CMake 2.6 and 2.8 documentation <http://www.cmake.org/cmake/help/documentation.html> so I'd rather not revert this without confirmation from Ralf. What version of CMake are you using? Ralf, what version of CMake caused you to need commit 6d2b67fc03a2a, assuming it was previously working?
Review of attachment 47436 [details] [review]: Mostly looks OK. I assume the files in expat/ are all unmodified from the Expat tarball? I'll delete all of expat/ and re-create it from the tarball if I commit this, just to confirm. This still needs configure.ac integration - even if configure.ac is only able to link against a system libexpat, we still need to teach `make dist` to include expat/ in D-Bus release tarballs. I can look into that if you don't know autotools? ::: cmake/CMakeLists.txt @@ +127,3 @@ if (WALL) set(WALL 1 CACHE STRING FORCE) + set(CMAKE_CXX_WARNING_LEVEL 4 CACHE TYPE STRING FORCE) Change to be dropped unless Ralf says it's OK @@ +141,3 @@ endif(CMAKE_C_FLAGS MATCHES "/W[0-4]") else (WALL) + set(CMAKE_CXX_WARNING_LEVEL 3 CACHE TYPE STRING FORCE) Change to be dropped unless Ralf says it's OK @@ +482,1 @@ add_definitions(-DHAVE_CONFIG_H=1) Why did you remove this install_files() call? ::: cmake/expat/CMakeLists.txt @@ +5,3 @@ +configure_file(${CMAKE_SOURCE_DIR}/expat/expat_config.h.cmake ${CMAKE_CURRENT_BINARY_DIR}/expat_config.h) + +add_definitions(-DHAVE_EXPAT_CONFIG_H=1 -DCOMPILED_FROM_DSP=1 ${LIBEXPAT_DEFINITIONS}) What does COMPILED_FROM_DSP mean and why do you define it? ::: expat/Changes @@ +1,1 @@ +Release 2.0.1 Tue June 5 2007 Is the expat/ directory exactly the 2.0.1 release tarball?
(In reply to comment #4) > I say - yes go ahead and embed expat but the maintainer > needs to be active about any critical exploits that might get fixed upstream. I note with some concern that: * I seem to be the only maintainer making numerous commits to dbus at the moment, and I can't guarantee to be this active at any given time * expat hasn't had an upstream release since 2007 despite CVE-2009-2625, CVE-2009-3560 and CVE-2009-3720 (looking at the changelog for the Debian packages), so we'd have to patch it for those vulnerabilities, as well as keeping up with any subsequent portability or security fixes so I'm not sure whether embedding expat in dbus is really such a great idea. Are some of dbus' target OSs really such bad development platforms that "build and install expat" is an onerous requirement? As much as I'm in favour of portability, I think embedding dependencies in each source tarball is no substitute for having a decent build infrastructure on whatever OS you're targeting. Having a useful external build infrastructure that knows about build-time dependencies would benefit everyone, not just D-Bus (all serious Linux distributions have either written such a thing, or adopted someone else's - e.g. RPM, dpkg, Gentoo ebuilds, and so on - so there are plenty to learn from).
I'm using cmake-2.8.4-win32-x86 (In reply to comment #12) > This still needs configure.ac integration - even if configure.ac is only able > to link against a system libexpat, we still need to teach `make dist` to > include expat/ in D-Bus release tarballs. I can look into that if you don't > know autotools? I' not so familiar with autotools, so yes: Please have a look at it. > @@ +482,1 @@ > add_definitions(-DHAVE_CONFIG_H=1) > > Why did you remove this install_files() call? That was not on purpose. Sorry for that. > > ::: cmake/expat/CMakeLists.txt > @@ +5,3 @@ > +configure_file(${CMAKE_SOURCE_DIR}/expat/expat_config.h.cmake > ${CMAKE_CURRENT_BINARY_DIR}/expat_config.h) > + > +add_definitions(-DHAVE_EXPAT_CONFIG_H=1 -DCOMPILED_FROM_DSP=1 > ${LIBEXPAT_DEFINITIONS}) > > What does COMPILED_FROM_DSP mean and why do you define it? This is required to enable the "windows-mode" in the compilation of expat. I presume that the creators of expat thought of "dsp" (which is an project format of Visual Studio 6) being the only way of building for windows. > > ::: expat/Changes > @@ +1,1 @@ > +Release 2.0.1 Tue June 5 2007 > > Is the expat/ directory exactly the 2.0.1 release tarball? Yes
(In reply to comment #13) > Are some of dbus' target OSs really such bad development platforms that "build > and install expat" is an onerous requirement? > > As much as I'm in favour of portability, I think embedding dependencies in each > source tarball is no substitute for having a decent build infrastructure on > whatever OS you're targeting. Having a useful external build infrastructure > that knows about build-time dependencies would benefit everyone, not just D-Bus > (all serious Linux distributions have either written such a thing, or adopted > someone else's - e.g. RPM, dpkg, Gentoo ebuilds, and so on - so there are > plenty to learn from). I'd love to have something like any of those on windows. But there is nothing similar. The only thing that would do the job is http://coapp.org/ which isn't even in beta yet.
(In reply to comment #11) > (In reply to comment #8) > > remark: The latest change from 6d2b67fc03a2a0de744bc343791d0b13a1cbf301 did't > > compile for me. So I reverted it partially. > > > > +++ b/cmake/CMakeLists.txt > > @@ -126,7 +126,7 @@ > > Fix: > > - set(CMAKE_CXX_WARNING_LEVEL 4 CACHE STRING FORCE) > > + set(CMAKE_CXX_WARNING_LEVEL 4 CACHE TYPE STRING FORCE) > > and later > > - set(CMAKE_CXX_WARNING_LEVEL 3 CACHE STRING FORCE) > > + set(CMAKE_CXX_WARNING_LEVEL 3 CACHE TYPE STRING FORCE) > > I have no idea how to drive CMake, but omitting TYPE is what's described in the > CMake 2.6 and 2.8 documentation > <http://www.cmake.org/cmake/help/documentation.html> so I'd rather not revert > this without confirmation from Ralf. What version of CMake are you using? > This issue is already fixed in git.
(In reply to comment #15) > (In reply to comment #13) > > Are some of dbus' target OSs really such bad development platforms that "build > > and install expat" is an onerous requirement? > > > > As much as I'm in favour of portability, I think embedding dependencies in each > > source tarball is no substitute for having a decent build infrastructure on > > whatever OS you're targeting. Having a useful external build infrastructure > > that knows about build-time dependencies would benefit everyone, not just D-Bus > > (all serious Linux distributions have either written such a thing, or adopted > > someone else's - e.g. RPM, dpkg, Gentoo ebuilds, and so on - so there are > > plenty to learn from). > > I'd love to have something like any of those on windows. But there is nothing > similar. > The only thing that would do the job is http://coapp.org/ which isn't even in > beta yet. KDE on windows provides such a build infrastructure, see http://techbase.kde.org/Getting_Started/Build/KDE4/Windows/emerge
(In reply to comment #14) > > ::: cmake/expat/CMakeLists.txt > > @@ +5,3 @@ > > +configure_file(${CMAKE_SOURCE_DIR}/expat/expat_config.h.cmake > > ${CMAKE_CURRENT_BINARY_DIR}/expat_config.h) > > + > > +add_definitions(-DHAVE_EXPAT_CONFIG_H=1 -DCOMPILED_FROM_DSP=1 > > ${LIBEXPAT_DEFINITIONS}) > > > > What does COMPILED_FROM_DSP mean and why do you define it? > > This is required to enable the "windows-mode" in the compilation of expat. I > presume that the creators of expat thought of "dsp" (which is an project format > of Visual Studio 6) being the only way of building for windows. The CMake build system can be used on Unix (at least, it can when someone reviews my patch on Bug #29228), and the autotools build system can be used on Windows (via mingw32 and/or Cygwin, either on Windows or cross-compiling from Unix), so you'll need some other way to select Windows-compatible mode. D-Bus itself has DBUS_UNIX and DBUS_WIN defines, which each build system sets automatically when appropriate.
(In reply to comment #14) > > What does COMPILED_FROM_DSP mean and why do you define it? > > This is required to enable the "windows-mode" in the compilation of expat. I > presume that the creators of expat thought of "dsp" (which is an project format > of Visual Studio 6) being the only way of building for windows. To give up to date informations: - The KDE on windows project has cmake support for expat since 2010 - There is a pending patch for having expat cmake support - see http://sourceforge.net/tracker/?func=detail&aid=3312568&group_id=10127&atid=310127
(In reply to comment #2) > Does this mean that libxml2 support is obsolate and could be dropped from > the code ? Yes, and now we have. \o/ (In reply to comment #13) > As much as I'm in favour of portability, I think embedding dependencies in > each source tarball is no substitute for having a decent build > infrastructure on whatever OS you're targeting. As the person who has done the majority of D-Bus releases in the last couple of years: sorry, I am not willing to do extra maintenance for an embedded copy of Expat, so this is WONTFIX. You'll have to compile your own Expat, which is now our only supported XML library.
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.