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

Update qt511 wrappers #137

Closed
wants to merge 2 commits into from

Conversation

YuriUfimtsev
Copy link
Contributor

  • Just pure regenerated 5.11.3 qt wrappers with 6f75669 generator version to reduce first the difference after fix
  • Regenerated with 10f9576 (after support enum class)

For example, PythonQtWrapper_QThread::setPriority and PythonQtWrapper_QThread::priority functions come back because QThread::Priority is enum class and with updated parser it is generated successfully.

@YuriUfimtsev
Copy link
Contributor Author

CI with wrappers generated with last version of generator isn't passed. Generated cpp will be actual later, after this problem will be fixed

@mrbean-bremen
Copy link
Contributor

I haven't checked all of the changes, but so far it all looks good. There are the changes for modernized C++ from an earlier commit that creates a lot of changes...

CI with wrappers generated with last version of generator isn't passed

I don't understand this - the last CI (yesterday before your PR) had passed fine, and this PR only changes the checked-in generated code that isn't used in the CI. What I am missing?

@iakov
Copy link
Contributor

iakov commented Nov 1, 2023

@mrbean-bremen , there is a problem in generated files for 5.11, probably, the origin is in the generator, before this PR.
Diff and the line in the generated files. Note the duplicates : https://github.com/MeVisLab/pythonqt/blob/ebe211da58091639ab1bc2cc9803365a4a19cdae/generated_cpp_511/com_trolltech_qt_core_builtin/com_trolltech_qt_core_builtin0.h#L1337C8-L1337C13

@iakov
Copy link
Contributor

iakov commented Nov 1, 2023

@YuriUfimtsev , as we discussed in person before: #138

@mrbean-bremen
Copy link
Contributor

Ah ok, I see the duplicates now. Hadn't checked this file yet... I still don't understand why the tests fail in the PR, if only the generated code is changed that is not used in the CI as far as Im understand...

@usiems
Copy link
Contributor

usiems commented Nov 1, 2023

The duplicates are in the generated "builtin" files, which are needed for the PythonQt library itself and thus compiled.
It seems we need to find out the reason for the duplicates.

@mrbean-bremen
Copy link
Contributor

mrbean-bremen commented Nov 1, 2023

I just check the wrappers generated in the CI builds, and in the MinGW 5.3 build (the only one using Qt 5.11 for generating) I can see the same duplication in master, but not in the qt6 branch. Going back through the builds with the generated wrapper code I found that the first build where the problem did not occur was the one after this PR by @jbowler.

Which is yet another reason why I think that we shall merge back the qt6 branch soon... Though I also noticed that the latest fix for enum class seems not to work in the qt6 branch, maybe there is some merge problem that we have to fix first.

Anyway, I took the liberty to cherry-pick that commit to this PR to check if it helps - it does not, so I guess there are more changes from earlier commits on the qt6 branch involved (I reverted the cherry-pick).

@jbowler
Copy link
Contributor

jbowler commented Nov 1, 2023

You just need the fix to generator/qtscript_masterinclude.h

The other stuff fixes the problem where the "generated_cpp_xxx" files picked up were somewhat unpredictable the stuff in build/common.prf isn't used inside generator/.

Take a look at https://github.com/MeVisLab/pythonqt/blob/master/build/common.prf Run through it with QT_MAJOR_VERSION having the value "5" and QT_MINOR_VERSION the value "15". The 'contains' operations are strict string contains:

contains(variablename, value)
Succeeds if the variable variablename contains the value value; otherwise fails. It is possible to specify a regular expression for parameter value.

That's from https://doc.qt.io/qt-6/qmake-test-function-reference.html#contains-variablename-value

So the minor version contains "1", "5" and "15", therefore when building for Qt5.15LTS the code seems to select the wrappers for Qt5.0 in this test:

      else:contains( QT_MINOR_VERSION, 1 ) {
        PYTHONQT_GENERATED_PATH = $$PWD/../generated_cpp_50
      }

