remove obsolete dependency on "pycrypto" #2873

New issue

Closed

opened 2017-03-29 21:38:47 +00:00 by warner · 5 comments

warner commented

2017-03-29 21:38:47 +00:00

Tahoe currently declares a dependency on pycrypto, despite not using it directly (the only reference is a from Crypto import Util in test_sftp.py, used to check whether we think SFTP is available). We should remove this.

(note: this ticket is not about removing pycryptopp, the minimal python wrapper that zooko initially wrote for the high-quality C++ library known as Crypto++. #2322 is about doing that)

We added this dependency years ago when twisted.conch (which powers our SFTP frontend) needed it for various crypto purposes. At that time, I considered the SFTP frontend to be "optional", and didn't want to impose additional dependencies on users who didn't want it. So I made the SFTP unit tests conditional on whether we could import Crypto or not, thinking that it was the one thing that might not be present (the rest of twisted.conch shows up "for free" as part of Twisted).

Later, we (maybe Daira, who did a lot of work on SFTP) decided to simplify the set of possibilities, and make all dependencies mandatory.

Since then, twisted.conch has switched from pycrypto to (pyca) cryptography.

The right way to express this dependency is by depending upon twistedconch (probably on twistedtls,conch if we need both).

Tahoe currently declares a dependency on [pycrypto](https://pypi.python.org/pypi/pycrypto), despite not using it directly (the only reference is a `from Crypto import Util` in test_sftp.py, used to check whether we think SFTP is available). We should remove this. (note: this ticket is not about removing `pycryptopp`, the minimal python wrapper that zooko initially wrote for the high-quality C++ library known as Crypto++. #2322 is about doing that) We added this dependency years ago when twisted.conch (which powers our SFTP frontend) needed it for various crypto purposes. At that time, I considered the SFTP frontend to be "optional", and didn't want to impose additional dependencies on users who didn't want it. So I made the SFTP unit tests conditional on whether we could import `Crypto` or not, thinking that it was the one thing that might not be present (the rest of twisted.conch shows up "for free" as part of Twisted). Later, we (maybe Daira, who did a lot of work on SFTP) decided to simplify the set of possibilities, and make all dependencies mandatory. Since then, twisted.conch has switched from pycrypto to (pyca) `cryptography`. The right way to express this dependency is by depending upon `twistedconch` (probably on `twistedtls,conch` if we need both).

warner added the

labels 2017-03-29 21:38:47 +00:00

warner added this to the 1.13.0 milestone 2017-03-29 21:38:47 +00:00

daira was assigned by warner

2017-03-29 21:38:47 +00:00

warner commented

2017-04-12 02:13:40 +00:00

Author

(also see #2766, about removing other unnecessary dependencies)

I'm playing with this on a branch.. it's a 3-line fix.

diff --git a/src/allmydata/_auto_deps.py b/src/allmydata/_auto_deps.py
index bec052fa0..9eccf7f8e 100644
--- a/src/allmydata/_auto_deps.py
+++ b/src/allmydata/_auto_deps.py
@@ -42,11 +42,6 @@ install_requires = [
     # * foolscap >= 0.12.6 has an i2p.sam_endpoint() that takes kwargs
     "foolscap >= 0.12.6",
 
-    # Needed for SFTP.
-    # pycrypto 2.2 doesn't work due to <https://bugs.launchpad.net/pycrypto/+bug/620253>
-    # pycrypto 2.4 doesn't work due to <https://bugs.launchpad.net/pycrypto/+bug/881130>
-    "pycrypto >= 2.1.0, != 2.2, != 2.4",
-
     # pycryptopp-0.6.0 includes ed25519
     "pycryptopp >= 0.6.0",
 
@@ -73,7 +68,7 @@ install_requires = [
     # * Twisted-16.1.0 fixes https://twistedmatrix.com/trac/ticket/8223,
     #   which otherwise causes test_system to fail (DirtyReactorError, due to
     #   leftover timers)
-    "Twisted[tls] >= 16.1.0",
+    "Twisted[tls,conch] >= 16.1.0",
 
     # We need Nevow >= 0.11.1 which can be installed using pip.
     "Nevow >= 0.11.1",
@@ -107,7 +102,6 @@ package_imports = [
     ('platform',         None),
     ('pyOpenSSL',        'OpenSSL'),
     ('OpenSSL',          None),
-    ('pycrypto',         'Crypto'),
     ('pyasn1',           'pyasn1'),
     ('service-identity', 'service_identity'),
     ('characteristic',   'characteristic'),

But switching to twistedtls,conch causes the check_all_requirements() in our _auto_deps.py to break, I think because it doesn't know how to parse the comma-separated list of extras:

py27 runtests: commands[1] | tahoe --version
Traceback (most recent call last):
  File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/bin/tahoe", line 11, in <module>
    load_entry_point('tahoe-lafs', 'console_scripts', 'tahoe')()
  File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 560, in load_entry_point
    return get_distribution(dist).load_entry_point(group, name)
  File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2648, in load_entry_point
    return ep.load()
  File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2302, in load
    return self.resolve()
  File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2308, in resolve
    module = __import__(self.module_name, fromlist=['__name__'], level=0)
  File "/Users/warner/stuff/tahoe/tahoe/src/allmydata/__init__.py", line 452, in <module>
    check_all_requirements()
  File "/Users/warner/stuff/tahoe/tahoe/src/allmydata/__init__.py", line 450, in check_all_requirements
    raise PackagingError(get_error_string(fatal_errors + _cross_check_errors, debug=True))
allmydata.PackagingError:
PackagingError: no version info or could not understand requirement 'Twisted[tls,conch] >= 16.1.0'

For debugging purposes, the PYTHONPATH was
  None
install_requires was
  ['setuptools >= 11.3', 'zfec >= 1.1.0', 'zope.interface >= 3.6.0, != 3.6.3, != 3.6.4', 'foolscap >= 0.12.6', 'pycryptopp >= 0.6.0', 'service-identity', 'characteristic >= 14.0.0', 'pyasn1 >= 0.1.8', 'pyasn1-modules >= 0.0.5', 'Twisted[tls,conch] >= 16.1.0', 'Nevow >= 0.11.1', 'pyOpenSSL >= 0.14', 'PyYAML >= 3.11', 'six >= 1.10.0']
sys.path after importing pkg_resources was
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/bin:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python27.zip:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/plat-darwin:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/plat-mac:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/plat-mac/lib-scriptpackages:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/lib-tk:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/lib-old:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/lib-dynload:
  /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7:
  /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-darwin:
  /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-tk:
  /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac:
  /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac/lib-scriptpackages:
  /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages:
  /Users/warner/stuff/tahoe/tahoe/src

I'm glad that the error was at least fairly straightforward ("could not understand requirement"), but again (#2749, #1872) I want to delete that code. We should not be duplicating the work of setuptools, especially when our function cannot do the job as well as setuptools.

(also see #2766, about removing other unnecessary dependencies) I'm playing with this on a branch.. it's a 3-line fix. ``` diff --git a/src/allmydata/_auto_deps.py b/src/allmydata/_auto_deps.py index bec052fa0..9eccf7f8e 100644 --- a/src/allmydata/_auto_deps.py +++ b/src/allmydata/_auto_deps.py @@ -42,11 +42,6 @@ install_requires = [ # * foolscap >= 0.12.6 has an i2p.sam_endpoint() that takes kwargs "foolscap >= 0.12.6", - # Needed for SFTP. - # pycrypto 2.2 doesn't work due to <https://bugs.launchpad.net/pycrypto/+bug/620253> - # pycrypto 2.4 doesn't work due to <https://bugs.launchpad.net/pycrypto/+bug/881130> - "pycrypto >= 2.1.0, != 2.2, != 2.4", - # pycryptopp-0.6.0 includes ed25519 "pycryptopp >= 0.6.0", @@ -73,7 +68,7 @@ install_requires = [ # * Twisted-16.1.0 fixes https://twistedmatrix.com/trac/ticket/8223, # which otherwise causes test_system to fail (DirtyReactorError, due to # leftover timers) - "Twisted[tls] >= 16.1.0", + "Twisted[tls,conch] >= 16.1.0", # We need Nevow >= 0.11.1 which can be installed using pip. "Nevow >= 0.11.1", @@ -107,7 +102,6 @@ package_imports = [ ('platform', None), ('pyOpenSSL', 'OpenSSL'), ('OpenSSL', None), - ('pycrypto', 'Crypto'), ('pyasn1', 'pyasn1'), ('service-identity', 'service_identity'), ('characteristic', 'characteristic'), ``` But switching to `twistedtls,conch` causes the `check_all_requirements()` in our _auto_deps.py to break, I think because it doesn't know how to parse the comma-separated list of extras: ``` py27 runtests: commands[1] | tahoe --version Traceback (most recent call last): File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/bin/tahoe", line 11, in <module> load_entry_point('tahoe-lafs', 'console_scripts', 'tahoe')() File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 560, in load_entry_point return get_distribution(dist).load_entry_point(group, name) File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2648, in load_entry_point return ep.load() File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2302, in load return self.resolve() File "/Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages/pkg_resources/__init__.py", line 2308, in resolve module = __import__(self.module_name, fromlist=['__name__'], level=0) File "/Users/warner/stuff/tahoe/tahoe/src/allmydata/__init__.py", line 452, in <module> check_all_requirements() File "/Users/warner/stuff/tahoe/tahoe/src/allmydata/__init__.py", line 450, in check_all_requirements raise PackagingError(get_error_string(fatal_errors + _cross_check_errors, debug=True)) allmydata.PackagingError: PackagingError: no version info or could not understand requirement 'Twisted[tls,conch] >= 16.1.0' For debugging purposes, the PYTHONPATH was None install_requires was ['setuptools >= 11.3', 'zfec >= 1.1.0', 'zope.interface >= 3.6.0, != 3.6.3, != 3.6.4', 'foolscap >= 0.12.6', 'pycryptopp >= 0.6.0', 'service-identity', 'characteristic >= 14.0.0', 'pyasn1 >= 0.1.8', 'pyasn1-modules >= 0.0.5', 'Twisted[tls,conch] >= 16.1.0', 'Nevow >= 0.11.1', 'pyOpenSSL >= 0.14', 'PyYAML >= 3.11', 'six >= 1.10.0'] sys.path after importing pkg_resources was /Users/warner/stuff/tahoe/tahoe/.tox/py27/bin: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python27.zip: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/plat-darwin: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/plat-mac: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/plat-mac/lib-scriptpackages: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/lib-tk: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/lib-old: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/lib-dynload: /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7: /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-darwin: /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/lib-tk: /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac: /usr/local/Cellar/python/2.7.13/Frameworks/Python.framework/Versions/2.7/lib/python2.7/plat-mac/lib-scriptpackages: /Users/warner/stuff/tahoe/tahoe/.tox/py27/lib/python2.7/site-packages: /Users/warner/stuff/tahoe/tahoe/src ``` I'm glad that the error was at least fairly straightforward ("could not understand requirement"), but again (#2749, #1872) I want to delete that code. We should not be duplicating the work of setuptools, especially when our function cannot do the job as well as setuptools.

exarkun commented

2018-07-19 18:47:43 +00:00

But switching to twistedtls,conch causes the check_all_requirements() in our _auto_deps.py to break, I think because it doesn't know how to parse the comma-separated list of extras:

This is possibly sad but maybe irrelevant because https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2740 means we should not actually declare a dependency on twistedconch, apparently.

Leaving out that part of the diff, I think this change is uncontroversial and I'm going to put it up for CI to play with.

> But switching to twistedtls,conch causes the check_all_requirements() in our _auto_deps.py to break, I think because it doesn't know how to parse the comma-separated list of extras: This is possibly sad but maybe irrelevant because <https://tahoe-lafs.org/trac/tahoe-lafs/ticket/2740> means we should not actually declare a dependency on `twistedconch`, apparently. Leaving out that part of the diff, I think this change is uncontroversial and I'm going to put it up for CI to play with.

exarkun commented

2018-07-19 18:59:43 +00:00

FWIW, Twisted 16.6.0 removes the gmpy dependency entirely. So bumping the Twisted minimum version to that would make it okay to declare twistedconch as a dependency. Then someone just has to deal with the problem warner encountered above - ideally, I would hope, by largely removing _auto_deps.py and relying on pip to figure out what the install requires. I have not yet internalized all of the purpose/functionality of _auto_deps.py so I'm not ready to do that myself.

FWIW, Twisted 16.6.0 removes the gmpy dependency entirely. So bumping the Twisted minimum version to that would make it okay to declare `twistedconch` as a dependency. Then someone just has to deal with the problem warner encountered above - ideally, I would hope, by largely removing `_auto_deps.py` and relying on pip to figure out what the install requires. I have not yet internalized all of the purpose/functionality of `_auto_deps.py` so I'm not ready to do that myself.

warner commented

2018-07-19 20:43:16 +00:00

Author

As y'all know, I'm inclined to delete _auto_deps.py and the entire contents of *init*.py, and move all the dependency data into setup.py. Back before setuptools became good, we had a lot of defensive code to double-check that all the dependencies we actually installed, and those checks needed to know the full list of dependencies (from "inside" the codebase), so they were stored in _auto_deps.py so they could be shared with "outside" the codebase (where setup.py could use them).

I think Daira was the last defender of this arrangement, and I don't know if she still has an opinion about it. I'd just delete it.

As y'all know, I'm inclined to delete `_auto_deps.py` and the entire contents of `*init*.py`, and move all the dependency data into `setup.py`. Back before setuptools became good, we had a lot of defensive code to double-check that all the dependencies we actually installed, and those checks needed to know the full list of dependencies (from "inside" the codebase), so they were stored in `_auto_deps.py` so they could be shared with "outside" the codebase (where `setup.py` could use them). I think Daira was the last defender of this arrangement, and I don't know if she still has an opinion about it. I'd just delete it.