[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-utils-devel
Subject: Re: Review Request 121342: Switch away from libarchive deprecated functions
From: "Raphael Kubo da Costa" <rakuco () FreeBSD ! org>
Date: 2015-01-04 13:26:11
Message-ID: 20150104132611.6567.97561 () probe ! kde ! org
[Download RAW message or body]
--===============3939381686943150995==
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 7bit
> On Dec. 4, 2014, 11:28 p.m., Raphael Kubo da Costa wrote:
> > 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.
>
> Elvis Angelaccio wrote:
> Yes, that's right. Should I commit on `frameworks` then?
>
> Raphael Kubo da Costa wrote:
> I think we can do a broader API update here. For example, that same wiki page says \
> the `ARCHIVE_COMPRESSION_*` macros are not part of libarchive3 (I still have them \
> defined here for compatibility, though), so there's room for more improvements.
> Elvis Angelaccio wrote:
> That's fine for me. Feel free to discard this review, it makes sense to create a \
> new one later.
> Raphael Kubo da Costa wrote:
> 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.
> Ivailo Monev wrote:
> You need to update the FindLibArchive cmake module to look for the new API too.
>
> Elvis Angelaccio wrote:
> Thanks, updated now.
>
> Ivailo Monev wrote:
> 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.
> Elvis Angelaccio wrote:
> 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.
Thanks everyone. Elvis, please send a new review request removing all the libarchive \
checks from `plugins/CMakeLists.txt` (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.
- Raphael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121342/#review71369
-----------------------------------------------------------
On Jan. 2, 2015, 6:39 p.m., Elvis Angelaccio wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121342/
> -----------------------------------------------------------
>
> (Updated Jan. 2, 2015, 6:39 p.m.)
>
>
> Review request for KDE Utils and Raphael Kubo da Costa.
>
>
> Repository: ark
>
>
> Description
> -------
>
> This patch replaces the deprecated `libarchive` functions used in \
> `libarchivehandler.cpp` with their replacements. Quick reference: \
> https://github.com/libarchive/libarchive/wiki/ManPageLibarchiveChanges3#Deprecated_Symbols
>
>
> Diffs
> -----
>
> CMakeLists.txt b17c30b4f0f57db6e6cc1e8ee77c5fe5e8b32e77
> cmake/modules/COPYING-CMAKE-SCRIPTS 4b417765f3a834ce6b0a216f6b6c0fe2d3f0bed5
> cmake/modules/FindLibArchive.cmake db14d7b279e67039c454fcaa82a21885feec26e0
> plugins/libarchive/libarchivehandler.cpp 0fba52864523bf07ba8c898aa6b15cd867001de6
>
> Diff: https://git.reviewboard.kde.org/r/121342/diff/
>
>
> Testing
> -------
>
> After compiling all the warnings about deprecated symbols are gone.
>
>
> Thanks,
>
> Elvis Angelaccio
>
>
--===============3939381686943150995==
MIME-Version: 1.0
Content-Type: text/html; charset="utf-8"
Content-Transfer-Encoding: 7bit
<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, 11:28 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;">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 5th, 2014, 1:24 a.m. EET, <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, 2:37 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 \
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, 4:40 p.m. EET, <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, 5:07 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'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, 5:55 p.m. EET, <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, 7:51 p.m. EET, <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, 4:11 a.m. EET, <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, 1:48 p.m. EET, <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>
</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;">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> <br />
<p>- Raphael</p>
<br />
<p>On January 2nd, 2015, 6:39 p.m. EET, 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, 6: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>
--===============3939381686943150995==--
_______________________________________________
Kde-utils-devel mailing list
Kde-utils-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-utils-devel
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic