Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add zchunk support #127

Merged
merged 5 commits into from Nov 26, 2018
Merged

Add zchunk support #127

merged 5 commits into from Nov 26, 2018

Conversation

jdieter
Copy link
Contributor

@jdieter jdieter commented Jun 12, 2018

This patchset adds zchunk support to librepo. It requires that libsolv also have zchunk support enabled, and, for it to be effective, the calling application needs to pass the handle a cache directory to find old zchunk files to delta against (I've submitted a patch to dnf).

@jdieter
Copy link
Contributor Author

jdieter commented Jun 12, 2018

Do please note, that, unlike the libsolv and dnf patches, these patches make zchunk a mandatory dependency. If you'd rather it be an optional dependency, please let me know.

@Conan-Kudo
Copy link
Member

@jdieter I suspect that it might be desired for the RHEL side of the house to be optional, but I don't see a reason why we wouldn't want it on in Fedora and Mageia.

@j-mracek
Copy link
Member

Without a merge of patch for libsolv it will not work?

@jdieter
Copy link
Contributor Author

jdieter commented Aug 21, 2018

That's correct. It will download the zchunk metadata, and send it to libsolv, but without openSUSE/libsolv#270, libsolv won't be able to read the zchunk metadata.

CMakeLists.txt Outdated
@@ -31,6 +31,12 @@ PKG_SEARCH_MODULE(LIBCRYPTO REQUIRED libcrypto openssl)
PKG_CHECK_MODULES(LIBXML2 libxml-2.0 REQUIRED)
FIND_PACKAGE(CURL REQUIRED)
FIND_PACKAGE(Gpgme REQUIRED)
FIND_LIBRARY(ZCHUNKLIB NAMES zck)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be pkg_check_modules

@dmach
Copy link

dmach commented Sep 6, 2018

zck.h should #include <stdbool.h> as the boolean type is part of public header

There are also couple compilation errors, such as:
/usr/include/zck.h:62:29: note: expected 'zckCtx *' {aka 'struct zckCtx *'} but argument is of type 'int'
librepo/downloader.c:1089:11: error: too few arguments to function 'zck_init_adv_read
librepo/downloader.c:1156:31: error: too few arguments to function 'zck_init_read'
librepo/downloader.c:1208:29: error: too few arguments to function 'zck_get_range_char'

  • tested with zchunk-devel-0.9.7-2.fc29

I also created PR to make zchunk optional - it's very important to us so we can turn it off on as needed:
jdieter#1

@jdieter
Copy link
Contributor Author

jdieter commented Sep 18, 2018

Ok, I've gone ahead and merged your PR and added <stdbool.h> to zck.h in zchunk-0.9.10. This now compiles fine with zchunk-0.9.10+, but it won't work with repositories generated using the current createrepo_c zchunk patches because of the format changes.

@jdieter jdieter force-pushed the zchunk branch 2 times, most recently from fdb6816 to 7e2e787 Compare September 27, 2018 14:55
@jdieter
Copy link
Contributor Author

jdieter commented Sep 28, 2018

@dmach, If you'd prefer these to be in fewer commits, I'll squash them down for you

@Conan-Kudo
Copy link
Member

@jdieter Could you please do so?

jdieter and others added 4 commits September 28, 2018 22:27
Signed-off-by: Jonathan Dieter <jdieter@gmail.com>
Signed-off-by: Jonathan Dieter <jdieter@gmail.com>
Signed-off-by: Jonathan Dieter <jdieter@gmail.com>
@jdieter
Copy link
Contributor Author

jdieter commented Oct 4, 2018

@dmach, @Conan-Kudo is there anything else you need me to do on these before review?

librepo.spec Outdated
@@ -38,6 +43,9 @@ BuildRequires: libcurl-devel >= 7.19.0
BuildRequires: pkgconfig(libxml-2.0)
BuildRequires: pkgconfig(libcrypto)
BuildRequires: pkgconfig(openssl)
%if %{with zchunk}
BuildRequires: zchunk-devel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use pkgconfig(zck) >= 0.9.11 here.

Signed-off-by: Jonathan Dieter <jdieter@gmail.com>
Copy link
Member

@Conan-Kudo Conan-Kudo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me.

@jdieter
Copy link
Contributor Author

jdieter commented Oct 4, 2018

Thanks!

@Conan-Kudo
Copy link
Member

@dmach, @j-mracek: This is now merged into libsolv, so can we get this reviewed and merged, as well as the corresponding PRs for the other components of the stack (as well as createrepo_c)?

@jdieter
Copy link
Contributor Author

jdieter commented Oct 17, 2018

Is there anything else you need from me before you review this?

@dmach dmach merged commit be410e7 into rpm-software-management:master Nov 26, 2018
@jdieter jdieter deleted the zchunk branch December 29, 2018 17:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants