Make qCoro usable on platforms with missing features #44
Conversation
rgriebl
commented
Jan 26, 2022
rgriebl
commented
- iOS has no QProcess
- Android has no QtDBus library
* iOS has no QProcess * Android has no QtDBus library
|
Just to give a bit more context: I'm not using qCoro as is with its cmake build system, but rather have a copy in my project that gets built via qmake (https://github.com/rgriebl/brickstore/blob/main/3rdparty/qcoro.pri) Here's the GH actions matrix I use to build for Win32/64, Linux, macOS, iOS and Android: https://github.com/rgriebl/brickstore/blob/main/.github/workflows/build_qmake.yml |
There was a problem hiding this comment.
Hmm, I think to fix this properly, we need to rethink feature detection at configuration time and
- (not) compile files with unsupported features at all
- add compiler definitions regarding what's supported by QCoro to config.h
- conditionally disable code in QCoro based on those definitions
This is a larger change, and probably needs a more complex CI matrix.
If you can confirm that this change is enough to unbreak QCoro on iOS (I'm suspicious regarding the QCoroProcess thing, see the other review comment), I'm fine with merging this now and I'll plan some time to work on the larger config update.
UPDATE: now I see that in your qmake code you already build qcoroprocess.cpp conditionally, which is likely why this change is indeed enough for you as it is. So I'd say let's merge this change now and I'll add proper support for feature detection later. It won't make iOS supported by the CMake build out of the box, but it won't break anything either ;-)
| #if !QT_CONFIG(process) | ||
| # include "qcoroprocess.h" | ||
| #endif |
There was a problem hiding this comment.
This won't unbreak QCoro build, though, right? There's still qcoroprocess.cpp that will get compiled (and fail), even if QProcess is not supported by Qt.
I also think the proper fix is to follow what Qt does and instead conditionally disable all problematic code (in thise case the whole QCoroProcess class) in the qcoroprocess.h header.
There was a problem hiding this comment.
yes, tested against both Qt5 and Qt6
| #if defined(QT_DBUS_LIB) | ||
| # include "qcorodbus.h" | ||
| #endif |
There was a problem hiding this comment.
This should probably take into account QCORO_WITH_QTDBUS and, since in theory this will make the header unusable when QCoro is built without DBus, even if Qt has DBus support.
There was a problem hiding this comment.
Yes - you are right. Adding this to cmake shouldn't be a big deal though: just check for if (TARGET Qt::DBus)
|
BTW: ios + c++20 + cmake is broken in Qt 6 at the moment: https://bugreports.qt.io/browse/QTBUG-100246 |
|
This one here is outdated. Closing in favor of #153 |
