Skip to content

Conversation

rominf
Copy link
Contributor

@rominf rominf commented Apr 6, 2025

What do these changes do?

Use trustme for test TLS certificate and key

With the current approach TLS certificate and key are hardcoded in separate files and distributed as part of wheels. This is unnecessary for end users of the package, also this brings difficulties while supporting Python 3.13, see: #473

Use trustme for generating TLS certificate and key on the fly, as it is done in other aio-libs packages.

Initially proposed in: #473 (comment)

Are there changes in behavior for the user?

Wheels no longer include test TLS certificate and key.

Related issue number

Checklist

  • I think the code is well written
  • Unit tests for the changes exist
  • tox testenvs have been executed in the following environments:
  • Documentation reflects the changes [not worthy for documentation]
  • Add a news fragment into the NEWS.rst file [not worthy for NEWS.rst]
    • Add under the "aiosmtpd-next" section, creating one if necessary
      • You may create subsections to group the changes, if you like
    • Use full sentences with correct case and punctuation
    • Refer to relevant Issue if applicable

@rominf rominf mentioned this pull request Apr 6, 2025
6 tasks
@webknjaz
Copy link
Member

webknjaz commented Apr 6, 2025

It's unclear to me what the pytest-asyncio plug-in has to do with TLS certificates or why it needs to be removed. Either way, it's best to avoid mixing up unrelated changes.

@rominf
Copy link
Contributor Author

rominf commented Apr 8, 2025

@webknjaz Unfortunately, tox tests did not pass on my machine, so I had to make this change. However, I understand your point about unrelated changes, so I split this PR into two. See: #554.

@rominf rominf force-pushed the rominf-trustme branch 2 times, most recently from 9c2a07d to 23af110 Compare April 13, 2025 06:42
@rominf
Copy link
Contributor Author

rominf commented Apr 13, 2025

I tried to add workaround for installing trustme: see #552 (comment) and https://github.com/aio-libs/aiosmtpd/pull/552/files#diff-4d7c51b1efe9043e44439a949dfd92e5827321b34082903477fd04876edb7552R8-R9, however it didn't work because of PyPy trying to build cryptography:

error: the configured PyPy interpreter version (3.8) is lower than PyO3's minimum supported version (3.9)

Hence, #555 should be reviewed and merged first.

@rominf rominf marked this pull request as draft April 13, 2025 06:51
@webknjaz
Copy link
Member

I'm pretty sure cryptography has older published wheels that would be compatible.

Copy link

codecov bot commented Apr 14, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 97.82%. Comparing base (d087090) to head (039d938).
⚠️ Report is 2 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #552      +/-   ##
==========================================
+ Coverage   97.78%   97.82%   +0.03%     
==========================================
  Files          23       23              
  Lines        5697     5709      +12     
  Branches      764      770       +6     
==========================================
+ Hits         5571     5585      +14     
+ Misses         80       78       -2     
  Partials       46       46              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@rominf rominf marked this pull request as ready for review April 14, 2025 06:20
@rominf
Copy link
Contributor Author

rominf commented Apr 14, 2025

I'm pretty sure cryptography has older published wheels that would be compatible.

You are right, cryptography==41.0.7 has compatible wheels published. I updated my PR. However, this version has a few security issues: https://security.snyk.io/package/pip/cryptography/41.0.7, so dependabot will likely create PRs.

@rominf rominf requested a review from webknjaz April 14, 2025 06:24
@webknjaz
Copy link
Member

It's possible to use env markers and only install the older version under PyPy but not others.

With the current approach TLS certificate and key are hardcoded in
separate files and distributed as part of wheels.
This is unnecessary for end users of the package, also this brings
difficulties while supporting Python 3.13, see:
aio-libs#473

Use trustme for generating TLS certificate and key on the fly, as it is
done in other aio-libs packages.

Initially proposed in:
aio-libs#473 (comment)
@rominf
Copy link
Contributor Author

rominf commented Apr 14, 2025

I had to read PEP 496, I didn't know about platform_python_implementation marker. Thanks! Done.

@webknjaz
Copy link
Member

It was rejected and superseded by PEP 508.

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