Create version-less KF cmake targets
Closed, WontfixPublic

Description

Since 5.15 Qt has version-less cmake targets (e.g. Qt::Core instead of Qt5::Core). Not only is it very useful when porting to Qt6 but it also makes sense in general to not have the major version as part of the target.

We probably want to have the same for Frameworks

dfaure added a subscriber: dfaure.Feb 28 2021, 10:49 AM

I'm wondering if this is really a good idea. Both for Qt and KF.

find_package(Qt 5 COMPONENTS Core) will happily pick up Qt5, Qt6 or Qt7, no? But that doesn't mean the code is ready for Qt6 or Qt7, so we don't want that for Qt5 code.

I see find_package does support a max version (since cmake 3.19), but that means people will need to remember to write this instead
find_package(Qt 5...<6)
or for Qt6,
find_package(Qt 6...<7)

The same problem will happen for KF5/KF6, no? What's the plan to avoid picking up a future major version that the code isn't compatible with?

Just a random thought, maybe use KF_CURRENT_VER which is version-less, and define KF_CURRENT_VER=5, then for 6 we just change one line and the rest will just use that value?

It could even be in ECM, so it's one line for the whole of KF?

I don't see how that would work, it just shifts the problem to "which version of ECM is installed on this system?".
You'd do find_package(ECM) in kmyapp, and hope it finds the version that defines the KF version to the one that this code is compatible with? Too much indirect magic.

And if just per Framework?

And if just per Framework?

Replying to myself, then it's just the same as the current situation since we can change all KF5 occurrences in a repo with one command (Kate search and replace in all files, or perl -pie).

I'm wondering if this is really a good idea. Both for Qt and KF.

find_package(Qt 5 COMPONENTS Core) will happily pick up Qt5, Qt6 or Qt7, no? But that doesn't mean the code is ready for Qt6 or Qt7, so we don't want that for Qt5 code.

Finding and using targets are two separate issues though. You can continue to use find_package(Qt5 ...) but link against Qt::Core. The versionless targets still reduce the number of places that hardcode a major version considerably in that case.

Ah! I misunderstood. Yes indeed, targets would be fine.

(I learned something new, and sorry for the noise).

skelly added a subscriber: skelly.Mar 27 2021, 12:39 PM

I'm wondering if this is really a good idea. Both for Qt and KF.

find_package(Qt 5 COMPONENTS Core) will happily pick up Qt5, Qt6 or Qt7, no? But that doesn't mean the code is ready for Qt6 or Qt7, so we don't want that for Qt5 code.

Finding and using targets are two separate issues though. You can continue to use find_package(Qt5 ...) but link against Qt::Core. The versionless targets still reduce the number of places that hardcode a major version considerably in that case.

As far as I know, it also makes it impossible to use Qt5-based and Qt6-based code in the same buildsystem. Doesn't seem worth it to me https://bugreports.qt.io/browse/QTBUG-83774 I'm not sure what the current state of the Qt 6 cmake files is though.

As far as I know, it also makes it impossible to use Qt5-based and Qt6-based code in the same buildsystem. Doesn't seem worth it to me https://bugreports.qt.io/browse/QTBUG-83774 I'm not sure what the current state of the Qt 6 cmake files is though.

For that there is a way to disable version-less targets in Qt though, for exactly that use-case. That's however not something we do in any of our code-bases at this point, is it? We shouldn't leak this into our CMake config files though to not break this for our consumers, but I think that's doable?

The proposal is to create unversioned KF:: targets in KF5 and in KF6 to be used for transition. The danger is that such targets get used permanently instead of just for transition. It might be better if such targets are created to use KFTransitional:: prefixed targets instead of KF::.

The technical reason versionless targets should not be used looks something like this:

User code such as

target_link_libraries(myapp PRIVATE
    Foo::FooLib
    KF::ItemModels
)

breaks if the Foo::FooLib has Qt::Core (where libFooLib.so requires libQt5Core.so) in its INTERFACE_LINK_LIBRARIES and KF::ItemModels has Qt::Core (where libKF6ItemModels requires libQt6Core.so) in its INTERFACE_LINK_LIBRARIES.

Assuming we can't influence the development of Foo::FooLib, we can at least ensure that KF:: frameworks do not ever use Qt:: targets in INTERFACE_LINK_LIBRARIES, but ensure that they use Qt6:: targets instead. Ensuring that is probably very difficult because it requires that no one ever add a Qt:: target in the target_link_libraries of any framework.

This applies to users of frameworks.

The bug in:

target_link_libraries(myapp PRIVATE
    KF6::ItemModels
    KF5::CoreAddons
)

is discoverable and almost obvious.

The bug in

target_link_libraries(myapp PRIVATE
    KF::ItemModels
    KF::CoreAddons
)

is not discoverable, but it can occur in various ways. A relatively discoverable way it could occur is

find_package(KF6ItemModels)
find_package(KF5CoreAddons)

This is only relatively discoverable because it is potentially very far away from the myapp target which the build will indicate is the problem.

A less obvious way this could occur is

find_package(KF6ItemModels)
find_package(Something) # uses find_package(KF5CoreAddons)

or

find_package(KF6ItemModels)
include(Something.cmake) # uses find_package(KF5CoreAddons)

or

find_package(KF6ItemModels)
find_package(${SOMETHING}) # SOMETHING is KF5CoreAddons, but where is it set?

or

find_package(KF6ItemModels)
# git grep find_package won't find this one:
do_something()

There is no need to expose ourselves to these kinds of problems.

So, the post-port-KF6 way to write cmake code should be to use versioned Qt6:: targets everywhere (so that we never accidentally add an unversioned Qt:: target to our INTERFACE_LINK_LIBRARIES) and we always use KF6:: targets so that our users don't have these problems either. Also, if that is what we decide to do, then we should create KFTransitional:: targets which are named to make it obvious that they are not for long-term use. KF:: targets would look like they are for long-term use.

After some more discussion with Steve yesterday I realized the full implications of my earlier statement that versionless targets must not leak into CMake config files:

We do not want version-less targets in the CMake config files for two reasons:

  • Their meaning there depends on the user code, not on what we expected. If that doesn't match you end up mixing different major versions with unpredictable consequence at compile-, link- and runtime.
  • This breaks users that have version-less targets disabled for whatever reason.

However, even a single use of a version-less target in a target_link_library call will cause such a leak, and that's exactly what we want to use it to make the transition easier.

Ideas that came up so far to address this:

  • Disable and ban version-less targets in everything that installs CMake config files. Prevents the above problem, but makes the transition harder.
  • Introduce special transition targets as Steve proposed. Those don't prevent the above problem, but at least limit them to a short transitional period.
  • Use transitional variables for this (ie. ${SOMEVAR_CONTAINING_Qt5_OR_Qt6}::Core). Ugly, but supports the transition while avoiding the use of the version-less targets.
  • User version less targets but have a safety check in the config files making sure the major version is what we expect. This could fix part of the problem, but would still prevent users from having version-less targets disabled.
ervin moved this task from Needs Input to In Discussion on the KF6 board.Mar 28 2021, 1:35 PM

I forgot about it during the discussion yesterday, but the COMPATIBLE_INTERFACE_STRING and INTERFACE_QT_MAJOR_VERSION property are designed to prevent the accidental mixing

Qt 4 has something like

set_property(TARGET Qt4::QtCore APPEND PROPERTY COMPATIBLE_INTERFACE_STRING QT_MAJOR_VERSION)
set_property(TARGET Qt4::QtCore APPEND PROPERTY INTERFACE_QT_MAJOR_VERSION 4)

Qt5 has:

set_property(TARGET Qt5::Core APPEND PROPERTY COMPATIBLE_INTERFACE_STRING QT_MAJOR_VERSION)
set_property(TARGET Qt5::Core APPEND PROPERTY INTERFACE_QT_MAJOR_VERSION 5)

Qt6 has:

set_property(TARGET Qt6::Core APPEND PROPERTY COMPATIBLE_INTERFACE_STRING QT_MAJOR_VERSION)
set_property(TARGET Qt6::Core APPEND PROPERTY INTERFACE_QT_MAJOR_VERSION 6)

The versionless targets should have the same properties available, and that should cause cmake to diagnose accidental mixing of the different versions which should mean that something like

target_link_libraries(myapp PRIVATE
    KF::ItemModels
    KF::CoreAddons
)

should either work (compatible Qts) or result in a cmake diagnosis of the problem.

However, code should also look correct when humans read it.

Those targets are probably fine for transition, but longer term (once the code depends on Qt 6 as a minimum) we should use versioned targets. The same will apply to KF6 targets and users of them (ie we can provide versionless KF:: targets but aim to only use them in transition and aim to migrate to KF6:: targets when possible).

ervin moved this task from In Discussion to Needs Input on the KF6 board.Mar 28 2021, 3:51 PM
dfaure added a comment.Apr 3 2021, 9:11 PM

@skelly From what I understand, the problem with versionless targets is when they "leak" into generated FooConfig.cmake files, and then users can end up with bad mix-and-match.

Wouldn't the best solution to this problem be a way to ask cmake to resolve targets at the time of writing out FooConfig.cmake files?
I understand this would require a change in cmake itself, but would it be doable?

@skelly From what I understand, the problem with versionless targets is when they "leak" into generated FooConfig.cmake files, and then users can end up with bad mix-and-match.

Yes.

Wouldn't the best solution to this problem be a way to ask cmake to resolve targets at the time of writing out FooConfig.cmake files?

It seems easier to just not use them in KF libraries, except during transition?

I understand this would require a change in cmake itself, but would it be doable?

Yes, it would be possible to implement I think. It would have to be a generic feature of "replace this target name with that target name in the INTERFACE_LINK_LIBRARIES strings" It would be a bit niche, and it would have to be configured somehow in the KF buildsystem files.

It seems easier to me to just not use them in KF libraries, except during transition? Something like that seemed to work well for the 4->5 transition.

Personally, and I am not an expert on cmake, versioned targets are more deterministic; yes the transition is slightly harder, but it's a one time effort, right? I mean we'd branch, change from Qt5 ->> Qt6 and KF5 ->> KF6, then we're done for the next lifetime span of Qt6/KF6.

Well, it makes sense to prolong as much as we can the transitional period where the same codebase can be built with both Qt5/KF5 and Qt6/KF6.
(e.g. I just ported libkode and KDSoap that way, master supports both Qt5 and Qt6. It's so much easier to have it all in a single branch...).

But indeed we're succeeding in doing this without versionless targets at the Qt level (just some QT${QT_MAJOR_VERSION}), so surely the same can be done for KF dependencies.

So I've changed my mind, let's just drop the idea of version-less KF cmake targets, for the same reason we're not using the Qt ones.

@nicolasfella (since you created this task) -- OK to close it?

dfaure moved this task from Needs Input to Done on the KF6 board.Apr 14 2021, 6:55 PM

Can't find a Canceled column.

So I set it to "Done" as in: done deciding not to do it :)

dfaure closed this task as Wontfix.Apr 14 2021, 6:55 PM
dfaure claimed this task.