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 #270

Merged
merged 1 commit into from Oct 8, 2018

Conversation

Projects
None yet
4 participants
@jdieter
Contributor

jdieter commented Jun 12, 2018

This patch adds zchunk support to libsolv.

@jdieter jdieter referenced this pull request Jun 12, 2018

Open

Add zchunk support #127

@@ -1,6 +1,9 @@
PROJECT (libsolv)
CMAKE_MINIMUM_REQUIRED (VERSION 2.4)
IF (COMMAND CMAKE_POLICY)
CMAKE_POLICY(SET CMP0003 NEW)

This comment has been minimized.

@ignatenkobrain

ignatenkobrain Jun 12, 2018

Collaborator

why?

This comment has been minimized.

@jdieter

jdieter Jun 12, 2018

Contributor

If I don't, I get the following:

CMake Warning (dev) at ext/CMakeLists.txt:137 (ADD_LIBRARY):
  Policy CMP0003 should be set before this line.  Add code such as

    if(COMMAND cmake_policy)
      cmake_policy(SET CMP0003 NEW)
    endif(COMMAND cmake_policy)

  as early as possible but after the most recent call to
  cmake_minimum_required or cmake_policy(VERSION).  This warning appears
  because target "libsolvext" links to some libraries for which the linker
  must search:

    zck

  and other libraries with known full path:

    /home/jonathan/Documents/programming/libsolv/build/src/libsolv.so.0

  CMake is adding directories in the second list to the linker search path in
  case they are needed to find libraries from the first list (for backwards
  compatibility with CMake 2.4).  Set policy CMP0003 to OLD or NEW to enable
  or disable this behavior explicitly.  Run "cmake --help-policy CMP0003" for
  more information.
This warning is for project developers.  Use -Wno-dev to suppress it.

This comment has been minimized.

@ignatenkobrain

ignatenkobrain Jul 2, 2018

Collaborator

I think this might indicate wrong pkg-config file for zck, however I can't test it since zck is not packaged for Fedora.

This comment has been minimized.

@jdieter

jdieter Jul 2, 2018

Contributor

There is a copr at https://copr.fedorainfracloud.org/coprs/jdieter/zchunk/, but, you're right, we really need to get this into Fedora.

Neal, are you up for a package review?

This comment has been minimized.

@ignatenkobrain

ignatenkobrain Jul 2, 2018

Collaborator

Just assign me for review.

This comment has been minimized.

@jdieter

jdieter Jul 2, 2018

Contributor

Great, thank you!

This comment has been minimized.

@Conan-Kudo

Conan-Kudo Jul 2, 2018

Member

I'm ready to review as well.

This comment has been minimized.

@jdieter

jdieter Jul 2, 2018

Contributor

Ok, Igor, I've assigned it to you, and, Neal, I've cc'd you into the bug. Thanks so much, both of you.

This comment has been minimized.

@ignatenkobrain

ignatenkobrain Jul 2, 2018

Collaborator

I don't see any bugs assigned to i.gnatenko.brain@gmail.com

This comment has been minimized.

@jdieter

jdieter Jul 2, 2018

Contributor

Sorry, I assigned it to ignatenko@redhat.com since that's what it looked like your bugzilla account was. I'll re-assign it.

@dmach dmach referenced this pull request Jun 13, 2018

Closed

Zchunk support for dnf #1107

@mlschroe

This comment has been minimized.

Member

mlschroe commented Jul 2, 2018

