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

Transition to Python 3.x #191

Merged
merged 97 commits into from Jul 20, 2018
Merged

Conversation

albertosottile
Copy link
Member

@albertosottile albertosottile commented Jul 17, 2018

Following the discussion of issue #169, this PR ports all the codebase to Python 3.x on all the platforms.

A few info and caveats:

  • Python 2.7 will be effectively abandoned. After merging, the minimum required version will be 3.4 (required by Twisted).
  • Windows: to avoid multiple transitions at the same time, in this PR Windows binaries will use Qt 4.8.7, PySide 1.2.4, and py2exe on Python 3.4 (see Transition to PySide2 and Qt 5.x #185 for details). Transitions to Qt 5.x and pyInstaller (and newer versions of Python) are deferred to the next releases.
  • macOS: this platform is already on Qt 5.x and binaries will migrate directly to Python 3.6.5, PySide2 5.11.0, and Qt 5.11.1.
  • Linux: users will be able to run Syncplay with any version of Python newer than 3.4, using Qt 4.x and PySide 1.x on 3.4, or using Qt 5.x and PySide2 on 3.5+.
  • For security reasons, a PR is not allowed to deploy binaries in the CI systems we currently use. If anyone wants to test Syncplay on Python 3.x, I can provide links for binaries, packed by my fork, for Windows and macOS.

Thanks to @Et0h and @xNinjaKittyx for their assistance and support during the port.

buildPy2exe.py Outdated
@@ -439,7 +439,7 @@ def get_nsis_path():
CreateShortCut "$$SMPROGRAMS\Syncplay\Syncplay.lnk" "$$INSTDIR\Syncplay.exe" ""
CreateShortCut "$$SMPROGRAMS\Syncplay\Syncplay Server.lnk" "$$INSTDIR\syncplayServer.exe" ""
CreateShortCut "$$SMPROGRAMS\Syncplay\Uninstall.lnk" "$$INSTDIR\Uninstall.exe" ""
WriteINIStr "$$SMPROGRAMS\Syncplay\SyncplayWebsite.url" "InternetShortcut" "URL" "https://syncplay.pl"
WriteINIStr "$$SMPROGRAMS\Syncplay\SyncplayWebsite.url" "InternetShortcut" "URL" "http://syncplay.pl"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason why this was changed to http?

Copy link
Member Author

Choose a reason for hiding this comment

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

EDIT: fixed. I was looking at the wrong URL, my bad. Thanks for spotting this.

fileList[new_root] = []
fileList[new_root].append(file_)
return fileList, totalSize

def prepareInstallListTemplate(self, fileList):
create = []
for dir_ in fileList.iterkeys():
for dir_ in fileList.keys():
Copy link
Contributor

Choose a reason for hiding this comment

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

In python3 dictionary iteration, it's often better to use for key in dict rather than for key in dict.keys(). dict.keys() returns a list rather than a generator-like object, which for key in dict does.

Copy link
Contributor

Choose a reason for hiding this comment

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

I retract my statement. It's essentially the same. I think dict in keys is still faster in most cases.

Copy link
Member Author

Choose a reason for hiding this comment

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

Lot of these changes were done by 2to3. If you want, I can edit this one, but I guess the same issue will appear in multiple other parts of the codebase. If you think these could cause potential problems, I can scroll all the changes and apply the same fix everywhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Yeah there's a lot of places that could be improved with shorter notation, but they're not necessarily "wrong".

I think I'll just ignore them for now, and there can be another PR that cleans up the code.

@@ -869,12 +869,12 @@ def _wrapinstance(func, ptr, base=None):

"""

assert isinstance(ptr, long), "Argument 'ptr' must be of type <long>"
assert isinstance(ptr, int), "Argument 'ptr' must be of type <long>"
Copy link
Contributor

Choose a reason for hiding this comment

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

Comment should be int and not long right?

@Et0h
Copy link
Contributor

Et0h commented Jul 20, 2018

@xNinjaKittyx Is everything okay now?

@xNinjaKittyx
Copy link
Contributor

@Et0h yeah

@albertosottile albertosottile merged commit 9ecb7c3 into Syncplay:master Jul 20, 2018
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.

None yet

4 participants