<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/126345/">https://git.reviewboard.kde.org/r/126345/</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 14th, 2015, 8:24 p.m. EET, <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;">Seems sensible to me. Are there any potential Python2 vs Python3 issues?</p></pre>
</blockquote>
<p>On December 14th, 2015, 9:03 p.m. EET, <b>Raphael Kubo da Costa</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 think there are any -- the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.pyc/.pyo</code> files are installed with a different name into a different location on Python >= 3.2, but that is already taken care of by the macro. This patch only deals with the path where the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">.py</code> file gets installed, which it does not change.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The only problem I see is in the use of the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">argparse</code> module: it does not exist on Python < 2.7, Python 3.0 and Python 3.1. If that's an issue, I can rewrite the code to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">optparse</code>, which is deprecated but exists on all Python versions, including the deprecated ones.</p></pre>
</blockquote>
<p>On December 14th, 2015, 10:51 p.m. EET, <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 guess it depends whether it effectively causes a dependency bump over what was previously required, and whether that will have any practical impact on people upgrading kdelibs if it does (ie: does anyone actually use those Python versions any more?).</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;">It has an effect on kdelibs's run-time dependencies in the sense that projects (PyKDE4 etc) using the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">PYTHON_INSTALL()</code> will need either Python >= 2.7 or Python >= 3.2. I doubt anyone is using Python 2.6 with a recent kdelibs these days (even Debian stable and oldstable ship Python 2.7), and if anyone is trying to use Python 3 with kdelibs they are very likely to be using a more recent version than 3.1 or 3.0. My guess is that this is not a big deal, and rewriting the code to use <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">optparse</code> is trivial should any bug reports be filed.</p></pre>
<br />
<p>- Raphael</p>
<br />
<p>On December 14th, 2015, 6:48 p.m. EET, Raphael Kubo da Costa 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 Build System, kdelibs and Alex Merry.</div>
<div>By Raphael Kubo da Costa.</div>
<p style="color: grey;"><i>Updated Dec. 14, 2015, 6:48 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kdelibs
</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;">The <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">PYTHON_INSTALL()</code> macro is a wrapper around the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">py_compile</code> Python module that also installs the byte-code (.pyc) file it generates.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">However, when a .py file is passed to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">py_compile</code> without any additional arguments, its full path is recorded in the .pyc file. This is problematic, as most distributions install all files into a build root instead of simply copying files to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">/</code> as part of the packaging process. In this case, the generated .pyc file will have something like
/wrkdir/buildroot/usr/lib/python2.7/site-packages/Foo/my_module.py
in it. Not only does this show up in exception tracebacks, but if the user later invokes <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">my_module.py</code> and has write access to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">my_module</code>'s directory, <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">my_module.pyc</code> will be rewritten with the right path to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">my_module.py</code> (without the build root). This can lead to uninstallation errors if the package management system checks each file before removal, for example.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Fix it by rewritting the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">PythonCompile.py</code> script so that it takes a <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">--destination-dir</code> argument that we use to pass the full path to <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">my_module.py</code> instead of letting it be (wrongly) deduced.</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;">Fedora has packaging macros that will regenerate .pyc and .pyo files with the right paths as part of the build, so it is not affected. Debian disables this macro in pykde4, FreeBSD and openSUSE contain wrong paths in its .pyc files. With this patch, I was able to verify with <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">hexdump</code> that the right path is present in the .pyc files installed by pykde4.</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>cmake/modules/PythonCompile.py <span style="color: grey">(156fea2)</span></li>
<li>cmake/modules/PythonMacros.cmake <span style="color: grey">(6a82d88)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/126345/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>