torproject / stem

Python controller library for Tor
https://stem.torproject.org/
GNU Lesser General Public License v3.0
257 stars 75 forks source link

Broken on Python 3.10 #109

Closed cculianu closed 2 years ago

cculianu commented 2 years ago

Python 3.10 took out collections.Iterable (now lives in collections.abc.Iterable), yet control.py in this package refers to that name.

So control.py fails to import, and thus the whole package fails on Python 3.10.

See: https://docs.python.org/3.9/library/collections.html

Deprecated since version 3.3, will be removed in version 3.10: Moved Collections Abstract Base Classes to the collections.abc module. For backwards compatibility, they continue to be visible in this module through Python 3.9.

atagar commented 2 years ago

Thanks Calin. I'm disappointed Python moved away from symantic versioning but c'est la vie. If someone would care to provide a patch I'd be happy to merge it.

cculianu commented 2 years ago

Indeed. Such breakage should be at a major version, not a minor one....

Sure I will submit a PR later today when I get home. It’s a 1-line change in control.py :)

atagar commented 2 years ago

It’s a 1-line change in control.py

That depends. Did the whole collections module move to collections.abc? If so there's twelve files involved...

% grep -R 'import collections' stem
stem/client/datatype.py:import collections
stem/descriptor/__init__.py:import collections
stem/descriptor/bandwidth_file.py:import collections
stem/descriptor/hidden_service.py:import collections
stem/descriptor/networkstatus.py:import collections
stem/directory.py:import collections
stem/util/test_tools.py:import collections
stem/util/conf.py:import collections
stem/util/system.py:import collections
stem/util/connection.py:import collections
stem/control.py:import collections
stem/manual.py:import collections
cculianu commented 2 years ago

Yea good point ; I’ll review the code carefully in that case.

I think just the abstract base classes moved to abc, and not all of it.

But yes I will review the code. Thanks for tip.

cculianu commented 2 years ago

The list of names that were moved to abc:

AsyncGenerator
AsyncIterable
AsyncIterator
Awaitable
ByteString
Callable
Collection
Container
Coroutine
Generator
Hashable
ItemsView
Iterable
Iterator
KeysView
Mapping
MappingView
MutableMapping
MutableSequence
MutableSet
Reversible
Sequence
Set
Sized
ValuesView

The list of names from above that are being referred to as collections.<name>:

$ grep -Er '(collections.AsyncGenerator)|(collections.AsyncIterable)|(collections.AsyncIterator)|(collections.Awaitable)|(collections.ByteString)|(collections.Callable)|(collections.Collection)|(collections.Container)|(collections.Coroutine)|(collections.Generator)|(collections.Hashable)|(collections.ItemsView)|(collections.Iterable)|(collections.Iterator)|(collections.KeysView)|(collections.Mapping)|(collections.MappingView)|(collections.MutableMapping)|(collections.MutableSequence)|(collections.MutableSet)|(collections.Reversible)|(collections.Sequence)|(collections.Set)|(collections.Sized)|(collections.ValuesView)' stem
stem/control.py:      elif isinstance(value, collections.Iterable):

So yes.. I think it very well is a 1-line change. Will submit PR now. :)

Question: The new names appear in abc since 3.3. stem doesn't happen to support python before 3.3... does it?

atagar commented 2 years ago

So yes.. I think it very well is a 1-line change. Will submit PR now. :)

Great! Thanks. :)

Question: The new names appear in abc since 3.3. stem doesn't happen to support python before 3.3... does it?

Nope, Stem's master branch supports 3.6+.

https://gitweb.torproject.org/stem.git/commit/?id=a396f57c

cculianu commented 2 years ago

Ok, PR opened: #110.

cculianu commented 2 years ago

Nope, Stem's master branch supports 3.6+.

https://gitweb.torproject.org/stem.git/commit/?id=a396f57c

Oh. LOL. I thought you supported older pythons.. your tox.ini needs updating then. :)

cculianu commented 2 years ago

Wait -- if it's 3.6+ only then I can simplify my PR.. hmm.

atagar commented 2 years ago

Wait -- if it's 3.6+ only then I can simplify my PR.. hmm.

Yup. Rather than doing a version check lets simply swap over to collections.abc. Please use the full name (rather than a 'from x' import).

cculianu commented 2 years ago

Wait -- if it's 3.6+ only then I can simplify my PR.. hmm.

Yup. Rather than doing a version check lets simply swap over to collections.abc. Please use the full name (rather than a 'from x' import).

Agreed. Already done.

atagar commented 2 years ago

Thanks! Merged.