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: Fix missing SSL libraries on Windows with PyQt5.QtNetwork #3520

Merged
merged 3 commits into from Jun 16, 2018

Conversation

Projects
None yet
4 participants
@remyroy
Contributor

remyroy commented May 17, 2018

Fixes #3511 . Tested to work on Windows.

I wish I could have added a functional test as well, but I can't seem to get the tests to run even after installing pypiwin32. I'm getting the following error:

> py.test -n 8 tests\functional
PyInstaller cannot check for assembly dependencies.
Please install PyWin32 or pywin32-ctypes.

pip install pypiwin32
@remyroy

This comment has been minimized.

Contributor

remyroy commented May 17, 2018

I guess I should have used the pywin32-ctypes package instead. Related to #3513 .

@bjones1 bjones1 self-assigned this May 24, 2018

@htgoebel htgoebel added the PyQt5 label May 27, 2018

@bjones1

This comment has been minimized.

Member

bjones1 commented May 29, 2018

This looks fairly good. Would you provide a test, or a code snippet I could use to test? See the discussion at #3511 (comment).

@remyroy

This comment has been minimized.

Contributor

remyroy commented Jun 1, 2018

I will expand this PR to include a test. I just gotta learn your testing framework.

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 1, 2018

Great! Something like this placed in tests/functional/test_libraries.py should do the trick:

@importorskip('PyQt5')
def test_PyQt5_QtNetwork(pyi_builder):
    pyi_builder.test_source(
        """
        from PyQt5.QtWidgets import QApplication
        from PyQt5.QtNetwork # or whatever it's called
        app = QApplication([])
        connection = QtNetwork.QNiftyNetworkTestThingy().go()
        # Exit Qt when the main loop becomes idle.
        QTimer.singleShot(0, app.exit)
        app.exec_()
        """)

Then run via pytest -k test_PyQt5_QtNetwork.

@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 11, 2018

To test SSL support, those lines are enough (to encapsulate in a function of course):

from PyQt5.QtNetwork import QSslSocket

assert QSslSocket.supportsSsl()

+1 for the merge of this fix :)

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 11, 2018

@BoboTiG, thanks for suggesting a test. However, this test passes even without the modified hook provided by @remyroy. Would you provide a test that fails without this hook, but passes with the hook?

@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 11, 2018

@bjones1, I use this test once the application is packaged to check for DLL misses. The QSslSocket.supportsSsl() function checks for DLLs in several common places.
Finally, it might be unrevelant here if we cannot be sure of the result.

@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 13, 2018

Actually I found a way to reproduce but I am trying to figure it out to automate the thing.
Using pyi_builder.test_source() or pyi_builder.test_script() will not show the bug but when manually packaging the app and running this app will show the bug.

Is there a way to simulate this behavior?

This simple test must show the bug:

@importorskip('PyQt5')
def test_PyQt5_SSL_support(pyi_builder):
    pyi_builder.test_source(
        """
        from PyQt5.QtNetwork import QSslSocket
        assert QSslSocket.supportsSsl()
        """)

You can try to create a simple app with those 2 lines and you will see the AssertionError. When running from the test suite, no error. There must be something that interfers, but I do not see what.

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 13, 2018

Strange. The test process should be very close to the manual process. Would you give me the script that fails manually, so I can check it out as well?

@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 13, 2018

Yeah, of course. Pretty simple:

# file: pyqt5_ssl_support.py
from PyQt5.QtNetwork import QSslSocket
assert QSslSocket.supportsSsl()

Then, both onedir and onefile will generate an exe that will raises:

python.exe -m pyinstaller pyqt5_ssl_support.py
pyqt5_ssl_support.exe
rem And you should see the AssertionError
@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 15, 2018

I found the cause of the not failing test. This is the content of os.environ when running the test:

# ...
'PATH': 'c:\\python-3.6.5\\lib\\site-packages\\PyQt5\\Qt\\bin;...',

DLLs are present into the path and the test does not fail.

While printing its value from the manually packaged app:

# ...
'PATH': 'C:\\Users\\TestW764\\AppData\\Local\\Temp\\_MEI23362\\PyQt5\\Qt\\bin;...'

And there, no DLLs: failure.

Do you have an idea for an eventual fix?

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 15, 2018

Yes, that's the problem. Thanks for the debug. Therefore, I'd like to merge this even though the test pass, and open a separate issue for the broken test environment. Would you include the test in test_libraries.py in this PR?

@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 15, 2018

I would include the test in this PR to be consistent with the fix, yes. I do not have the rights to push on this branch, but if @remyroy has time to add those lines, it would be very nice:

@importorskip('PyQt5')
def test_PyQt5_SSL_support(pyi_builder):
    pyi_builder.test_source(
        """
        from PyQt5.QtNetwork import QSslSocket
        assert QSslSocket.supportsSsl()
        """)
@BoboTiG

This comment has been minimized.

Contributor

BoboTiG commented Jun 15, 2018

FTR, I can certify that the fix is good. Tested manually.

@bjones1 bjones1 force-pushed the remyroy:win-qtnetwork-missing-ssl-dlls-fix branch from c2ec423 to aa3f846 Jun 15, 2018

@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 15, 2018

I went ahead on this, plus added a rebase to pull in the latest changes. I'll merge if the tests pass.

@bjones1 bjones1 merged commit abb09fe into pyinstaller:develop Jun 16, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@bjones1

This comment has been minimized.

Member

bjones1 commented Jun 16, 2018

@remyroy and @BoboTiG, thanks for your help on this!

@remyroy

This comment has been minimized.

Contributor

remyroy commented Jun 16, 2018

No problem. I wish I had more time to follow up on this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment