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

Migrate pygstc to asyncio based sockets. #252

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

jameshilliard
Copy link
Contributor

This should allow pygstc to be used properly in a non-blocking manner in addition to providing a cleaner abstraction over the TCP socket operations.

I also fixed the python tests to run properly in github actions.

@michaelgruner
Copy link
Contributor

I love this. I'll review calmly next week. Thanks for your multiple submissions.

@@ -11,7 +11,7 @@ on:

jobs:
build:
runs-on: ubuntu-18.04
runs-on: ubuntu-20.04
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we really want to move the testing version forward?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was due to 18.04 shipping with python 3.6 while I used features here requiring a python 3.7 minimum.

Comment on lines +47 to +50
'Programming Language :: Python :: 3.7',
'Programming Language :: Python :: 3.8',
'Programming Language :: Python :: 3.9'],
python_requires='>=3.7',
Copy link
Contributor

Choose a reason for hiding this comment

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

Does asyncio not support 3.5, otherwise we might be breaking support

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well basic asyncio does support 3.5, however specific features I used have a minimum version of 3.7.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI python 3.5 is already end of life, and 3.6 will be end of life within a couple of months looks like.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

python 3.6 is now end of life so requiring python 3.7 shouldn't be an issue anymore right?

tests/libgstc/Makefile.am Outdated Show resolved Hide resolved
import socket
from contextlib import asynccontextmanager
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This feature has a minimum python version requirement of 3.7. I can probably rework this to not require it but it makes socket cleanup a bit cleaner IMO.

@jameshilliard
Copy link
Contributor Author

I've rebased this however there's some failures unrelated to the changes in this pull request, I reworked the python test runner in #278 to fix the tests themselves(although some appear to still fail due to gstd bugs however).

@jameshilliard jameshilliard force-pushed the asyncio branch 4 times, most recently from 46628a8 to 8222898 Compare March 5, 2022 11:30
@jameshilliard
Copy link
Contributor Author

I've rebased this on top of #278 which fixes most tests.

@migueltaylor
Copy link
Contributor

The main issue I see with this is that we currently support python >= 3.5. I could see migrating the client to python 3.6 but not to python 3.7 just yet. A lot of GstD users run it on NVIDIA JetPack and the latest version 4.6 still uses Ubuntu 18.04 and python 3.6. Unless there is an easy to maintain way to provide backwards compatibly I think we will have to wait a while to include this change.

@jameshilliard
Copy link
Contributor Author

The main issue I see with this is that we currently support python >= 3.5.

Hmm, is it just NVIDIA JetPack using EOL python versions? 3.5 has been deprecated for a while now and 3.6 has been for a few months. This just updates the client library so I think if anyone need to use an older python they can just not install the new asyncio version probably right?

A lot of GstD users run it on NVIDIA JetPack and the latest version 4.6 still uses Ubuntu 18.04 and python 3.6. Unless there is an easy to maintain way to provide backwards compatibly I think we will have to wait a while to include this change.

Can't they just do apt install python3.7-dev? Seems to work fine for me on a normal ubuntu 18.04 system at least.

@TopperBG
Copy link

TopperBG commented Apr 1, 2022

Keep in mind many Yocto projects, where change of packet, specialy Python is not as easy as apt install.

@jameshilliard jameshilliard force-pushed the asyncio branch 5 times, most recently from 3869039 to eaadd41 Compare June 16, 2022 02:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants