<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/117393/">https://git.reviewboard.kde.org/r/117393/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On April 7th, 2014, 6:03 p.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I still do not understand the rational behind the change. Could you please explain why we would want this build option?</pre>
</blockquote>
<p>On April 7th, 2014, 6:10 p.m. CEST, <b>Michael Palimaka</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On April 7th, 2014, 6:20 p.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On April 7th, 2014, 6:23 p.m. CEST, <b>Michael Palimaka</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Can we at least move QtTest dependency to tests directories?</pre>
</blockquote>
<p>On April 7th, 2014, 6:39 p.m. CEST, <b>Johannes Huber</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On April 7th, 2014, 7:28 p.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
</blockquote>
<p>On April 8th, 2014, 2:52 p.m. CEST, <b>Michael Palimaka</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On April 8th, 2014, 4:14 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
<p>On April 8th, 2014, 4:44 p.m. CEST, <b>Michael Palimaka</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Of course we would find it useful, but it really is a bit of a stretch to call such an option Gentoo-specific.</pre>
</blockquote>
<p>On April 8th, 2014, 5:14 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?)</pre>
</blockquote>
<p>On April 9th, 2014, 8:08 a.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On April 9th, 2014, 12:44 p.m. CEST, <b>Johannes Huber</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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</pre>
</blockquote>
<p>On April 9th, 2014, 1:24 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">> 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.</pre>
</blockquote>
<p>On April 9th, 2014, 1:33 p.m. CEST, <b>Johannes Huber</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"># 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)</pre>
</blockquote>
<p>On April 9th, 2014, 1:38 p.m. CEST, <b>Thomas Lübking</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">"no developer will use this key" was meant to be read as "we'll *not* turn off testing"</pre>
</blockquote>
<p>On April 9th, 2014, 2:17 p.m. CEST, <b>Martin Gräßlin</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Consistent build system by a KDE community is a good reason, isnt it?</pre>
<br />
<p>- Johannes</p>
<br />
<p>On April 7th, 2014, 4:50 p.m. CEST, Michael Palimaka wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kwin and Plasma.</div>
<div>By Michael Palimaka.</div>
<p style="color: grey;"><i>Updated April 7, 2014, 4:50 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kwin
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Add option to disable building tests, and move the QtTest dependency to be required only for tests.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Builds. Tests pass (when enabled).</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>CMakeLists.txt <span style="color: grey">(35fb9ac3b0f8506e6f0fd92b48ba60e83524f212)</span></li>
<li>autotests/CMakeLists.txt <span style="color: grey">(475a7a5f9013ed16d37777bc05e9cba2ad033338)</span></li>
<li>kcmkwin/kwincompositing/CMakeLists.txt <span style="color: grey">(8eb170bedd32f04f5d2cc0fbd3079758e6138cc6)</span></li>
<li>kcmkwin/kwincompositing/test/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>libkwineffects/CMakeLists.txt <span style="color: grey">(0544b0d441f3685240160f15e6c9890c8a92fec1)</span></li>
<li>libkwineffects/autotests/CMakeLists.txt <span style="color: grey">(8973545cc21b010f1430cf7df20a29da5b14ab43)</span></li>
<li>tabbox/CMakeLists.txt <span style="color: grey">(76ba3a2499ca142bb82109db9d7239001ed7157e)</span></li>
<li>tabbox/autotests/CMakeLists.txt <span style="color: grey">(4e83fa7524483d64ea149f0eb1818ea9f61cffe0)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/117393/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>