search

sebastiandev / zipper

C++ wrapper around minizip compression library
MIT License
515 stars 123 forks source link

Add zlib submodule and modified cmake file #92

Closed yhyu13 closed 3 years ago

yhyu13 commented 3 years ago

Solve #69 where zlib is added as a submodule

yhyu13 commented 3 years ago

zlib.dll that is built is thus required on shadred library build

Lecrapouille commented 3 years ago

@yhyu13 Thanks I'll test it !

yhyu13 commented 3 years ago

@Lecrapouille I've updated the cmake file such that only windows uses the zlib submodule

Lecrapouille commented 3 years ago

@yhyu13 Sorry this still failing https://travis-ci.org/github/sebastiandev/zipper/jobs/762553598 Sorry I dunno why Travis-Ci no longer shows the status. I'm a little bored of these CI always broken by their maintainers :(

Else concerning your patch CMakeLists.txt I'm not found of if(WIN32) this could be used for Linux too. Ideally, I would have preferred to see the as OPTION(USE_NESTING_ZLIB "Use the zlib included as submodule", OFF). If OFF you can force it to ON and call your new code if if(NOT EXISTS "${ZLIB_INCLUDE_DIRS}/zlib.h") is triggered.

What about the zlib.h inside the submodule: is it installed to the /usr/include when you do sudo make install ? Does not conflict with the original zlib installed with apt-get install ?

jnhyatt commented 3 years ago

Have you considered adding this as an external project instead? If targets are exported correctly, you could add this using CMake ExternalProject, then set it to install to a directory within the build directory. From there, you simply add that install directory to the CMake prefix path and call find_package. This doesn't work with the main zlib repository, but Mizux has a fork that has properly exported modern CMake targets. The reason I'd prefer that approach is because it follows the more modern CMake paradigm, so the user of this library could decide whether they want to use their own installed zlib version or the zlib version included by the library. So you could add an option to add the external project, and if you don't, it'll just check the user's install directory. Thoughts?

Lecrapouille commented 3 years ago

I close this PR since #93 was commited first. Feel free to reopen it if needed