Bug 43222

Branch at URL attempts to implement this.

Summary of current faults from private correspondence:

* The user must be able to choose whether to build a qt4 or qt5 lib even if pkgconfig finds both, as it will in real world systems where qt4 and qt5 will co-exist for a long time

* The built library must be named such that versions built for qt4 and qt5 can be parallel installed

* We must have a summary of backwards incompat changes in NEWS to enable people to know how to port

* Conditional compilation to pass the right constants to attain the API's *default behavior* should not be required of the API's user (despite semantics change in QHostAddress::Any between qt4 and qt5). We must abstract away such things.

* All of the API break TODO/FIXME notes must be tackled before declaring stable >= 0.9 API. Better to do in a separate branch as they're nothing qt5 specific, although some have already been mixed in with this current branch. I've created bug 43223 for this.

-# Find qt4 version >= 4.5
-set (QT_MIN_VERSION "4.6.0")
-find_package(Qt4 REQUIRED)
+# Find qt4 version >= 4.8 or qt5 >= 5.0.0
+set(QT4_MIN_VERSION "4.8.0")

No reason to bump qt4 dep to 4.8.

+Requires.private: QtCore >= ${QT_MIN_VERSION}, QtDBus >= ${QT_MIN_VERSION}, QtNetwork >= ${QT_MIN_VERSION}, QtXml >= ${QT_MIN_VERSION}

Should be max version too, as they use the same name. So it doesn't find qt5 when you're linking against qt4 build of tp-qt.