(But doesn't all this belong in the transport layer, aka librepo? Why do you need that in libsolv at all?)

@jdieter

This comment has been minimized.

Contributor

jdieter commented Jul 2, 2018

Zchunk is a new compression format that libsolv needs to be able to decompress, just as it decompresses xz and gz files.

@mlschroe

This comment has been minimized.

Member

mlschroe commented Jul 5, 2018

I'll do a little zchunk review today. Stay tuned.

@jdieter

This comment has been minimized.

Contributor

jdieter commented Jul 5, 2018

Thanks so much!

@mlschroe

This comment has been minimized.

Member

mlschroe commented Jul 5, 2018

Ok, see my mail to rpm-ecosystem@lists.rpm.org
Thanks!

@Conan-Kudo

This comment has been minimized.

Member

Conan-Kudo commented Jul 17, 2018

@mlschroe Why'd you implement it by hand in 75d0305 instead of pulling this PR in and using the library?

The library is available in openSUSE and can be easily backported to SLE 12/Leap 42 if needed.

@jdieter

This comment has been minimized.

Contributor

jdieter commented Jul 17, 2018

@mlschroe If there's anything missing in the library, I would happily add it, and, if you're worried about control, I'll happily give you commit access. I'm just concerned that, as new features are added to the library, they'll also have to be manually added to libsolv before we can take advantage of them.

@mlschroe

This comment has been minimized.

Member

mlschroe commented Jul 17, 2018

Because it was fun to do and got me a good understanding for the format. And I somewhat don't like dragging in a dependency to some other library/libraries for just so little lines of code. Anyway, I can make this configurable if you like.

@ignatenkobrain

This comment has been minimized.

Collaborator

ignatenkobrain commented Aug 10, 2018

@jdieter can you rebase this please and add configurable switch to use system zchunk? ;)

@jdieter

This comment has been minimized.

Contributor

jdieter commented Sep 28, 2018

Ok, I've rebased this and rewritten it to use a new switch, USE_SYSTEM_ZCHUNK. I've tested with the switch both enabled and disabled, and it seems to work perfectly.

Add system zchunk support
Signed-off-by: Jonathan Dieter <jdieter@gmail.com>
@@ -156,6 +159,12 @@ IF (ENABLE_ARCHREPO OR ENABLE_DEBIAN)
SET (ENABLE_LZMA_COMPRESSION ON)
ENDIF (ENABLE_ARCHREPO OR ENABLE_DEBIAN)
IF (USE_SYSTEM_ZCHUNK)
SET (ENABLE_ZCHUNK_COMPRESSION ON)

This comment has been minimized.

@Conan-Kudo

Conan-Kudo Sep 29, 2018

Member

This check should be inverted. It should be USE_SYSTEM_ZCHUNK underneath ENABLE_ZCHUNK_COMPRESSION.

This comment has been minimized.

@jdieter

jdieter Sep 29, 2018

Contributor

I think it's right the way it is. It's saying that if USE_SYSTEM_ZCHUNK is enabled, we should automatically enable ENABLE_ZCHUNK_COMPRESSION. However, if you're using the embedded zchunk decompressor, ENABLE_ZCHUNK_COMPRESSION should be enabled, and USE_SYSTEM_ZCHUNK shouldn't be.

@jdieter

This comment has been minimized.

Contributor

jdieter commented Oct 4, 2018

@mlschroe, is there anything else you'd like me to do with this before you review it?

@mlschroe

This comment has been minimized.

Member

mlschroe commented Oct 4, 2018

No, it's fine as it is. I'll merge it tomorrow.

@mlschroe mlschroe merged commit 0e37bcc into openSUSE:master Oct 8, 2018

@mlschroe

This comment has been minimized.

Member

mlschroe commented Oct 8, 2018

(for a very loose definition of tomorrow...)

@jdieter

This comment has been minimized.

Contributor

jdieter commented Oct 8, 2018

That works! Thanks so much!

@jdieter jdieter deleted the jdieter:zchunk branch Oct 8, 2018

@mlschroe

This comment has been minimized.

Member

mlschroe commented Oct 8, 2018

I did some changes to the code:

commit 9b413e2 (HEAD -> master, origin/master)
Author: Michael Schroeder mls@suse.de
Date: Mon Oct 8 11:55:13 2018 +0200

Tweak system zchunk code

- rename USE_SYSTEM_ZCHUNK to WITH_SYSTEM_ZCHUNK
- fix file descriptor leak
- fix memory leaks

Could you please check that is still works? Thanks!

@jdieter

This comment has been minimized.

Contributor

jdieter commented Oct 8, 2018

I've just done a pull request, #282, that fixes some very small bugs in 9b413e2

@mlschroe

This comment has been minimized.

Member

mlschroe commented Oct 8, 2018

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment