stachenov / quazip

Qt/C++ wrapper over minizip
Other
582 stars 232 forks source link

Changes to QUAZIP_USE_QT_ZLIB #186

Closed cen1 closed 8 months ago

cen1 commented 9 months ago

After testing versions from 6.3.1 up to 6.6.2 and latest patch versions in between compiled with -qt-zlib , I could not get BundledZlib to work as a component name. All versions throw an error:

 Failed to find Qt component "BundledZLIB". Expected Config file at
  # "/path/to/Qt6/qtbase/lib/cmake/Qt6BundledZLIB/Qt6BundledZLIBConfig.cmake" does NOT exist 

The only name that works is ZlibPrivate because there is a Qt6ZlibPrivate CMake target. I did not test Versions 6.2, 6.1 and 6.0 because they don't compile on Debian 12.

The name was introduced in https://github.com/stachenov/quazip/pull/171 so maybe @aikawayataro has some insights how that worked. This is an example how my tests were done : https://github.com/cen1/quazip/actions/runs/8131704060/job/22221334441

Additional observations are that libQt6BundledZLIB.a exists in Qt installations but ZlibPrivate only exports headers, not this library. I suspect Qt just does not want anyone to use it so they excluded it from CMake. If I explicitely linked to that I could get tests to compile but I really do not like hacking like this.

aikawayataro commented 8 months ago

The name was introduced in #171 so maybe @aikawayataro has some insights how that worked.

This worked correctly on Windows, maybe something is different on Linux. Anyways, I'm sure it will only get worse, and we need to abandon QtZlib completely. You can always link the system zlib statically if required (it should be very simple on Linux: -static -lz, unlike Windows, where implib hell awaits you). Using QtZlib was a dirty hack that looked applicable at the moment.

I suggest a configuration option that will download Zlib and compile it as a static library for QuaZip (using a custom zlib prefix to avoid conflicts). Something like QuaZipZlib installed with QuaZip. Reminds us of QtZlib?

aikawayataro commented 8 months ago

QUAZIP_USE_QT_ZLIB doesn't make much sense on Linux, where the link procedure is independent of library type. On Windows, the names of imported functions are different, in dynamic libraries they have the prefix __imp_ causing a lot of inconvenience if a static library has already been compiled with a dynamic/static variant and the opposite is required.

cen1 commented 8 months ago

I've just added basic Windows CI support, I'll include this option in the matrix and see what happens. If it's just a matter of keeping the names different for WIN32 or reserving this option for WIN32 only, I guess we can leave it in for now.

I also added basic vcpkg and conan support so installing zlib is pretty easy with that.

cen1 commented 8 months ago

After testing through CI and locally on Windows, my conclusions are:

Qt5 works fine.

Qt6 with BundledZLIB does not work, tested 6.3.2 and 6.6.2. Fails at configure.

Qt6 with ZlibPrivate works at cmake configure step but fails to link.

I'll leave this open a bit more if someone reports a working Qt6 solution.

Otherwise my suggestion is to drop this param for Qt6, leave it for Qt5.

Conf reference:

cmake -DCMAKE_BUILD_TYPE="Release" -DBUILD_SHARED_LIBS=ON -DQUAZIP_USE_QT_ZLIB=ON -B build -DCMAKE_PREFIX_PATH="C:\Qt\Qt-6.6.2\lib\cmake"

cmake -DCMAKE_BUILD_TYPE="Release" -DBUILD_SHARED_LIBS=ON -DQUAZIP_USE_QT_ZLIB=ON -B build -DCMAKE_PREFIX_PATH="C:\Qt\Qt-6.3.2\lib\cmake"

cmake -DCMAKE_BUILD_TYPE="Release" -DBUILD_SHARED_LIBS=ON -DQUAZIP_USE_QT_ZLIB=ON -B build -DCMAKE_PREFIX_PATH="C:\Qt\Qt-5.15.13\lib\cmake"
aikawayataro commented 8 months ago

Using BundledZLIB requires some special setup, Qt compiled with the -qt-zlib option. Does your Qt satisfy this requirement?

cen1 commented 8 months ago

I used that. I used the same configures on Windows as the ones in Dockerfiles for this MR.

aikawayataro commented 8 months ago

Oh, I realized why. Accessing the BundledZLIB component requires Qt to be built statically. #182 was just a wild guess, and needs to be umerged. I tested #171 with Qt 6.4.3, and yeah, it works only with static Qt. Would you like to test it with newer version? If this works, we still can use QtZlib, just leave a static Qt requirement notice in README/doc.

cen1 commented 8 months ago

Linux report: must add -no-zstd for static build due to bug https://bugreports.qt.io/browse/QTBUG-119469

Then:

cmake -B build -DQUAZIP_QT_MAJOR_VERSION=6 -DBUILD_SHARED_LIBS=OFF -DQUAZIP_ENABLE_TESTS=ON -DQUAZIP_USE_QT_ZLIB=ON -DCMAKE_PREFIX_PATH="/home/git/personal/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake"
CMake Error at /home/git/personal/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake/Qt6/FindWrapZLIB.cmake:4 (include):
  include could not find requested file:

    QtFindWrapHelper
