Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Qt6 Compatibilty #111

Merged
merged 21 commits into from
Nov 2, 2023
Merged

Qt6 Compatibilty #111

merged 21 commits into from
Nov 2, 2023

Conversation

mrbean-bremen
Copy link
Contributor

This is the recreated PR that was accidentally closed/merged, with a few review findings fixed.

@mrbean-bremen mrbean-bremen force-pushed the qt6 branch 2 times, most recently from e052bd1 to 716cb05 Compare September 15, 2023 19:32
@githubuser0xFFFF
Copy link
Contributor

I just tried to build the qt6 branch with MinGW compiler with Qt 6.5.2. I succeeded after doing two changes:

First I had to replace the CONFIG += c++11 in src.pri with CONFIG += c++17. After this change, compiling via

qmake -r ..\PythonQt.pro
cd src
make -j12

was possible but I had the following linker error:

ld.exe: release/PythonQt.o:PythonQt.cpp:(.text+0x12afe): undefined reference to `PythonQt_init_QtCoreBuiltin(_object*)'
ld.exe: release/PythonQt.o:PythonQt.cpp:(.text+0x12b05): undefined reference to `PythonQt_init_QtGuiBuiltin(_object*)'

I was able to fix this by uncommenting the lines 224 and 225 in PythonQt.cpp

    //PythonQt_init_QtCoreBuiltin(nullptr);
    //PythonQt_init_QtGuiBuiltin(nullptr);

Should I create a pull request?

@mrbean-bremen
Copy link
Contributor Author

Thanks - go ahead! Just make sure that it still works for Qt5, so CONFIG += c++17 should only be done for Qt6.

@@ -257,7 +262,6 @@ void PythonQt::init(int flags, const QByteArray& pythonQtModuleName)
PythonQtRegisterToolClassesTemplateConverterForKnownClass(QPen);
PythonQtRegisterToolClassesTemplateConverterForKnownClass(QTextLength);
PythonQtRegisterToolClassesTemplateConverterForKnownClass(QTextFormat);
PythonQtRegisterToolClassesTemplateConverterForKnownClass(QMatrix);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know, perhaps leave QMatrix in for Qt5? Or are we sure that nobody ever used it?

src/PythonQtMethodInfo.cpp Show resolved Hide resolved
@jbowler
Copy link
Contributor

jbowler commented Oct 5, 2023

The Build/oldschool failure seems to be spurious, possibly temporary.

@jbowler
Copy link
Contributor

jbowler commented Oct 6, 2023

I was able to fix this by uncommenting the lines 224 and 225 in PythonQt.cpp

    //PythonQt_init_QtCoreBuiltin(nullptr);
    //PythonQt_init_QtGuiBuiltin(nullptr);

That's because there are no generated CPP files for Qt6; take a look at build/common.prf, only Qt5 is supported.

@jbowler
Copy link
Contributor

jbowler commented Oct 6, 2023

Somehow the generator has to be made to work again; it "works" but somehow the QMatrix methods (at least) have been dropped so the generated files won't compile (missing operator definitions).

I can't even get it to work on Qt 5.15LTS (I don't have the earlier versions) but it is apparently working in the Ubuntu jammy (22) workflow test. There are no generated files for 5.15 or, for that matter, 5.12; build/common.prf is using the generated files for 5.11. The default (which gets used for 5.15 and, curiously, builds but it seems risky) is 5.6

@jbowler
Copy link
Contributor

jbowler commented Oct 6, 2023

The issue seems to be related to QMatrix and it does not seem to be specific to Qt6. Trying to generate cpps with Qt5.15LTS I get this error message (this is the first of a long list, all basically the same):

../generated_cpp_515/com_trolltech_qt_core_builtin/com_trolltech_qt_core_builtin0.cpp: In member function ‘QLine PythonQtWrapper_QLine::__mul__(QLine*, const QMatrix&)’:
../generated_cpp_515/com_trolltech_qt_core_builtin/com_trolltech_qt_core_builtin0.cpp:275:31: error: no match for ‘operator*’ (operand types are ‘QLine’ and ‘const QMatrix’)
  275 |   return ( (*theWrappedObject)* m);
      |            ~~~~~~~~~~~~~~~~~~~^ ~
      |             |                   |
      |             QLine               const QMatrix
In file included from /usr/include/qt5/QtCore/qsize.h:44,
                 from /usr/include/qt5/QtGui/qtextdocument.h:45,
                 from /usr/include/qt5/QtGui/QTextDocument:1,
                 from ../generated_cpp_515/com_trolltech_qt_core_builtin/com_trolltech_qt_core_builtin0.h:4,
                 from ../generated_cpp_515/com_trolltech_qt_core_builtin/com_trolltech_qt_core_builtin0.cpp:1:

So somehow the "related non-member" (in Qt-speak) isn't visible in Qt5, let alone Qt6.

@jbowler
Copy link
Contributor

jbowler commented Oct 6, 2023

So somehow the "related non-member" (in Qt-speak) isn't visible in Qt5, let alone Qt6.

Speaking as a 63 year old C programmer it looks like a #define, or the C++ equivalent. It looks like the obsoleting of QMatrix in 5.15 (probably 5.14) was done in a way that the PythonQt generator can't handle. Or maybe this happened earlier; there is no generated_cpp_512. WIP.

@usiems
Copy link
Contributor

usiems commented Oct 6, 2023

Yes, we never got the generator to work on later versions of Qt, apparently the parser balks at some syntax, and we never figured out where and how to patch it (with the parser code not exactly being well documented).
Our current plan is to replace the generator completely for Qt6 with something that is derived from Shiboken, which is used by the PySide binding.
At least we should use an external C++ parser that can handle C++-17 - which probably would mean libclang (which is also used by Shiboken) - so we don't have to maintain the parser ourselves. But we didn't get around to look into this closer yet.

But quite possibly we will only do this for Qt6, so if you could get the generator to work for Qt5.12/5.15, this would be very welcome.

@jbowler
Copy link
Contributor

jbowler commented Oct 6, 2023

