Fix known Python 3 issues #198
Conversation
|
@Smeulf This should be using the best possible implementations for iter*. |
|
@CastagnaIT This requires #196 to be merged first. |
|
Catch-22. For testing Python 3 support, there is only a Windows binary. It appears that this Python 3 build does not ship with script.module.pycryptodome (pycryptodome), even though it appears to be installed, it is not. As a result I am stuck at that for testing. Update: I reported this issue to Kodi upstream at: xbmc/xbmc#16116 (comment) |
|
So with this PR all integration tests work on Python 3 (locally) but I cannot test it for real. |
|
to understand, then integration works only locally, for every pullrequest nothing will be verified |
|
@CastagnaIT Correct, we cannot enable the integration tests if they fail in Travis. That is still a work in progress. Also, this PR currently includes the removal of mysql-connector-python, but the script.module.mysql.connector will not be merged (become available) until our add-on plugin.video.netflix is accepted. So we cannot included those changes just yet. Not sure what you prefer to do ? Update: I guess the best way forward is move those changes (that would break the add-on) to #200 instead, and leave that one open until we are ready to release? |
|
So I think this is ready to be merged as well, but please do test this branch on your systems. I can't guarantee it will work on Python 3, as it is only tested locally outside of Kodi. But lacking a real usable Python 3 system at the moment, I would be willing to propose this upstream. But we have to be certain this branch works as well as before, on Python 2 ! |
|
As a reminder, Python 3 is still not enabled in Kodi, so if it still works on Python 2, it will be ok. Python3 is a recomandation from Kodi team to ensure migration when switching to Python3. But we will have some time to fix the potential remaining issues before it becomes a final release. I'll run some tests on Windows and then on RPi hopefully tomorrow, focused on Library export, StreamContinuity and ResumeManager. Plus some basic playing tests of course :) |
|
@Smeulf We have been testing it the whole evening, watching Netflix ;-) But I don't use some of the advanced functionality (MySQL, Library, etc.) and I don't have any integration tests for those. We do need to have unit tests and more integration tests. Also, if I cannot test the add-on in Kodi, it is likely that we are testing against a framework that does not closely mimic a real Kodi Python 3 environment. In any case, to me, with my limited testing, it works fine on Kodi Leia, on Windows, RPi and Android. |
I do have the requierement for those tests, including SQL. |
This fixes the known Python 3 issues reported by kodi-addon-checker. This PR includes: - Moving enum to enum34 so we can exclude it from Python 3 - Various known fixes for Python 3
|
At first start. Then I restarted and the error diseapear. Sorry more log is unavailable.
|
|
Plus I have an arror with MySQL, and I can't browse My List as an example. |
|
I couldn't reproduce the SQL error on LibreELEC x86 (virtual machine). Looks like a Pi issue(?). I'll try to get a more comprehensive log (remove the try block) in the late evening. |
|
After removing the try block in db_base_mysql / _execute_query, now that's interesting:
|
|
there is a problem with seem that the values converted from text is not not accepted to use in the query database. The values converted from text change the type to "newstr" that is different from the original unicode/str type |
i agree with you move the removal of mysql-connector-python to the other PR, so we can merge this pr (after fixes) without breaking nothing |
|
@Smeulf You should have |
|
I saw future being installed, but at first run I had that error. No more after, but I remember we had some error like that before and some module couldn't be loaded on OSMC. So it's more a warning than anything. |
|
By 'some error like that' I mean 'an error at first run in LibreELEC I couldn't reproduce after, but had other consequences in other environments' |
|
@Smeulf Yes, there is a general issue with Kodi that newly installed modules do not take effect immediately and you need to restart Kodi (or reboot) in order to make it work. |
|
So in relation to MySQL you will have to fix this yourself. Personally I don't like the MySQL integration and the complexity it adds to the code. I also don't see what the MySQL integration brings, people run Netflix from all kinds of devices without any MySQL integration and I think our add-on should work exactly like that. I have similar doubts with the Library synchronization. I think functionality like that are a distraction (and complexity that the majority of users are not looking for), and that we should focus on creating a Netflix add-on that works as well as the official Netflix offerings. |
It brings synchronization betweek devices for:
So when I start watching a show in the living room I set in english with french subtitles, I can restart the same show, in the same language, at the same point in the bedroom if I want to. Till we can't send watched status to Netflix, it's important to me. About library, it's been discussed for a long time: it's also a matter of unification between different sources (local and streams), plus it allows the use of Trackt. |
|
And it's been a lot of work to get that level of functionality, now it works (maybe it's not perfect) but I'd be pleased if we don't break everything ^^ |
Wouldn't it be better to look into sending this information to Netflix so it works everywhere instead? Also, I have exactly what you want as part of Kodi. All my Kodi boxes are connected to MySQL and they show what has been watched and remember the offset automatically. I don't need the Netflix addon to take care of this. I assume it is because of the below (can't remember, did this 2 or 3 years ago). .kodi/userdata/advancedsettings.xml <videodatabase>
<type>mysql</type>
<host>1.2.3.4</host>
<port>3306</port>
<user>username</user>
<pass>password</pass>
</videodatabase>
<videolibrary>
<importwatchedstate>true</importwatchedstate>
<importresumepoint>true</importresumepoint>
<cleanonupdate>true</cleanonupdate>
</videolibrary> |
Sure it would, but for now, no one has a clue on how to do it, Even ascidisco was stucked... If you can find something for sure it would be awesome.
If you never change the language for a tv show, sure you can't see the benefit of shared stream continuity data. |
Those settings just says you import watched status and resumepoints from NFO if they exists when scanning the sources. You gain the watched status and resume points because every "plugin://plugin.video.netflix...." "files" you watch are stored in the "files" table of the database. But if you use the library export feature, then the resume workaround can't work, |
We always watch the audio in the native language, and subtitles in English (except if it is Dutch).
I have no clue what library export does, or why this would be useful to me. And I wonder how many people are looking for this. But if you think it is worth it, fine. It does add a lot of complexity that I would prefer to disable. |
Ok, so the majority of the people have this out of the box with Kodi MySQL integration (except the language not being remembered on other devices) And we are adding our own MySQL integration (and lots of complexity) because of an issue in Kodi when doing library exports? That seems the wrong trade-off IMO. |
The unification of local videos and VOD content is something Kodi should facilitate and is something I don't understand they haven't figured this out (maybe they do, haven't seen any design). The same is true wrt. TV/Radio and online TV/Radio streams in add-ons. You would expect that add-on content could be registered (and expired) as Movies/TVshows/TV or Radio and it would appear where it belongs. This should not be something the add-on has to develop itself and it should not require a user to create a database instance and table in order to get this working. |
And to make it working correctly (ie retreive labels, etc). Try to play around with the export without the sql, and see the differences on your other client...
You found the right work: "Should". If you have any other way to make it work correctly on multiple devices, please share :) For now, until Kodi evolves, I'm not sure there's one... |
Again, I don't see why I would want to export to my Kodi library. Not sure what "retrieve labels" is and why I would care. The Netflix add-on works fine as it is.
So you think it is only logical that every add-on implements this themselves. I am convinced this is worse than not having this functionality at all (until it is part of Kodi). But that is probably because I don't mind to set the language myself on every device. (I have 3 but I seldom continue to watch the same program on another device, and I wonder how many users are in that same situation). |
Never said that. My point is we're (all people creating add-ons I think, including you) trying to do the best with what we have in our hands: a Python framework lacking a lot of functionality and events regarding Kodi (Even if I have to mention it's getting better and better every year) And here, to fix issues and keep this add-on alive, and fix with functions that was already present but was not compatible with a shared Kodi db. You're free not to care about people using the functions the add-on offers, but that's not a reason to break them... Anyway, that's not my call, it's @CastagnaIT one, so he will remain the one with the last word. |
|
@Smeulf I don't want to break anything, I am just concerned about the complexity and maintainability of the code. And the trade-offs that have been made. For all I care it can stay the way it is. But the complexity also makes it harder to test all the different use-case and combinations of functionality. So I cannot take responsibility for things I cannot test. |
I do understand. |
Check if this PR fulfills these requirements:
Types of changes
Description
This fixes the known Python 3 issues reported by kodi-addon-checker, tox and pylint.