Call Stack (most recent call first):
  /usr/share/cmake-3.25/Modules/CMakeFindDependencyMacro.cmake:47 (find_package)
  /home/git/personal/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake/Qt6/QtPublicDependencyHelpers.cmake:36 (find_dependency)
  /home/git/personal/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake/Qt6Core/Qt6CoreDependencies.cmake:30 (_qt_internal_find_third_party_dependencies)
  /home/git/personal/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake/Qt6Core/Qt6CoreConfig.cmake:41 (include)
  /home/git/personal/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake/Qt6/Qt6Config.cmake:164 (find_package)
  CMakeLists.txt:75 (find_package)

Seems completely broken. Checking Windows over the weekend.

cen1 commented 8 months ago

Same error on Windows, 6.6.2 and 6.4.3.

My steps:

  1. Get 6.6.2 or 6.4.3 source
  2. configure -release -qt-zlib -static -opensource -confirm-license -nomake examples -nomake tests -skip qtwebengine -skip qtwebview, build
  3. cmake -B build -DQUAZIP_QT_MAJOR_VERSION=6 -DBUILD_SHARED_LIBS=OFF -DQUAZIP_ENABLE_TESTS=ON -DQUAZIP_USE_QT_ZLIB=ON -DCMAKE_PREFIX_PATH="C:/Users/me/git/qt-everywhere-src-6.6.2-static/qtbase/lib/cmake"

It is as if qtbase/cmake is not added to CMAKE_MODULE_PATH by Qt for some reason. I must be missing something.

edit: looks like performing the install step fixes the cmake issue, weird how a shared build just works with prefix pointing to the build folder but not static one. Also a good idea to install using elevated prompt.

TODO: document to always install and not point to qtbase build dir

edit 2: Using BundledZLIB on master -> Cannot open include file: 'zlib.h'

edit3: looks like we also need to search for Qt6::ZlibPrivate together with Qt6::BundledZLIB. That provides the missing headers. Remaining problem: tests fail to link

aikawayataro commented 8 months ago

That's weird. I checked now on Linux, everything works as it should. My Qt configuration: qtbase-everywhere-src-6.6.2/configure -static -qt-zlib -no-xcb -prefix "$QTPREFIX". (no -no-zstd workaround here) My QuaZip configuration: cmake ../ -DQUAZIP_QT_MAJOR_VERSION=6 -DQUAZIP_USE_QT_ZLIB=ON

edit3: looks like we also need to search for Qt6::ZlibPrivate together with Qt6::BundledZLIB. That provides the missing headers. Remaining problem: tests fail to link

It works with system zlib headers for me. But searching ZlibPrivate for headers sounds more convenient.

I'll write dockerfiles to repro. At this moment I have everything working on versions 6.4.3 and 6.6.2 on both systems. Environments are ArchLinux and MSYS2 MinGW on Windows.

aikawayataro commented 8 months ago

Here is the Dockerfile.alpine.txt for the Linux, run it with docker build --build-arg QT_VERSION=X.X.X --progress=plain -f Dockerfile.alpine .

About Windows, I haven't worked with MSVC for a long time, could you make an MSVC build similar to this configuration? I will provide MinGW dockerfile later this week.

cen1 commented 8 months ago

Mixing system zlib headers and Qt zlib, I'd consider that cheating :)

Can confirm that the docker provided also links with qztest so it must be a windows specific thing I'm facing. I think I'll figure it out soon.

cen1 commented 8 months ago

Almost fully working now, passes qt5 and qt6 on ubuntu and Windows: https://github.com/cen1/quazip/actions/runs/8460101363

Interestingly enough, a Japanese test is failing on Qt5 and Ubuntu for some reason, need to investigate that, then finally clean up and we're good to go.

One minor thing is that I don't see the full ctest verbose log in Win job for some reason. Displays fine in ubuntu and local Win run.

I couldn't find a better way of getting Win Qt static build on the Win runner other than straight up building it from source so the initial run is quite long but cache makes it acceptable.

cen1 commented 8 months ago

Closing in favour of https://github.com/stachenov/quazip/pull/194

higaski commented 7 months ago

I could not get this to work with dynamically linking Qt6 and I'm not sure going down that route is a good idea. After all the Qt maintainers seem to discourage this usage. I seems that in the long run quazip should either integrate zlib as optional target or simply add it as dependency.

cen1 commented 7 months ago

@higaski you should get a failure at configure step for dynamic builds, it only works for static. I've documented that this flag is not recommended in the readme. The only reason it's not dropped is that a lot of people apparently wanted it through the years and a lot of hours went into fixing all the issues so might as-well keep it (for now).

higaski commented 7 months ago

Sorry, must have missed the part where this was already documented!

cen1 commented 7 months ago

The info about BUILD_SHARED_LIBS incompatibility is missing, I'll add that now.