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

Fix QtWebEngine with PyQt6 framework builds #6892

Merged
merged 2 commits into from
Jun 17, 2022

Conversation

The-Compiler
Copy link
Contributor

With a PyPI-installed PyQt6-WebEngine (i.e., from what I understand, a framework-based install) on macOS, some libraries seem to be missing for the renderer process.

The first issue I got was that QtWebEngineProcess can't find QtOpenGL (which is new in Qt 6 and split from QtGui):

dyld: Library not loaded: @rpath/QtOpenGL.framework/Versions/A/QtOpenGL
  Referenced from: [...]/dist/test/PyQt6/Qt6/lib/QtWebEngineCore.framework/Helpers/QtWebEngineProcess.app/Contents/MacOS/QtWebEngineProcess
  Reason: image not found

Adding QtOpenGL to the libraries being copied in a framework build seems to help.

With that fixed (pip install git+https://github.com/the-compiler/pyinstaller@4d6cf4b), I'm getting a similar error about QtDBus, via QtGui:

dyld: Library not loaded: @rpath/QtDBus.framework/Versions/A/QtDBus
  Referenced from: [...]/dist/test/PyQt6/Qt6/lib/QtGui.framework/Versions/A/QtGui
  Reason: image not found

Why QtGui seems to depend on QtDBus on macOS (!) is beyond me, but whatever, let's add it as well.

With both fixes, only this is remaining:

Init Parameters:
  *  application-name  
  *  browser-subprocess-path [...]/dist/test/PyQt6/Qt6/lib/QtWebEngineCore.framework/Helpers/QtWebEngineProcess.app/Contents/MacOS/QtWebEngineProcess 
  *  [...]

Qt WebEngine resources not found at [...]/dist/test//Users/florian/proj/qutebrowser/qtwetest/dist/test. Trying parent directory...
Qt WebEngine resources not found at [...]/dist/test/PyQt6/Qt6. Trying application directory...
Installed Qt WebEngine locales directory not found at location /Users/florian/proj/qutebrowser/qtwetest/dist/test//Users/florian/proj/qutebrowser/qtwetest/dist/test/qtwebengine_locales. Trying application directory...

which seems to be non-fatal, as it seems to work in the end.

@rokm
Copy link
Member

rokm commented Jun 15, 2022

Nice catch! This has gone unnoticed because the tests are passing in spite of the QtWebEngineProcess crashing and leaving an error message window.

@rokm
Copy link
Member

rokm commented Jun 15, 2022

Please add a news entry for the PR; then this should be good to go.

@The-Compiler
Copy link
Contributor Author

Updated! Will take a look if I can make the test more strict.

@The-Compiler
Copy link
Contributor Author

Test pushed too now. Fails for me locally without the change, and will hopefully pass everywhere on CI.

@The-Compiler
Copy link
Contributor Author

Rebased to merge 4729a83 and 9b77094 into one commit, with no other changes. Should be ready now.

news/6892.bugfix.rst Outdated Show resolved Hide resolved
Copy link
Member

@rokm rokm left a comment

Choose a reason for hiding this comment

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

Aside from the backticks in news fragment, this looks good. Thanks for taking care of the test as well!

@The-Compiler
Copy link
Contributor Author

Whoops! News fragment fixed now. FWIW over at pytest we set Sphinx' default_role to literal to make single backticks work - but not sure I really like it, because it leads to situations like this (and to somewhat inconsistent usage).

@bwoodsend
Copy link
Member

I quite like to set the default Sphinx role to :any:`xxx` so that you can put cross references in docstrings without the raw un-sphinx-ified help text having distracting :func:`...` s all over the place. I didn't fancy touching the default role for PyInstaller because a lot of our docs currently uses single backticks instead of single asterisks for itallics.

@bwoodsend bwoodsend added the merge-on-ci-pass This PR is ready to merge providing CI passes label Jun 16, 2022
@The-Compiler
Copy link
Contributor Author

@bwoodsend added the merge-on-ci-pass label

FYI, just in case you weren't aware: Since some while, GitHub supports enabling a "Enable auto-merge" button which does this automatically:

If you're already aware and there's a reason why this doesn't work out for PyInstaller, ignore me 😅

@bwoodsend
Copy link
Member

You know, I can't figure out how to get that feature working at all. I've got the box checked under https://github.com/pyinstaller/pyinstaller/settings#merge-button-settings...

image

... which I expect to lead to an automerge button appearing somewhere on:

image

But there's no visible changes at all. Nor can I find anywhere to specify exactly what our merge requirements should be (CI passing and at least one stamp of approval from one of the core devs).

@bwoodsend bwoodsend merged commit 6889f34 into pyinstaller:develop Jun 17, 2022
@The-Compiler
Copy link
Contributor Author

Thanks for the merge! 🎉

I believe those are set up as branch protection rules, under "Branches" in the repository settings: Managing a branch protection rule - GitHub Docs

@bwoodsend
Copy link
Member

Ahh got it now. I needed to add a rule to make all changes to develop require a pull request, then everything fell into place from there. Thanks for the help.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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

3 participants