I formed the opinion that it was mind bogglingly confusing, at least for my mind, so I wrote it to use equality (which wasn't originally in qmake but is now.)

You probably want that part of the PR too; I can't see any reason for relying on the master version of common.prf

@jbowler
Copy link
Contributor

jbowler commented Nov 1, 2023

Ubuntu 22.04 passes because it ends up using generated_cpp_56. Ubuntu 20.04 fails with generated_cpp_511. My own builds, against Qt5.15, succeed using generated_cpp_511.

In another PR I added qmake messages to say what the Qt version and the generator_cpp_ directory is; commit ab7c445 I think that change is pretty useful too.

@mrbean-bremen
Copy link
Contributor

mrbean-bremen commented Nov 2, 2023

You just need the fix to generator/qtscript_masterinclude.h

Yes, that is what I thought, though that seemed not to be sufficient to fix the problem in this PR (as I wrote - I added this change by cherrypicking that commit, and it still failed, after which I reverted it).
After writing this, I realized that of course it didn't fix it directly - I forgot that this PR only commits the generated code, and does not generate the code itself 🤦🏼‍♂️

The other stuff fixes the problem where the "generated_cpp_xxx" files picked up were somewhat unpredictable the stuff in build/common.prf isn't used inside generator/.

Yes, I saw that and also think that it should be merged to main.
Generally speaking, do you see anything in your many commits that you would not be comfortable to merge to mainmaster?

Side note: I always confuse master with main, being used to main being the default nowadays - maybe we should change master to main...

We could cherrypick the relevant changes, but I would actually prefer to merge it all to master (after we have understood why the enum class fix does not work there), and go from there.
I'm also hoping for @iakov to make a PR for adding compiling the generators to the CI 😁. This would reduce the possibility for regressions in the generator.

Ubuntu 22.04 passes because it ends up using generated_cpp_56

Ah, I missed that - that makes sense (I mean it doesn't make sense that it used this version, but that it passed...). That also explains why it failed after I cherrypicked your commit, as it would also have used 5.11 due to your fix.

@mrbean-bremen
Copy link
Contributor

after we have understood why the enum class fix does not work there

This was an incorrect assumption. The missing enum (QMetaFile::Type) is not declared as enum struct, just as enum. It also has no Q_DECLARE_FLAGS (other than TypeFlag), and no Q_ENUM, so it may just be correct that it is not generated - I'm not completely sure about the logic there, have to check that.

@jbowler
Copy link
Contributor

jbowler commented Nov 2, 2023

Generally speaking, do you see anything in your many commits that you would not be comfortable to merge

No and anyway I don't expect that errors in the generator itself are going to impact typical users. Someone out there probably uses the generator to produce subsets or supersets of the current generator build files; generated/build_all.txt (the typesystem file; it's XMLish) and qtscript_masterinclude.h; the file that pulls in the Qt headers. Those people can deal with new issues and will have their own .h and .xml (typesystem) files.

A lot of the changes I made were to do with attempting to get better error messages out of the generator. I didn't make much, if any, improvement but those changes should be harmless.

However the first step is to check for issues with the generated files on the qt6 branch. I have limited testing ability; I can only check those generated files against Qt5.15.11LTS and Qt6.6.0. The latter has always failed so it's not very useful.

I've double checked the "generated_cpp_511" on my system, this passes. The generated files are from a download from qt.io of the latest 5.11 they have for free download (5.11.3) and the test is against 5.15.11 (with Gentoo patches). I'll sanity check 5.12.12 and 5.13.2 generated files in the same environment.

Once I have done this I'll generate a strawman PR which includes the new generated_cpp_511 as well as _512 and _513 and so on insofar as they work against 5.15.11. This will allow tests of the generator output from the qt6 branch with the CI system and this will exercise more of the possible test cases (where generated_cpp_foo is used with some other, later, Qt environment.)

If that works we can be pretty sure that any CI failures that show in a merge back into "master" are due to changes only in master...

@YuriUfimtsev
Copy link
Contributor Author

YuriUfimtsev commented Nov 2, 2023

The missing enum (QMetaFile::Type) is not declared as enum struct, just as enum. It also has no Q_DECLARE_FLAGS (other than TypeFlag), and no Q_ENUM, so it may just be correct that it is not generated

That is an interesting question, why QMetaType::Type (if we talk about it) was not added in typesystem earlier despite the warning about it. In the previous PR I added in typesystem new parsed class enum enums and also some enums that were not added, but were enums according to the qt docs and sources, and about which there were warnings from generator.

About Q_DECLARE_FLAGS, I think, that is no problem, because there are other enums that have no Q_DECLARE_FLAGS, but was declared in the generated_cpp_511 in the master branch (e.g. nearby declared in com_trolltech_qt_core1.h enum QMimeDatabase::MatchMode).
And this enum also hasn't Q_ENUM in the qt sources.

@jbowler
Copy link
Contributor

jbowler commented Nov 2, 2023

I'm not completely sure about the logic there, have to check that.

I believe enums, including enums that are declared within a class, have to be explicitly declared in typesystem XML file whereas functions and members are automatically included. At least this is the case with shiboken6, but it's necessary to use --no-suppress-warnings to see the warnings from shiboken6.

@jbowler
Copy link
Contributor

jbowler commented Nov 2, 2023

The qt6 generated files from the official Qt source for 5.11.3, 5.12.12 and 5.13.2 passed the full build tests on my system. Qt5.14.2 fails with what looks like a typesystem.xml issue; the wrappers for QPolygon::cbegin and QPolygon::cend, which return QVector::const_iterator, use QChar* as the return type and so compilation fails.

I've submitted a PR, #139 to test 511, 512 and 513 against more systems.

@mrbean-bremen
Copy link
Contributor

I believe enums, including enums that are declared within a class, have to be explicitly declared in typesystem XML file

True. The enum in question had been added to the typesystem file in the enum class commit and only after that appeared in the generated sources. Though for some reason it did not appear in the qt6 branch despite the same change - I may have a closer look into this tomorrow.

The qt6 generated files from the official Qt source for 5.11.3, 5.12.12 and 5.13.2 passed the full build tests on my system.

Great! In my book that means that we can merge back qt6 into main and make future PRs against main. I'll do this as soon as you give your ok (or the next day due to time zone issues). Getting correct 5.15 generated files would be the next step - there are more fixes in the generator needed.

@jbowler
Copy link
Contributor

jbowler commented Nov 2, 2023

I'll do this as soon as you give your ok (or the next day due to time zone issues). Getting correct 5.15 generated files would be the next step - there are more fixes in the generator needed.

It's good to go; PR #139 passes all CI checks. I suggest you rebase qt6 back in as it is, without #139, then I can probably re-purpose PR #139 (or create a new request if necessary) to perform the same tests against "master".

@mrbean-bremen
Copy link
Contributor

@jbowler - ok, done! I think we can close this PR as it will be superseded with your new PR, if that is ok with @YuriUfimtsev.

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