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

Python 3.12 asyncore to asyncio #520

Merged
merged 3 commits into from
Jun 24, 2024

Conversation

FosanzDev
Copy link
Contributor

@FosanzDev FosanzDev commented Jun 18, 2024

Since asyncore is not available in python 3.12+, an alternative is added using asyncio.

The asyncore alternative will only be imported if available, so both options are available for Python 3.11 (or less) users.


This change is Reviewable

@tomato42
Copy link
Member

pep8 errors in codeclimate are relevant

@tomato42 tomato42 added the enhancement new feature to be implemented label Jun 18, 2024
@tomato42 tomato42 added this to the v0.8.0 milestone Jun 18, 2024
@tomato42
Copy link
Member

Codeclimate marks (wrongly) similarities in two completely different methods.

yes, it's a bit too sensitive, don't worry about this, won't block merge based on that

that being said, the failures on python 2.7 are relevant too (possibly other versions), please address those too

@FosanzDev
Copy link
Contributor Author

that being said, the failures on python 2.7 are relevant too (possibly other versions), please address those too

On it tomorrow!

@tomato42
Copy link
Member

tomato42 commented Jun 19, 2024

unfortunately removing the accent only in the last patch will not work for the bisection tests... :( you'll need to fixup the previous patch with this change

@FosanzDev
Copy link
Contributor Author

Best way to do this?
New branch from my branch, doing a pr and merging in my repo will work with this?
Anyways I have to add try-except block since asyncio is not in python2 :/

@tomato42
Copy link
Member

feel free to rebase the patches and force push to this branch; that's why we use Reviewable, so that rebased patches are still easy to review

@FosanzDev FosanzDev force-pushed the asyncore-to-asyncio branch 3 times, most recently from 7992395 to 25f732e Compare June 19, 2024 11:10
@FosanzDev
Copy link
Contributor Author

Force pushing interactive rebases 3 times is wild, but code and commits are definitely cleaner!

tomato42
tomato42 previously approved these changes Jun 19, 2024
Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

I won't call it wild until we go into double digits :)

overall looks good, but we need the CI passing before I'll merge it

Reviewed 3 of 3 files at r7, 3 of 3 files at r8, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @FosanzDev)

@FosanzDev
Copy link
Contributor Author

I didn't realize that ImportError was risen by Python 2 CI runs. Anyways, it is solved now. CI on Python 3.8 failed due to some error on coveralls.
Hearing back from you.

@tomato42
Copy link
Member

Sorry, still see issues with the asyncio import on python 2.7...

Since python 3.12 does no longer support asyncore, a asyncio alternative is presented.

This class does mix in with asyncio.Protocol.

api.py imports TLSAsyncioDispatcherMixIn always and only imports TLSAsyncDispatcherMixIn if asyncore is available.

Changed ModuleNotFoundError to ImportError superclass for CI compatibility on Python 2 and 3.
@FosanzDev
Copy link
Contributor Author

My bad. Completely forgot to force-push the actual changes.

@tomato42
Copy link
Member

looks like one more test case needs changes

@FosanzDev
Copy link
Contributor Author

Yeah. Now the errors are in the actual unit test I made for my module. Since it is not available in Python 2, how should I solve this? May I remove the unit test?.

@tomato42
Copy link
Member

skip it for the old python versions, there are examples of doing that in other places; just make a comment to make it clear why it's disabled on older pythons

@FosanzDev
Copy link
Contributor Author

Thanks for the help. I'm not used to unittest.

@FosanzDev
Copy link
Contributor Author

Ran tests with Python 2 and 3. Now it should work fine. Sorry for the inconvenience!
I did it with trial and error really hehe.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 3 files at r14, 1 of 4 files at r18, 3 of 3 files at r19, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @FosanzDev)


unit_tests/test_tlslite_integration_tlsasynciodispatchermixin.py line 14 at r19 (raw file):

import sys
# No unittest2 features are used in this file

actually there are: skipIf :) that's why py2.6 test run failed
please use

try:
   import unittest2 as unittest
except ImportError:
   import unittest

Codeclimate marks (wrongly) similarities in two completely different methods.
Copy link
Contributor Author

@FosanzDev FosanzDev left a comment

Choose a reason for hiding this comment

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

Reviewable status: 2 of 6 files reviewed, 1 unresolved discussion (waiting on @tomato42)


unit_tests/test_tlslite_integration_tlsasynciodispatchermixin.py line 14 at r19 (raw file):

Previously, tomato42 (Hubert Kario) wrote…

actually there are: skipIf :) that's why py2.6 test run failed
please use

try:
   import unittest2 as unittest
except ImportError:
   import unittest

Done.

Copy link
Member

@tomato42 tomato42 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 4 files at r20, 3 of 3 files at r21, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @FosanzDev)

@tomato42 tomato42 merged commit 4d2c6b8 into tlsfuzzer:master Jun 24, 2024
79 of 87 checks passed
@tomato42
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement new feature to be implemented
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants