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 to python3 #607

Open
wants to merge 19 commits into
base: master
Choose a base branch
from
Open

migrate to python3 #607

wants to merge 19 commits into from

Conversation

dlgoodr
Copy link

@dlgoodr dlgoodr commented Sep 30, 2021

  • 2to3'd
  • autopep8'd
  • dropped custom DNS server implementation in favor of dnslib
  • various deprecated functions updated for python 3.9, most notably around XML, f-strings and string/byte encodings

@dlgoodr
Copy link
Author

dlgoodr commented Oct 4, 2021

@moodyblue?

@moodyblue
Copy link
Collaborator

moodyblue commented Oct 4, 2021

Thanks for submitting this pull request. I did not reply earlier because it raised some concerns on my side and I could not find time during the week end to discuss them with you. Pls do not think that I'm not happy with this pull request (I am very happy) but, not being a developer myself, I need to understand what are the benefits of having a py3 version.

I think that the py2 version needs to coexist with a py3 version because some people may have installed PlexConnect in a device where py2 is available but py3 is not (such as jb'd ATV's) or where py2 is used by default (such as docker distros). Also I would need to rewrite some wiki sections to install the py3 version, and future patches need to be applied on both versions.

Last but not least, I need to find some time to test this py3 version.

@dlgoodr
Copy link
Author

dlgoodr commented Oct 4, 2021

Pls do not think that I'm not happy with this pull request

I don't! no worries! just a bump because I didn't know how actively this repo was watched.

I need to understand what are the benefits of having a py3 version.

py2 is dead.

such as jb'd ATV's

good point, I don't have any job's jb'd ATVs, so I didn't realize that was a constraint. I think there's a couple paths, one is to just say that 0.7.4 is the last py2 version because it is EOL but I don't know if that's fair or reasonable. You can conceivably manage a py2 repo and a py3 repo, but that kind of ends up being twice the work. I'm open to ideas but I don't love staying on python 2 long-term for supportability reasons.

such as docker distros

put a Dockerfile in this repo and roll an 'official' version into dockerhub and this becomes a non-problem. I'm happy to help there, I just wanted to get to py3 first.

@moodyblue
Copy link
Collaborator

Started to test. PlexConnect.py starts ok, but running as daemon gives error

[/share/PlexConnect-py3] # sh PlexConnect_daemon.bash start
Starting PlexConnect...
Traceback (most recent call last):
  File "/share/CACHEDEV3_DATA/PlexConnect-py3/PlexConnect_daemon.py", line 84, in <module>
    daemonize(args)
  File "/share/CACHEDEV3_DATA/PlexConnect-py3/PlexConnect_daemon.py", line 49, in daemonize
    si = file('/dev/null', 'r')
NameError: name 'file' is not defined

@dlgoodr
Copy link
Author

dlgoodr commented Oct 6, 2021

Started to test. PlexConnect.py starts ok, but running as daemon gives error

I didn't even look at the daemon, i'll give that a look tonight.

@moodyblue
Copy link
Collaborator

moodyblue commented Oct 6, 2021

I still see 3 problems:

  1. Even though you've changed PlexConnect_daemon.py the daemon soes not start (nor gives any error)
  2. When logged in plex.tv no Plex servers are found
  3. When not logged in plex.tv my local Plex server is found but I can't play any media

Logs:
PlexConnect-local.log
PlexConnect-plextv.log
PlexConnect-daemon.log

@dlgoodr
Copy link
Author

dlgoodr commented Oct 6, 2021

Thanks, i'll start digging in to this. the lack of a test environment in the code makes this very much a "works for me" sort of test so i appreciate the extra data.

@moodyblue
Copy link
Collaborator

Suggestion: include pillow in requirements.txt

@moodyblue
Copy link
Collaborator

moodyblue commented Oct 6, 2021

I'm able to play media after installing pillow (no plex.tv login). Probably I had fanart enabled in settings.cfg (copied from my prod environment) and this caused the previous error. This leaves us with two issues: daemon and plex.tv access

@moodyblue
Copy link
Collaborator

Thanks, i'll start digging in to this. the lack of a test environment in the code makes this very much a "works for me" sort of test so i appreciate the extra data.

I can ask for testers in the forum.

@moodyblue
Copy link
Collaborator

Do you think that it will be possible to fix #608 in this new version ?

@dlgoodr
Copy link
Author

dlgoodr commented Oct 7, 2021

Interesting, discovering servers via multicast to GDM does not work for me. I can even try with nc from my machine and it doesn't work:

dlg@nyx:~$ echo 'M-SEARCH * HTTP/1.0'|nc -u  239.0.0.250 32414
^C

but hitting the ip directly is fine:

dlg@nyx:~$ echo 'M-SEARCH * HTTP/1.0'|nc -u  tvbox 32414
HTTP/1.0 200 OK
Content-Type: plex/media-server
Host: xxx.plex.direct
Name: tv box
Port: 32400
Resource-Identifier: xxx
Updated-At: 1633577916
Version: 1.24.3.5033-757abe6b4

did discovery work for you in the py2 version?

@dlgoodr
Copy link
Author

dlgoodr commented Oct 7, 2021

Do you think that it will be possible to fix #608 in this new version ?

we can test once the plex.tv discovery is working again, i ran out of time last night troubleshooting.

@moodyblue
Copy link
Collaborator

did discovery work for you in the py2 version

It used to but I no longer use it. Could be usefull if I had several local PMS's. I can test again. Some switches and routers block multicast traffic, so setting up GDM is not so simple as most people thinks.

@moodyblue
Copy link
Collaborator

GDM is working for me in py2.

@dlgoodr
Copy link
Author

dlgoodr commented Oct 16, 2021

back at it tonight, sorry about the long silence.

@dlgoodr
Copy link
Author

dlgoodr commented Oct 16, 2021

@moodyblue, does this fix GDM? it finds servers when you're logged in now.

.is_alive() is the new name for .isAlive() in 3.9, i wonder if moving to a traditional .join() instead of hacking our own makes sense in light of this.

Withdrawn, .is_alive() has been around since 2.6: https://docs.python.org/2/library/threading.html#threading.Thread.is_alive

bloerwald referenced this pull request Dec 5, 2021
XMLConverter: fix: youtube/get_video_info 404s without &html5=1 these days
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants