<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/121342/">https://git.reviewboard.kde.org/r/121342/</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 4th, 2014, 9:28 p.m. UTC, <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;">This means Ark won't built with libarchive 2.8 anymore, right? If so, I'd rather do it for frameworks and leave the KDE4 version supporting a wider range of libarchive releases.</p></pre>
</blockquote>
<p>On December 4th, 2014, 11:24 p.m. UTC, <b>Elvis Angelaccio</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;">Yes, that's right. Should I commit on <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">frameworks</code> then?</p></pre>
</blockquote>
<p>On December 5th, 2014, 12:37 p.m. UTC, <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 think we can do a broader API update here. For example, that same wiki page says the <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">ARCHIVE_COMPRESSION_*</code> macros are not part of libarchive3 (I still have them defined here for compatibility, though), so there's room for more improvements.</p></pre>
</blockquote>
<p>On December 5th, 2014, 2:40 p.m. UTC, <b>Elvis Angelaccio</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;">That's fine for me. Feel free to discard this review, it makes sense to create a new one later.</p></pre>
</blockquote>
<p>On December 5th, 2014, 3:07 p.m. UTC, <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'm fine with updating this review once you have the bandwidth to work on it instead of opening a new one, after all my suggestion is just an extension of the original idea behind this patch.</p></pre>
</blockquote>
<p>On December 24th, 2014, 3:55 p.m. UTC, <b>Ivailo Monev</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;">You need to update the FindLibArchive cmake module to look for the new API too.</p></pre>
</blockquote>
<p>On December 24th, 2014, 5:51 p.m. UTC, <b>Elvis Angelaccio</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;">Thanks, updated now.</p></pre>
</blockquote>
<p>On January 4th, 2015, 2:11 a.m. UTC, <b>Ivailo Monev</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;">There are additional checks in ark/plugins/libarchive/CMakeLists.txt like HAVE_LIBARCHIVE_CAB_SUPPORT and HAVE_LIBARCHIVE_RPM_SUPPORT. Also, you can drop the CLI plugins for Zip and 7z formats and update the MIME types that are supported by the libarchive plugin as these formats are supported since 3.0.0. It may be too much for a single review request but I just want to let you know.</p></pre>
</blockquote>
<p>On January 4th, 2015, 11:48 a.m. UTC, <b>Elvis Angelaccio</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;">Thanks, indeed there are some checks still to be removed. I'm not sure if I am supposed to reopen this review or create a new one (let's wait what Raphael says).
Regarding the 7z and zip formats in libarchive, yes it's something that needs a proper discussion. For example, I have another review request for the creation of password protected archives. AFAIK, that feature would not be possible by using libarchive.</p></pre>
</blockquote>
<p>On January 4th, 2015, 1:26 p.m. UTC, <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;">Thanks everyone. Elvis, please send a new review request removing all the libarchive checks from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">plugins/CMakeLists.txt</code> (I'm suspecting the libarchive plugin is not even being built at the moment). Ivailo, removing the CLI plugins is something else entirely: as Elvis pointed out, it has many other consequences and libarchive support for formats such as zip and rar may not be feature complete yet, so I'd rather not do anything like that without a lot of investigation.</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;">As far as I know password protected Zip, 7z, rar archives are not supported yet by libarchive but planned. So, yes, it would make sense the keep the plugins for those formats if that feature is desired. Or, implement it in libarchive and make everyone happy - not just KDE but libarchive users too.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">And yes, LIBARCHIVE_FOUND needs to be changed to LibArchive_FOUND for the plugin to be build as that is what the bundled libarchive CMake module defines.</p></pre>
<br />
<p>- Ivailo</p>
<br />
<p>On January 2nd, 2015, 4:39 p.m. UTC, Elvis Angelaccio 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 KDE Utils and Raphael Kubo da Costa.</div>
<div>By Elvis Angelaccio.</div>
<p style="color: grey;"><i>Updated Jan. 2, 2015, 4:39 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
ark
</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 patch replaces the deprecated <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">libarchive</code> functions used in <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">libarchivehandler.cpp</code> with their replacements.
Quick reference: https://github.com/libarchive/libarchive/wiki/ManPageLibarchiveChanges3#Deprecated_Symbols</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;">After compiling all the warnings about deprecated symbols are gone.</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">(b17c30b4f0f57db6e6cc1e8ee77c5fe5e8b32e77)</span></li>
<li>cmake/modules/COPYING-CMAKE-SCRIPTS <span style="color: grey">(4b417765f3a834ce6b0a216f6b6c0fe2d3f0bed5)</span></li>
<li>cmake/modules/FindLibArchive.cmake <span style="color: grey">(db14d7b279e67039c454fcaa82a21885feec26e0)</span></li>
<li>plugins/libarchive/libarchivehandler.cpp <span style="color: grey">(0fba52864523bf07ba8c898aa6b15cd867001de6)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/121342/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>