Support installing qt5 and qt6 versions in parallel

[DarthGandalf]

I tried to make old find_package(QXlsx) still work as before, but this looks pretty ugly...

[xakod]

Do you really wanna break existing users cmakelists with this changes? Looks like if i use find_package(QXlsx) cmakelist will be broken with this changes

[DarthGandalf]

This is why first I added the fallback option with QXlsx without specifying Qt... But @dantti didn't like that

[dantti]

Sure, CMake support was just recently added and such change makes it future prof for Qt7 as well, this project has no API/ABI guarantees so if one is to update it's version it should be aware of breakages.

[xakod]

Sure, CMake support was just recently added and such change makes it future prof for Qt7 as well, this project has no API/ABI guarantees so if one is to update it's version it should be aware of breakages.

Maybe it will be better to add target without version specified? Current naming has been working for more than 1.5 years and i don't see really necessary to break it. Of course there is no api guarantees but it's not a reason to break api.

[dantti]

Changing find_package has nothing to do with API

[DarthGandalf]

It's part of API. Existing software relied on find_package(QXlsx), but this won't work anymore

[dantti]

@Jihadist I don't like breaking things, but currently this is already broken for me too now, as I need both installed, also relying on QXlsx::QXlsx-qt6 target won't help if both are installed but find_package finds the qt5 first, this is even the case for Qt it self that if you have both from distro QtCreator doesn't set the proper CMAKE_PREFIX_PATH @DarthGandalf there is also another issue that I noticed while testing cpack that the headers are both installing to the same place...

[DarthGandalf]

Yes, headers are installed to the same place, but why is it a problem? Is there something in the headers themselves which is different between qt5 vs qt6?

[dantti]

looking again at current changes, please don't append QtX to it, only at the target files, this way the only needed change is find_package( +QtX)

[DarthGandalf]

If there's something different, I can make it easily install to different places, no big deal

[DarthGandalf]

looking again at current changes, please don't append QtX to it, only at the target files, this way the only needed change is find_package( +QtX)

So you want the target name to stay QXlsx::QXlsx, right? Sure, can do. The downside is that it will be impossible to use the qt5 version and qt6 version in the same cmake project for different executables. But probably that's not a big problem

[dantti]

I believe that's probably already impossible or if possible not something I would care :) same binary can't use both anyway

[xakod]

relying on QXlsx::QXlsx-qt6 target won't help if both are installed but find_package finds the qt5 first, this is even the case for Qt it self that if you have both from distro QtCreator doesn't set the proper CMAKE_PREFIX_PATH

If it's the only one problem it can be fixed like this without breaking api:

if(USE_QT6)
    find_package(QT NAMES Qt6)
else()
    find_package(QT NAMES Qt5)
endif(USE_QT6)

[dantti]

That's the same thing just making more ugly cmake files, I have lots of stuff on Qt6 already, but due "new" Qt5 projects they are now broken.. it's not like users would need to rewrite their entire software

[DarthGandalf]

Renamed the imported target back to QXlsx::QXlsx

Left headers as is, because I see no need to install them in different dirs

[dantti]

if you don't change the headers dirs then you can't install Qt5 and Qt6 deb packages for example.

[DarthGandalf]

I think debian package would have headers in something like qxlsx-common package, and both qxlsx-qt5-dev and qxlsx-qt6-dev would depend on that package

[dantti]

debian will likely never package qxlsx, but I only use qxlsx with deb packages thanks to cpack.

[DarthGandalf]

I think it soon will. Debian packages Stellarium, and QXlsx is a dependency of Stellarium since the latest release. In fact the reason for this my PR is because I'm trying to package it for Gentoo.

QXlsx was a dependency of Stellarium even before, but it was done by including a copy of it in directly in sources (with small modifications), which is against policies of linux distros:

• https://fedoraproject.org/wiki/Bundled_Libraries
• https://www.debian.org/doc/debian-policy/ch-source.html#embedded-code-copies
• https://wiki.gentoo.org/wiki/Why_not_bundle_dependencies

[DarthGandalf]

Fedora and FreeBSD packaged it already, because of Stellarium

https://repology.org/project/qxlsx/history

[dantti]

Downstream can patch and fix that there if they want, but likely it shouldn't be available as -dev packages, but if we are fixing it here it should be a proper fix, fixing a find_package is a nobrainer fix

[DarthGandalf]

Downstream can patch and fix that there if they want

Sure, I can. But it's easier to not have any patches, because they may need to be updated whenever upstream releases new version. That's why I opened this PR, to figure out with you how do you want it to look like, so that I can: 1. apply a patch only for this version of qxlsx, 2. drop it for the next version, 3. not forced to update the patch to stellarium when you release the next version of qxlsx.

This is mostly to decide on these details like which string is passed to find_package, and how the imported target will look like, etc, which is part of API, believe it or not :) This already changed several times during this PR review, but it's fine - it's better now than changing the package after it's packaged already

I only use qxlsx with deb packages thanks to cpack.

I have no idea how cpack works, and I don't know whether Debian packagers will be able to use it or how. On Gentoo we run make install to a prefix, and if the library has several variants which can be installed at the same time (like, qt 5 and qt 6), these variants all run make install to the same prefix, then the whole dir is packaged as a single package which is then installed on the system; and depending on user settings the package would contain only qt5 lib, or only qt6 lib, or both.

[DarthGandalf]

but likely it shouldn't be available as -dev packages

AFAIK, they still split headers and other development-only files to a separate -dev package, to avoid need to install useless headers for user who only needs binaries

[xakod]

@DarthGandalf i looked through your changes and have a question. Can i use find_package(QXlsx5) or find_package(QXlsx6) and link against target QXlsx::QXlsx or not? I think this should be mentioned somewhere in docs, it would be nice (here for example https://github.com/QtExcel/QXlsx/blob/master/HowToSetProject-cmake.md).

[DarthGandalf]

@Jihadist in the current version of the PR you use find_package(QXlsxQt5) (or 6), then link to QXlsx::QXlsx.

In the previous versions it varied, from linking to QXlsxQt5::QXlsxQt5 to having find_package(QXlsxQt5) finding Qt5 version specifically and find_package(QXlsx) finding any installed which would kinda preserve back compatibility but will randomly break if the wrong one is found.

Re docs: done. And updated HelloWorld

[xakod]

@DarthGandalf thank you! It looks great. Can you add something more to docs for cmake? I mean cmake provide to use library without direct instaling https://cmake.org/cmake/help/latest/module/FetchContent.html How it was mentioned there https://github.com/QtExcel/QXlsx/issues/49#issuecomment-907870633

[DarthGandalf]

In Stellarium I used CPM to find the system version (via find_package), with fallback to downloading it (via FetchContent).

https://github.com/Stellarium/stellarium/blob/5da735d219dc957eb89c0d3353c645fa239f19eb/CMakeLists.txt#L805-L824

If not for #134, that code could look much simpler:

     CPMFindPackage(NAME QXlsx
	 URL https://github.com/QtExcel/QXlsx/archive/refs/tags/v1.4.4.zip
	 URL_HASH SHA256=3efbd6f63a1ffd521c535dce7b5a5a7e9ebd23db51e6ae8e3e2eb89796e57675
	 EXCLUDE_FROM_ALL ON
	 SOURCE_SUBDIR QXlsx)
     target_link_libraries(myapp QXlsx::QXlsx)

I could put that to docs, but I don't think it would be appropriate there yet, considering #134

PS. of course, I'll need to update Stellarium to use the correct name after this PR is merged.

[DarthGandalf]

...though that issue affects plain add_subdirectory too...

[dantti]

@DarthGandalf I don't understand what's the relation with #134. Now this patch is 99% good, cpack will pack the files that make install would copy to /usr, this means that if we don't fix the headers location it will not work, this doesn't break API because cmake will export the base path, but will fix cpack.

If in 2 years we drop Qt5 support and distros want to keep it, then their -dev will not be the same as header files won't match. If you prefer we can fix on another PR, but should be a simple fix.

[DarthGandalf]

This PR isn't related to #134 directly. I meant it in context of the update to docs.

If in 2 years we drop Qt5 support and distros want to keep it

That's essentially maintaining a fork, which is not very trivial

then their -dev will not be the same as header files won't match

But if they really decide to do that, I'm sure they'll figure out how to deal with headers.