<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/115863/">https://git.reviewboard.kde.org/r/115863/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 19th, 2014, 3:21 p.m. UTC, <b>Alex Merry</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="https://git.reviewboard.kde.org/r/115863/diff/1/?file=244733#file244733line23" style="color: black; font-weight: bold; text-decoration: underline;">CMakeLists.txt</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">23</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5Co<span class="hl">reAddons</span></span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">22</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">find_package</span><span class="p">(</span><span class="s">KF5Co<span class="hl">mpletion</span></span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">24</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5<span class="hl">GuiAddons</span></span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">23</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">find_package</span><span class="p">(</span><span class="s">KF5<span class="hl">Config</span></span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">25</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5DBusAddons</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">26</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5Service</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">27</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5WindowSystem</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span> <span class="err">#</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">28</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5Crash</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">29</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5Init</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
<tr>
<th bgcolor="#ebb1ba" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">30</font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5JS</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#ebb1ba" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#ffc5ce" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#f0f0f0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">31</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5ConfigWidgets</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#f0f0f0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">24</font></th>
<td bgcolor="#ffffff" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">find_package</span><span class="p">(</span><span class="s">KF5ConfigWidgets</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">32</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="nb">find_package</span><span class="p">(</span><span class="s">KF5<span class="hl">ItemView</span>s</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">25</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">find_package</span><span class="p">(</span><span class="s">KF5<span class="hl">CoreAddon</span>s</span> <span class="o">${</span><span class="nv">KF5_VERSION</span><span class="o">}</span> <span class="s">REQUIRED</span><span class="p">)</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">KCompletion, KConfig[Widgets] and KCoreAddons are used, but never linked against. So there's not much point searching for them: we're already depending on them being bought in by other libraries.</pre>
</blockquote>
<p>On February 20th, 2014, 10:41 a.m. UTC, <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;">The listed frameworks are directly used. I don't see how linking or not makes a difference - khtml will fail to build without them. If we trust that one dependency will always bring in some other dependencies that we happen to also use, I am sure there are others we could remove too.</pre>
</blockquote>
<p>On February 20th, 2014, 11:09 a.m. UTC, <b>Alex Merry</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, my view is that you can either find them and link against them (explicit dependencies), or not find or link against them (implicitly trust that the libraries you *do* link against bring them in). Finding them and not linking against them just seems a bit pointless.</pre>
</blockquote>
<p>On February 20th, 2014, 11:39 a.m. UTC, <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;">A dependency can still be explicit even though a binary link isn't required, for example:
src/rendering/render_form.cpp:#include <kservice.h>
src/rendering/render_form.cpp: KService::List providers = KServiceTypeTrader::self()->query("SearchProvider");
src/rendering/render_form.cpp: foreach (const KService::Ptr &provider, providers) {
If you feel that strongly about it though, I'll drop those changes. They additions don't affect me at all, I just was aiming for a 'correct' change since I was touching this framework anyway.</pre>
</blockquote>
<p>On February 20th, 2014, 11:46 a.m. UTC, <b>Alex Merry</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;">Yes, but this code won't work unless you link against KF5::Service, either explicitly or implicitly. If it's explicit, you should also have find_package(KF5Service). If it's implicit, you're depending on one of your explicit dependencies including KF5::Service in its link interface anyway, in which case that dependency will also pull in the KF5Service package.
I mean, in the end it doesn't matter that much. I'm not going to say "don't ship it unless you do this". But I think that searching for packages in CMake and then not making any use of those packages in CMake (even if it doesn't make any difference to whether the package is required) just clutters things up unnecessarily.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Sorry, I just realised I misunderstood your original comment. The final product does actually link to KCompletion etc. but indeed they are not explicitly marked that way. I guess for the case of KCompletion the link is happening anyway because it is a public dependency of KTextWidgets.</pre>
<br />
<p>- Michael</p>
<br />
<p>On February 18th, 2014, 8:01 a.m. UTC, 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 KDE Frameworks and Martin Tobias Holmedahl Sandsmark.</div>
<div>By Michael Palimaka.</div>
<p style="color: grey;"><i>Updated Feb. 18, 2014, 8:01 a.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
khtml
</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;">- QtTest is only required for autotests
- QtX11Extras is only required for X11 builds, and for tests
- Remove KCrash, KDBusAddons, KGuiAddons, KInit, and KItemViews as they are not used</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.</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">(3a3dbab90e6572cf953ba5edc1fcb60a7e30b493)</span></li>
<li>autotests/CMakeLists.txt <span style="color: grey">(33f1ecb7ba65f223baef242eb21cd34457b020da)</span></li>
<li>tests/CMakeLists.txt <span style="color: grey">(8fc438fa932ec43492b6c2a8e5bc8f0337550d1a)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/115863/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>