Cmake update patch for dbus master

[Romain Pokrzywka]

Hi Ralf,

Thanks for looking at the patch !
I have some more comments below

Best regards,
Romain

On Friday 05 February 2010 10:35:21 you wrote:
> Romain Pokrzywka schrieb:
> > Hi again,
> >
> > Ok here is the promised patch which updates the CMake build - at last !
>
> very nice :-)
>
> > It's many things actually:
> >
> > - it implements a split between client and internal library like
> > autotools do. Note that the .def file is generated dynamically based on
> > build options,
>
> looks good
>
> > since tests/verbose-mode and others affect the symbols that are exported
> > :-o Anyway I've tested it will all possible combinations and it's fine
> > now. In theory the def.in files (which are combined by cmake) can be
> > shared used by the autotools build too, but I haven't implemented it for
> > autotools yet. Marcus: maybe that's something you could look at ?
> >
> > - it contains fixes for the install target: basically the DBUSDIR
> > wouldn't be taken into account even when specified, and the summary was
> > confusing regarding where things would be going for the install
>
> okay,the logic seems clear now:
>
> first try  DBUSDIR
> then ty DBUSDIR from env
> third try DBUS_INSTALL_DIR
> forth use CMAKE_INSTALL_PREFIX
>
> > - it removes the debug postfix thing for the executables. Debug postfix
> > only makes sense for libraries when you want to distinguish them or
> > package both versions. But for executables you don't need that
> > distinction, you'll only ship release versions anyway. This is how Qt is
> > built and packaged on windows, for example: even when building in debug
> > mode, qmake is still called qmake, same for designer and the others.
>
> which makes problems especial with qt when configuring in
> debug_and_release mode. Is it guarantated to have an executable in a
> specific build mode by the build system ? The answer for qt is no

Well yes, I can positively say that for Qt it is guaranteed that all executables are built in release mode when using 
debug_and_release. It wouldn't make sense otherwise.

If you think of it, the only reason the d postfix exists on windows is for libraries packaging, because each build type 
uses a different CRT, and you want both types to allow the user to link to the library no matter what mode he's building 
his own apps in. But for executables, it's really only the release version we're interested in, it wouldn't make sense 
to ship debug versions IMHO. (in that case the person will build its own from source)

Now the problem I see with cmake is that it doesn't have a debug_and_release mode. The closest you could get would be to 
run a Debug build, and then a Release build on the same build dir. However that's suboptimal because you end up building 
the executables twice, whereas only the lib is really needed for the debug mode in that case. But that's as close as it 
gets to qmake's debug_and_release mode, and without the executable debug postfix you get the same results : both 
libraries (dbus-1.dll and dbus-1d.dll), and only one set of executables in release mode, without any postfix. Of course, 
the risk then is to end up with debug executables if you start by the Release build, then the Debug build after. But 
that should only be a risk for packagers on windows, and I assume these people will now what they are doing (and so far 
it's not many of them anyway, so we can make sure everybody knows about this :) )

> Because this feature is configurable on configure time i would not
> remove it - peoples build chain may depends on this feature.

I understand. But let met argue with that : it's only a change on windows, only for 1.3.x, and it may only affect 
packagers (which is only emerge afaik) and a few others who use dbus-daemond.exe explicitely. The basic developer who 
just wants his single build won't even notice the change. Plus dbus 1.3.x is not an official stable release, so there's 
nothing really preventing us to do it. So why not take the opportunity to make that change now while it's still not a 
big decision ? I can even take care of adjusting the emerge packages if that helps ;)

Again, I really think this option doesn't make sense, and even so, its default value triggers the wrong behavior, ie. by 
default people get the d postfix for the executables when they don't specify otherwise. IMHO at the very least this 
should be the other way around, that people explicitely have to enable it to get the debug postfixes on the executables.

You could argue that it's only a matter of naming so why bother, but unfortunately it's not: there are places where you 
do start dbus-daemon from the code, like dbus-launcher, and we have similar code in KDE as well. I guess you see the 
problem: all of a sudden you have to check if dbus-daemon.exe exists, and if not try dbus-daemond.exe... This is a real 
can of worms, bound to be breaking as soon as somebody adds such code, and for no real benefit in the end.

I hope that'll convince you to change you mind ;)

>
> > - it contains some cleanup in the cmake files: better option scoping,
> > removed obsolete comments, etc.
> >
> > - it makes DAEMON_NAME an option for cmake too, as a way to specify an
> > alternative name for the daemon, and fixes a few places where it wasn't
> > used in the code.
>
> I renamed this to DBUS_DAEMON_NAME to have the same naming scheme as
> other public options (see changed patch 0013 and new patch 0019)
>

Yep, fine with me

> > - it adds remaining parts of an old commit to fix the OSX build in
> > dbus4win which hadn't been committed because some parts failed during
> > merge or were already upstream.
> >
> > I've created the patch using: git format-patch -k --stdout rev1..rev2 >
> > cmake_build_changes.patch so you have the individual commits and
> > diffstats in one same file.
> >
> > I've tested it on freedesktop's dbus master branch with the command "git
> > am -3 -k < cmake_build_changes.patch" and it applied cleanly, so it
> > should be all fine and automatic, without losing the individual commits.
>
> on Windows it was required to convert the patch file to unix eol
>
> > It builds fine with msvc and mingw in various combinations. Only one of
> > them fails: building with mingw with the DBUS_USE_OUTPUT_DEBUG_STRING
> > option, then there's some build error related to swprintf. Doesn't seem
> > harmful, I can look into this afterwards but I'd rather commit the patch
> > now while it applies cleanly.
>
> I could build the patch on msvc without any problems.

Yes msvc was fine for me too. might be a mingw bug, I still haven't looked at it closely.

>
> > Feel free to review and send your questions and comments before
> > committing upstream. I tried making the stuff as good as possible and
> > testing all combinations, but there might be some stuff in there that you
> > don't like. I can also send you categorized patches with only commits
> > specific to each item in the list above, in case you want to apply some
> > but not others.
>
> Otherrwise the patches looks good to me.
>
> Regards
>  Ralf

Thanks !

-- 
Romain Pokrzywka | romain at kdab.com | Certified Qt Software Engineer & Trainer
Klarälvdalens Datakonsult AB, a KDAB Group company
Tel. Sweden (HQ) +46-563-540090, USA +1-866-777-KDAB(5322)
KDAB - Qt Experts - Platform-independent software solutions