CMake: use PRIVATE linking

[dbermond]

According to Qt6 documentation, Qt libraries should use PRIVATE linking if the headers does not mention anything from Qt6::Widgets. This is the case with QHotkey, and the resulting shared libary libqhotkey.so does not link to libQt{5,6}Widgets.so. Qt5 docs does not say anything about link type, but most probably the logic is the same.

A practical consequence of PUBLIC linking in the current code, is that a Qt5 GNU/Linux (or UNIX) client application is forced to add X11Extras to find_package() when using QHotkey from a shared library (QHotkey built with -DBUILD_SHARED_LIBS=ON). This is mostly undesired.

By applying the change of this commit, Qt5 client applications using the shared library will be free of specifying X11Extras in find_package(). And Qt5 client applications using the static library are unaffected in practical terms, as they will obviously still need to specify X11Extras in find_package(), since the static object code will need to call X11Extras functions. Qt6 client applications will be also unaffected in practical terms, because there is no X11Extras in Qt6.

Tested on Arch Linux x86_64 with Qt 6.2.0 and Qt 5.15.2+kde+r228, each one with both shared and static library, and works fine for me.

Probably a test on Windows and MacOS would be good. I think it will have no impact there, at least not on the current code.

[Shatur]

You shouldn't link all libraries as PRIVATE. All headers that mentioned in public headers of the library should have PUBLIC linkage. So I believe we should link Core as PUBLIC and the rest as PRIVATE. If you agree with me, can I ask you to also switch to target_link_libraries in-place instead of using list and linking all of them later? Also I believe that we can remove linkage to Widgets.

[dbermond]

I think there is no problem in using PRIVATE linking in this case, specially under Qt documentation guidance. But I don't have any objections to use QtX::Core as PUBLIC either, as long as the undesired X11Extras can be gone from a Qt5 client's find_package() :)

I also think that QtX::Widgets can be really removed, since it's not used. When removing it, Qt6::Gui is needed for building on Qt6, otherwise a compile error is shown about the QKeySequence header being not found.

can I ask you to also switch to target_link_libraries in-place instead of using list and linking all of them later?

Something like this? If so, this way looks better and more concise in my humble opinion.

[Shatur]

I think there is no problem in using PRIVATE linking in this case, specially under Qt documentation guidance.

The Qt documentation uses linking to an executable file. If you linking to a library in CMake all dependencies that used in public headers should be satisfied. This is why we need to use PUBLIC for Core. But all other stuff can be moved to PRIVATE for sure :)

I also think that QtX::Widgets can be really removed, since it's not used. When removing it, Qt6::Gui is needed for building on Qt6, otherwise a compile error is shown about the QKeySequence header being not found.

Oh, right, I think we just need to replace Widgets with Gui. And maybe make it PUBLIC if there is any public header that use QKeySequence.

Something like this? If so, this way looks better and more concise in my humble opinion.

Yes, but honestly I like the first variant (variable-less) a little more.

[dbermond]

The Qt documentation uses linking to an executable file

They mention about linking a Qt library too.

From the Qt documentation:

• If helloworld was a library rather than an executable, then either PRIVATE or PUBLIC should be specified (PUBLIC if the library mentions anything from Qt6::Widgets in its headers, PRIVATE otherwise).

Yes, but honestly I like the first variant (variable-less) a little more.

No problem, I'll push the changes :)

[Shatur]

From the Qt documentation:

Oh, right, this is exactly what I talking about :)