search

matrix-org / sygnal

Sygnal: reference Push Gateway for Matrix
Apache License 2.0
155 stars 140 forks source link

Declare missing dependencies #290

Closed squahtx closed 2 years ago

squahtx commented 2 years ago

It's best to look at the commits separately. The first commit sorts the list of dependencies.

Fixes #289

squahtx commented 2 years ago

The minimum package versions for attrs and typing-extensions are copied from Synapse: https://github.com/matrix-org/synapse/blob/eb39da6782b57c939450839097f32a14cba3ebfc/synapse/python_dependencies.py#L81-L85

The minimum package version for pyopenssl is copied from aioapns: https://github.com/Fatal1ty/aioapns/blob/master/setup.py#L30

DMRobertson commented 2 years ago

Another thought: why did CI miss this? I guess there's no equivalent of the olddeps job that Synapse has?

callahad commented 2 years ago

Another thought: why did CI miss this? I guess there's no equivalent of the olddeps job that Synapse has?

I think we're not testing against older python versions (e.g., I think setting up a matrix of ['3.7', '3.x'] would've found it)

reivilibre commented 2 years ago

Another thought: why did CI miss this? I guess there's no equivalent of the olddeps job that Synapse has?

Possibly because it was brought in by a dev dependency (e.g. mypy) and was present during the tests. :/

reivilibre commented 2 years ago

Another thought: why did CI miss this? I guess there's no equivalent of the olddeps job that Synapse has?

I think we're not testing against older python versions (e.g., I think setting up a matrix of ['3.7', '3.x'] would've found it)

3.7, the one we test with, is the oldest version we support iirc.

squahtx commented 2 years ago

I'm happy if CI is happy. Sanity check: would this have fixed the earlier deployment to matrix.org?

It ought to, assuming we do a pip install as part of the deployment. I have no idea how we'd test this apart from doing another deployment.

DMRobertson commented 2 years ago

I have no idea how we'd test this apart from doing another deployment.

Maybe a pip install -e .? But yeah, probably best to try it and see.

squahtx commented 2 years ago

Maybe a pip install -e .? But yeah, probably best to try it and see.

A pip install -r <(./scripts-dev/old_deps.py) seems to be okay.

On another note, mypy really does not like conditional imports. Getting the right combination of type: ignores is far from simple.

Python 3.7:

sygnal/__init__.py:17: error: Module 'importlib.metadata' has no attribute 'PackageNotFoundError'  [attr-defined]
sygnal/__init__.py:17: error: Module 'importlib.metadata' has no attribute 'version'  [attr-defined]

Python 3.8, importlib_metadata installed:

sygnal/__init__.py:22: error: Incompatible import of "PackageNotFoundError" (imported name has type "Type[importlib_metadata.PackageNotFoundError]", local name has type "Type[importlib.metadata.PackageNotFoundError]")  [misc]

Python 3.8, importlib_metadata not installed:

sygnal/__init__.py:22: error: Name 'PackageNotFoundError' already defined (possibly by an import)  [no-redef]
sygnal/__init__.py:22: error: Name 'version' already defined (possibly by an import)  [no-redef]
DMRobertson commented 2 years ago

On another note, mypy really does not like conditional imports. Getting the right combination of type: ignores is far from simple.

Ouch, thanks for working through that. Is there any way that we could use TYPE_CHECKING to help? I guess not, since it's sensitive to the version of the stdlib that's running mypy(?)

squahtx commented 2 years ago

Ouch, thanks for working through that. Is there any way that we could use TYPE_CHECKING to help? I guess not, since it's sensitive to the version of the stdlib that's running mypy(?)

I can't see how, unless we unconditionally use importlib_metadata when type checking.

and you've made me realise that the dependencies are wrong. typing_extensions is only installed on Python <3.8, but we use it in type checking mode regardless.

squahtx commented 2 years ago

mypy brings in typing-extensions for us, but we should be explicit.

DMRobertson commented 2 years ago

I'm happy if CI is happy. Sanity check: would this have fixed the earlier deployment to matrix.org?

It ought to, assuming we do a pip install as part of the deployment. I have no idea how we'd test this apart from doing another deployment.

Ahhh, I was more getting at "why did the deployment fail?" so we could check this fixes it. The cause was an inability to import typing_extensions, but that's now been made a hard requirement.

Edit: a hard requirement on sufficiently old Pythons.

So I think this is hopefully good to go! :+1: