rpm-software-management / librepo

A library providing C and Python (libcURL like) API for downloading packages and linux repository metadata in rpm-md format
http://rpm-software-management.github.io/librepo/
GNU Lesser General Public License v2.1
74 stars 90 forks source link

If zchunk is supported, `librepo` pkgconfig is missing a dependency on zchunk #305

Closed kloczek closed 3 months ago

kloczek commented 3 months ago

With 66c99da1 has been changed ABI and when librepo is build with zchunk support <librepo/downloadtarget.h> header needs <zck.h>. Issue is that if librepo is build that way content of the installed librepo.pc has no zchunk in Requires: field. This as result causes that for example PackageKit build fails with

FAILED: backends/dnf/libpk_backend_dnf.so.p/dnf-backend.c.o
/usr/bin/gcc -Ibackends/dnf/libpk_backend_dnf.so.p -Ibackends/dnf -I../backends/dnf -Isrc -I../src -Ilib/packagekit-glib2 -I../lib/packagekit-glib2 -Ilib -I../lib -I. -I.. -I/usr/include/gl
ib-2.0 -I/usr/lib64/glib-2.0/include -I/usr/include/gio-unix-2.0 -I/usr/include/appstream -fdiagnostics-color=always -Wall -Winvalid-pch -Wextra -std=c99 -DHAVE_POLKIT_0_114=1 -DHAVE_SYSTEM
D_SD_DAEMON_H=1 -DHAVE_SYSTEMD_SD_LOGIN_H=1 -DPK_BUILD_LOCAL=1 -DPK_ENABLE_DAEMON_TESTS=1 -DPLYMOUTH_0_9_5=1 -D_DEFAULT_SOURCE -DGLIB_VERSION_MIN_REQUIRED=GLIB_VERSION_2_44 -DPK_COMPILATION
 -Wall -Wcast-align -Wno-uninitialized -Wmissing-declarations -Wredundant-decls -Wpointer-arith -Wcast-align -Wwrite-strings -Winit-self -Wreturn-type -Wformat-nonliteral -Wformat-security
-Wmissing-include-dirs -Wmissing-format-attribute -Wclobbered -Wempty-body -Wignored-qualifiers -Wsign-compare -Wtype-limits -Wuninitialized -Wno-unused-parameter -Waggregate-return -Wdecla
ration-after-statement -Wshadow -Wno-strict-aliasing -Winline -Wmissing-parameter-type -Woverride-init -m64 -mtune=generic -fasynchronous-unwind-tables -fstack-clash-protection -fcf-protect
ion -O2 -g -grecord-gcc-switches -pipe -mtls-dialect=gnu2 -specs=/usr/lib/rpm/redhat/redhat-annobin-cc1 -fno-omit-frame-pointer -mno-omit-leaf-frame-pointer -fdata-sections -ffunction-secti
ons -fstack-protector-strong -specs=/usr/lib/rpm/redhat/redhat-hardened-cc1 -flto=auto -Wp,-U_FORTIFY_SOURCE,-D_FORTIFY_SOURCE=3 -Wp,-D_GLIBCXX_ASSERTIONS -Wall -Werror=format-security -Os
-fPIC -pthread -D_FILE_OFFSET_BITS=64 '-DG_LOG_DOMAIN="PackageKit-DNF"' '-DLIBEXECDIR="/usr/libexec"' -MD -MQ backends/dnf/libpk_backend_dnf.so.p/dnf-backend.c.o -MF backends/dnf/libpk_back
end_dnf.so.p/dnf-backend.c.o.d -o backends/dnf/libpk_backend_dnf.so.p/dnf-backend.c.o -c ../backends/dnf/dnf-backend.c
In file included from /usr/include/librepo/metadata_downloader.h:29,
                 from /usr/include/librepo/librepo.h:31,
                 from /usr/include/libdnf/dnf-repo.h:33,
                 from /usr/include/libdnf/dnf-sack.h:30,
                 from /usr/include/libdnf/hy-goal.h:30,
                 from /usr/include/libdnf/dnf-transaction.h:28,
                 from /usr/include/libdnf/dnf-context.h:29,
                 from /usr/include/libdnf/libdnf.h:31,
                 from ../backends/dnf/dnf-backend.c:28:
/usr/include/librepo/downloadtarget.h:27:10: fatal error: zck.h: No such file or directory
   27 | #include <zck.h>
      |          ^~~~~~~
compilation terminated.
ppisar commented 3 months ago

The ABI change was a mistake fixed with a subsequent e2abd5dd2c1bf3a644de3bcb08c874185a0bd1e9 commit. The dependency on was there even before 66c99da1206c96fefc1b8279f777afefb79dc614 commit.

Your report is a genuine bug in librepo/librepo.pc.cmake file which has been there since the beginning. Thanks for finding it. I will fix it.

ppisar commented 3 months ago

Problem is that if I add zck among Requires into librepo.pc, then your application will needlessly link to zck because "pkg-config --libs librepo" will produce "-lrepo -lzck" instead of "-lrepo".

I do not want the overlinking. I don't think pkg-config supports no-library dependencies. Clean solution would be zchunk project to deliver zck-headers.pc which would not define "Libs". Then librepo could "Require: zck-headers". However, I do not believe that zchunk would be happy and add a pkg-config file like that only for the sake of librepo.

I'm keen not fixing this issue with the claim the pkg-config does not support a dependency like that. I will open a feature request at pkgconf because this issue is no specific to zck/librepo. But my hopes are low.

ppisar commented 3 months ago

I opened #307 as an example how the fix in pkg-config file would look like.

Another approach is not to add zck to Requires. Instead only add zck include path to librepo's pkg-config and document the dependency by words in README. We already declare the dependncy in librepo.spec.

ppisar commented 3 months ago

I also open #308 which mostly improves a documentation instead of adding the dependency. We will need to pick one of the pull requests.

ppisar commented 3 months ago

We will go with #307 which uses a not-well-known pkgconf feature that Requires.private are effective for non-static cflags.

kloczek commented 3 months ago

Do you know that "not-well-known feature" comes from rpm bug which I've even reported? From https://github.com/rpm-software-management/rpm/blob/b96abccec8cd3c466be2f901136587b13b2f9906/scripts/pkgconfigdeps.sh#L42-L58

-R|--requires)
    while read filename ; do
    case "${filename}" in
    *.pc)
        i="`expr $i + 1`"
        [ $i -eq 1 ] && echo "$pkgconfig"
        DIR="`dirname ${filename}`"
        export PKG_CONFIG_PATH="$DIR:$DIR/../../share/pkgconfig"
        $pkgconfig --print-requires --print-requires-private "$filename" 2> /dev/null | while read n r v ; do   # <<<< HERE
            [ -n "$n" ] || continue
            echo -n "pkgconfig($n) "
            [ -n "$r" ] && [ -n "$v" ] && echo -n "$r" "$v"
            echo
        done
    esac
    done
    ;;

rpm forced --print-requires --print-requires-private as same as --print-requires IIRC I've reported that is issue few years ago and it was refused as bug.

kloczek commented 3 months ago

Yeah .. I've even proposed PR to fix that https://github.com/rpm-software-management/rpm/pull/411 In mean time I've managed to spread all necessary changes across all affected packages with pkgconfig files to trim results of that bug. In other words currently if that PR would be merged quite possible that nothing will be affected.