Various fixes for deprecated functions, adds Qt6 support #9
Conversation
QRegExp has been deprecated as of Qt6 in favor of QRegularExpression which was introduced in Qt5. https://doc-snapshots.qt.io/qt6-dev/qregexp.html Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
qVariantFromValue has been deprecated as per https://doc.qt.io/qt-5/qvariant-obsolete.html#qVariantFromValue Replace by QVariant::fromValue. Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
|
I have tried to recompile your branch on Manjaro as it seems commit 5c90061 resolves trouble with compilation caused by new gcc or new default compiler settings. In the end compilation failed with following error message: |
|
@jmlich Unfortunately I didn't fully understand the actual origin of this error, as just including the header shouldn't trigger errors related to the templates declared later on in the cpp... Out of spite I tried just moving the include a bit later in the cpp, before we actually use what's declared in the header. That worked well with Qt 6, Qt 5.15, compiled with gcc 11. It could be the issue is still unsolved for gcc 12. |
|
@Thaodan This replaces the other PR, I've cut it up into separate commits with more documentation now |
|
The suggested approach/commit didn't helped. My issue is not directly related to this pull request. I don't want to hijack it. (-; |
libconnman-qt/marshalutils.cpp
Outdated
| #include "vpnconnection.h" | ||
|
|
||
| #include "marshalutils.h" | ||
|
|
||
| #if defined(__clang__) && __clang_major__ >= 11 || __GNUC__ >= 12 | ||
| Q_DBUS_EXPORT const QDBusArgument &operator>>(const QDBusArgument &a, RouteStructure &v); | ||
| Q_DBUS_EXPORT QDBusArgument &operator<<(QDBusArgument &a, const RouteStructure &v); |
There was a problem hiding this comment.
This looks strange to me in may ways. First the Q_DBUS_EXPORT not being a documented, looks like it should end up as Q_DECL_IMPORT/EXPORT. Then again the symbol visibilities shouldn't generally depend on compiler. And then again generally the visibilities should only matter on something using this from a shared library. And not sure does the library here get even compiled with hidden symbols by default. There are no Q_DECL_... usage elsewhere so I'm guessing no. And finally this is exporting something from the anon namespace which by definition should be internal to the file.
I'm having a feeling this might be fixing something by accident.
There was a problem hiding this comment.
It could be that Q_DBUS_EXPORT is just useless here, I didn't really check that
There was a problem hiding this comment.
Perhaps, but even without that this would be having different declaration for content inside the anon namespace.
There was a small compiler error snippet in the commit message. Would you have the bigger error message available in case it helps figuring out what's the problem.
There was a problem hiding this comment.
Perhaps, but even without that this would be having different declaration for content inside the anon namespace.
There was a small compiler error snippet in the commit message. Would you have the bigger error message available in case it helps figuring out what's the problem.
Here's a full log: http://errors.yoctoproject.org/Errors/Details/655056/
There was a problem hiding this comment.
@pvuorela And another one: https://pastebin.ubuntu.com/p/D5RRy77K8v/
There was a problem hiding this comment.
@pvuorela Any thoughts based on above logs? I got a build system with GCC 12 here to test if needed.
There was a problem hiding this comment.
Thanks. Tested also local build with qt6 and the current version here didn't work.
But tried a minor adjustment which could make more sense. Suggest:
- remove this export thing
- move the empty namespace declaration two methods downwards so the dbus argument operators are not inside it.
There was a problem hiding this comment.
Thanks. Tested also local build with qt6 and the current version here didn't work.
But tried a minor adjustment which could make more sense. Suggest:
- remove this export thing
- move the empty namespace declaration two methods downwards so the dbus argument operators are not inside it.
Sorry I'm not that great when it comes to C(++) ;)
You mean like this? webOS-ports@eb38903
BTW: This was tested with GCC 11/12.1 and Qt5.15/6.3 and compiling here without issues.
There was a problem hiding this comment.
Same effect, yes. The pre-declarations are unnecessary there, though, when the implementations come right after them.
There was a problem hiding this comment.
Same effect, yes. The pre-declarations are unnecessary there, though, when the implementations come right after them.
OK removed those as well and cleaned up the commits into a single one.
|
Beside that dbus export this looks good. Ideally could squash the commits which adjust the same code based on review comments, though. |
toTime_t() has been deprecated as per https://doc.qt.io/qt-5/qdatetime-obsolete.html#fromTime_t-1 In order to keep backwards compatibility with Qt 5.6 used by SFOS, use toMSecsSinceEpoch instead. Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
qsrand() has been deprecated as per https://doc.qt.io/qt-5/qtglobal-obsolete.html#qsrand QRandomGenerator was introduced in Qt 5.10, so for older versions of Qt such as 5.6 used in SFOS keep using qsrand. Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
Herrie82
force-pushed
the
herrie/qt6-clean
branch
from
5361c00
to
0968084
Compare
I squashed those. The Q_DBUS_EXPORT I cherry-picked from meta-qt5 layer and included it in this PR, so I'm just the messenger here. @kraj is the author of the original patch, so he might have more background on it. |
|
|
||
| class VpnModelPrivate; | ||
| class VpnManager; |
There was a problem hiding this comment.
This looks fishy as there are only pointers in the header. There seems to be system include of the vpnmanager in the cpp file. Maybe source should look like:
#include <QtDBus>
#include "vpnconnection.h"
#include "vpnmodel.h"
#include "vpnmodel_p.h"
#include "vpnmanager.h"
#include "vpnmanager_p.h"
In case you happen to have connman-qt5-devel installed the at same time, wrong might get picked up.
That would mean that vpnmodel.h would have "class VpnManager;" forward declaration in place.
There was a problem hiding this comment.
There is a QML property that uses this type, and therefore it isn't possible anymore in Qt6 to forward-declare the associated type.
There are two ways to fix this:
- as it has been done here, with a simple include
- using the Q_MOC_INCLUDE macro, to include it during moc processing
See: https://doc.qt.io/qt-6/qtcore-changes-qt6.html#type-registration
There was a problem hiding this comment.
@rainemak Happy to try it with Q_MOC_INCLUDE if that's more to your liking and adjust accordingly
There was a problem hiding this comment.
@rainemak: Tested with Q_MOC_INCLUDE that seems fine as well, so pushed that instead and squashed the commits.
There was a problem hiding this comment.
Q_MOC_INCLUDE was introduced in qt 6.0 so a common #include should be cleaner here, avoiding version specific handling.
There was a problem hiding this comment.
Even if used, it would need to be protected with qt version #ifdef and something else provided for Qt5. I wouldn't expect Raine either favoring such. Just about the first time we are adding Qt6 support so the forward declaration vs #include on Q_PROPERTIES wasn't a self-evident change.
There was a problem hiding this comment.
Should be good enough too. But maybe on next one just do the basic #include.
There was a problem hiding this comment.
Should be good enough too. But maybe on next one just do the basic #include.
Which next one?
There was a problem hiding this comment.
Meaning if you do another similar pull request on another project.
Herrie82
force-pushed
the
herrie/qt6-clean
branch
from
0968084
to
9b1bebe
Compare
Herrie82
force-pushed
the
herrie/qt6-clean
branch
from
9b1bebe
to
b20683f
Compare
libconnman-qt/marshalutils.cpp
Outdated
| @@ -183,6 +188,7 @@ QVariantMap MarshalUtils::propertiesToQml(const QVariantMap &fromDBus) | |||
| return rv; | |||
| } | |||
|
|
|||
| #include <QDBusMetaType> | |||
There was a problem hiding this comment.
Includes to the beginning of the file.
… are 'QDBusArgument' and 'const RouteStructure')
This appears with clang-11 and gcc 12:
Fixes: In file included from ../../recipe-sysroot/usr/include/QtDBus/QDBusMetaType:1,
from ../../git/libconnman-qt/marshalutils.cpp:34:
../../recipe-sysroot/usr/include/QtDBus/qdbusmetatype.h: In instantiation of 'QMetaType qDBusRegisterMetaType() [with T = RouteStructure]':
../../git/libconnman-qt/marshalutils.cpp:194:42: required from here
../../recipe-sysroot/usr/include/QtDBus/qdbusmetatype.h:71:59: error: no match for 'operator<<' (operand types are 'QDBusArgument' and 'const RouteStructure')
71 | auto mf = [](QDBusArgument &arg, const void *t) { arg << *static_cast<const T *>(t); };
| ~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from ../../recipe-sysroot/usr/include/QtDBus/qdbusmetatype.h:45,
from ../../recipe-sysroot/usr/include/QtDBus/QDBusMetaType:1,
from ../../git/libconnman-qt/marshalutils.cpp:34:
/mnt/5ba5d474-0b2d-49d6-a5a6-9de20c3ac967/honister-qt6/webos-ports/tmp-glibc/work/core2-64-webos-linux/libconnman-qt/1.2.35+gitAUTOINC+088350de01-r0/recipe-sysroot/usr/include/QtDBus/qdbusargument.h:87:20: note: candidate: 'QDBusArgument& QDBusArgument::operator<<(uchar)'
87 | QDBusArgument &operator<<(uchar arg);
Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
Q_PROPERTY needs to be fully defined in QT6 as per https://stackoverflow.com/questions/66581395/q-property-must-be-fully-defined-error-in-qt-6 Therefore use Q_MOC_INCLUDE when using Qt6 Signed-off-by: Herman van Hazendonk <github.com@herrie.org>
Herrie82
force-pushed
the
herrie/qt6-clean
branch
from
b20683f
to
d068445
Compare
|
|
I can confirm, that current version works for me. |
Great, thanks for testing and confirming! |
|
This is now tagged as 1.2.46, but the version in .pro and .spec file is still 1.2.35 as set in 347f20f should it be bumped to match the git tag? |
|
Sailfish picks the version from the tag, .spec is commonly kept roughly in sync e.g. to help local builds but commonly does not matter too much if it lags a bit. The .pro file could have effect on other distributions as it affects the .so naming but wouldn't expect even that to matter too much. |
* the version in .pro and .spec doesn't match with the git tag: sailfishos/libconnman-qt#9 (comment) sailfish passes the version based on git tag: sailfishos/libconnman-qt#9 (comment) lets do the same Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
* the version in .pro and .spec doesn't match with the git tag: sailfishos/libconnman-qt#9 (comment) sailfish passes the version based on git tag: sailfishos/libconnman-qt#9 (comment) lets do the same Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com>
Source: meta-qt5 MR: 125002 Type: Integration Disposition: Merged from meta-qt5 ChangeID: 2b71dbe Description: * the version in .pro and .spec doesn't match with the git tag: sailfishos/libconnman-qt#9 (comment) sailfish passes the version based on git tag: sailfishos/libconnman-qt#9 (comment) lets do the same Signed-off-by: Martin Jansa <Martin.Jansa@gmail.com> Signed-off-by: Jeremy A. Puhlman <jpuhlman@mvista.com>
Will now work with both Qt 5.15 and Qt 6.x.
Has not been tested on older versions of Qt 5.
Signed-off-by: Herman van Hazendonk github.com@herrie.org