Ahem that also means we should also cater for parallel installing both a TelepathyQt4 and a TelepathyQt5 cmake file, just like the lib binary.

    explicit inline SharedPtr(const QWeakPointer<T> &o)
    {
        if (o.data() && o.data()->strongref.fetchAndAddOrdered(0) > 0) {
            d = static_cast<T*>(o.data());
            d->ref();
        }

Used like that, the atomic integer is completely useless. First after the o.data() check, an another thread can cause it to go NULL, and then after the refcount check, an another thread can cause it to drop to zero and the object be deleted.

This is perhaps also why they removed the implicit integer conversion in qt5 - the whole point of the atomic integer is to be able to implement inspection/test AND modify in one atomic operation to guard against an another thread ruining our day in between them. And that removal of API helped us to notice the pathological usage here.

If the answer is "even this part of tp-qt4 doesn't need to be thread-safe" then why are we even using an atomic integer? If so, it serves no purpose other than to slow things down even in single-threaded usage... However passing object refs across threads safely (even if not using the objects pointed to from multiple threads at a time) seems useful so it might be wise to clone this into an another bug, to be resolved before committing this inline header to a 0.9.0 release.

I suggest taking a look at other Qt shared pointer impls (webkit qt's ? qsharedpointers? qexplicitlyshared...'s ?) for ideas on getting the thread safety right.

channel-class-spec.h:

    // TODO: Use new TP_QT_... constants
    QString channelType() const
    {
        return qdbus_cast<QString>(
                property(TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")));

It seems we do now. In general, please check the TODOs for stuff you might have already done.

+ * method will always fail unless QHostAddress::Any (also QHostAddress::AnyIPv4 if using Qt5) or

"or AnyIPv4 in Qt5"

> QtDBus >= ${QT_MAX_VERSION}, QtDBus < ${QT_MIN_VERSION}

Doesn't look like that would be satisfiable very easily :)

(In reply to comment #2)
> Summary of current faults from private correspondence:
> 
> * The user must be able to choose whether to build a qt4 or qt5 lib even if
> pkgconfig finds both, as it will in real world systems where qt4 and qt5 will
> co-exist for a long time

Actually already possible with DESIRED_QT_VERSION, the mechanism just needs to be careful to not accidentally pick up Qt5 although one tried selecting qt4 and vice versa. So double check both directions before merge please...

> 
> * The built library must be named such that versions built for qt4 and qt5 can
> be parallel installed

Done now! Also extends to pkgconfig and even headers after some discussion. So we're going to have full parallel installability of also -dev packages.

> 
> * We must have a summary of backwards incompat changes in NEWS to enable people
> to know how to port

To be done per-branch when merging, as the branch is now thankfully split as per the different kinds of changes.

> 
> * Conditional compilation to pass the right constants to attain the API's
> *default behavior* should not be required of the API's user (despite semantics
> change in QHostAddress::Any between qt4 and qt5). We must abstract away such
> things.

Andre's currently working on this. We hit a Qt5 bug in the process... fun times.

(In reply to comment #4)
> + * method will always fail unless QHostAddress::Any (also
> QHostAddress::AnyIPv4 if using Qt5) or
> 
> "or AnyIPv4 in Qt5"
Done
 
> > QtDBus >= ${QT_MAX_VERSION}, QtDBus < ${QT_MIN_VERSION}
> 
> Doesn't look like that would be satisfiable very easily :)
Done
 
> > * Conditional compilation to pass the right constants to attain the API's
> > *default behavior* should not be required of the API's user (despite semantics
> > change in QHostAddress::Any between qt4 and qt5). We must abstract away such
> > things.
> 
> Andre's currently working on this. We hit a Qt5 bug in the process... fun
> times.
Disabled tubes tests in this branch until Qt5 bugs are fixed.

(In reply to comment #3)
> -# Find qt4 version >= 4.5
> -set (QT_MIN_VERSION "4.6.0")
> -find_package(Qt4 REQUIRED)
> +# Find qt4 version >= 4.8 or qt5 >= 5.0.0
> +set(QT4_MIN_VERSION "4.8.0")
> 
> No reason to bump qt4 dep to 4.8.
Fixed
 
>     explicit inline SharedPtr(const QWeakPointer<T> &o)
>     {
>         if (o.data() && o.data()->strongref.fetchAndAddOrdered(0) > 0) {
>             d = static_cast<T*>(o.data());
>             d->ref();
>         }
> 
> Used like that, the atomic integer is completely useless. First after the
> o.data() check, an another thread can cause it to go NULL, and then after the
> refcount check, an another thread can cause it to drop to zero and the object
> be deleted.
> 
> This is perhaps also why they removed the implicit integer conversion in qt5 -
> the whole point of the atomic integer is to be able to implement
> inspection/test AND modify in one atomic operation to guard against an another
> thread ruining our day in between them. And that removal of API helped us to
> notice the pathological usage here.
> 
> If the answer is "even this part of tp-qt4 doesn't need to be thread-safe" then
> why are we even using an atomic integer? If so, it serves no purpose other than
> to slow things down even in single-threaded usage... However passing object
> refs across threads safely (even if not using the objects pointed to from
> multiple threads at a time) seems useful so it might be wise to clone this into
> an another bug, to be resolved before committing this inline header to a 0.9.0
> release.
> 
> I suggest taking a look at other Qt shared pointer impls (webkit qt's ?
> qsharedpointers? qexplicitlyshared...'s ?) for ideas on getting the thread
> safety right.
This is true, could you please file a bug for that with the rationale above. In the meantime I will leave this branch using fetchAndAddOrdered, so it works with both qt4 and qt5
 
> channel-class-spec.h:
> 
>     // TODO: Use new TP_QT_... constants
>     QString channelType() const
>     {
>         return qdbus_cast<QString>(
>                 property(TP_QT_IFACE_CHANNEL + QLatin1String(".ChannelType")));
> It seems we do now. In general, please check the TODOs for stuff you might have
> already done.
This is bug #43223

(In reply to comment #5)
> (In reply to comment #4)
> > + * method will always fail unless QHostAddress::Any (also
> > QHostAddress::AnyIPv4 if using Qt5) or
> > 
> > "or AnyIPv4 in Qt5"
> Done

Still:

- * Returning the pair (QHostAddress::Any, 0) (also the pair (QHostAddress::AnyIPv4, 0) if using Qt5)
+ * Returning the pair (QHostAddress::Any, 0) (or also the pair (QHostAddress::AnyIPv4, 0) in Qt5)

"(or alternatively (QHostAddress::AnyIPv4, 0) in Qt5)" would be cleaner

> 
> > > QtDBus >= ${QT_MAX_VERSION}, QtDBus < ${QT_MIN_VERSION}
> > 
> > Doesn't look like that would be satisfiable very easily :)
> Done

Yes
 
> > Andre's currently working on this. We hit a Qt5 bug in the process... fun
> > times.
> Disabled tubes tests in this branch until Qt5 bugs are fixed.

+    # FIXME - Re-enable tube tests once Qt5 QHostAddress bugs are fixed

Please file an actual Qt bug report at https://bugreports.qt.nokia.com/ with the concrete usecases for QHostAddress which are failing, and mention the resulting ID in this CMakeLists.txt comment. And also file a fd.o bug against tp-qt4 to re-enable the test once Qt is fixed, and add the URL to the Qt bug there (sadly a different bug tracker though).

Otherwise the Qt issue has no chance of getting fixed. And we don't remember to reintroduce the test even when it gets fixed.

We could also add this to a "known issues" section of NEWS of releases until the bug gets fixed in Qt5, so people don't file bug against tp-qt4 because that specific thing doesn't work because of Qt upstream bugs.

The rest looks good now. So provided you now double-check that all combinations of finding Qt4 or Qt5 and the user specifying Qt4/Qt5 build OR autodetect try to produce the right kind of library, pkgconfig file and header installation paths, please merge.

(In reply to comment #6)
> (In reply to comment #3)
> >     explicit inline SharedPtr(const QWeakPointer<T> &o)
> >     {
> >         if (o.data() && o.data()->strongref.fetchAndAddOrdered(0) > 0) {
> >             d = static_cast<T*>(o.data());
> >             d->ref();
> >         }
> > 
> > Used like that, the atomic integer is completely useless. First after the
> > o.data() check, an another thread can cause it to go NULL, and then after the
> > refcount check, an another thread can cause it to drop to zero and the object
> > be deleted.
> This is true, could you please file a bug for that with the rationale above. In
> the meantime I will leave this branch using fetchAndAddOrdered, so it works
> with both qt4 and qt5
> 

Done now, as bug 43239. That is a blocker for finishing the tp-qt >= 0.9 ABI break. Put a big note (mental or comment) somewhere that this needs to be fixed, because actually otherwise SharedPtr *does not* in fact work fully in its purpose in either qt4 or qt5, although yes the code compiles.

(In reply to comment #7)
> Still:
> 
> - * Returning the pair (QHostAddress::Any, 0) (also the pair
> (QHostAddress::AnyIPv4, 0) if using Qt5)
> + * Returning the pair (QHostAddress::Any, 0) (or also the pair
> (QHostAddress::AnyIPv4, 0) in Qt5)
> 
> "(or alternatively (QHostAddress::AnyIPv4, 0) in Qt5)" would be cleaner
Done

> 
> > > Andre's currently working on this. We hit a Qt5 bug in the process... fun
> > > times.
> > Disabled tubes tests in this branch until Qt5 bugs are fixed.
> 
> +    # FIXME - Re-enable tube tests once Qt5 QHostAddress bugs are fixed
> 
> Please file an actual Qt bug report at https://bugreports.qt.nokia.com/ with
> the concrete usecases for QHostAddress which are failing, and mention the
> resulting ID in this CMakeLists.txt comment. And also file a fd.o bug against
> tp-qt4 to re-enable the test once Qt is fixed, and add the URL to the Qt bug
> there (sadly a different bug tracker though).
> 
> Otherwise the Qt issue has no chance of getting fixed. And we don't remember to
> reintroduce the test even when it gets fixed.
> 
> We could also add this to a "known issues" section of NEWS of releases until
> the bug gets fixed in Qt5, so people don't file bug against tp-qt4 because that
> specific thing doesn't work because of Qt upstream bugs.
Qt bugs:
https://bugreports.qt.nokia.com/browse/QTBUG-22898 and https://bugreports.qt.nokia.com/browse/QTBUG-22899

tp-qt bug:
https://bugs.freedesktop.org/show_bug.cgi?id=43243

> The rest looks good now. So provided you now double-check that all combinations
> of finding Qt4 or Qt5 and the user specifying Qt4/Qt5 build OR autodetect try
> to produce the right kind of library, pkgconfig file and header installation
> paths, please merge.
I did, but if someone else with another Qt5 setup could double check I'd appreciate.

And tnx for the reviews.

Changes merged upstream. It will be in tp-qt 0.9.0

*** Bug 42258 has been marked as a duplicate of this bug. ***

(In reply to comment #10)
> Changes merged upstream. It will be in tp-qt 0.9.0

Thanks. Is upstream still the telepathy-qt4 repository?

(In reply to comment #12)
> (In reply to comment #10)
> > Changes merged upstream. It will be in tp-qt 0.9.0
> 
> Thanks. Is upstream still the telepathy-qt4 repository?

Yes, for now. We are going to add a new telepathy-qt repository that mirrors telepathy-qt4 (left for legacy) and also setup release/doc dirs, bugzilla component, etc once releasing 0.9.0.