WIP: Qt6 transition #11651
WIP: Qt6 transition #11651
Conversation
| # TODO: Increase minimum to 2.19.1 and drop Argon2 package | ||
| find_package(Botan REQUIRED) | ||
| if(BOTAN_VERSION VERSION_GREATER_EQUAL "3.0.0") | ||
| set(WITH_BOTAN3 TRUE) |
There was a problem hiding this comment.
There is still a reference to the old define in src/fdosecrets/objects/SessionCipher.cpp:#ifdef WITH_XC_BOTAN3
|
Any chance of this making it into v2.7.10? Apparently, Qt 5 is pretty much unsupported at this point. |
No. This needs more work. |
|
@varjolintu Gotcha. Keep up the good work! |
|
QT5 in Gentoo and other (GNU/)Linux distros will be gettting slowly removed. Please see also: https://bugs.gentoo.org/949231 |
* Remove individual feature flags in favor of a single `KPXC_MINIMAL` flag that removes advanced features from the build. Basic features are no longer guarded by feature flags. * Basic features: Auto-Type, Yubikey, KeeShare * Advanced features include: Browser (and passkeys), SSH Agent, and Secret Service * Networking, Documentation, and Update Checking remain as feature flags to accommodate various distro requirements. This change also cleans up the main CMakeLists.txt by re-arranging some content and placing macros into a dedicated include file. The minimum CMake version was bumped to 3.16.0 to conform to our minimum Ubuntu support of Focal (20.04). This also allows us to default to C++20, we fall back to C++17 for Qt versions less than 5.15.0 due to lack of support. Lastly this change removes the KEEPASSXC_BUILD_TYPE="PreRelease" which is never used. We only support "Snapshot" and "Release" now.
- Fix missing X11 library for linker - Other Linux fixes NOTE: FDOSECRETS CURRENTLY CRASHES WHEN ATTEMPTING TO BE USED
* Also reduce minimum Qt version to 6.4.2 to align with Ubuntu 24.04
* Remove Qt6 from vcpkg, prefer use of installed libraries using Qt6_DIR CMake variable. * Update baseline to latest and remove version minimums since they are not actually minimums but just "something newer than the baseline". See https://devblogs.microsoft.com/cppblog/take-control-of-your-vcpkg-dependencies-with-versioning-support/#how-versioning-works-in-vcpkg
* Revert back to using char instead of wchar_t for PCSC function calls. Linux and macOS don't support widechar calls yet * Fix signal/slot break in Password Generator * Add missing POST_BUILD parameters to cmake custom command calls
| #ifdef WITH_XC_X11 | ||
| auto keycode = XKeysymToKeycode(dpy, qcharToNativeKeyCode(key)); | ||
| #ifdef WITH_X11 | ||
| auto keycode = XKeysymToKeycode(dpy, qcharToNativeKeyCode(QChar(key))); |
There was a problem hiding this comment.
I had to make this change:
- auto keycode = XKeysymToKeycode(dpy, qcharToNativeKeyCode(QChar(key)));
+ auto keycode = XKeysymToKeycode(dpy, qcharToNativeKeyCode(QChar(static_cast<uint>(key))));to get it to build on my setup.
There was a problem hiding this comment.
I this pr already usable?
It builds, runs and can open my KDBX database. I haven't tested further. There might be a showstopping bug somewhere that @varjolintu isn't happy with since he mentioned it "needs more work"... I can't tell.
There was a problem hiding this comment.
Everything just needs to be checked. It's possible that we merge this when possible and the do more fixes later when possible problems are found.
There was a problem hiding this comment.
It doesn't make sense to force a Qt::Key to QChar. We should use qtToNativeKeyCode directly.
| auto keycode = XKeysymToKeycode(dpy, qcharToNativeKeyCode(QChar(key))); | |
| auto keycode = XKeysymToKeycode(dpy, qtToNativeKeyCode(key)); |
There was a problem hiding this comment.
@Aetf This is on my fix list too. I'll update this branch this week when I have the time.
|
Stuff under |
|
I may be able to take a look this weekend. Too sad that I couldn't spend more time on this. What is breaking about fdosecrets? It has some custom code to make handling fdosecrets secret service related dbus objects easier. I don't remember anything too out of ordinary from top of my head that would break due to Qt6 though... |
For one, The type ID bool DBusMgr::deliverMethod(...)
{
...
// output args need to be converted before they can be directly sent out:
for (int i = 0; i != outputArgs.size(); ++i) {
auto& outputArg = outputArgs[i];
if (!outputArg.convert(method.outputTargetTypes.at(i))) {
qWarning() << "Internal error: Failed to convert message output to type"
<< method.outputTargetTypes.at(i);
return false;
}
}
...The // Under CollectionProxy of FdoSecretsProxy.h
inline PropertyReply<bool> locked() const
{
IMPL_GET_PROPERTY(Locked); // crashes here, specifically at the QDBusConnection::call call, inside of which stack traces aren't available
} |
I don't think this does what you think it does?
|
|
@Aetf Turns out the https://doc.qt.io/qt-6/qvariant-obsolete.html Same with the What I've done so far: diff --git a/src/fdosecrets/dbus/DBusDispatch.cpp b/src/fdosecrets/dbus/DBusDispatch.cpp
index eee63bdb..0165c62d 100644
--- a/src/fdosecrets/dbus/DBusDispatch.cpp
+++ b/src/fdosecrets/dbus/DBusDispatch.cpp
@@ -20,7 +20,6 @@
#include <QDBusMetaType>
#include <QThread>
-#include <QtDBus>
namespace FdoSecrets
{
@@ -32,29 +31,37 @@ namespace FdoSecrets
return camel.at(0).toUpper() + camel.mid(1);
}
- bool prepareInputParams(const QVector<int>& inputTypes,
+ bool prepareInputParams(const DBusClientPtr& client,
+ const QVector<int>& inputTypes,
+ bool needsCallingClient,
const QVariantList& args,
+ DBusResult& ret,
QVarLengthArray<void*, 10>& params,
QVariantList& auxParams)
{
- // prepare params
+ // reserve to avoid reallocation
+ auxParams.reserve(inputTypes.size() + (needsCallingClient ? 1 : 0));
+
+ // fill auxParams
+ if (needsCallingClient) {
+ auxParams.append(QVariant::fromValue(client));
+ }
+
for (int count = 0; count != inputTypes.size(); ++count) {
const auto& id = inputTypes.at(count);
const auto& arg = args.at(count);
if (arg.userType() == id) {
- // shortcut for no conversion
- params.append(const_cast<void*>(arg.constData()));
+ // skip conversion
+ auxParams.append(arg);
continue;
}
- // we need at least one conversion, allocate a slot in auxParams
- auxParams.append(QVariant(id));
- auto& out = auxParams.last();
- // first handle QDBusArgument to wire types
+ QVariant out{QMetaType(id)};
+
if (arg.userType() == qMetaTypeId<QDBusArgument>()) {
QMetaType wireId(typeToWireType(id).dbusTypeId);
- out = QVariant(wireId, nullptr);
+ out = QVariant(QMetaType(wireId), nullptr);
const auto& in = arg.value<QDBusArgument>();
if (!QDBusMetaType::demarshall(in, wireId, out.data())) {
@@ -63,18 +70,24 @@ namespace FdoSecrets
return false;
}
} else {
- // make a copy to store the converted value
out = arg;
}
- // other conversions are handled here
- if (!out.convert(id)) {
- qDebug() << "Internal error: failed conversion from" << arg << "to type" << QMetaType::typeName(id)
- << id;
+
+ if (!out.convert(QMetaType(id))) {
+ qDebug() << "Internal error: failed conversion from" << arg << "to type"
+ << QMetaType::typeName(id) << id;
return false;
}
- // good to go
- params.append(const_cast<void*>(out.constData()));
+
+ auxParams.append(std::move(out));
+ }
+
+ // fill params from auxParams
+ params.append(&ret); // first slot for return value
+ for (const QVariant& var : auxParams) {
+ params.append(const_cast<void*>(var.constData()));
}
+
return true;
}
@@ -341,16 +354,8 @@ namespace FdoSecrets
QVarLengthArray<void*, 10> params;
QVariantList auxParams;
- // the first one is for return type
- params.append(&ret);
-
- if (method.needsCallingClient) {
- auxParams.append(QVariant::fromValue(client));
- params.append(const_cast<void*>(auxParams.last().constData()));
- }
-
// prepare input
- if (!prepareInputParams(method.inputTypes, args, params, auxParams)) {
+ if (!prepareInputParams(client, method.inputTypes, method.needsCallingClient, args, ret, params, auxParams)) {
qDebug() << "Failed to prepare input params";
return false;
}
@@ -358,7 +363,7 @@ namespace FdoSecrets
// prepare output args
outputArgs.reserve(outputArgs.size() + method.outputTypes.size());
for (const auto& outputType : asConst(method.outputTypes)) {
- outputArgs.append(QVariant(outputType));
+ outputArgs.append(QVariant(QMetaType(outputType)));
params.append(const_cast<void*>(outputArgs.last().constData()));
}
@@ -378,10 +383,17 @@ namespace FdoSecrets
// output args need to be converted before they can be directly sent out:
for (int i = 0; i != outputArgs.size(); ++i) {
auto& outputArg = outputArgs[i];
- if (!outputArg.convert(method.outputTargetTypes.at(i))) {
- qWarning() << "Internal error: Failed to convert message output to type"
- << method.outputTargetTypes.at(i);
+
+ QMetaType fromType = outputArg.metaType();
+ QMetaType toType = QMetaType(method.outputTargetTypes.at(i));
+
+ QVariant result(toType); // default-construct target type
+ if (!QMetaType::convert(fromType, outputArg.constData(), toType, result.data())) {
+ qWarning() << "Internal error: Failed to convert message output from type"
+ << fromType.name() << "to type" << toType.name();
return false;
+ } else {
+ outputArg = result;
}
}This should get a handful of tests to pass. Some test cases under testguifdosecrets are still failing/crashing, I'll try to find out what went wrong with those. For now @varjolintu if you can, please apply the patch above to |
|
|
|
Working on the code now. Also need the following patch to get gui tests to compile, due to diff --git a/tests/gui/TestGuiFdoSecrets.cpp b/tests/gui/TestGuiFdoSecrets.cpp
index fc7e218e..88d20db5 100644
--- a/tests/gui/TestGuiFdoSecrets.cpp
+++ b/tests/gui/TestGuiFdoSecrets.cpp
@@ -131,6 +131,7 @@ char* toString(const QDBusObjectPath& path)
return QTest::toString("ObjectPath(" + path.path() + ")");
}
+TestGuiFdoSecrets::TestGuiFdoSecrets() = default;
TestGuiFdoSecrets::~TestGuiFdoSecrets() = default;
void TestGuiFdoSecrets::initTestCase()
diff --git a/tests/gui/TestGuiFdoSecrets.h b/tests/gui/TestGuiFdoSecrets.h
index 1624eed4..c6809f38 100644
--- a/tests/gui/TestGuiFdoSecrets.h
+++ b/tests/gui/TestGuiFdoSecrets.h
@@ -55,6 +55,7 @@ class TestGuiFdoSecrets : public QObject
Q_OBJECT
public:
+ explicit TestGuiFdoSecrets();
~TestGuiFdoSecrets() override;
private slots:
@@ -145,6 +146,8 @@ private:
}
private:
+ Q_DISABLE_COPY(TestGuiFdoSecrets)
+
QScopedPointer<MainWindow> m_mainWindow;
QPointer<DatabaseTabWidget> m_tabWidget;
QPointer<DatabaseWidget> m_dbWidget; |
|
Thank you :) This makes our job a little bit easier. |
|
@mutativesystems Thanks for spotting the deprecated QVariant methods. That's super hard to debug! However I think your patch for Therefore after the patch even if the code can progress, there are multiple other errors. The real issue is outputArgs are all null somehow after qt_metacall. Here's the log output after I added more debug logging: I still don't get why outputArg is null... Too late for me so I have to stop here. But I feel we are close if we can manage to fix this. |
Edit 1: I take it back, you're right. I'll debug this some more. Edit 2: this can be fixed like so: diff --git a/src/fdosecrets/dbus/DBusDispatch.cpp b/src/fdosecrets/dbus/DBusDispatch.cpp
index 0165c62d..6c5f1abd 100644
--- a/src/fdosecrets/dbus/DBusDispatch.cpp
+++ b/src/fdosecrets/dbus/DBusDispatch.cpp
@@ -363,7 +363,7 @@ namespace FdoSecrets
// prepare output args
outputArgs.reserve(outputArgs.size() + method.outputTypes.size());
for (const auto& outputType : asConst(method.outputTypes)) {
- outputArgs.append(QVariant(QMetaType(outputType)));
+ outputArgs.append(QVariant(QMetaType(outputType), QMetaType(outputType).create()));
params.append(const_cast<void*>(outputArgs.last().constData()));
}
@@ -383,17 +383,12 @@ namespace FdoSecrets
// output args need to be converted before they can be directly sent out:
for (int i = 0; i != outputArgs.size(); ++i) {
auto& outputArg = outputArgs[i];
+ auto toType = QMetaType(method.outputTargetTypes.at(i));
- QMetaType fromType = outputArg.metaType();
- QMetaType toType = QMetaType(method.outputTargetTypes.at(i));
-
- QVariant result(toType); // default-construct target type
- if (!QMetaType::convert(fromType, outputArg.constData(), toType, result.data())) {
+ if (!outputArg.convert(toType)) {
qWarning() << "Internal error: Failed to convert message output from type"
- << fromType.name() << "to type" << toType.name();
+ << outputArg.metaType().name() << "to type" << toType.name();
return false;
- } else {
- outputArg = result;
}
}Basically to make a |
For this I'll wait for someone else to figure this out - I'm not familiar with Qt or KPXC's codebase; for now I'm just self-compiling and using it with fdosecrets disabled in CMake. |
I took a quick look, but couldn't find the reason yet. I'm pretty sure it's related to the new code changes in DBusDispatch.cpp, and not the events or EDIT: If I revert the line |
@varjolintu You probably don't want to do that. Qt6's |
For this, the expectation is that the Simply initializing the output QVariants to a default value may make the convert pass, but I'm afraid that the actual outputs from the method call aren't passed out. IMHO the problem is probably because |
I know. Just tried to narrow the possible cause for the crash. |
|
I've tried to compile the current state with qt 6.10 on arch. My changes can be found in https://github.com/the-nic/keepassxc/tree/qt6_ver2 It seems to work fine, mostly. The tests in Also, with ASan enabled, I get a lot of Leak issues reported, and I had to set |
I've been testing your fork on Arch Linux as well, and the only test that's been failing here is diff --git a/tests/TestUrlTools.cpp b/tests/TestUrlTools.cpp
index b514f6cc..a787ccb1 100644
--- a/tests/TestUrlTools.cpp
+++ b/tests/TestUrlTools.cpp
@@ -141,25 +141,25 @@ void TestUrlTools::testIsUrlValidWithLooseComparison()
urls["\"https://github.com/login\""] = true;
urls["https://*.github.com/"] = true;
urls["*.github.com"] = true;
- urls["https://*.com"] = false;
+ urls["https://*.com"] = true;
urls["https://*.computer.com"] = true; // TLD in domain (com) should not affect
urls["\"\""] = false;
urls["\"*.example.com\""] = false;
urls["http://*"] = false;
urls["*"] = false;
urls["****"] = false;
- urls["*.co.jp"] = false;
- urls["*.com"] = false;
+ urls["*.co.jp"] = true;
+ urls["*.com"] = true;
urls["*.computer.com"] = true;
urls["*.computer.com/*com"] = true; // TLD in path should not affect this
urls["*com"] = false;
- urls["*.com/"] = false;
- urls["*.com/*"] = false;
+ urls["*.com/"] = true;
+ urls["*.com/*"] = true;
urls["**.com/**"] = false;
urls["*.*"] = false;
- urls["https://example.*"] = false;
- urls["https://*.example.*"] = false;
- urls["https://example.c*"] = false;
+ urls["https://example.*"] = true;
+ urls["https://*.example.*"] = true;
+ urls["https://example.c*"] = true;
QHashIterator<QString, bool> i(urls);
while (i.hasNext()) { |
|
Please note that Qt 6 switched from |
|
|
Need to test with this issue post-merge: #10763 |
|
Gotta do some reviews for #11003 so we can rebase this branch to develop. |
11 participants
Continues work from #10267, using #11003 as the base.
Fixes #7774.
Type of change