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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add six to install_requires #1985

Closed
wants to merge 1 commit into from
Closed

Conversation

sondrelg
Copy link
Contributor

Hi 馃憢

six is imported and used, in Paramiko, here.

Unfortunately, since it's not listed as a dependency, importing paramiko seems to fail:

Feb 10 10:53:57 info   File "/app/....py", line 6, in <module>
Feb 10 10:53:57 info     import paramiko
Feb 10 10:53:57 info   File "/usr/local/lib/python3.10/site-packages/paramiko/__init__.py", line 22, in <module>
Feb 10 10:53:57 info     from paramiko.transport import SecurityOptions, Transport
Feb 10 10:53:57 info   File "/usr/local/lib/python3.10/site-packages/paramiko/transport.py", line 91, in <module>
Feb 10 10:53:57 info     from paramiko.dsskey import DSSKey
Feb 10 10:53:57 info   File "/usr/local/lib/python3.10/site-packages/paramiko/dsskey.py", line 37, in <module>
Feb 10 10:53:57 info     from paramiko.pkey import PKey
Feb 10 10:53:57 info   File "/usr/local/lib/python3.10/site-packages/paramiko/pkey.py", line 30, in <module>
Feb 10 10:53:57 info     import six
Feb 10 10:53:57 info ModuleNotFoundError: No module named 'six'

This PR adds it to setup.py's install_requires. Currently haven't specified a version range.

@bitprophet
Copy link
Member

I'm actually loathe to do this as we're about to drop six (and Python 2 ... ok, in the opposite order) so having a release to change official deps twice in a row seems a bit silly! But I appreciate the PR.

What's odd is this is the first time I've seen this come up. Another contributor guesses maybe we got six via a transitive dependency which has since dropped it 馃 (which I'd guess is a Python 2 drop on that project, which would also break things for any Paramiko folks still on Py2... @bskinn have you seen any reports of that flavor of the problem lately? Maybe it's moot if they're using the interpreter requirements in their metadata correctly...)

Can you provide a bit more info about how you got here? Had you installed Paramiko successfully prior (& which version/s of it), what dependency versions did that use, what versions did you get this time, etc? Thanks!

@bskinn
Copy link
Contributor

bskinn commented Feb 11, 2022

No, I haven't seen anything anywhere about issues with a six ImportError, though I don't touch the Python 2 sphere of the ecosystem very much.

It's very odd, because any broader cause I can think of, I would think would be a much noisier matter for the paramiko bugtracker than it has been.

@sondrelg
Copy link
Contributor Author

What's odd is this is the first time I've seen this come up. Another contributor guesses maybe we got six via a transitive dependency which has since dropped it

This seems like the most likely explanation to me. We use poetry and auto-update dependencies within our specified major ranges daily, assuming tests pass, so unfortunately I'm not sure I'll be able to trace the change very easily.

I agree dropping six/v2 support altogether sounds reasonable here assuming that's days away, but it sound like it might be worth a patch bump otherwise? I doubt I'll be the last to see this issue 馃檪 Just out of curiosity, is the release process very involved?

@sondrelg
Copy link
Contributor Author

sondrelg commented Feb 11, 2022

I think it's a 3.10 issue.

  • Running python -m venv .venv,
  • activating it
  • running pip install paramiko
  • and importing paramiko

doesn't replicate the issue on my home computer (3.9.7), since bcrypt is a dependency of paramiko and six is a dependency of bcrypt.

That being said, bcrypt no longer lists six in their setup.py. See pyca/bcrypt#225.

@sondrelg
Copy link
Contributor Author

We have bcrypt = ">=3.1.3" in our lockfile, which explains it.

@bskinn
Copy link
Contributor

bskinn commented Feb 11, 2022

Thanks for diagnosing, @sondrelg! Not sure how this affects your thought process, @bitprophet.

@bitprophet
Copy link
Member

The release process itself isn't that involved (though I admit I'm still way too conservative about it 馃槀 ) but I have an allergy to dependency updates - even innocuous-looking ones end up causing weird corner case issues in far-off areas of the ecosystem.

That said...this one should really be a noop for anyone with older bcrypt, and would fix this issue on newer bcrypt, so what the heck.

bitprophet added a commit that referenced this pull request Feb 25, 2022
@bitprophet
Copy link
Member

bitprophet commented Feb 25, 2022

Cherry-picked this back to 2.8 and merged up. Thanks! I intend to pop out bugfix releases later today or maybe over the weekend. (sobbing in oncall shift)

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