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

Make aiosmtpd dependency optional #40

Merged
merged 5 commits into from
Jun 2, 2022
Merged

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented May 21, 2022

Move the aiosmtpd dependency to "smtp" extra, and handle the lack of it
gracefully in tests. The aiosmtpd package is no longer maintained
and had major bugs that lead to it being queued for removal from Gentoo.
FWICS all packages in Gentoo that are using pytest-localserver are only
using the HTTP support.

Move the aiosmtpd dependency to "smtp" extra, and handle the lack of it
gracefully in tests.  The aiosmtpd package is no longer maintained
and had major bugs that lead to it being queued for removal from Gentoo.
FWICS all packages in Gentoo that are using pytest-localserver are only
using the HTTP support.
@mgorny
Copy link
Contributor Author

mgorny commented May 21, 2022

BTW I've seen you considering removing it entirely in aio-libs/aiosmtpd#144 — I don't mind either way.

@diazona
Copy link
Contributor

diazona commented May 28, 2022

Sorry I didn't reply earlier, I was away with limited internet access. Anyway, thanks for the change! I was also considering doing something like this when I switched to aiosmtpd, but I guess I decided against it because I figured it would be less disruptive to people's workflows not to have to specify an extra to get the same functionality that was there before. Or something. I don't quite remember now, and in retrospect it probably doesn't make all that much sense 😆

Anyway, bottom line, I am on board with this. 👍 I suppose we might as well keep the smtp functionality as an extra since people won't be getting it by default, so it doesn't hurt to just have it there for now. It would be nice to change tox.ini to run tests both with and without the smtp extra, but if you don't want to add that now, I can do it in another PR later.

Is there any urgent need on Gentoo's behalf to get out a new version that includes this PR? I see that you patched the ebuild to remove the aiosmtpd dependency so I'm guessing this can wait for a while, e.g. for the 0.7 release, but I can make another release sooner if necessary.

mgorny added 3 commits May 29, 2022 07:52
In this package, the GH actions always run a consistent set of tox
environments only.  Using tox-gh-actions has no real advantage over
running a dedicated tox env (i.e. plain `py`) and requires duplicating
all the environment declarations.
@mgorny
Copy link
Contributor Author

mgorny commented May 29, 2022

Anyway, bottom line, I am on board with this. +1 I suppose we might as well keep the smtp functionality as an extra since people won't be getting it by default, so it doesn't hurt to just have it there for now. It would be nice to change tox.ini to run tests both with and without the smtp extra, but if you don't want to add that now, I can do it in another PR later.

Done that. I've permitted myself to update the obsolete py.test names with pytest too, and to remove tox-gh-actions since it only made all the config a few times longer. Now both tox and GHA cover up to py3.11 + pypy3, and GHA runs py and py-smtp tox envs which correspond to the selected interpreter.

Is there any urgent need on Gentoo's behalf to get out a new version that includes this PR? I see that you patched the ebuild to remove the aiosmtpd dependency so I'm guessing this can wait for a while, e.g. for the 0.7 release, but I can make another release sooner if necessary.

No. no urgent need. Thanks!

Copy link
Contributor

@diazona diazona left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Just a couple suggestions, and then I do want to wait a few more days in case any of the other maintainers have comments; otherwise I'll merge this later this week.

setup.py Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
@@ -23,6 +25,6 @@ jobs:
- name: Install dependencies
run: |
python -m pip install --upgrade pip
pip install tox tox-gh-actions
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me ping @redtoad in case he has any comments, since he's the one who originally added the Github Actions config.

@diazona diazona merged commit 5950014 into pytest-dev:master Jun 2, 2022
@diazona diazona added this to the v0.7.0 milestone Jul 8, 2022
@diazona
Copy link
Contributor

diazona commented Sep 13, 2022

BTW @mgorny I just noticed the ebuild for pytest-localserver-0.7.0 still uses the patch in src_prepare, which shouldn't be needed anymore, I think? I'd be willing to submit a request to change the ebuild through the proper channels if that would be best, but I'm not set up to do so easily.

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 this pull request may close these issues.

2 participants