Declare missing dependencies #290

squahtx · 2021-12-14T13:51:41Z

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

Fixes #289

squahtx · 2021-12-14T14:02:22Z

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

Thanks for this!

DMRobertson · 2021-12-14T14:12:50Z

setup.py

-        "mypy==0.812",
        "mypy-zope==0.3.0",
-        "tox",
+        "mypy==0.812",


I'd put mypy-zope after mypy. But perhaps mypy- comes before mypy=?

Fine as it is, just an aside.

Good spot, I'll fix it.

DMRobertson · 2021-12-14T14:14:06Z

setup.py

@@ -23,6 +23,7 @@

 INSTALL_REQUIRES = [
    "aioapns>=1.10",
+    "attrs>=19.2.0",
    "cryptography>=2.6.1",
    "idna>=2.8",
    "importlib_metadata",


Do we want/have a lower bound here?

Similarly to comment on typing-extensions, should we declare this as importlib_metadata; python_version < "3.8" (assuming we don't need the new importlib.metadata bits from 3.10)

Perhaps. I have no idea what the lower bound should be

For reference, this is what we do in python-signedjson: matrix-org/python-signedjson#9

DMRobertson · 2021-12-14T14:14:39Z

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

callahad · 2021-12-14T14:20:43Z

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)

callahad · 2021-12-14T14:21:11Z

setup.py

    "sentry-sdk>=0.10.2",
+    "service_identity>=18.1.0",
+    "Twisted>=19.7",
+    "typing-extensions>=3.7.4",


Should we make this conditional so it's only installed on Py < 3.8? (And do the same for the import that needs it)

I think we typically import directly from typing_extensions; would have to change the source to do a try...except ImportError... dance

On the import side, mypy does not appreciate the obvious attempt:
https://mypy-play.net/?mypy=latest&python=3.10&gist=9ac99f23480e5c8bc86c68f94ce65b6c

if TYPE_CHECKING or sys.version_info < (3, 8, 0): from typing_extensions import Literal else: from typing import Literal

works with mypy but always requires typing_extensions in dev.

There's a PR to do the same for python-signedjson here: matrix-org/python-signedjson#12

reivilibre · 2021-12-14T14:36:27Z

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 · 2021-12-14T14:37:22Z

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.

DMRobertson

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

squahtx · 2021-12-14T16:10:06Z

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 · 2021-12-14T16:13:29Z

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 · 2021-12-14T16:25:08Z

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 · 2021-12-14T16:43:12Z

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 · 2021-12-14T16:45:49Z

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 · 2021-12-14T16:55:06Z

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

DMRobertson · 2021-12-14T17:03:34Z

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! 👍

Sort dependencies

2dfe97b

squahtx requested a review from a team as a code owner December 14, 2021 13:51

squahtx force-pushed the squah/declare_dependencies branch from 97df4d8 to 90e7cac Compare December 14, 2021 13:52

Add missing dependencies to setup.py

31404de

squahtx force-pushed the squah/declare_dependencies branch from 90e7cac to 31404de Compare December 14, 2021 13:58

DMRobertson reviewed Dec 14, 2021

View reviewed changes

fix bad sort

3c496ab

callahad reviewed Dec 14, 2021

View reviewed changes

Sean Quah added 3 commits December 14, 2021 15:55

Don't require importlib_metadata on Python 3.8+

cbfa2a3

Don't require typing-extensions on Python 3.8+

7a12e82

Fix CI

94e3e01

DMRobertson previously approved these changes Dec 14, 2021

View reviewed changes

Fix mypy errors

148f904

squahtx dismissed DMRobertson’s stale review via 148f904 December 14, 2021 16:18

squahtx requested a review from DMRobertson December 14, 2021 16:25

Always require typing-extensions in dev mode

3202ef5

DMRobertson approved these changes Dec 14, 2021

View reviewed changes

squahtx merged commit d85aa6c into main Dec 14, 2021

squahtx deleted the squah/declare_dependencies branch December 14, 2021 17:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Declare missing dependencies #290

Declare missing dependencies #290

squahtx commented Dec 14, 2021 •

edited

Loading

squahtx commented Dec 14, 2021

DMRobertson left a comment

DMRobertson Dec 14, 2021

squahtx Dec 14, 2021

DMRobertson Dec 14, 2021

callahad Dec 14, 2021

squahtx Dec 14, 2021

squahtx Dec 14, 2021

DMRobertson commented Dec 14, 2021

callahad commented Dec 14, 2021

callahad Dec 14, 2021

DMRobertson Dec 14, 2021

squahtx Dec 14, 2021

squahtx Dec 14, 2021

reivilibre commented Dec 14, 2021

reivilibre commented Dec 14, 2021 •

edited

Loading

DMRobertson left a comment

squahtx commented Dec 14, 2021

DMRobertson commented Dec 14, 2021

squahtx commented Dec 14, 2021

DMRobertson commented Dec 14, 2021

squahtx commented Dec 14, 2021

squahtx commented Dec 14, 2021

DMRobertson commented Dec 14, 2021 •

edited

Loading

Declare missing dependencies #290

Declare missing dependencies #290

Conversation

squahtx commented Dec 14, 2021 • edited Loading

squahtx commented Dec 14, 2021

DMRobertson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DMRobertson commented Dec 14, 2021

callahad commented Dec 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

reivilibre commented Dec 14, 2021

reivilibre commented Dec 14, 2021 • edited Loading

DMRobertson left a comment

Choose a reason for hiding this comment

squahtx commented Dec 14, 2021

DMRobertson commented Dec 14, 2021

squahtx commented Dec 14, 2021

DMRobertson commented Dec 14, 2021

squahtx commented Dec 14, 2021

squahtx commented Dec 14, 2021

DMRobertson commented Dec 14, 2021 • edited Loading

squahtx commented Dec 14, 2021 •

edited

Loading

reivilibre commented Dec 14, 2021 •

edited

Loading

DMRobertson commented Dec 14, 2021 •

edited

Loading