<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/121544/">https://git.reviewboard.kde.org/r/121544/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On December 16th, 2014, 3:34 p.m. CET, <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="https://git.reviewboard.kde.org/r/121544/diff/1/?file=333443#file333443line155" style="color: black; font-weight: bold; text-decoration: underline;">cmake/modules/TelepathyDefaults.cmake</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">155</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">set</span><span class="p">(</span><span class="s">LIB_SUFFIX</span> <span class="s2">""</span> <span class="s">CACHE</span> <span class="s">STRING</span> <span class="s2">"Define suffix of library directory name (32/64)"</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">155</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">set</span><span class="p">(</span><span class="s">LIB_DESTINATION</span> <span class="s2">"${CMAKE_INSTALL_FULL_LIBDIR}"</span> <span class="s">CACHE</span> <span class="s">PATH</span> <span class="s2">"The subdirectory where libraries will be installed (default is ${CMAKE_INSTALL_PREFIX}/lib${LIB_SUFFIX})"</span> <span class="s">FORCE</span><span class="p">)</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">156</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">set</span><span class="p">(</span><span class="s"><span class="hl">LIB</span>_INSTALL_DIR</span><span class="hl"> </span><span class="s2"><span class="hl">"lib${LIB_SUFFIX}"</span></span><span class="hl"> </span> <span class="s">CACHE</span> <span class="s">PATH</span> <span class="s2">"The subdirectory where <span class="hl">librari</span>es will be installed (default is ${CMAKE_INSTALL_PREFIX}/<span class="hl">lib${LIB_SUFFIX}</span>)"</span> <span class="s">FORCE</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">156</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">set</span><span class="p">(</span><span class="s"><span class="hl">INCLUDE_DESTINATION</span></span><span class="hl"> </span><span class="s2"><span class="hl">"${CMAKE</span>_INSTALL_<span class="hl">FULL_INCLUDE</span>DIR<span class="hl">}"</span></span> <span class="s">CACHE</span> <span class="s">PATH</span> <span class="s2">"The subdirectory where <span class="hl">header fil</span>es will be installed (default is ${CMAKE_INSTALL_PREFIX}/<span class="hl">include</span>)"</span> <span class="s">FORCE</span><span class="p">)</span></pre></td>
</tr>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">157</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">set</span><span class="p">(</span><span class="s"><span class="hl">INCLUD</span>E_INSTALL_DIR</span> <span class="s2"><span class="hl">"include"</span></span><span class="hl"> </span> <span class="s">CACHE</span> <span class="s">PATH</span> <span class="s2">"The subdirectory where <span class="hl">header</span> files will be installed (default is ${CMAKE_INSTALL_PREFIX}/<span class="hl">include)"</span></span><span class="hl"> </span><span class="s"><span class="hl">FORCE</span></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">157</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "><span class="nb">set</span><span class="p">(</span><span class="s"><span class="hl">DATA_DESTINATION</span></span><span class="hl"> </span><span class="s2"><span class="hl">"${CMAK</span>E_INSTALL_<span class="hl">FULL_DATA</span>DIR<span class="hl">}/telepathy"</span></span> <span class="s">CACHE</span> <span class="s">PATH</span> <span class="s2">"The subdirectory where <span class="hl">data</span> files will be installed (default is ${CMAKE_INSTALL_PREFIX}/<span class="hl">share/telepathy)"</span></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">You should not offer these <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">XXX_DESTINATION</code> variables to the user. <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMAKE_INSTALL_XXXDIR</code> are already cached, modifying these variables will modify the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">DESTINATION</code> ones too.
In this way you will just get extra cmake cached variables in GUIs, and you won't be able to modify them, since they are forced every time.
You can either make them <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">INTERNAL</code> instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">PATH</code> (<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">FORCE</code> is implied in this case), or you can just set them as normal variables since you are including the file in the main <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMakeLists.txt</code> (therefore you don't have problems with variable scope), and since it does not make much sense to cache them.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Also I think I remember that there is some reason to use the <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">relative</strong> path instead of <strong style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">absolute</strong> when doing <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">install()</code> (therefore <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMAKE_INSTALL_XXXDIR</code> instead of <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">CMAKE_INSTALL_FULL_XXXDIR</code>), but I might be wrong here...</p></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;"><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;">Also I think I remember that there is some reason to use the relative path instead of absolute when doing install() (therefore CMAKE_INSTALL_XXXDIR instead of CMAKE_INSTALL_FULL_XXXDIR), but I might be wrong here...</p>
</blockquote>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">IIRC there was a bug in some cmake version, where INTERFACE target_include_directories couldn't be constructed with <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">relative</em> path...
and we need the <em style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">FULL</em> vars due to pc file at least. i know at least some distros are accustomed to kdelibs4-buildsystem where it was perfectly safe to pass absolute path's.</p></pre>
<br />
<p>- Hrvoje</p>
<br />
<p>On December 16th, 2014, 7:02 p.m. CET, Hrvoje Senjan 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 Telepathy and Aleix Pol Gonzalez.</div>
<div>By Hrvoje Senjan.</div>
<p style="color: grey;"><i>Updated Dec. 16, 2014, 7:02 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
telepathy-logger-qt
</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;">this is less fragile in abs. vs. relative paths, etc..</p></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">so far tried only cmake sets correct vars...</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">(0022600)</span></li>
<li>TelepathyLoggerQt/CMakeLists.txt <span style="color: grey">(125b56b)</span></li>
<li>TelepathyLoggerQt/TelepathyLoggerQt.pc.in <span style="color: grey">(508fe25)</span></li>
<li>TelepathyLoggerQt/TelepathyLoggerQtConfig.cmake.in <span style="color: grey">(0a4eef9)</span></li>
<li>cmake/modules/TelepathyDefaults.cmake <span style="color: grey">(ec091e2)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121544/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>