<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/122206/">https://git.reviewboard.kde.org/r/122206/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 17th, 2015, 4:37 a.m. CET, <b>Albert Vaca Cintora</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 know this is merged already but this patch is being applied to every KDE package and I want to keep the discussion in a single place.
We already have a toggle option in CMake that is "BUILD_TESTING". If Gentoo wants to not build the tests (I'm not judging if they should, let them be free to do it), they can just set BUILD_TESTING to OFF. I understand that CMake will still try to find Qt5Test and fail, but here is where I think we got it wrong:
This patch does the following:
if (Qt5Test is not found)
BUILD_TESTING = OFF
What I think this patch should be doing is this:
if (BUILD_TESTING == OFF)
Don't look for Qt5Test
Did I miss something or this seems more reasonable to you guys as well?</pre>
</blockquote>
<p>On March 17th, 2015, 1:59 p.m. CET, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One of the original versions of these test patches looked something like:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">if (BUILD_TESTING)
add_subdirectory(autotests)
endif ()
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">find_package(Qt5Test)</code> inside the autotests directory. While this is used a bit in frameworks, it was rejected from a lot of plasma packages because it relies on a magic variable (although it is defined by ECM).</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As a result there's a whole range of approaches across frameworks/plasma/apps all doing the same thing. It would be nice if we could agree on something and be consistent.</p></pre>
</blockquote>
<p>On March 17th, 2015, 6:48 p.m. CET, <b>Albert Vaca Cintora</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;">Is that the reason why we are doing it this way? :/ I don't think this is a magic variable at all, and if you want it to be even less magic you can set it in advance in a line before the if.</pre>
</blockquote>
<p>On March 17th, 2015, 8:32 p.m. CET, <b>Albert Astals Cid</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't like any of the patches, but i prefer Albert's suggestion way over the automagic disabling of the tests. If you don't want tests, just manually specify it.</p></pre>
</blockquote>
<p>On March 18th, 2015, 2:52 a.m. CET, <b>Ben Cooksley</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 long as the variable name is consistent across all packages, and specifying it to on forces Qt5Test to be searched for i'm happy.
Albert's above proposal therefore makes more sense from a "less magic" point of view.
Any package which deviates from the consistent name (at this time: BUILD_TESTING) won't have test coverage on the CI system.</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">@Ben why? It is enabled by default anyway.</p></pre>
<br />
<p>- Aleix</p>
<br />
<p>On February 7th, 2015, 1:14 a.m. CET, Andreas Sturmlechner wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for KDE Frameworks.</div>
<div>By Andreas Sturmlechner.</div>
<p style="color: grey;"><i>Updated Feb. 7, 2015, 1:14 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kio
</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">[kio] Make tests optional
This is a small patch to CMakeLists.txt to only depend on Qt5Test if BUILD_TESTING.</p></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">(c1ed03f6cac648517828aec60e896baf9fbcfd9d)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/122206/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>