Skip to content
This repository has been archived by the owner on Nov 5, 2019. It is now read-only.

Simplify OpenSSL checks in crypt.py. #182

Merged
merged 1 commit into from
May 18, 2015

Conversation

craigcitro
Copy link
Contributor

Previously, when confirming OpenSSL is installed, we checked that the module
was installed, and verified that crypto.py was contained in the package
directory.

However, this can cause issues in cases of non-standard installs. In addition,
the imp.find_module call already raises an ImportError in the case that
the module is missing, so this second check isn't needed.

Instead, we just drop the explicit check for crypto.py in the OpenSSL
directory.

PTAL @dhermes

Previously, when confirming OpenSSL is installed, we checked that the module
was installed, and verified that `crypto.py` was contained in the package
directory.

However, this can cause issues in cases of non-standard installs. In addition,
the `imp.find_module` call already raises an `ImportError` in the case that
the module is missing, so this second check isn't needed.

Instead, we just drop the explicit check for `crypto.py` in the `OpenSSL`
directory.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.03%) to 65.12% when pulling a687e96 on craigcitro:crypto_check into e7340f3 on google:master.

@dhermes
Copy link
Contributor

dhermes commented May 15, 2015

@craigcitro I looked at #159 and didn't find any discussion, but I'm fairly certain that imp.find_module does not throw an ImportError in all cases, so your proposed fix doesn't work. I'm going to dig around and report back.

@dhermes
Copy link
Contributor

dhermes commented May 15, 2015

I'm fairly certain I used http://stackoverflow.com/q/14050281/1068170 as my reference and also recall using pkgutil before deciding on imp.

It seems the most reasonable explanation for

if not os.path.isfile(os.path.join(_package_dir, 'crypto.py')):

is to verify that the removed line

from OpenSSL import crypto

still works. Testing out in 0.13 instead of 0.14 it seems that OpenSSL.crypto comes via crypto.so instead of crypto.py:

~/Desktop $ virtualenv venv
~/Desktop $ source venv/bin/activate
(venv) ~/Desktop $ pip install pyOpenSSL==0.13
Collecting pyOpenSSL==0.13
...
(venv) ~/Desktop $ ls -1 venv/local/lib/python2.7/site-packages/OpenSSL/
crypto.so*
__init__.py
__init__.pyc
rand.so*
SSL.so*
test/
tsafe.py
tsafe.pyc
version.py
version.pyc
(venv) ~/Desktop $ cat venv/local/lib/python2.7/site-packages/OpenSSL/version.py
# Copyright (C) AB Strakt
# Copyright (C) Jean-Paul Calderone
# See LICENSE for details.

"""
pyOpenSSL - A simple wrapper around the OpenSSL library
"""

__version__ = '0.13'

@dhermes
Copy link
Contributor

dhermes commented May 15, 2015

@craigcitro WDYT of all that?

@craigcitro
Copy link
Contributor Author

@dhermes I guess I'm confused - the python docs explicitly say find_module will throw if the module isn't found, and that SO link only mentions a problem for dotted modules.

where will this fail?

@dhermes
Copy link
Contributor

dhermes commented May 15, 2015

Yeah you can disregard my first comment. It's safe(ish, mostly) to say that it will throw when it doesn't exist.

More WDYT of the check of OpenSSL.crypto and the original intent of the code.

@craigcitro
Copy link
Contributor Author

do we think there's a case where someone could have OpenSSL installed by not have an OpenSSL.crypto module?

@dhermes
Copy link
Contributor

dhermes commented May 15, 2015

No I don't think so. The flipside: we could check for crypto.py OR crypto.so.

craigcitro added a commit that referenced this pull request May 18, 2015
Simplify OpenSSL checks in crypt.py.
@craigcitro craigcitro merged commit 418d5a6 into googleapis:master May 18, 2015
@craigcitro craigcitro deleted the crypto_check branch May 18, 2015 05:42
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants