-
Notifications
You must be signed in to change notification settings - Fork 650
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
Python3 experimental support #1805
Conversation
@Vlsarro this is great work! However having everything in a single PR like this is going to be very difficult to review. |
Hallo @ccrisan, this branch is really promising. With the few additional PRs all trying to bring python3 to motioneye a lot of work is being wasted. What can I do to help you gtm? I understand that this are a lot of changes and not all of them are python3 specific. Best |
@herostrat this PR has a lot of stuff that has been automatically converted/adjusted (or at least that's my impression). There are changes that simply break the functionality of motionEye. I haven't had the time to take a serious look over it, since it's quite a huge amount of changes, but trying to run it will simply show various functionalities that don't work. I'm not saying this PR doesn't contain good stuff. What I'm saying is that it probably has good changes that would be welcomed (either for py2 or for py3), but given the way they are put together, noone's going to be able to read through them and actually apply/merge them. |
I think, we should merge smaller merge requests into the python3 branch and try to get that working. Once the python3 branch works, we should merge that back to dev. So, this can become a working scheme, where people can start to participate. |
@ccrisan First, thanks a lot for the answer. I unterstand and feel you, I wouldn‘t want to review it myself 👍 Anyone interested in a working group to reduce dublication of efforts and makes motioneye ready for python3? |
I'm a software engineer and am also using motioneye happily (incl mobile app), thank you. Would love to use it on python3 :-) @ccrisan There are obviously a lot of edits here, and the various authors have spent a good time trying to split them up and describe them individually. If I were in charge here (and I'm glad I'm not!) I think I would just bite the bullet and make a python3 branch from this one, but not make a release. I would then put a note in the main git readme (or on the homepage of the project) about testing python3 being needed and any known issues. Given the large number of commits/changes that appear to be needed, and the nature of the project, I don't think there is anything to be gained (at this point) in trying for smaller commits. I would expect you will see a good number of contributors to make it, once there is a clear path to follow, but at present there isn't because the branch is not obvious and neither is this issue/PR. If I can help I will, but I'm only a dabbler in python, not a specialist. |
I've cloned Visarro's repo and had a play, using my existing 0.42 setup. I needed to install tornado, pycurl, jinja2 ... would it be beneficial to have a requirements.txt file? In motioneye/utils.py: There is an issue with for y in range(ny): in parse_editable, same file. In that function, for me, the range(ny) errors out because 'ny' is a float. I put some debugging just before it:
and it printed:
I am not sure what is intended here, so will move on by casting ny to int, but I'm sure that's not the right answer. .... and indeed, when I do that I get 'list index out of range at line 1009 ( I keep getting an error 'finish() called twice' from the tornado code via: With that, I get most of the web page working, including login, auth, settings page. No video showing though I do see a camera icon, and those are highlighted. I'll stop here for the moment. Hope the above is useful. |
I have a new version of this branch with fixed bugs, may be later I'll add new commits. Didn't have time to pursue this endeavor properly :( |
Vlsarro I'd rather use your fork at this point as it supports python3. |
This is an attempt to migrate motionEye to python 3.
Firstly I just ran 2to3 program and manually fixed any leftovers. Also did general refactoring (PEP8). Secondly I have splitted handlers into separate modules because it was hard to work with all these classes in one file so I just decided to decouple them a little bit. I have also tried to split utils functions into different modules because there were many disparate functions, some of which could be combined into one group (according to their purpose). Ctl files also were moved to their own distinct package.
Next I have added some basic tests. Only a few unfortunately. I sincerely wanted to test all functionality related to IOStream usage, but couldn't wrap my head around how to do that, always got some errors which set me back. Alas, may be in the future.
In addition I have tried to make use of async/await syntax to simplify motionEye code.
Overall the python3 branch looks quite good to me and satisfies my needs, but I'd really appreciate if someone with good knowledge of motionEye inner workings tested the project functionality. Because there is a clear lack of unit and integration tests and I didn't checked all the stuff.