<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="http://git.reviewboard.kde.org/r/110870/">http://git.reviewboard.kde.org/r/110870/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On June 7th, 2013, 5:14 p.m. UTC, <b>Daniele E. Domenichelli</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="http://git.reviewboard.kde.org/r/110870/diff/1/?file=149002#file149002line18" 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="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">18</font></th>
<td bgcolor="#c5ffc4" 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">AccountsQt</span><span class="p">)</span></pre></td>
</tr>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">19</font></th>
<td bgcolor="#c5ffc4" 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">SignOnQt</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;">since these are optional, perhaps use macro_optional_find_package?</pre>
</blockquote>
<p>On June 7th, 2013, 5:23 p.m. UTC, <b>Aleix Pol Gonzalez</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 wouldn't do that. It's only use is to explicitly disable the finding from the cache.
That said, if it was me, I'd make them REQUIRED.</pre>
</blockquote>
<p>On June 7th, 2013, 5:45 p.m. UTC, <b>David Edmundson</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;">Being able to enable/disable independently would be a bad thing. AccountsQt without SignOnQt or vice versa would be really bad, so we would have to manually write our own combo of OPTION and FIND_PACKAGE.. and that's probably too much effort for the reward.
Adding REQUIRED would be really bad as that half our distributions wouldn't be able to compile and everyone would be stuck on 0.6 forever.
I'm fine with this as-is.</pre>
</blockquote>
<p>On June 7th, 2013, 6:04 p.m. UTC, <b>Àlex Fiestas</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;">Dropping it then.</pre>
</blockquote>
<p>On June 7th, 2013, 11:21 p.m. UTC, <b>Daniele E. Domenichelli</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 also useful to print a pretty output about the packages found and not found using macro_log_feature, I think it's quite useful to detect which features won't be build because some package was not found.
Being able to enable/disable them independently would just mean that if one of the 2 libraries, HAVE_SSC would be disabled, exactly like if you had just one library installed. The only check required is "if(ACCOUNTSQT_FOUND AND SIGNONQT_FOUND)" that is already there. So I don't see any real difference, except that if you don't have the required libraries you will see it in the output.</pre>
</blockquote>
<p>On June 7th, 2013, 11:36 p.m. UTC, <b>David Edmundson</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;">We still get
-- Could NOT find accountsqt (missing: ACCOUNTSQT_LIBRARY ACCOUNTSQT_INCLUDE_DIR)
-- Could NOT find signonqt (missing: SIGNONQT_LIBRARY SIGNONQT_INCLUDE_DIR)
But you're right, it doesn't do that nice:
-- The following OPTIONAL packages have not been found:
* accountsqt , AccountQt , <http://SomePlace.com>
Support for WebAccounts
Which would be useful to allow people to actually use this.</pre>
</blockquote>
<p>On June 7th, 2013, 11:39 p.m. UTC, <b>David Edmundson</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;">Edit:
Which have nothing to do with find_package vs macro_optional_find_package it's part of
set_package_properties()
which properly is worth including</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;">Will do.</pre>
<br />
<p>- Àlex</p>
<br />
<p>On June 7th, 2013, 6:03 p.m. UTC, Àlex Fiestas wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://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 Telepathy.</div>
<div>By Àlex Fiestas.</div>
<p style="color: grey;"><i>Updated June 7, 2013, 6:03 p.m.</i></p>
<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;">Adds Accounts-SSO support to ktp-auth-handler by checking if the account has a StorageIdentifier (which we assume comes from Accounts) and X-FACEBOOK-PLATFORM mechanism exsts.
We'll probably have to implement something special for Google as well.</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;">Tested with and without support, it builds and works in both cases.</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">(6c51903)</span></li>
<li>cmake/modules/FindAccountsQt.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>cmake/modules/FindSignOnQt.cmake <span style="color: grey">(PRE-CREATION)</span></li>
<li>getcredentialsjob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>getcredentialsjob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>sasl-auth-op.h <span style="color: grey">(6e0e5ef)</span></li>
<li>sasl-auth-op.cpp <span style="color: grey">(b745234)</span></li>
<li>x-telepathy-sso-operation.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>x-telepathy-sso-operation.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/110870/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>