Opened at 2017-03-29T21:38:47Z
Closed at 2018-07-26T15:10:47Z
#2873 closed defect (fixed)
remove obsolete dependency on "pycrypto"
Reported by: | warner | Owned by: | daira |
---|---|---|---|
Priority: | normal | Milestone: | 1.13.0 |
Component: | packaging | Version: | 1.12.1 |
Keywords: | Cc: | ||
Launchpad Bug: |
Description
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 twisted[conch] (probably on twisted[tls,conch] if we need both).
Change History (5)
comment:1 Changed at 2017-04-12T02:13:40Z by warner
comment:2 Changed at 2018-07-19T18:47:43Z by exarkun
But switching to twisted[tls,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 twisted[conch], 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.
comment:3 Changed at 2018-07-19T18:59:43Z by exarkun
FWIW, Twisted 16.6.0 removes the gmpy dependency entirely. So bumping the Twisted minimum version to that would make it okay to declare twisted[conch] 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.
comment:4 Changed at 2018-07-19T20:43:16Z by warner
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.
comment:5 Changed at 2018-07-26T15:10:47Z by exarkun
- Resolution set to fixed
- Status changed from new to closed
(also see #2766, about removing other unnecessary dependencies)
I'm playing with this on a branch.. it's a 3-line fix.
But switching to twisted[tls,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:
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.