src/PythonQt is clearing missing QTransform and QMatrix4x4 but quite apart from that a lot of the syntax is hand written in generator/*.xml. In particular typesystem_gui.xml explicitly references QMatrix and contains XML that seems to modify QMatrix, including a line that seems to be making operator* private. and contains comments like "Obsolete in 4.3".

Nevertheless ATM generated_cpp_65 (at least) is required; generated_cpp_511 doesn't work, it fails at #include <qmatrix.h>, as would be expected.

@jbowler
Copy link
Contributor

jbowler commented Oct 6, 2023

@usiems it's mainly lack of support for 'constexpr' in the parser, but Qt6 also includes one use of 'auto' (on a class member without a type). I also fixed the handling of 'mutable' (it's not a storage class!) My fix for 'auto' is just to skip it, it may work to transfer it to the definition but I would have to understand too much of the type generation heuristics to be able to do it in reasonable time.

I'm currently blocked on this:

class PythonQtPublicPromoter_QApplicationStateChangeEvent : public QApplicationStateChangeEvent
{ public:
inline QApplicationStateChangeEvent&  promoted_operator_assign(QApplicationStateChangeEvent&  arg__1) { return this->operator=(arg__1); }
inline QApplicationStateChangeEvent*  py_q_clone() const { return QApplicationStateChangeEvent::clone(); }
}; 

I can understand how it is meant to work, there should be a constructor for a PythonQtPublicPromoter_QApplicationStateChangeEvent from a QApplicationStateChangeEvent, but clearly there isn't a constructor in the class at all, so this happens:

../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h:102:127: error: no matching function for call to ‘PythonQtPublicPromoter_QApplicationStateChangeEvent::operator=(QApplicationStateChangeEvent&)’
  102 | inline QApplicationStateChangeEvent&  promoted_operator_assign(QApplicationStateChangeEvent&  arg__1) { return this->operator=(arg__1); }
      |                                                                                                                ~~~~~~~~~~~~~~~^~~~~~~~
../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h:100:7: note: candidate: ‘constexpr PythonQtPublicPromoter_QApplicationStateChangeEvent& PythonQtPublicPromoter_QApplicationStateChangeEvent::operator=(const PythonQtPublicPromoter_QApplicationStateChangeEvent&)’
  100 | class PythonQtPublicPromoter_QApplicationStateChangeEvent : public QApplicationStateChangeEvent
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h:100:7: note:   no known conversion for argument 1 from ‘QApplicationStateChangeEvent’ to ‘const PythonQtPublicPromoter_QApplicationStateChangeEvent&’
../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h: In member function ‘QChildEvent& PythonQtPublicPromoter_QChildEvent::promoted_operator_assign(QChildEvent&)’:

Maybe someone who understands the generator can offer wisdom?

@jbowler
Copy link
Contributor

jbowler commented Oct 8, 2023

What was the problem? I.e. the problem which caused the checks not to pass for several days.

@mrbean-bremen
Copy link
Contributor Author

mrbean-bremen commented Oct 9, 2023

What was the problem? I.e. the problem which caused the checks not to pass for several days.

It was a problem in the CI - I just restarted the test. As I had been travelling, I was not really paying attention to this...

Speaking as a 63 year old C programmer it looks like a #define

My first programming book was the first edition of "The C Programming Language" by K&R (just had to comment on this... 😀 )

@usiems
Copy link
Contributor

usiems commented Oct 10, 2023

I'm currently blocked on this:

class PythonQtPublicPromoter_QApplicationStateChangeEvent : public QApplicationStateChangeEvent
{ public:
inline QApplicationStateChangeEvent&  promoted_operator_assign(QApplicationStateChangeEvent&  arg__1) { return this->operator=(arg__1); }
inline QApplicationStateChangeEvent*  py_q_clone() const { return QApplicationStateChangeEvent::clone(); }
}; 

I can understand how it is meant to work, there should be a constructor for a PythonQtPublicPromoter_QApplicationStateChangeEvent from a QApplicationStateChangeEvent, but clearly there isn't a constructor in the class at all, so this happens:

../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h:102:127: error: no matching function for call to ‘PythonQtPublicPromoter_QApplicationStateChangeEvent::operator=(QApplicationStateChangeEvent&)’
  102 | inline QApplicationStateChangeEvent&  promoted_operator_assign(QApplicationStateChangeEvent&  arg__1) { return this->operator=(arg__1); }
      |                                                                                                                ~~~~~~~~~~~~~~~^~~~~~~~
../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h:100:7: note: candidate: ‘constexpr PythonQtPublicPromoter_QApplicationStateChangeEvent& PythonQtPublicPromoter_QApplicationStateChangeEvent::operator=(const PythonQtPublicPromoter_QApplicationStateChangeEvent&)’
  100 | class PythonQtPublicPromoter_QApplicationStateChangeEvent : public QApplicationStateChangeEvent
      |       ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h:100:7: note:   no known conversion for argument 1 from ‘QApplicationStateChangeEvent’ to ‘const PythonQtPublicPromoter_QApplicationStateChangeEvent&’
../../generated_cpp/com_trolltech_qt_core/com_trolltech_qt_core0.h: In member function ‘QChildEvent& PythonQtPublicPromoter_QChildEvent::promoted_operator_assign(QChildEvent&)’:

Maybe someone who understands the generator can offer wisdom?

I'm not completely sure, but I believe there is no constructor missing, the compiler is just trying to use the wrong assignment operator: Obviously the assignment operator that was generated implicitly for the new class, but not the assignment operator of the class from which this class was derived from, which would be the correct choice. This might be a consequence of the raised C++ standard level.

If I remember correctly these classes only serve the purpose to make protected methods/operators available for use in Python (so you can use them in classes derived in Python). There is no constructor because the types are simply cast to the promoter type where needed.

There is a syntax to explicitly use a method/operator from the super class, which the generator should use here. It might be
return this->QApplicationStateChangeEvent::operator=(arg__1); in this case.

@jbowler
Copy link
Contributor

jbowler commented Oct 10, 2023

There is a syntax to explicitly use a method/operator from the super class, which the generator should use here. It might be
return this->QApplicationStateChangeEvent::operator=(arg__1); in this case.

That would avoid the creation of a new object which was apparently happening, but the problem may be happening because a template is involved and templates seem to be removed by the typesystem*.xml files.

@jbowler
Copy link
Contributor

jbowler commented Oct 12, 2023

The use of auto here, and I assume other places where template classes are used, may cause problems:

https://doc.qt.io/qt-6.5/qhash.html

'auto' is used extensively in 5.15.11 (211 occurrences in 69 of the 5652 the Qtheader files I have installed). Most of these are in inline code although there is at least one occurrence of a template definition using both 'auto' and 'decltype' (the latter was previously unparsed, so it looked like an id).

In 6.5.3 there are 816 instances in 174 files but I currently only have 4738 files in total (I'm using a gentoo ~dev overlay).

@mrbean-bremen
Copy link
Contributor Author

Given that the changes in this branch also should benefit Qt 5.15, and the CI is running fine, I think we can already merge this back to master and make further PRs against master. Should there be any disruptive changes for Qt6 in the future, we may think about branching again.

@jbowler, @usiems - what do you think?

@jbowler
Copy link
Contributor

jbowler commented Oct 28, 2023

Of course this set of changes may break PR#136 and vice versa. The typographical change tabs->spaces (129) and "remove anonymous structs" (121) are both likely to create merge conflicts.

I'll run a build test with PR#136 rebased or cherry-picked into qt6 to see what happens.

@mrbean-bremen
Copy link
Contributor Author

I think I'm just going to merge #136 into master, and we can rebase against master as you wrote.
You may also have a look at the PR I made for this branch, which also concerns the parser. I still get a lot of parsing errors if running against Qt6, but that PR cuts these at least in half.

@jbowler
Copy link
Contributor

jbowler commented Oct 28, 2023

PR136 merges into the current qt6 HEAD just fine.

The generated headers for 5.11.3, however, don't work in generated_cpp for a Qt build. Also the generator starts producing "something horrible is here" at Qt 5.14; Qt 5.13 seems not to have them.

I'm not sure if the generated headers issue is my changes to 136; I haven't done such an extensive test before as I assumed the qt6 branch is a WIP and isn't required to work on Qt5.

@jbowler
Copy link
Contributor

jbowler commented Oct 28, 2023

False alarm: it was a problem in my bash function definition. I'll update this with the generated_cpp files that pass and fail with 5.15LTS (i.e. a full build of PythonQt with generated_cpp containing the generated files from the relevant Qt release). So far:

5.11.3: PASS
5.12.2: PASS
5.13.2: PASS
5.14.2: FAIL

I won't go further than that; I expect the generate_cpp to be invalid after Qt5. The build fails, after lots of warnings in other files, while compiling com_trolltech_qt_core_builtin0.cpp multiplying a QLine by a QMatrix. IRC QMatrix was pretty much removed; not unreasonable because the operation is not likely to result in anything useful.

@mrbean-bremen
Copy link
Contributor Author

mrbean-bremen commented Oct 29, 2023

I just checked the CI builds in the master and qt6 branches, and at least the number of generated classes is the same in both branches. The generated code has a few changes, but these seem mostly be related to the different ordering (so some classes turn up in different files).
We actually need a step in the CI to compile the generated files to see if there at least no syntax errors...

I also noticed that the wrapper generation in the MSVC CI (Qt 5.11.3) fails in all branches. As the same Qt version succeeds in the MinGW builds, it is probably a problem in the build configuration.

@jbowler
Copy link
Contributor

jbowler commented Oct 29, 2023

The "failure" with MSVC is a consequence of the Qt header files not being found:

Generated:
  - header....: 0 (0)
  - impl......: 0 (0)
  - modules...: 0 (0)
  - pri.......: 0 (0)

Done, 6 warnings (1812 known issues)

Or, rather, the ones that would lead to wrapper generation. It's a feature of the generator (one I was trying to fix but gave up on); it silently ignores missing files. I agree, it's probably the build setup. I found that I got more comprehensible results when running the generator by not setting QTDIR and suppling the required directories (usually just the top level one) in --include-paths

@jbowler
Copy link
Contributor

jbowler commented Oct 29, 2023

On the "unpredictable" output I found I could use sort on the generated files then diff to see if there were significant changes. It's no help if the Qt headers differ, as between different Qt versions, but for changes to the parser it reveals if a significant change has occurred. It would, of course, be better to have the output in a consistent order preferably reflecting a consistent lexical order (e.g. class name).

The topological sort implemented in abstractmetabuilder.cpp outputs all the "base" classes then all the first-level derived classes and so on. A better partial order would be each base class followed by it's derived classes, using lexical order within the groups; the "depth-first" algorithm.

Anyway the random-order thing is probably coming from some later step; the topo sort is the first line of GeneratorSetQtScript::generate() It's followed by creation of a QSet<QString> declaredTypeNames

@mrbean-bremen
Copy link
Contributor Author

While there are no big changes in the generated code, there are some. Checking only the generated core headers for Qt 5.12 manually showed actually 2 changes: the wrapper for QMetaType misses the enum eclaration for Type in the qt6 branch, and the constructor the QMutex wrapper takes QBasicMutex instead of QMutex as argument (I haven't checked why).

So it is probably best to wait with merging back the branch until the changes are understood, or only cherry pick the changes that may be helpful for Qt5 (like generator fixes).

@mrbean-bremen
Copy link
Contributor Author

Checking only the generated core headers for Qt 5.12 manually showed actually 2 changes: the wrapper for QMetaType misses the enum eclaration for Type in the qt6 branch, and the constructor the QMutex wrapper takes QBasicMutex instead of QMutex as argument

The missing enum is actually fixed by PR for enum class, so I guess I had checked before this was merged back. I will check again later.

So against my earlier judgement, I would like to get this branch back to master as soon as possible, or at the very least cherrypick the changes that certainly won't break anything and may help with 5.15 code (like the generator changes for constexpr & co, and the formatting changes to avoid merge conflicts). I already see merge conflicts creeping up, and there will be more PRs in both branches.

richard42 and others added 21 commits November 1, 2023 12:58
- fix iterator usage as mentioned in review
* Fix handling of compiler standard, 
(Qt6 requires support for C++17) 
* Remove unnecessary QStringList declaration
This function was deprecated in Qt 5.14 then removed in Qt6, along with
'store()'.  The deprecated function information in Qt 5.15LTS says to
use 'loadRelaxed', most likely on the basis that 'load' didn't guarantee
better memory ordering that relaxed provides (see the standard c++
explanations of std::memory_order_relaxed etc).

Signed-off-by: John Bowler <[email protected]>
- missing consta caused a g++ compile error
- adding the const is safe, as it does not modify the LHS
* replace qSort and qStableSort by 
std::stable_sort and std::sort
This completes the qSort handling in generator/ including related
changes caused by the removal of QSet<>::toList() and QSet<>::fromList()
in Qt6 (obsoleted in Qt5.14).
 - replaced it and added backwards compatibility

- provides compatibility with the removal of the global ::endl
for use with QTextStream; Qt6 requires Qt::endl
- includes Qt6 specific changes necessary to deal with the
replacement of QTextCodec by QStringConverter and the slight changes
to the default setup of QTextStream (to UTF-8 with auto-detect of
UTF-16)

---------

Signed-off-by: John Bowler <[email protected]>
The warnings are already only emitted with GCC so changing them to the
GCC diagnostic pragma is safe.
- works around the need to rewrite the XML parser in
generator/typesystem.cpp by invoking the Qt5 compatibility library;
this is the only thing in the generator which requires it but the
alternative is a complete rewrite of the XML reading code
- parses constexpr (in the manner of const) and decltype (in the manner of __typeof) 
  and adds flags for them
These changes use the new and existing support inside the parser to
handle constexpr in Qt6 files and to ignore (with a warning) 'auto'
functions.
Also add the alternate types that are available in Qt5 to replace the
types deprecated in Qt5.14 (allows Qt5.15LTS users to make changes
for compatibility with Qt6.)
- a previous fix for Qt5.12 breaks pythonqt_generator for 5.11
- includes better checking for precompiled headers and errors out early rather than
  failing when linking the src/ with mysterious missing symbols.
Expand all the tabs in non-binary source files
This is a minimal change to give the three anonymous structs used
in the generator names so that the code conforms to the existing
C++ standards.
This adds 'context' to ReportHandler warning messages; just a string
saying what is being done.
In some places the list was sorted by pointer (resulting in a random
sort) in other cases by name, sometimes with stable_sort, sometimes not.
Now sort by name and maintain do a stable_sort.
@mrbean-bremen mrbean-bremen merged commit 941948e into master Nov 2, 2023
11 checks passed
@mrbean-bremen mrbean-bremen deleted the qt6 branch November 2, 2023 19:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants