Differential D15369

Add support for Zstd-compressed Tar-archives
ClosedPublic
Actions

Authored by rthomsen on Sep 9 2018, 9:52 AM.

Details

Reviewers
elvisangelaccio
dakon
Commits
R36:d876ba3d9ebc: Add support for Zstd-compressed Tar-archives
Summary

Libarchive 3.3.3 was released on Sept. 4. and added support for the Zstandard compression filter. This compression method was designed to give similar compression ratio as gzip but with much greater (de)compression speeds. Libarchive uses the libzstd.so library, but seems to use the zstd binary as fallback if it wasn't built with libzstd.

This diff enables the filter in the libarchive plugin. There is still no mimetype in shared-mime-info, so a custom mimetype for zstd-compressed tar archives is added. This necessitated re-adding functionality in Ark for custom mime-types (was removed in commit 73cd5e8d2e7b347d84438b23afdc892eddb78ed2).

Arch linux already added libarchive 3.3.3 and I think most distros should have packaged this version by December, so should be ok to bump the dep version?

This is my first commit in a long while, so please check I didn't forget something ;)

Bug 384040

There's a bugreport for adding application/zstd mimetype to shared-mime-info. Maybe we should add one for application/x-zstd-compressed-tar?

Test Plan
  • Create a new archive using the new compression filter.

Diff Detail

Repository
R36 Ark
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
rthomsen created this revision.Sep 9 2018, 9:52 AM
Restricted Application added subscribers: Ark, kde-utils-devel. · View Herald TranscriptSep 9 2018, 9:52 AM
rthomsen requested review of this revision.Sep 9 2018, 9:52 AM
rthomsen edited the summary of this revision. (Show Details)Sep 9 2018, 9:55 AM
rthomsen edited the summary of this revision. (Show Details)Sep 9 2018, 10:31 AM
rthomsen edited the summary of this revision. (Show Details)Sep 9 2018, 10:39 AM
elvisangelaccio requested changes to this revision.Sep 16 2018, 3:42 PM

Looks good except the version bump.

Can you also add a test archive and test case in loadtest?

CMakeLists.txt
67

I'm pretty sure the CI doesn't have this version of libarchive (being based on openSUSE).

If we want to merge this now, we need to stick with 3.2.x and check at build-time if we are building against 3.3.x

This revision now requires changes to proceed.Sep 16 2018, 3:42 PM
elvisangelaccio added a comment.Oct 27 2018, 3:04 PM

@rthomsen Ping? Feature-freeze is getting close ;)

rthomsen updated this revision to Diff 44756.Nov 3 2018, 10:06 AM

Add loadtest.

rthomsen added a comment.Nov 3 2018, 10:09 AM
In D15369#326826, @elvisangelaccio wrote:

Looks good except the version bump.

If we still cant depend on libarchive 3.3.3, do you have some example code to check for libarchive version at runtime? I guess we also need to check if libarchive was built with zstd-support?

elvisangelaccio added a comment.EditedNov 3 2018, 10:53 AM
In D15369#353225, @rthomsen wrote:

If we still cant depend on libarchive 3.3.3, do you have some example code to check for libarchive version at runtime?

git checkout 22adc9e7bd2436833 will show you how we used to conditionally depend on libarchive 3.2.

Basically we need this in cmake

if(LibArchive_VERSION VERSION_EQUAL "3.3.3" OR
   LibArchive_VERSION VERSION_GREATER "3.3.3")
  target_compile_definitions(kerfuffle_libarchive PRIVATE -DHAVE_LIBARCHIVE_3_3_3)
endif()

and then #ifdef HAVE_LIBARCHIVE_3_3_3 in the code.

I guess we also need to check if libarchive was built with zstd-support?

That would be even better, sure. But I'm not sure if that's possible...

rthomsen updated this revision to Diff 44775.Nov 3 2018, 3:07 PM

Only enable Zstandard support if libarchive >= 3.3.3.

dakon requested changes to this revision.Nov 3 2018, 5:25 PM
dakon added a subscriber: dakon.
dakon added inline comments.
plugins/libarchive/CMakeLists.txt
9

if(NOT LibArchive_VERSION VERSION_LESS 3.3.3)

57

Again.

75

Again. Maybe create a boolean flag if zstd support should be enabled?

plugins/libarchive/readwritelibarchiveplugin.cpp
351

filename().rightRef(3).compare(QLatin1String("lz4"), Qt::CaseInsensitive)

355

Again.

This revision now requires changes to proceed.Nov 3 2018, 5:25 PM
rthomsen updated this revision to Diff 44792.Nov 3 2018, 6:00 PM

Implement suggestions.

rthomsen marked 5 inline comments as done.Nov 3 2018, 6:01 PM
rthomsen added inline comments.
plugins/libarchive/readwritelibarchiveplugin.cpp
351

This is not part of my diff. We can update the other comparisons separately.

elvisangelaccio added inline comments.Nov 4 2018, 5:26 PM
plugins/libarchive/CMakeLists.txt
97–109

Can you put a similar check/message for the zstd binary?

rthomsen updated this revision to Diff 44921.Nov 5 2018, 4:21 PM

Check for zstd executable.

rthomsen marked an inline comment as done.Nov 5 2018, 4:21 PM
elvisangelaccio accepted this revision.Nov 5 2018, 8:15 PM

arc didn't download the test file here, but ok.

Note that the dependency freeze is on 8th november, make sure to push before then ;)

This revision was not accepted when it landed; it landed in state Needs Review.Nov 6 2018, 4:50 PM
Closed by commit R36:d876ba3d9ebc: Add support for Zstd-compressed Tar-archives (authored by rthomsen). · Explain Why
This revision was automatically updated to reflect the committed changes.
rthomsen mentioned this in R36:d876ba3d9ebc: Add support for Zstd-compressed Tar-archives.
yurchor added a subscriber: yurchor.Nov 6 2018, 7:18 PM
yurchor added inline comments.
plugins/libarchive/readwritelibarchiveplugin.cpp
355

Maybe it is better to use the old-style comparison without c++17 features? It breaks the compilation on oldish gccs:

https://build.kde.org/job/Applications/job/ark/job/kf5-qt5%20SUSEQt5.9/65/consoleFull

yurchor added inline comments.Nov 6 2018, 7:46 PM
plugins/libarchive/readwritelibarchiveplugin.cpp
355

Sorry. Disregard this.

Revision Contents
Changeset List

Diff 44973

View Options

CMakeLists.txt

Loading...
View Options

autotests/kerfuffle/data/simplearchive.tar.zst

Loading...
View Options

autotests/kerfuffle/loadtest.cpp

Loading...
View Options

kerfuffle/CMakeLists.txt

Loading...
View Options

kerfuffle/mime/kerfuffle.xml

Loading...
View Options

plugins/libarchive/CMakeLists.txt

Loading...
View Options

plugins/libarchive/kerfuffle_libarchive.json.cmake

Loading...
View Options

plugins/libarchive/libarchiveplugin.cpp

Loading...
View Options

plugins/libarchive/readwritelibarchiveplugin.cpp

Loading...