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

Yet another Python3 wip #2258

Closed
wants to merge 33 commits into from
Closed

Yet another Python3 wip #2258

wants to merge 33 commits into from

Conversation

Mictronics
Copy link

Feel free to review, test and pull in if you like.

Test environment 1:

  • Raspberry Pi 3+ with Raspberry v2.1 camera
  • Debian bullseye 32bit (DietPi v7.9.3 distribution)
  • motion v4.4.0 bullseye armhf release
  • Python v3.9.2 (with pip3, tornado v6.1, jinja2, pillow, pycurl, six)
  • Pi camera setup as v4l2 or MMAL in motioneye

Test environment 2:

  • x86 PC with USB web cam
  • Linux Mint 20.2 Uma (Ubuntu 20.04 focal)
  • motion v4.4.0 focal amd64 release
  • Python v3.9.7 (with pip3, tornado v6.1, jinja2, pillow, pycurl, six)
  • USB camera setup as v4l2 device in motioneye

@zagrim
Copy link
Collaborator

zagrim commented Dec 30, 2021

I don't count as authority, but why oh why did you make non-Python3 related changes, too? Like add Motion 4.4 support, and change configs, and alter source code formatting? Just the formatting changes make reviewing this awfully more tedious than it need to, and mixing two major changes like Python3 and Motion4.4 in the same bunch (even in the same feature branch) is IMO just wrong way to do things.
That being said, I am really grateful for the efforts you've taken to get python3 support moving on. Myself I'd just remove the Motion 4.4 commits and config changes (which hopefully are in separate commits, too) from this. I kind of maybe understand why you've also included c94863d but that doesn't really relate to python3, either.

@Mictronics
Copy link
Author

It's a voluntary contribution. Pull it if you like, close the PR if not.

@zagrim
Copy link
Collaborator

zagrim commented Dec 30, 2021

Curious attitude you have. But as I said, I'm no authority, and my comment was meant to be helpful so that your contribution doesn't get wasted. But it's the project owner's call.

@MichaIng MichaIng added the python update python 2 > python 3 label Mar 8, 2022
@MichaIng MichaIng changed the base branch from python3 to dev March 9, 2022 19:08
@MichaIng
Copy link
Member

MichaIng commented Mar 9, 2022

To deep conflicts to resolve. I opened a partial PR where we need to add other wanted changes from this PR: #2318

@cclauss
Copy link
Member

cclauss commented Mar 11, 2022

Please rebase.

@MichaIng
Copy link
Member

This cannot really be rebased as of too many conflicts. See my post above with the PR based on a conservative rebase, i.e. consequently preferring current dev code. But it is possible that other changes from here are good to manually port to dev.

@cclauss
Copy link
Member

cclauss commented Mar 11, 2022

Why chmod so many files like: motioneye/handlers.py 100644 → 100755, etc.?

setup.py Show resolved Hide resolved
@MichaIng
Copy link
Member

@cclauss
Please do not review here, this PR cannot be merged. E.g. the setup.py doesn't exist anymore etc 😉. At best, if we see some enhancements here which have not landed in dev yet, then we can create a dedicated PR to merge them back. However, most changes are for Python 3 compatibility only and have landed exactly or similarly in dev already.

MichaIng added a commit that referenced this pull request Mar 20, 2022
Use floor division instead of regular division follow by int() conversion. Also remove redundant parenthesis.

Taken from #2258 by @Mictronics.

Signed-off-by: MichaIng <micha@dietpi.com>
@MichaIng MichaIng removed the feature label Mar 20, 2022
MichaIng added a commit that referenced this pull request Mar 21, 2022
Use floor division instead of regular division follow by int() conversion. Also remove redundant parenthesis.

Taken from #2258 by @Mictronics.

Signed-off-by: MichaIng <micha@dietpi.com>

elif self._auth_mode == 'digest': # in digest auth mode, the header is built upon receiving 401
self.write(b'GET / HTTP/1.1\r\n\r\n')
self.write(b'GET /%d/stream HTTP/1.1\r\n\r\n' % self._camera_id)
Copy link
Member

@MichaIng MichaIng Mar 23, 2022

Choose a reason for hiding this comment

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

@Mictronics
These GET requests are the only change which is not yet applied the same or similar way in dev already. Since generally MJPG cameras and the MJPG stream to browser work fine, can you explain the rationale behind this change?

@MichaIng
Copy link
Member

Marking this as closed now. The open question above remains, but since everything works as expected it is not a mandatory change and can be done any time later via dedicated PR and proper explanation why and how to test.

@MichaIng MichaIng closed this Mar 24, 2022
@MichaIng MichaIng removed the request for review from ccrisan March 24, 2022 13:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement python update python 2 > python 3
Development

Successfully merging this pull request may close these issues.

5 participants