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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Commit 61df0e2f5e90 introduced a requirement for asgiref 3.6.0+ #1255

Closed
vainu-arto opened this issue Sep 15, 2023 · 4 comments · Fixed by #1261
Closed

Commit 61df0e2f5e90 introduced a requirement for asgiref 3.6.0+ #1255

vainu-arto opened this issue Sep 15, 2023 · 4 comments · Fixed by #1261

Comments

@vainu-arto
Copy link

In commit 61df0e2 the middleware started using function iscoroutinefunction from asgiref.sync. This function is introduced in version 3.6.0.

It seems that this commit also introduced a hard dependency on asgiref which previously didn't exist (the use in models.py is conditional).

The easy fix is to add a versioned requirement for asgiref.

@ddabble
Copy link
Member

ddabble commented Sep 15, 2023

As far as I've understood, we only use asgiref because it's what Django itself uses and recommends using for async middleware, and so we will only ever have the need to use the exact same version as Django requires. In other words, if Django changed its required asgiref version or stopped using it altogether, we would most likely (have to) follow suit.

Because of this, I'm not entirely sure why not adding asgiref to our requirements could be a problem; would you mind explaining? 🙂

@vainu-arto
Copy link
Author

Not all supported versions of Django require asgiref 3.6.0. As an obvious example, Django 3.2 requires asgiref 3.3.2: https://github.com/django/django/blob/3.2.21/setup.cfg#L45

The problem could also be solved by directly requiring Django 4.2, but that doesn't sound right to me.

ddabble added a commit that referenced this issue Sep 25, 2023
See #1255
for discussion.

Link to Django 4.1's required `asgiref` version:
https://github.com/django/django/blob/4.1.11/setup.cfg#L42
Link to Django 4.2's required `asgiref` version:
https://github.com/django/django/blob/4.2/setup.cfg#L42
@ddabble
Copy link
Member

ddabble commented Sep 25, 2023

Ah, I missed your point above that iscoroutinefunction() was introduced in asgiref 3.6.0.

I can see from the CI test logs for the commit you mentioned that asgiref==3.7.2 was actually installed for all Django versions (which I'm guessing is because that version is cached, which pip seems to prefer), which explains why the tests didn't fail.

@ddabble
Copy link
Member

ddabble commented Sep 25, 2023

I opened #1261 to implement the versioned requirement you mentioned initially.

ddabble added a commit that referenced this issue Sep 26, 2023
See #1255
for discussion.

Link to Django 4.1's required `asgiref` version:
https://github.com/django/django/blob/4.1.11/setup.cfg#L42
Link to Django 4.2's required `asgiref` version:
https://github.com/django/django/blob/4.2/setup.cfg#L42
amstilp added a commit to UW-GAC/gregor-django that referenced this issue Nov 16, 2023
django-simple-history introduced a dependency on asgiref>=3.6 but
didn't include it in their requirements. Add this dependency at
least until we upgrade to Django 4.2. See issue:
jazzband/django-simple-history#1255
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 a pull request may close this issue.

2 participants