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 hook for _ssl #13

Merged
merged 4 commits into from Jul 7, 2019
Merged

Add hook for _ssl #13

merged 4 commits into from Jul 7, 2019

Conversation

LeonarddeR
Copy link
Contributor

In python 3.7 and up, _ssl.pyd relies on libcrypto and libssl. These aren't picked up by py2exe, which is problematic for NVDA (see nvaccess/nvda#9831)

This patch is based on marcelotduarte/cx_Freeze#470 by @sekrause. I made sure that after building an NVDA distribution with this patch applied, the libcrypto and libssl modules were copied successfully.

@albertosottile
Copy link
Member

I am currently using _ssl in one of my projects packed via py2exe and in our bundle the libraries that you mention are there. Perhaps that is because we also use 'packages' : 'OpenSSL' in our setup scripts. Is this a viable approach for you?

@LeonarddeR
Copy link
Contributor Author

If I try to import either openssl or OpenSSL from an interpreter, I get a module not found. I'm also unable to import the cryptography package.

If this requires an installation from Pip, I'm afraid it goes a bit too far for our approach.
I will research some alternatives though.

@albertosottile
Copy link
Member

Is libcrypto available by default in Windows python installations? Because as far as I know that is not the case on other platforms. In those, you need to install cryptography or pyOpenSSL via pip.

@LeonarddeR
Copy link
Contributor Author

I have both libcrypto-1_1.dll and libssl-1_1.dll. If I remove either one of these from my python directory, import ssl fails with a dll load error.

@albertosottile
Copy link
Member

albertosottile commented Jun 27, 2019

I think the key point in this is that all this occurs only in Python 3.7 and higher, while I pack the app with 3.6 yet. I'll test this PR and merge it as soon as possible. I can confirm though that the hook is not required if pyOpenSSL is included as a package by the setup script.

@LeonarddeR
Copy link
Contributor Author

LeonarddeR commented Jun 27, 2019 via email

@albertosottile
Copy link
Member

To improve the test coverage of py2exe, I have put together a functional test for ssl and socket. Unfortunately, perhaps this test is too minimal because it does not expose the bug addressed by this PR. You can find details about the test in the ssl_test branch, including the code and the results. Do you have any advice about the methods that I should call to trigger the missing DLL error on 3.7? I thought that importing ssl was enough, but it does not look that way.

@LeonarddeR
Copy link
Contributor Author

I'm afraid that running a functional test on appveyor isn't going to work. That's also the problem we have with NVDA, the builds run just fine within our system testing framework on appveyor. but don't run on a local system. I think this is because appveyor adds something to the path variable it should not add. See nvaccess/nvda#9831 (comment)

If I build your test locally and try to run it, I get a traceback as expected. If you can't reproduce your problem locally, could it be that you have the DLLs folder iyour path variable?

@albertosottile
Copy link
Member

I understand the problem, but I believe that to address this and other similar issues systematically we need to find a way to perform these functional tests on AppVeyor, hence apparently we need to set up a clean environment. I am trying to reproduce this error in the CI, so far with no luck. I tried to use - ps: $env:PATH = '' as described here, but it did not help. Any ideas?

@LeonarddeR
Copy link
Contributor Author

LeonarddeR commented Jul 1, 2019 via email

@albertosottile
Copy link
Member

I am afraid there is not much we can do, at least not for this particular case. I restricted the whole environment to a single variable SystemRoot = 'C:\WINDOWS' using subprocess. With that env, the SSL test passes on all the Python versions. Deleting also that one makes it fail on all the platforms (Fatal Python error: failed to get random numbers to initialize Python). So, it seems that the offending DLLs are copied in C:\Windows in the AppVeyor configuration. The fact that we do not know what else is contained in there and that there is no way to exclude them without making packed apps unusable makes me quite sad and worried.

That being said, I will test this PR manually and merge it as soon as possible. Thanks for your assistance and your patience.

@FeodorFitsner
Copy link

@albertosottile what build image is that?

@albertosottile
Copy link
Member

@albertosottile what build image is that?

Visual Studio 2015. You can look at a build here: https://ci.appveyor.com/project/albertosottile/py2exe/builds/25647974

On Python 3.7, the test is supposed to fail because libssl and libcrypto are not packed by the bundler, but the executable still finds them in C:\WINDOWS\System32.

@albertosottile
Copy link
Member

albertosottile commented Jul 2, 2019

@FeodorFitsner I just realized I have not tagged you in my answer before.

@FeodorFitsner
Copy link

I remember we were discussing this issue internally and agreed to leave those DLLs in C:\WINDOWS\System32...for now. The issue is those libs were installed as a dependency of some other software we didn't identify yet. So, there is a risk to break some other stuff on the image.

You can just add a line to your build removing those libs from C:\WINDOWS\System32. Hope that helps.

@albertosottile
Copy link
Member

albertosottile commented Jul 3, 2019

@FeodorFitsner I asked if those had been deleted because that's what I understood from the issue in your tracker. Thanks for your suggestion.

EDIT: read the comment below.

@albertosottile
Copy link
Member

Found them, they were in C:\Windows\SysWOW64. Now the SSL test is behaving as expected: failing only on cp37 on both archs. I just included this ssl_test in master. @LeonarddeR could you please rebase your fork on the current master, so that AppVeyor will run again on this PR with this new test? Thanks.

@LeonarddeR
Copy link
Contributor Author

@albertosottile Sorry that this took some time, I've just merged master.

@albertosottile albertosottile merged commit b0c299c into py2exe:master Jul 7, 2019
@albertosottile
Copy link
Member

The SSL test seems to serve its purpose now, and the hook works also as expected. Merged this now. Thank you as usual for your valuable contributions to this project.

@albertosottile
Copy link
Member

@LeonarddeR I see from your repository that NVDA 2019.3 is close, that it should run on Python 3.7, and that it should use py2exe 0.9.2.2. Would you like me to release 0.9.2.2 in the next days, so that you can refer to this version number and to the corresponding stable wheels, instead of pointing to a commit number?

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.

None yet

3 participants