Qt5 support #3
Qt5 support #3
Conversation
|
Looks like |
|
Um... I have no idea. Did I write that test? 😁 |
|
Git says no, but is anyone else still around Shiboken who would know more? |
|
Well, you might also ask on pyside-dev@googlegroups.com... |
|
If this gets merged to smtk-head, then we need to tag before merging so that people with Qt4 can continue to build SMTK. Or this should accommodate both Qt4 and Qt5. |
|
@vibraphone Agreed. Email sent to pyside-dev@. |
|
And email bounced from pyside-dev@. Looks like we're on our own here :/ . |
|
You have to join the group. |
|
So upstream is non-responsive. @mwoehlke-kitware, mind looking over the changes to see if there's anything I missed when porting? |
| #include <QTime> | ||
| #include <QQueue> | ||
| #include <QDir> | ||
| #include <QtCore/QDebug> |
There was a problem hiding this comment.
Pedantic: using tll(Qt5::Core), I don't believe this is necessary... if supporting both Qt4/Qt5 is a goal, it might be better to omit these changes.
There was a problem hiding this comment.
...that said, the only modules Shiboken should care about are ones that have not been refactored (i.e. Shiboken is not using headers that exist in different modules between Qt4 and Qt5), and this style of include in theory works with Qt4 also. (More to the point, I noticed there is at least one such include prior to these changes...)
There was a problem hiding this comment.
Dual Qt4/Qt5 support isn't a necessity. It can be done if it isn't too much work, but seeing as upstream doesn't care much about this, I expect we may be left with a fork here anyways :( .
QAtomicPointer no longer has the method and its usage is never atomic anyways, so just reimplement the method.
|
Totally unrelated to QAtomicPointer. The underlying problem is that enums are not being processed in well-defined order (they're being processed in the order they appear in a QHash, and QHash in Qt 5 has explicitly non-deterministic ordering, even for same inputs), where they need to be processed in the order they are declared. See https://codereview.qt-project.org/106505 and https://codereview.qt-project.org/106506 for patches. |
|
OK, that fixes the non-deterministic failures; now to track down the actual failures. |
|
Seems to be related to failures around |
Modify _ScopeModelItem to return enums (from the enums() method) in the order that they were added (which presumably is the order in which they were declared). We must do this because we must process enumerations in the same order in order to resolve values, as later declared enums may refer to values from earlier declared enums (and in fact, this is exactly the case in the 'testenum' test), and the order we get just from QHash may not match declaration order. Change-Id: I15a05df98a2cee7ecccb6c82d3f9017735281245
In the same vein as the previous commit, process global enums in order added (which presumably is declaration order). For what we're doing at this point, this may not be as critical, but again is needed to avoid a spurious test failure. Change-Id: If32a07fee2e7e9b7699e01eda3408ed57855e947
mathstuf
force-pushed
the
qt5-support
branch
from
0ae8072
to
83b4e6c
Compare
|
Did you do a clean build? The only test failure I'm seeing is sample_templateinheritingclass. (After a clean build. I was getting some odd behavior prior, but didn't dig as the clean build made it go away.) |
|
On Tue, Feb 17, 2015 at 11:49:43 -0800, Matthew Woehlke wrote:
Same here now with a clean build. Shiboken needs to get its deps fixed I |
|
Oh, joy... it's a heisenbug; in the process of throwing around qDebug()s to try to isolate where it's going sideways, I've made it go away entirely... It looks like it's failing to add functions (methods) from a base class, although the inheritence is set up correctly AFAICT. |
|
Ah... it's worse than that; it looks like shiboken's output is no longer deterministic (probably for similar reasons). Just doing repeated builds, sometimes the test passes, sometimes it doesn't. (Also, I can verify that the generated code differs between runs.) |
|
The non-determinism also seems to be the cause of most of the tests crashing for some builds. Things are being initialized in non-deterministic order, with the result that some times base classes get initialized first, and some times they don't, depending on a random seed when writing the generated code. |
|
So.... dcee65c is part of the problem; it pokes holes in the topology. Why was this change needed? The commit message isn't very helpful. |
|
I don't remember exactly :( . Is there some reason why that spot shouldn't check for template characters? If it is reverted, is all well again? My guess is that it has to do with finding |
Er... no. Did you mean "template base classes"?
Funny you should mention that... 😀 That's also broken (looks like that's what went wrong when you get a build with lots of crashing). I suspect there is no topology for those happening at all. (Which isn't too surprising, since it's a feature that we added.) I'm inclined to think that dcee65c should be reverted... |
This reverts commit dcee65c. I can't figure out why the original change is needed, and Ben doesn't remember either, and it pokes holes in the topology generation, by breaking the graph anywhere inheritance happens through a template type, resulting in an incorrect ordering of the output class list. (Which in turn causes bugs because derived classes get initialized before their bases.)
|
On Fri, Feb 20, 2015 at 11:16:47 -0800, Matthew Woehlke wrote:
Well, checking for '<' is how it detected templates, so yes. I think the
If things still work, I have no attachment to it. |
That... may be an issue. In fact, the crash I'm looking at right now is because More accurately, |
|
...and on that note, you specifically added that behavior in b2e7c19. |
|
See https://codereview.qt-project.org/106919 ...although it only addresses part of the problem. |
When building the class topology, don't skip classes, even if we are not going to generate code for them. This is necessary to get the topology order correct in a case such as C derived from B derived from A, where B is not generated, but initializing C depends on A being initialized first. Without this change, there is no guaranteed ordering between A and C in such a case. (In particular, this comes up in the Photon test; Photon::ValueIdentity derives from Photon::TemplateBase, which derives from Photon::Base. However, this was not being reflected in the topology, and as a result, it was just luck that the initialization order was correct anyway.)
Remove concept of "injected dependencies"; this overloads the "injected" term to mean something totally different, and no longer has any use since the previous commit. Add a concept of "extra" dependencies, used to form topology edges without actually modifying the class hierarchy, which is used to ensure that pointer wrappers follow the classes that they wrap. (This may not be strictly necessary, but it seems advisable.) Add comments explaining better what traverseInstantiation is doing with respect to the class hierarchy. Only treat a pointer-to-B as a subclass of a pointer-to-A when there is only one template argument.
Always check for an exact match first, before checking for a template match. This fixes failure to find the base class when the base is an instantiation.
|
These commits by @mwoehlke-kitware seem to fix things and works for SMTK locally. |
|
I think we can merge what @mwoehlke-kitware did and open a new issue / pull request explaining the "other part of the problem" Who agree with that? |
This is on top of smtk-head. Qt4 is no longer supported here. One failing test; uninspected.
CC: @robertmaynard