stachenov / quazip

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

Build breaking on Windows #121

Closed jacquetc closed 3 years ago

jacquetc commented 3 years ago

Hello,

I tested the latest merged PR #111 for fun in my CI. Building on Linux is OK. Yet, it's missing a Qt5::Zlib in Windows 10 despite having zlib available. Ckecking out back to 7d7661de38843fbe47af2132d800bcf52d645a55 (#109) solves the problem.

I am using standard Qt 5.15.2 MinGW 64 bit with CMake

Oh, and despite this bug, nice work for Qt6 ;-) v1.1 runs like a charm !

stachenov commented 3 years ago

What exact build errors are you getting? I've just tested it with the same setup, works just fine.

jacquetc commented 3 years ago

CMake Error at src/plugins/plumeCreatorImporter/CMakeLists.txt:34 (add_library): Target "skribisto-plugin-plumeCreatorImporter" links to target "Qt5::Zlib" but the target was not found. Perhaps a find_package() call is missing for an IMPORTED target, or an ALIAS target is missing?

stachenov commented 3 years ago

This CMakeLists.txt is not QuaZip's one. It could be an error caused by the recent QuaZip changes, or it could be an error caused by that module, and QuaZip only played a part of triggering that error. Need to dig deeper into that CMakeLists.txt.

Standalone QuaZip compiles just fine on Windows 10 + Qt 5.15.2 MinGW 64, all tests pass.

jacquetc commented 3 years ago

I found this little issue with other problems with Qt5::Zlib. It seems that Qt5::Zlib is not meant to be used by projects external to Qt, what do you think of it ?

You can find my CMakeLists.txt here which is triggered by this script. In CMakeLists.txt, you only have to replace the GIT_TAG v1.1 by GIT_TAG de8125cf8cf8422dae5df87ed6b5be9305219594 in quazip section (second block)

I didn't test to build without including any zlib

stachenov commented 3 years ago

On one hand, using Qt5Zlib is indeed looks like a dirty hack. But on another hand, installing a separate copy of zlib on Windows is, well, a bit painful. So I thought that using Qt5Zlib when it's available and falling back to a stand-alone zlib is a nice compromise.

I'll have a deeper look into those scripts when I can.

jacquetc commented 3 years ago

I understand that it would be tempting to use this internal Zlib. Less hassle.

For test purpose, I removed all mentions of my Zlib library, hoping that Quazip would hook unto the Qt5 one. No luck.

  Could NOT find ZLIB (missing: ZLIB_INCLUDE_DIR)
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.16/Modules/FindPackageHandleStandardArgs.cmake:393 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.16/Modules/FindZLIB.cmake:115 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  C:/Program Files/CMake/share/cmake-3.16/Modules/CMakeFindDependencyMacro.cmake:47 (find_package)
  C:/Users/jacqu/Devel/build_skribisto_Release/3rdparty/quazip/lib/cmake/QuaZip-Qt5-1.1/QuaZip-Qt5Config.cmake:29 (find_dependency)
  src/plugins/plumeCreatorImporter/CMakeLists.txt:74 (find_package)

CMakeLists.txt includes Quazip this way :

find_package(QuaZip-Qt${QT_VERSION_MAJOR} REQUIRED)
target_link_libraries(${PROJECT_NAME} PRIVATE QuaZip::QuaZip)
target_include_directories(${PROJECT_NAME} SYSTEM PUBLIC ${QuaZip_INCLUDE_DIRS})
simonmeaden commented 3 years ago

I have a similar problem, only with Qt6 on Linux rather than windows. I am including QuaZip as a git submodule and have ZLIB::ZLIB on my machine (including devel). BUILD_SHARED_LIBS is set OFF and add_subdirectory(quazip) in my main CMakeLists.txt.

The errors I got were:

-- Could NOT find Qt6Zlib (missing: Qt6Zlib_DIR)
CMake Warning at /usr/lib64/cmake/Qt6/Qt6Config.cmake:120 (message):
  Failed to find Qt component "Zlib" config file at ""
Call Stack (most recent call first):
  quazip/CMakeLists.txt:62 (find_package)

eventually I just commented out the section that searched for the Qt zlib libraries.

#find_package(Qt${QUAZIP_QT_MAJOR_VERSION} OPTIONAL_COMPONENTS Zlib)
#if (Qt${QUAZIP_QT_MAJOR_VERSION}Zlib_FOUND)
#   set(QUAZIP_LIB_LIBRARIES ${QUAZIP_LIB_LIBRARIES} Qt${QUAZIP_QT_MAJOR_VERSION}::Zlib)
#else()
  find_package(ZLIB REQUIRED)
    set(QUAZIP_LIB_LIBRARIES ${QUAZIP_LIB_LIBRARIES} ZLIB::ZLIB)
#endif()

and it now works.

simonmeaden commented 3 years ago

OK looks like I should have surrounded the commented code with markdown sorry.

jacquetc commented 3 years ago

Hello, you can always edit your previous message 😁

stachenov commented 3 years ago

Turns out I can edit it too. The # character stands for headings in Markdown, hence that huge font. Fixed that in the simplest way I could.

Back to topic, that warning doesn't look like a fatal error. It's exactly what's expected to happen when Qt6Zlib is not found, and QuaZip falls back to ZLIB::ZLIB. It does specify Zlib as OPTIONAL for that very reason. Do you mean it still fails to build then? Or does it simply produce a warning that you found annoying and therefore commented out that code?

Need to look into it further, but I'm starting to think that maybe it's a good idea to make use of QtXZlib as an OPTION defaulting to OFF. This way someone who builds QuaZip on Windows with no zlib installed can still use the dirty hack of (ab)using QtXZlib, but they will have to ask for it explicitly.

jacquetc commented 3 years ago

Good idea !

Such change of behavior could break exotic and more complex builds. Making it an option can ease a lot of work for maintainers, but offer a serious help for new developers using Quazip.

simonmeaden commented 3 years ago

Ah my bad I think. I forgot that I set target_compile_options(libepubedit PRIVATE -Werror). That's probably why it failed. I agree that using QtXZlib as an option would be a good idea however.

stachenov commented 3 years ago

For some reason, I get a different error on this project:

CMake Error at src/plugins/plumeCreatorImporter/CMakeLists.txt:34 (add_library):
  Target "skribisto-plugin-plumeCreatorImporter" links to target "Qt5::Zlib"
  but the target was not found.  Perhaps a find_package() call is missing for
  an IMPORTED target, or an ALIAS target is missing?

Looking at the line 34 doesn't help at all:

add_library(${PROJECT_NAME} SHARED ${SRCS} ${RESOURCES})

How Qt5::Zlib comes out of this line is anyone's guess. I don't really have time nor desire to dig deeper into this CMake hell, so I'm making it an option defaulting to OFF and let the users decide.