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

hooks: update QtNetwork hooks for Qt 6.2 compatibility #6276

Merged
merged 6 commits into from
Oct 9, 2021

Conversation

rokm
Copy link
Member

@rokm rokm commented Oct 6, 2021

Qt 6.2 introduced new set of tls plugins that need to be collected for QSslSocket to work.

Simply filtering out "*d.dll" is insufficient as
we now have Qt plugins whose names end with
"backend" (e.g., "tls/qopensslbackend.dll").

Therefore, upgrade the filtering logic to exclude
a DLL whose name ends with "d" only if the
corresponding "release" name (i.e., name shortened
by one character) also exists.
As of Qt 6.2, we need to collect "tls" plugins as part of QtNetwork
for QSslSocket to work.

There is another group of plugins that belongs to QtNetwork in Qt 6.2,
called "networkinformation". These were introduced in Qt 6.1 as
"networkinformationbackends". So collect both variants.
@rokm
Copy link
Member Author

rokm commented Oct 7, 2021

The switch to offscreen Qt backend will hopefully take care of failures such as seen here and here (two subsequent attempts). I think for our testing purposes, using offscreen backend makes more sense than hoping that display works properly on the CI. This approach is also suggested in #5120.

@rokm rokm marked this pull request as ready for review October 7, 2021 19:47
@rokm rokm added the needs backport This pull request needs cherry picking onto the v4 branch. label Oct 7, 2021
Copy link
Member

@bwoodsend bwoodsend left a comment

Choose a reason for hiding this comment

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

Looks good to me.

We currently set QT_DEBUG_PLUGINS=1, but only on ubuntu runner.
We should either use it on all OSes, or just enable the extra
verbosity when we suspect a problem with plugin loading instead
of having it always on.
Run Qt-based tests with offscreen backend instead of default
display one. Should both speed up the tests that display the UI,
as well as make them more resilient to CI pipeline's display issues
(e.g., lack of OpenGL on macOS runners).
@bwoodsend bwoodsend added the merge-on-ci-pass This PR is ready to merge providing CI passes label Oct 8, 2021
@bwoodsend
Copy link
Member

Do you want to try just setting the timeout limit higher for that dodgy QtQml test? The variants that are passing are all being flagged up as very slow tests getting close to the 300 seconds mark.

@rokm
Copy link
Member Author

rokm commented Oct 8, 2021

Do you want to try just setting the timeout limit higher for that dodgy QtQml test? The variants that are passing are all being flagged up as very slow tests getting close to the 300 seconds mark.

Ah, that's a good catch, thanks!

I had re-run it again to see if it will consistently fail on that particular combination again, so I can try reproducing it locally with the same python version.

But if it's a timing issue (slow build and/or slow startup), then we should just raise the timeout limit.

@bwoodsend
Copy link
Member

Also this makes me think that that Windows + Python 3.7 + pandas sporadic failure we've been chasing for ages is also just a bit slow.

For some tests, 5 minutes seems to be cutting it too close... so
increase the safety margin.
@rokm rokm merged commit c085839 into pyinstaller:develop Oct 9, 2021
@rokm rokm deleted the qt62-qtnetwork branch October 9, 2021 09:19
@rokm rokm added backported Backported to v4 branch and removed needs backport This pull request needs cherry picking onto the v4 branch. labels Oct 9, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backported Backported to v4 branch merge-on-ci-pass This PR is ready to merge providing CI passes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants