Skip to content

Port to Qt6/KF6 #26

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

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

Port to Qt6/KF6 #26

wants to merge 6 commits into from

Conversation

davispuh
Copy link
Contributor

@davispuh davispuh commented Mar 6, 2023

This allows to build it against Qt6/KF6. Depends on #25

@j6t
Copy link
Owner

j6t commented Mar 12, 2023

Thank you for your contribution. Unfortunately, in the current form I cannot merge the branch because it requires Qt6 and KF6 unconditionally. This is a precondition that we cannot require from our consumers at this time.

The best course of action would be to have a build system that adjusts to the libraries that are available or can choose between Qt5 and Qt6. But I am not a cmake expert and do not know how to accomplish this or if it is possible at all.

Some of the patches look like they could be applied to a Qt5-based build nevertheless, e.g., perhaps the sorry() -> error() change. Shuffling around the patches would probably suffice, i.e., that the switch to Qt6/KF6 happens as late as possible in the series.

@davispuh
Copy link
Contributor Author

I don't really need it merged. Just putting it out here :P Also yes it's perfectly possible to support both Qt5 and Qt6 at same time but this was just quick way to get it build.

There are several approaches to make it work, you can take a look at https://doc.qt.io/qt-6/cmake-qt5-and-qt6-compatibility.html

Anyway I won't be investing any more time in this so if you think any commits from here could be useful you can just take those (and fix commit messages aswell).

@a17r
Copy link
Contributor

a17r commented Sep 9, 2024

@j6t what do you think about this now, 1.5 years later?

@davispuh meanwhile, it doesn't build anyway with errors like these:

regwnd.cpp:232:34: error: conversion from ‘unsigned int’ to ‘QChar’ is ambiguous
  232 |         result.insert(0, (v & 7) + '0');
      |                          ~~~~~~~~^~~~~

and

CMake Error at kdbg/CMakeLists.txt:100 (install):
  install FILES given no DESTINATION!

(needs porting away from deprecated KDEInstallDirs)

@j6t
Copy link
Owner

j6t commented Sep 9, 2024

My system is still on KF5 and I assume that I am not the only one stuck to KF5. So my stance hadn't changed: an unconditional switch to KF6 is not welcome. The other points I mentioned above are still valid.

@davispuh
Copy link
Contributor Author

davispuh commented Sep 9, 2024

@davispuh meanwhile, it doesn't build anyway with errors like these:

regwnd.cpp:232:34: error: conversion from ‘unsigned int’ to ‘QChar’ is ambiguous
  232 |         result.insert(0, (v & 7) + '0');
      |                          ~~~~~~~~^~~~~

and

CMake Error at kdbg/CMakeLists.txt:100 (install):
  install FILES given no DESTINATION!

(needs porting away from deprecated KDEInstallDirs)

When I submitted this PR then it built fine for me on Arch Linux. It's possible now it needs more changes to get it working with everything latest.

@a17r
Copy link
Contributor

a17r commented Nov 9, 2024

My system is still on KF5 and I assume that I am not the only one stuck to KF5. So my stance hadn't changed: an unconditional switch to KF6 is not welcome. The other points I mentioned above are still valid.

Why not cherry-pick from this MR what you know will work for KF5 still?

@hartwork
Copy link
Contributor

@j6t I can offer help teaming on a KF5/KF6 hybrid in a pair programming video call in English or German if of interest. If so please reach out via the e-mail address in my profile so we can find a day and time that works for both of us. In case you're attending Fosdem in February, teaming up about it there could also work. Just an idea, don't feel pushed please.

@a17r
Copy link
Contributor

a17r commented Jan 28, 2025

At least Gentoo and Ubuntu have started Qt5 removal trackers:
https://bugs.gentoo.org/show_bug.cgi?id=qt5-removal
https://ubuntu-archive-team.ubuntu.com/transitions/html/qt5-removal.html

@j6t
Copy link
Owner

j6t commented Jan 29, 2025

Thank you for the heads-up.

@a17r
Copy link
Contributor

a17r commented Apr 5, 2025

@hartwork I've rebased this PR with some fixes on top, if you'd like to test in particular wrt KWindowSystem change: https://github.com/j6t/kdbg/compare/master...a17r:kdbg:qt6?expand=1

@hartwork
Copy link
Contributor

hartwork commented Apr 5, 2025

@a17r thank you! 👍

@j6t
Copy link
Owner

j6t commented Apr 6, 2025

Sorry for being slow. I have a months-old branch that contains many of the changes that are in this PR. The reason I did not publish it so far is that it requires set(KF5_MIN_VERSION "5.101.0"), but that breaks one of the CI runs (example).

What do we do about this?

@a17r
Copy link
Contributor

a17r commented Apr 6, 2025

if you must keep it lower, put it behind an ifdef.

... otherwise, my recommendation has been, and still is: branch off qt5-stale, then throw away the old stuff and merrily raise minimum versions in git master.

@j6t
Copy link
Owner

j6t commented Apr 6, 2025

I do not want to have fewer CI runs. I can agree to move to a newer distribution in the CI run and leave old distributions untested that aren't going to back-port new versions anyway. However, I would have to familiarize myself with CI infrastructure before I can make such a change. Any help would be appreciated.

@a17r
Copy link
Contributor

a17r commented Apr 6, 2025

I think @hartwork may be just waiting to help with that ^

... finally, if you need help - if this is still your plan - in making a dual purpose Qt5/Qt6 CMakelists.txt, that is very easily done after all the code deprecations were dealt with, and I can look to producing such a PR.

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.

4 participants