<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/125139/">https://git.reviewboard.kde.org/r/125139/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 10th, 2015, 7:42 p.m. CEST, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'm wary about this change as it technically breaks module compatibility. Specifically, upgrading e-c-m could cause packages that formerly built to suddenly stop building, and they wouldn't even have any way to fix that. extragear/base/wacomtablet is an example of such a project.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I understand the wish to discourage cavalier use of XINPUT, though. Printing a warning if XINPUT was specifically requested is an option, but there's no way to know if someone calling <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">find_package(XCB)</code> will make use of the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">XCB_XINPUT_FOUND</code> variable.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">As a compromise, I'd be willing to consider a solution where XINPUT wasn't in the default set of things to search for, but could be specified explicitly (and that could be accompanied by a warning if you like). Note that this would require extending <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ecm_find_package_parse_components</code> to allow a separate list of default components.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">That solution shouldn't break anything too much, because if a project is not explicitly specifying components, then they are optional by default, so such a project will already need to cope in some way with XINPUT not being found (unless they are just letting <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">feature_summary</code> enforce the requirement).</p></pre>
</blockquote>
<p>On September 11th, 2015, 8:16 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;"><blockquote style="text-rendering: inherit;padding: 0 0 0 1em;border-left: 1px solid #bbb;white-space: normal;margin: 0 0 0 0.5em;line-height: inherit;">
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">they wouldn't even have any way to fix that. extragear/base/wacomtablet is an example of such a project.</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">yes, that's my aim here. I do want to explicitly break wacomtablet, because currently it's just implicitly broken - which is worse IMHO. For the record: wacomtablet triggered this review request.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I would also not say that this is a breakage, but a bug fix. It's a bug that ecm finds xinput in the first place.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">My suggestion as a comporomise is to add an option to allow finding it:
set(ECM_XCB_I_KNOW_XINPUT_IS_NOT_STABLE_AND_WONT_WORK_ON_DISTROS TRUE)</p></pre>
</blockquote>
<p>On September 12th, 2015, 11:24 p.m. CEST, <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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I disagree. If FindXCB.cmake were going in for the first time, then yes, this change would be fine. However, the problem with compatibility guarantees is that you have to live with your mistakes. I'm not willing to have an update of ECM break a use that was previously documented to work, no matter how misguided you believe that use was.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I don't even really like removing it from the default set of components to search for, TBH, but I'm willing to accept it in the circumstances. That and a big fat warning when you explicitly specify XINPUT will have to be sufficient.</p></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;">Ok, I'll discard the review. Unfortunately I don't know how to go forward as my CMake knowledge is not sufficient enough to add such a warning.</p></pre>
<br />
<p>- Martin</p>
<br />
<p>On September 10th, 2015, 4:52 p.m. CEST, Martin Gräßlin 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 Extra Cmake Modules, Alex Merry and Harald Sitter.</div>
<div>By Martin Gräßlin.</div>
<p style="color: grey;"><i>Updated Sept. 10, 2015, 4:52 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
extra-cmake-modules
</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;">The XCB bindings for XINPUT are not finished and explictly disabled
in XCB, see for xcb 1.11 [1].
Because of that most distributions do not include xinput. Trying to find
xinput only creates problems - the distro of the user might provide it
and they might not be aware that it's not available normally.
Thus let's not pretend it exists. Make life of everybody easier.
[1] http://cgit.freedesktop.org/xcb/libxcb/tree/configure.ac?id=1.11#n234</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>find-modules/FindXCB.cmake <span style="color: grey">(b2a800f73058382ee84f7d93132a96f8852b4aa7)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/125139/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>