On Thursday 07 August 2008, Andreas Pakulat wrote: > On 07.08.08 22:11:11, Thiago Macieira wrote: > > Andreas Pakulat wrote: > > >On 07.08.08 21:32:32, Thiago Macieira wrote: > > >> Andreas Pakulat wrote: > > >> >On 07.08.08 19:59:52, Thiago Macieira wrote: > > >> >> Andreas Pakulat wrote: > > >> >> >Hi, > > >> >> > > > >> >> >since we require Cmake 2.6 now I think we should support CMake's > > >> >> >"integrated" way of version-checking. CMake 2.6 supports this: > > >> >> > > > >> >> >find_package(KDE4 4.2.0) > > >> >> > > > >> >> >and I'm proposing the attached patch to make this work. This > > >> >> > removes the need for setting an extra variable before calling > > >> >> > find_package, still KDE_MIN_VERSION is given authority over the > > >> >> > cmake-way for backwards compatibility reasons. > > >> >> > > > >> >> >Objections against comitting this? > > >> >> > > >> >> Are you sure? This reads Boost_*. > > >> > > > >> >Damn, sorry. Here's the right patch :) > > >> > > > >> >There's one drawback of this whole thing I just noticed: When > > >> > somebody puts: > > >> > > > >> >find_package(KDE4 4 REQUIRED) > > >> > > > >> >in his CMakeLists.txt that won't work unless he runs KDE5. I'm not > > >> > sure how to best fix this, one way would be to require at least > > >> > MAJOR+MINOR version and else use 3.9 as default. The other would be > > >> > to "0" as default value for minor. > > >> > > >> Then I don't approve of your patch :-) > > >> > > >> If the user didn't set anything, the minimum version is 4.0.0 (no one > > >> should be using pre-4.0 releases anymore). > > > > > >Ok, I was thinking the same. > > > > > >> If the major is set, it has to be 4. Otherwise, error out. > > >> > > >> If the minor isn't set, set min version to 4.0.0. If it is set, set > > >> minimum version to 4.x.0. > > >> > > >> We shouldn't need dependencies on patch-level releases. > > > > > >This has a problem, I can't depend on KDE 4.2.0 until its released and > > >hence modules in trunk/ cannot have a dependency on kdelibs from trunk/ > > > > > >For example kdevplatform now has a hard dependency on kdelibs from > > >trunk/, so what I need is require KDE >= 4.1.60 (current version used in > > >trunk/) not 4.2.0 as that doesn't exist yet and 4.1.0 doesn't work > > >anymore. > > > > Right, sorry. I hadn't thought of pre-releases. > > Thanks for doing the logic-thinking for me :) Patch attached, it uses a > negation of your pseudo-code to avoid having to create new variables. > > Andreas
Looks good from the technical side. Still there is one thing: right now the way to specify the required KDE version is to set the KDE_MIN_VERSION variable before calling find_package(KDE4). This patch now adds a second way how to specify the required version. Pro: -this is how find_package() now supports specifying the minimum version number, i.e. in some way the officially recommended way Con: -with the patch there are now two ways how to do the same thing (and the old way has to stay, in order to stay source compatible) -several other modules also support FOO_MIN_VERSION or similar variables, so using such a variable is currently kind of de-facto standard for specifying the minimum version. Comments ? Alex _______________________________________________ Kde-buildsystem mailing list [email protected] https://mail.kde.org/mailman/listinfo/kde-buildsystem
