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

add a hook for qtmodern #305

Merged
merged 6 commits into from
Aug 26, 2021
Merged

add a hook for qtmodern #305

merged 6 commits into from
Aug 26, 2021

Conversation

sanzoghenzo
Copy link
Contributor

closes #304

@sanzoghenzo sanzoghenzo requested review from a team and rokm and removed request for a team August 25, 2021 19:54
@bwoodsend
Copy link
Member

Ughh, I'm seeing Windows+onefile only failures. This looks like a nightmare to debug.

@rokm
Copy link
Member

rokm commented Aug 25, 2021

I don't think the proposed import-only test will catch the missing data files, as those are accessed only when qtmodern.windows.ModernWindow is instantiated (resources/frameless.qss) or when qtmodern.style.dark() or light() is called (resources/style.qss). So the test should make use of those, e.g.,

import sys

from PyQt5 import QtCore, QtWidgets
import qtmodern.windows
import qtmodern.styles

app = QtWidgets.QApplication(sys.argv)

window = QtWidgets.QWidget()
window.setWindowTitle("Hello world!")
modern_window = qtmodern.windows.ModernWindow(window)
modern_window.show()

qtmodern.styles.dark(app)

QtCore.QTimer.singleShot(1000, app.quit)

app.exec()

or even without the last two calls, as we do not really need to start the event loop (nor quit it via timer) for this test.

@rokm
Copy link
Member

rokm commented Aug 25, 2021

Ughh, I'm seeing Windows+onefile only failures. This looks like a nightmare to debug.

It's not onefile per se that's problematic, but rather the PyQt5-based test that comes second. Which probably means that PyQt5 was first imported within the first test, and the second test somehow lost the PATH modification that happened during the import...

@rokm
Copy link
Member

rokm commented Aug 25, 2021

And the "somehow" part boils down to pyi_builder fixture using monkeypatch...

I suppose if we want to use PyQt5 in tests here, we will have to do explicit import outside of the tests, same as we do in main repository's tests...

@bwoodsend
Copy link
Member

bwoodsend commented Aug 25, 2021

Something I never understood - where do the PyQt5 DLL paths get added to the master process's PATH? Surely, it should all happen in subproceses?

@rokm
Copy link
Member

rokm commented Aug 26, 2021

Something I never understood - where do the PyQt5 DLL paths get added to the master process's PATH? Surely, it should all happen in subproceses?

Yeah, it should...

But what actually happens is that get_module_file_attribute (called from here) is not properly isolated. I.e., if you call it with top-level package name, all is well, but if we call get_module_file_attribute('PyQt5.QtCore'), that ends up importing PyQt5...

>>> from PyInstaller.utils.hooks import get_module_file_attribute
>>> import sys
>>> 'PyQt5' in sys.modules
False
>>> get_module_file_attribute('PyQt5')
'[...]/lib64/python3.9/site-packages/PyQt5/__init__.py'
>>> 'PyQt5' in sys.modules
False
>>> get_module_file_attribute('PyQt5.QtCore')
'[...]/lib64/python3.9/site-packages/PyQt5/QtCore.abi3.so'
>>> 'PyQt5' in sys.modules
True

@bwoodsend
Copy link
Member

Ahh so get_module_file_attribute('PyQt5.QtCore') is the leak. In which case it should jump straight to the subprocess behaviour if the module name contains a ..

@rokm
Copy link
Member

rokm commented Aug 26, 2021

Ahh so get_module_file_attribute('PyQt5.QtCore') is the leak. In which case it should jump straight to the subprocess behaviour if the module name contains a ..

Would there be too much overhead if we @isolated.decorate the whole function, and removed the exec_statement part (i.e, run the statement's code "directly")?

@rokm
Copy link
Member

rokm commented Aug 26, 2021

Ahh so get_module_file_attribute('PyQt5.QtCore') is the leak. In which case it should jump straight to the subprocess behaviour if the module name contains a ..

Would there be too much overhead if we @isolated.decorate the whole function, and removed the exec_statement part (i.e, run the statement's code "directly")?

Although that's applicable only to develop; if we want this also fixed in v4, we should go with the fix you suggested.

@bwoodsend
Copy link
Member

@sanzoghenzo I'm afraid you're unlucky in that you've just put your foot into a hornet's nest. For now can you put:

if is_win:
    # This is a hack to prevent monkeypatch from interfering with PyQt5's additional PATH entries. See:
    # https://github.com/pyinstaller/pyinstaller/commit/b66c9021129e9e875ddd138a298ce542483dd6c9
    try:
        import PyQt5
    except ImportError:
        pass

just above your test_qtmodern(). That should fix the failures.

@sanzoghenzo
Copy link
Contributor Author

sanzoghenzo commented Aug 26, 2021

@sanzoghenzo I'm afraid you're unlucky in that you've just put your foot into a hornet's nest.

Yeah, usually I use conda to install these packages ad avoid these troubles on windows...
I just re-read the discussion and I get that the issue is not what I was thinking, even though I don't get the error on my conda environment on windows...

I'll fix it ASAP

@bwoodsend
Copy link
Member

src/_pyinstaller_hooks_contrib/tests/test_libraries.py:747:9: F401 'PyQt5' imported but unused

Ughh, better make that import PyQt5 # noqa: F401.

@bwoodsend
Copy link
Member

Ahh, you're ahead of me...

@bwoodsend
Copy link
Member

And here's a CI run. The Linux ones failed but that's just because I hadn't enabled Xfvb virtual screen. They pass locally.

@bwoodsend bwoodsend merged commit fd70762 into pyinstaller:master Aug 26, 2021
@bwoodsend
Copy link
Member

Nice work @sanzoghenzo.

@sanzoghenzo sanzoghenzo deleted the qtmodern branch August 26, 2021 16:28
bwoodsend pushed a commit that referenced this pull request Aug 26, 2021
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.

Add qtmodern hook
4 participants