> On April 16, 2014, 6:48 p.m., Martin Gräßlin wrote:
> > as all unit tests use ecm_mark_as_test the tests should not be built even
> > if they are included in the CMakeLists.txt. Given that I think that change
> > is not needed at all. If there are tests which do not use ecm_mark_as_test
> > they should get fixed.
> >
> > From documentation:
> >
> > # ECMMarkAsTest
> > # -------------
> > #
> > # Marks a target as only being required for tests.
> > #
> > # ::
> > #
> > # ecm_mark_as_test(<target1> [<target2> [...]])
> > #
> > # This will cause the specified targets to not be built unless either
> > # BUILD_TESTING is set to ON or the user invokes the ``buildtests`` target.
> > #
> > # BUILD_TESTING is created as a cache variable by the CTest module and by
> > the
> > # :kde-module:`KDECMakeSettings` module.
> >
>
> Michael Palimaka wrote:
> That's true (and it seems there's some stuff here and there that should
> use it), but configure will still fail if QtTest is not present.
>
> Martin Gräßlin wrote:
> > configure will still fail if QtTest is not present
>
> how much is that a problem? Do your users not have it installed?
>
> Thomas Lübking wrote:
> given the review description, this will have been a major point, as i
> take it.
>
> "[...] and move the QtTest dependency to be required only for tests"
>
> Michael Palimaka wrote:
> > how much is that a problem? Do your users not have it installed?
> A user wouldn't normally have it installed unless something required it
> or they turned on testing. I apologise, I forgot that qtbase is typically
> kept together (we split it into its various components) so yes the issue this
> addresses is definitely more distro-specific than I originally thought.
> However, since we now know that anything marked with ecm_mark_as_test can be
> turned off, one could argue that it's technically incorrect to search for a
> package that will not be used.
>
> Would you consider having just 'if (BUILD_TESTING) find_package(Qt5Test)
> endif()' in the root?
>
> Hrvoje Senjan wrote:
> just to chip in, as a packager from another distro; we have also splited
> (development) packages for qtbase module (so e.g. libQt5Gui-devel,
> libQt5Network-devel, libQt5Test-devel, etc). also we don't usually build
> testing for packaging purposes, but personally i wont be bothered if they'll
> be required for building KWin(5)
> Would you consider having just 'if (BUILD_TESTING) find_package(Qt5Test)
> endif()' in the root?
I don't like "magic" build options (it's magic in the sense that it's a build
option which isn't defined in the main CMakeLists.txt). So I'd suggest to turn
it the other way around. Make it an optional dependency and set BUILD_TESTING
to FALSE if not found. That's how it's already done for XCB-ICCCM:
# and the optional XCB dependencies
find_package(XCB COMPONENTS ICCCM)
add_feature_info("XCB-ICCCM" XCB_ICCCM_FOUND "Required for building test
applications for KWin")
- Martin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117393/#review55896
-----------------------------------------------------------
On April 7, 2014, 4:50 p.m., Michael Palimaka wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117393/
> -----------------------------------------------------------
>
> (Updated April 7, 2014, 4:50 p.m.)
>
>
> Review request for kwin and Plasma.
>
>
> Repository: kwin
>
>
> Description
> -------
>
> Add option to disable building tests, and move the QtTest dependency to be
> required only for tests.
>
>
> Diffs
> -----
>
> CMakeLists.txt 35fb9ac3b0f8506e6f0fd92b48ba60e83524f212
> autotests/CMakeLists.txt 475a7a5f9013ed16d37777bc05e9cba2ad033338
> kcmkwin/kwincompositing/CMakeLists.txt
> 8eb170bedd32f04f5d2cc0fbd3079758e6138cc6
> kcmkwin/kwincompositing/test/CMakeLists.txt PRE-CREATION
> libkwineffects/CMakeLists.txt 0544b0d441f3685240160f15e6c9890c8a92fec1
> libkwineffects/autotests/CMakeLists.txt
> 8973545cc21b010f1430cf7df20a29da5b14ab43
> tabbox/CMakeLists.txt 76ba3a2499ca142bb82109db9d7239001ed7157e
> tabbox/autotests/CMakeLists.txt 4e83fa7524483d64ea149f0eb1818ea9f61cffe0
>
> Diff: https://git.reviewboard.kde.org/r/117393/diff/
>
>
> Testing
> -------
>
> Builds. Tests pass (when enabled).
>
>
> Thanks,
>
> Michael Palimaka
>
>
_______________________________________________
Plasma-devel mailing list
Plasma-devel@kde.org
https://mail.kde.org/mailman/listinfo/plasma-devel
