On Python 3.10, must use a sufficiently new frozendict version

DMRobertson commented 2 years ago

andrewsh noticed the following

FYI https://buildd.debian.org/status/fetch.php?pkg=matrix-synapse&arch=all&ver=1.47.0-1&stamp=1637184874&raw=0

I: pybuild base:237: python3.10 setup.py clean 
Traceback (most recent call last):
  File "/<<PKGBUILDDIR>>/setup.py", line 84, in <module>
    version = exec_file(("synapse", "__init__.py"))["__version__"]
  File "/<<PKGBUILDDIR>>/setup.py", line 80, in exec_file
    exec(code, result)
  File "<string>", line 44, in <module>
  File "/usr/lib/python3/dist-packages/canonicaljson.py", line 20, in <module>
    from frozendict import frozendict
  File "/usr/lib/python3/dist-packages/frozendict/__init__.py", line 16, in <module>
    class frozendict(collections.Mapping):
AttributeError: module 'collections' has no attribute 'Mapping'
E: pybuild pybuild:354: clean: plugin distutils failed with: exit code=1: python3.10 setup.py clean

The above from frozendict 1.2.

This was fine in 3.9, but failed in 3.10. The Python 3.9 docs for the collection module says:

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.

We should support Python 3.10. I'd suggest we ought to raise the required version of frozendict. I don't know if there's a way to make that conditional on 3.10? We should start by finding out the first version of frozendict that is supported by Python 3.10; I'd guess we want the first version where frozendict inherits from dict.

(Though come to think of it, it's odd that a frozendict inherits from dict, because it has a strictly smaller feature set (no-mutability).)

DMRobertson commented 2 years ago

I think this is particularly confusing because we don't actually require frozendict to work. We just support it if it happens to be available.

I think we should try to import frozendict at import time

frozendict_type: Optional[Type]
try:
    from frozendict import frozendict as frozendict_type
except ImportError:
    frozendict_type = None

and then only try to support frozendict if it's installed, changing line 26 here

https://github.com/matrix-org/python-canonicaljson/blob/e5d6d75ce03a60cb0ffa821b9914362bd0d47207/canonicaljson.py#L25-L38

to

    if frozendict_type is not None type(obj) is frozendict_type:

maybe with a fallback that returns dict(obj) if the above fails but isinstance(obj, collections.abc.Mapping)?

BURG3R5 commented 2 years ago

Hi, I'd like to work on this issue. Can you assign me?

richvdh commented 2 years ago

matrix-org / python-canonicaljson

On Python 3.10, must use a sufficiently new frozendict version #45