Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow building against Qt6 #445

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

nicolasfella
Copy link
Contributor

This adds a new option qt6 to build against Qt6 instead of Qt5

It is incompatible with qt, i.e. it is only possible to build against either Qt5 or Qt6 at a time

@nicolasfella
Copy link
Contributor Author

Needs #443 and #444 to actually build against Qt6

@aleixpol
Copy link
Collaborator

Shouldn't this be bumping sonames?

It might be a good opportunity to review the API and see if there's something we can do better. For example it would make sense to have the strings passed as QStringView to it on queries.

@ximion
Copy link
Owner

ximion commented Nov 1, 2022

It might be a good opportunity to review the API and see if there's something we can do better. For example it would make sense to have the strings passed as QStringView to it on queries.

That's what I am thinking as well. I do actually also wonder whether it would make sense to wait with this for the AppStream 1.0 release (which I expect to happen either this year or early next year (January/February)) and make the Qt library Qt6-only while also breaking API anyway for the 1.0 changes (those changes would certainly be smaller than for libappstream, because libappstream-qt's API surface is a lot smaller).
With @aleixpol's KDE Discover being probably the largest consumer of the Qt interface, that break would be an ideal opportunity to make any changes useful for that project.

@nicolasfella
Copy link
Contributor Author

I do actually also wonder whether it would make sense to wait with this for the AppStream 1.0 release (which I expect to happen either this year or early next year (January/February)) and make the Qt library Qt6-only while also breaking API anyway for the 1.0 changes

If we do that then we probably need to make sure that 0.x and 1.0 are coinstallable. Otherwise this is going to be a problem for every consumer of appstream-qt that isn't on Qt6 yet

@Conan-Kudo
Copy link
Contributor

Conan-Kudo commented Feb 12, 2023

Please do a soname bump for AppStream and AppStreamQt for 1.0 so that they can be co-installable. That way, if needed, both can exist in parallel in Fedora (and especially EPEL, where appstream is part of RHEL and we might need the newer one for Plasma...).

@nicolasfella
Copy link
Contributor Author

How to we proceed here? We need some way of having Qt5 and Qt6 variants coinstallable, either by having some kind of version number suffix or by having 0.x for Qt5 and 1.0 for Qt6 (and have 0.x and 1.0 coinstallable)

@ximion
Copy link
Owner

ximion commented Feb 21, 2023

by having 0.x for Qt5 and 1.0 for Qt6 (and have 0.x and 1.0 coinstallable)

This. AppStream 1.0 will have a SONAME bump anyway because it will break API, and while we're at it we can also make the Qt library Qt6-only. That simplifies maintenance and also aligns extremely well with KDE Discover's schedule, which is the biggest consumer of the Qt API (the 5.27 series is frozen, and the next one will be Qt6).
So, that plan is the one I favour heavily, as it solves all issues quite nicely :-) (and also means only one API break on the Qt side, instead of two)

nicolasfella and others added 2 commits March 3, 2023 00:57
This adds a new option qt6 to build against Qt6 instead of Qt5

It is incompatible with qt, i.e. it is only possible to build against either Qt5 or Qt6 at a time
@nicolasfella
Copy link
Contributor Author

Is there a timeline for 1.0?

With Plasma we're in the unfortunate situation where we require Qt6 now, but can't build any of the things that depend on appstream (most notably Discover). A release based on Qt6 is still several months in the future, but for development it would be good to have appstream-Qt6 available.

Would it make sense to merge this to master now so that we have at least the ability to develop Plasma against appstream master?

@ximion
Copy link
Owner

ximion commented Mar 5, 2023

Is there a timeline for 1.0?

No, but I expect to branch it very soon, so you could use a development version (still have a bunch of other tasks to do prior to that though, so I'd most likely start the 1.0 development by the end of this week / early next week).
A release will definitely happen until this summer (but I don't have a specific date).

@aleixpol
Copy link
Collaborator

aleixpol commented Mar 6, 2023

The reason why I wanted to have this transition under control is because I'd like to switch the APIs here to be using QAnyStringView for Qt 6 instead of QString. This would allow us to save a number of utf8 -> utf16 -> utf8 conversions.

Now this is something not trivial and it might as well not be that many conversions to save in practice, so let's not make our life more difficult than it needs to be.

Regarding soname bump, it could be complex if we are talking about keeping support for both Qt 5 and 6 for a while, so I guess we should version the library name.

Copy link
Collaborator

@aleixpol aleixpol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's do this?

Copy link
Collaborator

@aleixpol aleixpol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ehm just re-read my last message. @ximion how would you like to do with Qt 5 and Qt 6 builds?

@aleixpol
Copy link
Collaborator

To get the discussion un-stuck, we could consider doing this for now, adding a 6 on the .so file name and we have some flexibility until we decide to keep its ABI. Would that be a good solution?

diff --git a/qt/cmake/AppStreamQtConfig.cmake.in b/qt/cmake/AppStreamQtConfig.cmake.in
index 0e3eb118..79fd4034 100644
--- a/qt/cmake/AppStreamQtConfig.cmake.in
+++ b/qt/cmake/AppStreamQtConfig.cmake.in
@@ -32,8 +32,8 @@ add_library(AppStreamQt SHARED IMPORTED)
 set_target_properties(AppStreamQt PROPERTIES
   INTERFACE_INCLUDE_DIRECTORIES "${PACKAGE_PREFIX_DIR}/include/"
   INTERFACE_LINK_LIBRARIES "Qt::Core"
-  IMPORTED_LOCATION "@LIBDIR_FULL@/libAppStreamQt.so.${AppStreamQt_VERSION}"
-  IMPORTED_SONAME "libAppStreamQt.${AppStreamQt_VERSION_MAJOR}"
+  IMPORTED_LOCATION "@LIBDIR_FULL@/libAppStreamQt@AppStreamQt_POSTFIX@.so.${AppStreamQt_VERSION}"
+  IMPORTED_SONAME "libAppStreamQt${AppStreamQt_POSTFIX}.${AppStreamQt_VERSION_MAJOR}"
 )
 
 ####################################################################################
diff --git a/qt/meson.build b/qt/meson.build
index 6e2189da..36966d57 100644
--- a/qt/meson.build
+++ b/qt/meson.build
@@ -72,7 +72,13 @@ if get_option('maintainer')
     asqt_cpp_args += ['-Wno-inline']
 endif
 
-appstreamqt_lib = library ('AppStreamQt',
+asqt_postfix = ''
+if get_option('qt6')
+    asqt_postfix = '6'
+else
+    asqt_postfix = ''
+endif
+appstreamqt_lib = library ('AppStreamQt' + asqt_postfix,
     [asqt_src,
      asqt_pub_headers,
      asqt_priv_headers,
@@ -106,6 +112,7 @@ cmake_data = configuration_data()
 cmake_data.set('LIBDIR_FULL', join_paths(get_option('prefix'), get_option('libdir')))
 cmake_data.set('PREFIX', get_option('prefix'))
 cmake_data.set('VERSION', as_version)
+cmake_data.set('AppStreamQt_POSTFIX', asqt_postfix)
 
 configure_file (input: 'cmake/AppStreamQtConfig.cmake.in',
                 output: 'AppStreamQtConfig.cmake',

@nicolasfella
Copy link
Contributor Author

Any news on a 1.0/Qt6 branch?

Just having a branch available that builds with Qt6 would be helpful for us for now

@ximion
Copy link
Owner

ximion commented Apr 7, 2023

I'll get that done early next week :-) (likely Monday/Tuesday)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants