Review Request 117393: Make building tests optional

Wed Apr 9 12:17:20 UTC 2014

> On April 7, 2014, 6:03 p.m., Martin Gräßlin wrote:
> > I still do not understand the rational behind the change. Could you please explain why we would want this build option?
> 
> Michael Palimaka wrote:
>     It's just nice to have, useful for some people, not useful for others. It's not a new option, it's been provided by ECM for over a year and of course KDE4_BUILD_TESTS was floating around before that.
> 
> Martin Gräßlin wrote:
>     right, but it was a deliberate decision to not honor KDE4_BUILD_TESTS in kde-workspace/kwin. So I'm just wondering what's the usecase or better put the advantage for kwin development to have a build option to disable testing. Personally I don't see any. If your a developer you to run the tests, thus it should be enabled. If you are a distro you want to run the tests as part of your package building process. So who would benefit from such a build option?
> 
> Michael Palimaka wrote:
>     Can we at least move QtTest dependency to tests directories?
> 
> Johannes Huber wrote:
>     As a source distro the tests run uncoditionally (on end user boxes). So there is a benefit and please follow the KF5 standardized way of doing things. Thanks in advance.
> 
> Martin Gräßlin wrote:
>     > Can we at least move QtTest dependency to tests directories?
>     
>     To which one? There are multiple autotests subdirectories. Also the XCB::ICCCM dependency is not in the tests but in the root CMakeLists.txt and I like it that way because all XCB dependencies are defined at one place.
>     
>     > So there is a benefit
>     
>     The benefit has to be weight against the costs. I see an additional cost on maintenance because we have to remember to add the build dependency whenever a new subdirectory gets tests or autotests. I see here a high breakage risk which I do not want to bear (been there with broken build options - remember OpenGL build option?). If this is really just a build option to suit the needs of one distribution I think the general rule of no distro-specific patches has to apply.
>     
>     > please follow the KF5 standardized way of doing things
>     
>     This is not a KF5 library, thus any rules for KF5 do not apply to KWin. You might notice that we also don't have a src/ subdirectory.
> 
> Michael Palimaka wrote:
>     Well the zero maintenance option is to leave everything where it is making the test-specific stuff conditional on BUILD_TESTING. I am really having trouble understanding why doing this is so offensive, given that it has zero impact on people that don't use it and having such an option is a fairly common thing.
> 
> Thomas Lübking wrote:
>     Martin wrote:
>     > what's the usecase or better put the advantage for kwin development to have a build option to disable testing.
>     
>     I'd say it's a gentoo specific thing, since users build kwin, but can not make use of tests etc. since they're users of the distro - not its maintainers nor (in any case) developers or interested in kwin development.
>     Building/running the tests will only slow down their update process.
>     
>     And, surprise: Michael & Johannes both have gentoo mail addresses ;-P
> 
> Michael Palimaka wrote:
>     Of course we would find it useful, but it really is a bit of a stretch to call such an option Gentoo-specific.
> 
> Thomas Lübking wrote:
>     Well, I see the usecase for source based distros - got another?
>     
>     If not, the bottom line of http://distrowatch.com/search.php?category=Source-based is "makes sense for gentoo" - that's not meant as disqualification, but to point the reason (it's also of use for gobolinux users, but what exactly is a "gobolinux" then?)
> 
> Martin Gräßlin wrote:
>     The point is that this is a build option which doesn't make sense during development. For development the tests need to be built, the same in the CI system. Thus it's a build option nobody working on KWin will use. This bears a high risk of breakage. We have been there with the optional OpenGL dependency, which was basically always broken because nobody used it.
>     
>     I try to learn from the mistakes from the past. Having a build option which nobody uses, sounds like a very bad idea to me. My aim as the maintainer is to do decisions which are best for the development of KWin. It's certainly not to please every stake holder.
> 
> Johannes Huber wrote:
>     Dear Martin, 
>     
>     as you already noticed that the test are only usefull for developers, CI systems and users interested in tests. 
>     
>     The proposed changes from Michael:
>     
>     
>     1. Will NOT disrupt your workflow:
>     
>     https://projects.kde.org/projects/kdesupport/extra-cmake-modules/repository/revisions/master/entry/kde-modules/KDECMakeSettings.cmake
>     
>     # TEST
>     #
>     #   Testing is enabled by default, and an option (BUILD_TESTING) is
>     #   provided for users to control this. See the CTest module
>     #   documentation in the CMake manual for more details.
>     #
>     #   This section can be disabled by setting
>     #     KDE_SKIP_TEST_SETTINGS
>     #   to TRUE before including this module.
>     
>     2. Will HELP all source based distro not to pull in unconditionally a new dependency (qttest)
>     
>     3. Will HELP to not execute tests unconditionally
>     
>     4. Upstreaming is the way to go as you already written down in blog post
>     http://blog.martin-graesslin.com/blog/2013/10/how-code-flows-about-upstream-and-downstream/
>     
>     So makes upstream buildsystem patching in downstream sense in this case?
>     
>     Greetings,
>     Johannes
> 
> Thomas Lübking wrote:
>     > 1. Will NOT disrupt your workflow:
>     
>     You're missing his point.
>     
>     Martins concern is that realistically no developer will use this key.
>     We therefore might easily introduce changes that break building w/o BUILD_TESTING and those would be released unnoticed, ie. the key would not be maintained, thus virtually not exist.
> 
> Johannes Huber wrote:
>     # Testing is enabled by default
>     
>     > option(BUILD_TESTING "Build the testing tree." ON)
>     
>     Just for the record it is enabled by default. (Even when it is not set)
> 
> Thomas Lübking wrote:
>     "no developer will use this key" was meant to be read as "we'll *not* turn off testing"

exactly, the point is that we would introduce a build option which looks to your users like we support it. Which is not the case, we cannot guarantee that this option is doing what it's supposed to do. We would not be able to guarantee that KWin compiles (unlikely as it's tests, but bugs happens, right?) and we would not be able to guarantee that it properly excludes all tests, which would turn the build option into a joke.

We made bad experiences with contributed features which increased the maintenance costs. From this experience we derived some rules to decide whether a new feature can be accepted. Please see: http://community.kde.org/KWin/Mission_Statement#Developer_Perspective

Our experience is that we can only include features we are willing and able to maintain.

In this case I just fail to see the advantage in having the build option. The costs of maintenance are to me higher than the possible benefits. To me it looks like solving the very special need of Gentoo users, but adding costs for us on development side. As you linked my blog post I can also quote it: "Nevertheless not all code should be upstreamed. If the code is needed for the downstream integration task it needs to be kept downstream."

Obviously if you present a good reason on how this would help KWin development or how it is not just relevant for Gentoo, I can be convinced that the benefits of this build option outweight the costs.

- Martin

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117393/#review55183
-----------------------------------------------------------

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
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/plasma-devel/attachments/20140409/91b6d872/attachment-0001.html>