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

GitHub Action to lint Python code #2308

Merged
merged 4 commits into from
Mar 9, 2022
Merged

Conversation

cclauss
Copy link
Member

@cclauss cclauss commented Mar 8, 2022

Test results: https://github.com/cclauss/motioneye/actions are now green.

This PR fixes a few basic Python 3 syntax errors like print() function, new style exceptions, and lambda parameters. Those fixes are compatible with both Python 2 and Python 3.

execfile(), file(), unicode() and xrange() were removed in Python 3. #2310

@MichaIng
Copy link
Member

MichaIng commented Mar 8, 2022

Would need to be updated for Python 3, but generally good. I also just added it to CodeFactor: https://www.codefactor.io/repository/github/motioneye-project/motioneye

But it makes sense to do all wanted tests via GitHub actions.

@MichaIng MichaIng added the CI/CD label Mar 8, 2022
@cclauss
Copy link
Member Author

cclauss commented Mar 8, 2022

Let’s put the guardrails in now, not later.

@MichaIng
Copy link
Member

MichaIng commented Mar 8, 2022

I just want to avoid that we resolve Python 2 specific issues which may not be relevant anymore after #1572 has been merged. Generally I'd vote for doing this as very first step and then concentrate testing and developing for Python 3 only.

@cclauss
Copy link
Member Author

cclauss commented Mar 8, 2022

I prefer to see red tests turning green as progress is being made rather than tests that never were red.

@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

Okay, I get your point. Not sure however whether tests against current Python 2 dev branch and Python 3 can be really compared. Let's see what others think about it.

@starbasessd
Copy link

May I comment? Until the python3 port is ready for prime time (or beta test of an RC), I know many users would like to see the 'fixes' for python2 that are 'in the queue' be tested and applied. You could make an announcement stating that after they are applied, there will be a 'freeze' on fixes, with focus shifting to python3. I would not flag any python2 specific issues at that point for fixes. It has been nearly 2 years without any fixes, patches, whatever being applied, and for 'normal' end users, it doesn't look good. I've done my best to keep interest alive, but...

@cclauss
Copy link
Member Author

cclauss commented Mar 9, 2022

Python 2 died on 1/1/2020 which is almost 800 days ago.
Let’s not focus on legacy Python at this late date.

@starbasessd
Copy link

The original EOL date was to be 2014 (8+ years ago) and kept getting moved. IIRC motionEye was started after the first EOL was announced. (9 years ago by the oldest file in github). Would it kill anyone to do one last patch /update if they already exist? Not fix any new issues, just apply ones that have been requested and ready to apply? And what level are you writing the python3 code? Are you looking to remove any Python3.7 and prior specific instructions? 3.7 EOLs next year, and all the earlier 3.x are already past EOL. (Sorry, I am often an end-user advocate)

@starbasessd
Copy link

Also should this type conversation be taken to another forum or format? Or leave it public?

@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

I personally don't see any point in spending any further time on Python 2, at least I won't, of course if anyone else wants to, that is fine. The two forks however, where much effort has been gone into, are Python 3 ones. If anyone really requires it and there is someone which wants to take care of it, we could create a python2 branch from dev now, and merge/backport fixes, then do further 0.42.x Python 2 releases, while Python 3 releases would then start with 0.43.0 (as a suggestion)?

EDIT: I just created a python2 branch from current dev, to not loose compatibility when we merge #1572: https://github.com/motioneye-project/motioneye/tree/python2

Also should this type conversation be taken to another forum or format? Or leave it public?

IMHO everything related to the future of motionEye should be discussed publicly. We need all help and opinions that we can get to keep the project going, and hence the existing community should be involved as much as possible, also or especially regarding the question whether there is indeed still notable need for Python 2 releases.

@kleini
Copy link
Collaborator

kleini commented Mar 9, 2022

I would not put any effort into Python2 code. Let's get motionEye running on Python3 basically and keep provided fixes open. Port those fixes to Python3, if necessary, and merge them then. Let's get onto newest base as everything else has no future.

@cclauss
Copy link
Member Author

cclauss commented Mar 9, 2022

#2309 lints legacy Python on the Python 2 branch for those who desire to end-user advocate for an insecure platform.

@starbasessd
Copy link

"lints legacy Python on the Python 2 branch for those who desire to end-user advocate for an insecure platform."

I feel this is a dig at me, please correct me if I'm wrong.

@cclauss
Copy link
Member Author

cclauss commented Mar 9, 2022

It was a friendly dig... We are all traveling in the same direction.

As you can see in the 17 commits on #2309, it takes a lot of work to make Python 2 continue to sing. That work would be better spent establishing forward progress.

@starbasessd
Copy link

Sir, I don't appreciate any digs. I won't give them, I won't take them, I don't see why there was a need to make one.
As I stated, I just wanted to see all the 'fixes' that had been presented to be applied, since it had been neglected for so long. After this session of patches / updates, I don't / wouldn't expect any more to be accepted or applied.
If you want me to go away, feel free to say so. As you pointed out, this is a volunteer effort, and I put in a LOT of time and effort to support CCrisan's product.
Was I disappointed he had to drop it? Yes I am.
Was I willing to help end users to the best of my ability and resources, for no compensation? Yes I am.
But if people feel free to take digs (friendly or otherwise) at other people trying to support the project or end users, or if CCrisan had felt that way when he ran the project, I know I would not have continued supporting in my way. I volunteered supporting end users before I was granted collaborator status and started updating the WiKi documentation.

@cclauss
Copy link
Member Author

cclauss commented Mar 9, 2022

The GitHub Actions at https://github.com/cclauss/motioneye/actions on this PR and #2309 should now be green.

This PR fixes a few basic Python 3 syntax errors like print() function, new style exceptions, and lambda parameters. Those fixes are compatible with both Python 2 and Python 3.

@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

@starbasessd
Do you actually have a system where you are stuck with Python 2 and have no change to upgrade to Python 3? I am really wondering if there is a single user out there which wouldn't be able to run motionEye with Python 3 immediately, which IMHO is the main question. Reviewing PRs, testing on two instead of one motionEye builds, opening additional PRs etc requires time, so it really depends on whether you or someone else is willing to do the extra testing/verification on a Python 2 build of motionEye, clone the related PR and maintain the now existing python2 branch. As said, from my end, any time I can spend here, IMHO is way better spend in getting motionEye running stable on Python 3, but this is my personal opinion and choice about my personal contribution time, while this project should invite anyone to focus on those aspects seen personally important. We should be able to setup repo permissions to allow PR merges and requiring reviews by (only) the collaborator(s) willing to take the Python 2 lead.

@cclauss
Many thanks for the CI setup on python2. Also requirements.txt is a reasonable step, later we can add requirements-dev.txt for e.g. Docker builds and/or other CI/CD workflows which require Python modules not required for regular runtime. About Travis CI @ccrisan do you still maintain an account there or is this obsolete? Generally moving to GitHub actions seems reasonable, being FLOSS and free of costs and well integrated with GitHub.

@starbasessd
Copy link

I have not asked for any new patches / updates for python2, just apply those that have already been created. They Python3 branch has been out there in various forms for a long time. Do you really believe the python3 version will be instantly stable when presented? Have you taken into consideration of ALL the features and their changes? Dropbox now needs a new method to support their short term key vs long term key. Drop Dropbox Support? Google is changing their GDrive and GMail access for 'non-secure' access May 30. Python3 dropped support for PyCamera library (among other things) that affect PiCam on Bullseye, so YES there are many systems out there that can't go to python3 and still use motionEye, as PyCamera library wasn't all that RPiFoundation broke with Bullseye.

Python 3.7 goes EOL next year. Are you being pro-active for removal / changes to currently supported functions? Are you considering migrating to another language since python4 will probably never come about, and python3 will probably end up like the python2 train...

@starbasessd
Copy link

Also there are many users who have python2 set as default, can you have any references to python that need python3 to specify python3 and pip3?

@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

I have not asked for any new patches / updates for python2, just apply those that have already been created.

They still need to be reviewed, tested etc. Not all of us are sufficiently deep in this project to know all PRs already, whether they are safe to merge etc 😉.

Do you really believe the python3 version will be instantly stable when presented?

IMO and argument to really focus all efforts on getting it stable as fast as possible.

Dropbox now needs a new method to support their short term key vs long term key. Drop Dropbox Support? Google is changing their GDrive and GMail access for 'non-secure' access May 30.

Is this really Python 3 related or related to changes at Dropbox and Google APIs? I do not use proprietary cloud servers, so I don't know much about this, but I know about the Gmail topic, and that isn't related to Python 3 at all and can be solved very easily by using 2FA and an application key instead of the regular username's password for SMTP authentication. I just did that change for our phpBB instance.

PyCamera supports Python 3, or do you mean PiCamera? The latter is as well not related to Python 3 but that Raspberry Pi OS with Bullseye dropped parts of the legacy proprietary GPU libraries and moved to modern upstream libraries like libcamera and V4L2 basis.

Also there are many users who have python2 set as default, can you have any references to python that need python3 to specify python3 and pip3?

On most distros it is and always was so that one needs to install and use python3 and pip3 while python and pip refers to Python 2. You can hence install use both perfectly fine together, i.e. install a Python 3 version of motionEye along with python3 and pip3 as a test instance before switching.

cclauss and others added 3 commits March 9, 2022 16:50
Co-authored-by: MichaIng <micha@dietpi.com>
Co-authored-by: MichaIng <micha@dietpi.com>
Co-authored-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng merged commit 19881a6 into motioneye-project:dev Mar 9, 2022
@starbasessd
Copy link

Is this really Python 3 related or related to changes at Dropbox and Google APIs? I do not use proprietary cloud servers, so I don't know much about this, but I know about the Gmail topic, and that isn't related to Python 3 at all and can be solved very easily by using 2FA and an application key instead of the regular username's password for SMTP authentication.

It does involve the python scripting in motionEye, and that scripting will have to be fixed for email notifications, and file uploading.

@cclauss cclauss deleted the patch-1 branch March 9, 2022 17:07
@starbasessd
Copy link

I tell you what. Let's remove me from the 'team'. I may continue helping with the legacy for a bit, but I am not feeling good about the python3 conversion team, attitude, or environment. I wish you luck.

@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

That is really sad to read. I fully agree that we must talk respectfully with each others, despite possibly different priorities. That naturally comes along with a team. This is meant in both directions, respecting and taking serious a possible demand for further Python 2 support, as well as respecting that free volunteers decide to spend their time on Python 3 supporting code only. IMO we simply must give every contributor the freedom to focus on those aspects personally seen as important. In case of Python 2 vs Python 3 it doesn't conflict at all and doesn't cause any issue if some are keeping Python 2 compatibility up and related code fixed via separate branch, while the others focus on getting Python 3 compatibility stable. There is a sufficient amount of ends where work needs to be done, much of which is unrelated to Python 2 vs Python 3, also on the documentation side, Docker and motionEyeOS, I guess. And reviewing/cleanup of issues and open pull requests is another topic on its own.

It does involve the python scripting in motionEye, and that scripting will have to be fixed for email notifications, and file uploading.

But how is this related to Python 2 vs Python 3. Of course if an external API changes, the related Python code needs to be updated, but that affects any Python version. Doing so for two Python versions means the extra I want to avoid for my own contribution time. The major question still is if there is anything which prevents you from switching Python 3, aside of that you will currently run into the one or the other syntax error where fixes will be merged soon (but unrelated to the issues you mentioned)?

@starbasessd
Copy link

I have several PiCams on RPi hardware, running RPiOS (Debian derived) that has issues with Buster and the newer Python3 and missing libraries. Could they be moved to something else (even with the name 'motionEye'? Possibly, but if they ain't broke... And I won't risk changing them over for a period of time while the python3 is being tested. And with several 'features' that are going to break soon (gdrive, gmail email notifications, or have already done so (dropbox) and the responses that have been given here, if I have to upgrade / change my environments (I use GPS for a timebase and they are still using python2 libraries) the amount of work involved will justify also looking at another product altogether that will still have those features. And if I have to learn another system, I'll end up dropping this one. One reason I have been learning python2/3 for personal use was motionEye.

Another question: What is the intent to maintain motionEye/motionEyeOS interoperability? Any? That would be another reason to not shift to Python3, if it won't work with motionEyeOS, too. Several PiZeroW Cameras have motionEueOS on them and they interact with my primary server (Debian 11, python2, motionEye 0.42.1, motion 4.3.2) just fine. To have to drop one to python3 would force me to drop all? Since motionEyeOS is not a priority, does this force me to lose that functionality?

@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

I have several PiCams on RPi hardware, running RPiOS (Debian derived) that has issues with Buster and the newer Python3 and missing libraries.

But how is this related to Python 3? It is an RPi OS distro change, but you can run Python 3 on ancient Raspbian Jessie systems, if you like, as well as Python 2 on Raspberry Pi OS Bullseye, that is not related at all with each other, aside of the fact the Bullseye makes it more difficult to install Python 2 via APT packages.

Gmail, GDrive and Dropbox will break for Python 2 just the same way, though I already mentioned that Gmail is easy to fix only by using an application password, so for this there is no motionEye code change required.

What is the intent to maintain motionEye/motionEyeOS interoperability?

Of course the intention is to update motionEyeOS to use the motionEye Python 3 build, once stable. It is one thing to have software which supports EOL libraries/dependencies, but it is irresponsible to ship OS images with EOL software, at least from security point of view.

To have to drop one to python3 would force me to drop all?

I need to have a closer look into how multiple instances are communication to each other, but I cannot imagine that the Python version has any effect on this, given that public APIs do not change, just the backends. But this is something which should be tested first of course, e.g. installing a Python 3 motionEye on a dedicated SD card with one RPi Zero and simply test it.

And keep in mind that you can run both Python versions concurrently. So while motionEye uses Python 3 any other software you may depend on can use Python 2, and this is true on all Debian/Raspbian versions, on Bullseye at least when installing the apt install python2, pip via https://bootstrap.pypa.io/pip/2.7/get-pip.py and other Python (2) modules via pip, since as mentioned Bullseye does not ship many Python 2 modules via APT packages anymore.

@kleini
Copy link
Collaborator

kleini commented Mar 9, 2022

This project should go to Python 3. Keep already provided pull requests open, migrate then to Python 3 once the Python 3 version runs and then merge those pull requests. An initial version running on Python 3 will not be perfect from its beginning, but we should begin to use Python 3 as everything else is totally outdated and maybe even vulnerable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

4 participants