Fix libzip cmake module
ClosedPublic

Authored by elvisangelaccio on Mar 18 2017, 3:31 PM.

Details

Summary

libzip installs zipconf.h in $prefix/$libdir/libzip/include/. Some
systems (e.g. archlinux) symlinks this file under $prefix/$libdir/include
for compatibility reasons, but cmake fails on systems that do not (e.g.
flatpak). Since libzip supports pkg-config, we can just query pkg-config
which gives us the proper path we are looking for. While at it, use
pkg-config to get the library version, replacing our custom parsing code
which does not work.

This breaks the Applications dependency freeze, but it's an important
fix so it should go in.

Test Plan
  1. ark master flatpak now can build the new libzip plugin.
  2. ark now builds the libzip plugin only if libzip >= 1.2 is found.

Diff Detail

Repository
R36 Ark
Branch
master
Lint
No Linters Available
Unit
No Unit Test Coverage
Restricted Application added a project: Ark. · View Herald TranscriptMar 18 2017, 3:31 PM
Restricted Application added subscribers: Ark, kde-utils-devel. · View Herald Transcript
dakon added a subscriber: dakon.Mar 18 2017, 4:32 PM

Why does this break the dependency freeze? Because it introduces an optional dependency on pkg-config?

cmake/modules/FindLibZip.cmake
11–12

Shouldn't there be some PC_* hint here too?

It violates the CMake standards anyway: those variables that are searched are *_DIR, and *_DIRS are variables that are just set, in this case probably containing both this and LibZip_INCLUDE_CONF_DIRS.

32–33

The second include dir is missing here, or is it really optional?

In D5101#95921, @dakon wrote:

Why does this break the dependency freeze? Because it introduces an optional dependency on pkg-config?

Yes, but it's not a big deal, indeed.

cmake/modules/FindLibZip.cmake
32–33

It's required but yes, is missing here. It's currently set in the LibZip_INCLUDE_CONF_DIRS variable which we use later in the cmakelists file of the plugin:

include_directories(${LibZip_INCLUDE_DIRS} ${LibZip_INCLUDE_CONF_DIRS})

Do you think it would be better to have a single include variable with both paths?

dakon added inline comments.Mar 18 2017, 4:45 PM
cmake/modules/FindLibZip.cmake
32–33

Yes, put both into *_DIRS, list *_DIR here, and mark_as_advanced() them.

  • More HINTS from PC variables
  • Use a single variable with both include paths
elvisangelaccio marked 3 inline comments as done.Mar 18 2017, 5:54 PM
  • Remove debug output
  • Also mark LibZip_INCLUDE_CONF_DIR as required.
  • Remove unnecessary comma
dakon accepted this revision.Mar 18 2017, 8:37 PM
This revision is now accepted and ready to land.Mar 18 2017, 8:37 PM
This revision was automatically updated to reflect the committed changes.