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

Update Windows Packaging #331

Closed
wants to merge 4 commits into from
Closed

Conversation

doadin
Copy link
Contributor

@doadin doadin commented Jan 4, 2022

  • Check that deluge-console works.

  • Plugins Execute & Extractor Won't start:

09:58:44.350 [ERROR   ][deluge.pluginmanagerbase          :78  ] Unable to instantiate plugin 'Execute' from 'e:\\program files\\deluge\\deluge\\plugins\\execute-1.3-py3.10.egg'!
09:58:44.350 [ERROR   ][deluge.pluginmanagerbase          :78  ] No module named 'twisted.internet.utils'
Traceback (most recent call last):
  File "deluge\pluginmanagerbase.py", line 147, in enable_plugin
  File "e:\program files\deluge\deluge\plugins\execute\deluge_execute\__init__.py", line 14, in __init__
    from .core import Core as _pluginCls
  File "e:\program files\deluge\deluge\plugins\execute\deluge_execute\core.py", line 14, in <module>
    from twisted.internet.utils import getProcessOutputAndValue
ModuleNotFoundError: No module named 'twisted.internet.utils'

Answer: 3aec79f

  • Check for compatibility with newer python versions.
  • Twisted error: [9660][twisted/internet/_glibbase] Fix Typeerror in simulate for py3 twisted/twisted#1679
    Answer: e8874aa
  • Edit path for openssl? Why?
    Answer: libtorrent + pyinstaller requires a lib(ssl/crypto)-1_1.dll and lib(ssl/crypto)-1_1-x64.dll odd quirk but solveable by just having two copies. Maybe later compiling our own libtorrent.
  • vc-redist check
    Answer: Not needed libtorrent isn't build static and pyinstaller grabs the redist files.
  • With UI/Plugin files copied to data, do not need the deluge folder.
    Answer: Not needed but comes from collect_all('deluge'):hiddenimports which is needed.

@doadin doadin force-pushed the develop branch 4 times, most recently from 3588671 to 246f305 Compare January 4, 2022 06:15
@DjLegolas
Copy link
Contributor

Looks great, thanks.
If you can, please add a job to the CI for automatic creating the installer. It will need to have the ability to compile everything it needs (GTK3, OpenSSL, etc.).

@doadin
Copy link
Contributor Author

doadin commented Jan 4, 2022

@DjLegolas
I'm working on that atm. The hardest part kind of is compiling GTK. Others have public windows builds. Could use a build host for this but artifacts don't last forever. And GTK takes quite a bit of time to compile.

@doadin
Copy link
Contributor Author

doadin commented Jan 4, 2022

@DjLegolas
The best option would probably be to have a build of GTK hosted somewhere then use that plus the already available builds of others eg. just pip install libtorrent and use prebuilt openssl from the ci host, as most things are available or easy to get like with pip, GTK is the only thing really hard to get.

@cas--
Copy link
Member

cas-- commented Jan 4, 2022

@doadin I agree if we can start simple with dependencies the better, where needed we can modify it later to suit. For GTK we could fork the gvsbuild repo and create releases or store them on deluge ftp, either way we want a simple reproducible process.

@doadin
Copy link
Contributor Author

doadin commented Jan 4, 2022

@cas-- Yes I plan on on making an CD github actions, since that seems to be what we are using for other things, that will making an installer using prebuilts from github actions image, which the only thing I see needed out side of that is GTK ill make the script to use from appveyor since thats where im currently setting up a build, should be a easy change in url to a different host depending on where you would like to get it from.

@doadin
Copy link
Contributor Author

doadin commented Jan 4, 2022

@DjLegolas @cas--
It seems that github actions uses powershell to run so I added a sample CD.yml based on that which should only need a correction for the URL where we want to get GTK from. The deluge spec file already has path setup to be able to find openssl and such so unless the wheel of libtorrent from pip requires something else should be able to find all the deps deluge needs.

Edit: also the path to gtk may need editing all depends on how we get gtk and how the zip for it looks and is extracted and so on. Again though should be a fairly easy edit of the spec file to make any correction for how the build environment is setup. Appveyor and github actions (maybe others) seem to put openssl and such in the same location so should be pretty easy to make work any where really.

Also I made the sample only build on pushed tags so any time you want to make a release should be as simple as adding a tag. We could maybe build for PRs too or something but you guys can decide that.

@doadin doadin force-pushed the develop branch 3 times, most recently from 884c8e7 to 8b6bebf Compare January 4, 2022 15:03
.github/workflows/cd.yml Outdated Show resolved Hide resolved
@DjLegolas
Copy link
Contributor

BTW, great work @doadin 🥇

packaging/win/delugensis.py Outdated Show resolved Hide resolved
@doadin doadin force-pushed the develop branch 2 times, most recently from d7b72e3 to ed4e3c8 Compare January 5, 2022 01:40
@doadin
Copy link
Contributor Author

doadin commented Jan 5, 2022

@DjLegolas @cas-- I think I have addressed the points you guys have brought up, still uncertain about nsis use again I'm not very familiar but I think other than it being cleaner its at least close to proper syntax?

@doadin doadin force-pushed the develop branch 9 times, most recently from de7916e to b250a12 Compare January 5, 2022 02:33
@doadin
Copy link
Contributor Author

doadin commented Jan 13, 2022

@cas-- I combined some of the commits that aimed to do the same thing so its done in 5 commits now vs 12 so its still seperate to see the process that happened to implement fixes, idk if you like this better or not or want it reduced even more? Or maybe this is worse? I saved a copy of how it was in a branch on my fork if you don't like it like this.

@doadin doadin force-pushed the develop branch 2 times, most recently from 664b790 to e1cbb6a Compare January 13, 2022 04:45
* Rename instances of win32 to generic win or the appropriate bit where applicable
* Remove files used in GTK2
* Add spec file for use with PyInstaller
* Remove Python bbfreeze Script
* Add Github Action To Build Releases
* Add Modified script to make files used by NSIS
* Update Readme
@doadin
Copy link
Contributor Author

doadin commented Jan 13, 2022

the only thing not separate is the process we went through to be able to go from py3.7 to newer other than that its still all separate so the process can be seen, just in fewer steps instead of going back and forth and having commits just changing libtorrent versions.(things I think didn't need to be separate got squished) Again I have a copy of how it was before though if this is not better.

@DjLegolas
Copy link
Contributor

@doadin @mhertz first of all, great work guys. The long waited installer is an important milestone for Deluge 2.0 🥇 💯

Regarding only the commits, I would suggest to add to the reason for the patched twisted to the commit message because without it, one would never know why it was needed.

And again, thanks for this (>130 comments 🤣) MR!

Comment on lines +7 to +9
pull_request:
branches:
- develop
Copy link
Contributor

Choose a reason for hiding this comment

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

Needs to be dropped

Copy link
Contributor

@CirnoT CirnoT left a comment

Choose a reason for hiding this comment

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

Few changes proposed. Overall okay and is stable and working. See my further comment for additional details on possible issues however.


# Add Deluge Hidden Imports
tmp_ret = collect_all('deluge')
hiddenimports += tmp_ret[2]
Copy link
Contributor

Choose a reason for hiding this comment

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

- tmp_ret = collect_all('deluge')
- hiddenimports += tmp_ret[2]
+ hiddenimports += collect_all('deluge')[2]

tmp_ret = collect_all('deluge')
hiddenimports += tmp_ret[2]

#Add Hidden Imports for Plugins
Copy link
Contributor

Choose a reason for hiding this comment

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

- #Add Hidden Imports for Plugins
+ # Add Hidden Imports for Plugins
- tmp_ret2 = collect_all('twisted')
- hiddenimports += tmp_ret2[2]
+ hiddenimports += collect_all('twisted')[2]

packaging/win/delugewin.spec Show resolved Hide resolved
# -*- mode: python ; coding: utf-8 -*-
import os
import sys
import deluge.common
Copy link
Contributor

Choose a reason for hiding this comment

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

Only used by get_version which itself is unused.

- import deluge.common

pip install $pycairopath
pip install $PyGObjectpath
pip install https://github.com/doadin/twisted/releases/download/latest/Twisted-21.7.0.post0-py3-none-any.whl
python -m pip install libtorrent==${{ matrix.libtorrent }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Any specific reason why rest of file uses pip while this line uses python -m pip? Note that pip might reference different binary than module loaded by python -m.

pip install https://github.com/doadin/twisted/releases/download/latest/Twisted-21.7.0.post0-py3-none-any.whl
python -m pip install libtorrent==${{ matrix.libtorrent }}
pip install -r requirements.txt
pip install pyinstaller
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason this is done separately from libtorrent?

$PyGObjectpath = Get-Childitem –Path "C:\GTK\release\python\" -Include PyGObject*.whl -File -Recurse -ErrorAction SilentlyContinue | select -expand FullName
pip install $pycairopath
pip install $PyGObjectpath
pip install https://github.com/doadin/twisted/releases/download/latest/Twisted-21.7.0.post0-py3-none-any.whl
Copy link
Contributor

Choose a reason for hiding this comment

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

While I have nothing against it by itself, this is introducing dependency on binary release that is outside of Deluge control. We should patch it ourselves or keep patched repository under deluge-torrent organization.

- name: Fix OpenSSL For Libtorrent
if: ${{ matrix.arch == 'x64' }}
run: |
Copy-Item -Path $env:GITHUB_WORKSPACE\packaging\win\freeze\Deluge\libssl-1_1.dll -Destination $env:GITHUB_WORKSPACE\packaging\win\freeze\Deluge\libssl-1_1-x64.dll
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you checked if this is necessary on GH instance? A quick run from 64-bit Windows shows that there are no libssl-1_1.dll files produced when running with 64-bit Python, only x64 variants. Perhaps GH x64 arch uses 32-bit Python?

block_cipher = None


a = Analysis([os.path.abspath(os.path.join(HOMEPATH,os.pardir,os.pardir)) + '\Scripts\deluge-console-script.py'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This section looks good however it could be cleaner if we put the executable generation in a loop, so that any changes need to be done only once, not 6 times.

@CirnoT
Copy link
Contributor

CirnoT commented Jan 13, 2022

I have not checked the version that adds hiddenimports from Twisted, however can we be reasonable sure that plugins such as YaRSS2 will work? For example, YaRSS2 for v2 depends on http.cookies via requests and email.mime.*. Both of these are part of stdlib for Python and are presumed to be always available, however pyinstaller does not treat them as such and produced binaries might not have necessary modules available.

This was an issue with v1 too, back then YaRSS2 had its own custom code for email.mime.* part for Windows users, however since release of v2 it started to depend on various 3rd-party modules such as requests, which bring their own dependencies that plugin does not necessarily control. To my knowledge they've done best job they could trying to package these external dependencies into egg they provide, however of course Python's stdlib is out of their control.


The other issue that remains is how broken GTK3 is on Windows. Seeing some weird issues where clicking Deluge icon on taskbar does bring window to front but doing so again does not minimize it as one would expect. Using GTK_CSD=0 is a solution to this (and many more). We should of course never modify user's environment globally when installing Deluge, so the question remains whether there is some way we can have Deluge disable decorations when running on Windows without relying on environment variables.


In addition, do we want to also rebase this PR for 2.0.5 so that Windows users can have a stable release? 2.1 introduces some breaking Python2 related changes that will again require attention from some plugin developers so we might want to port back some of the fixes that @cas-- pushed to 2.0.5, make it a 2.0.6 release and push a Windows installer with it, while work on 2.1 continues. For reference, even without these fixes 2.0.5 is very much usable using this PR, however I only tested it as thin-client so YMMV.

@mhertz
Copy link
Contributor

mhertz commented Jan 13, 2022

@CirnoT, I specified e.g gtk_csd=0 other place before, but my understanding was that it wasent global and only enherented for running proces, and not transferred. Wrong?

Edit: Always been somewhat leery though, as stated to possibly have a performance penalty, didnt notice any though, but hard to judge, when only quick testing.

@cas--
Copy link
Member

cas-- commented Jan 13, 2022

@CirnoT Thanks for suggestions 👍

I know that for the dependency issues (e.g. email.mime) were manual fixed in bbfreeze and py2app:

includes = glib, gio, cairo, pango, pangocairo, atk, gobject, gtk.keysyms,


Can you more specific about Python2 breaking changes? I probably will have to create a 2.0.x release branch anyway so certainly possible to release 2.0.5/6 for Windows.


Using GTK_CSD=1 is a solution to this

Did you mean GTK_CSD=0? This is only a env var. Perhaps could be done in runtime-hook?

@CirnoT
Copy link
Contributor

CirnoT commented Jan 13, 2022

Did you mean GTK_CSD=0? This is only a env var. Perhaps could be done in runtime-hook?

Yes. If its possible to set environment variable for executable via runtime-hook in pyinstaller than that would be solution.

Edit: Always been somewhat leery though, as stated to possibly have a performance penalty, didnt notice any though, but hard to judge, when only quick testing.

Less performance on drawing items is in my opinion preferable to broken interaction with Windows DWM.

but my understanding was that it wasent global and only enherented for running proces, and not transferred. Wrong?

I am not sure where you set it. Environment variables are usually either set when running process or globally for user/computer (in which case it would apply to everything). If as mentioned above runtime-hook can do it for pyinstaller then it would indeed be isolated.

Can you more specific about Python2 breaking changes? I probably will have to create a 2.0.x release branch anyway so certainly possible to release 2.0.5/6 for Windows.

Some plugins (in this example YaRSS2) still retain some level of compatibility for Python 2, even on releases that are meant for Deluge 2. See https://bitbucket.org/bendikro/deluge-yarss-plugin/issues/67/deluge-210-removed-all-py2-support for details. The common offenders will most likely be PY2 (from deluge.common) and six (if plugin decided to use it, but unlikely).

Perhaps we could keep PY2 as a dumb stub (PY2 = false) to make plugins happy?

@CirnoT
Copy link
Contributor

CirnoT commented Jan 13, 2022

I know that for the dependency issues (e.g. email.mime) were manual fixed in bbfreeze and py2app:

Ideally we would package all standard modules, so that there is no difference between available environment for Windows and Linux. Python 3.10 would make it easy with sys.stdlib_module_names, however as we need to target lower versions, we'd need to come up with some small hook for pyinstaller that enumerates available modules (some options are on https://stackoverflow.com/questions/6463918/how-can-i-get-a-list-of-all-the-python-standard-library-modules). We could also produce a static list for versions below 3.10 (and use sys.stdlib_module_names from 3.10 onwards) and provide that as hidden imports directly.

For reference, here is full list of available modules for Python versions between 3.7 and 3.9 (note that some of them will not exist depending on version used, for example 'graphlib', 'zoneinfo' were introduced in 3.9 while 'macpath' was removed in 3.8):

['pickletools', 'argparse', 'gettext', 'turtledemo', 'runpy', 'copy', 'pydoc', 'dbm', 'ssl', 'struct', 'glob', 'subprocess', 'contextvars', 'trace', 'mailbox', 'winreg', 'concurrent', 'sunau', '__main__', 'encodings', 'random', 'weakref', 'code', 'os', 'fcntl', 'tabnanny', 'shutil', 'winsound', 'profile', 'gc', 'bz2', 'statistics', 'parser', 'fnmatch', 'chunk', 'tkinter', 'fractions', 'bdb', 'compileall', 'sqlite3', 'itertools', 'tempfile', 'traceback', 'curses', 'bisect', 'uu', 'csv', 'textwrap', 'formatter', 'smtplib', '_dummy_thread', 'calendar', 'xml', 'codecs', 'imghdr', '_thread', 'nis', 'sndhdr', 'urllib', 'multiprocessing', 'zoneinfo', 'decimal', 'gzip', 'mimetypes', 'timeit', 'math', 'warnings', 'cProfile', 'io', 'socketserver', 'socket', 'types', 'ast', 'wave', 'operator', 'signal', 'zipapp', 'linecache', 'builtins', 'doctest', 'tokenize', 'netrc', 'hashlib', 'audioop', 'binhex', 'aifc', 'configparser', 'pty', 'quopri', 'cmd', 'marshal', 'poplib', 'xdrlib', 'telnetlib', 'tracemalloc', 'site', 'ctypes', 'select', 'time', 'asyncio', 'nntplib', 'pickle', 'typing', 'asynchat', 'smtpd', 'cgi', 'tarfile', 'atexit', 'dataclasses', 'optparse', 'pwd', 'lzma', 'py_compile', 'syslog', 'pyclbr', '__future__', 'test', 'ossaudiodev', 'html', 'pipes', 'graphlib', 'dummy_threading', 'contextlib', 'heapq', 'http', 'copyreg', 'plistlib', 'fileinput', 'logging', 'numbers', 'ensurepip', 'distutils', 'xmlrpc', 'datetime', 'webbrowser', 'binascii', 'imaplib', 'grp', 'hmac', 'locale', 'queue', 're', 'getopt', 'sched', 'stat', 'stringprep', 'zipfile', 'cmath', 'termios', 'zipimport', 'sysconfig', 'tty', 'spwd', 'difflib', 'rlcompleter', 'symtable', 'token', 'turtle', 'filecmp', 'secrets', 'venv', 'msilib', 'unittest', 'keyword', 'shelve', 'codeop', 'symbol', 'email', 'modulefinder', 'reprlib', 'mailcap', 'pprint', 'readline', 'unicodedata', 'posix', 'selectors', 'json', 'colorsys', 'pathlib', 'pstats', 'sys', 'pkgutil', 'crypt', 'ftplib', 'lib2to3', 'shlex', 'array', 'uuid', 'errno', 'getpass', 'abc', 'dis', 'base64', 'mmap', 'wsgiref', 'inspect', 'functools', 'ipaddress', 'resource', 'threading', 'faulthandler', 'imp', 'asyncore', 'cgitb', 'string', 'platform', 'msvcrt', 'zlib', 'enum', 'collections', 'pdb', 'macpath', 'importlib']

Providing it as-is to hidden imports in pyinstaller SHOULD be fine, it silently skips over imports it can't enumerate.

@mhertz
Copy link
Contributor

mhertz commented Jan 13, 2022

Very quick, as not liking intrude on your important things, but just remembered that I otherplace added requests(not stdlib) and pygame, from pip, when installing deluge from pip, so springjools's autoremoveplus fork can even be enabled/used(don't know if relevant to you), and notifications default plugin works for playing sound. Don't need to respond. Thanks for everything you good people do :)

@CirnoT
Copy link
Contributor

CirnoT commented Jan 13, 2022

added requests(not stdlib) and pygame, from pip, when installing deluge from pip, so springjools's autoremoveplus fork can even be enabled/used(don't know if relevant to you)

Appreciated to know, however non-stdlib requirements are something plugin developers must solve themselves, otherwise users are forced to take manual steps (like you did), so this is not something we can accommodate when packaging Windows version. For example, YaRSS2 depends on requests too but they do vendor it themselves.

@mhertz
Copy link
Contributor

mhertz commented Jan 13, 2022

Wanted to let doadin answer, but as I had tested it anyways, then maybe save him some time... With CirnoT hiddenimports list added to spec, and

tmp_ret3 = collect_all('email')
datas += tmp_ret3[0]; binaries += tmp_ret3[1]; hiddenimports += tmp_ret3[2]

Then yarss2 still fails because of that python2 support removed, so as not sure how add py2 dumb stub as suggested, then just tested deluge master branch, and worked with above, sorry for not cutting down propperly the collect_all.

Edit: Sorry, don't use yarss2, and just saw it was possible to enable yarss2 now and the plugin sub-menu came through in gtkui propperly.

@cas--
Copy link
Member

cas-- commented Jan 13, 2022

Perhaps we could keep PY2 as a dumb stub (PY2 = false) to make plugins happy?

Done via 6da4c4b

@cas-- cas-- closed this in b9a208f Jan 13, 2022
@cas--
Copy link
Member

cas-- commented Jan 13, 2022

@doadin So I am happy to merge this just now and we can work on cleaning it up further in new PRs

Thank you again for all the work in getting to this stage!

These are some the issues that have come up in and still need addressed:

  • Cleanup comments by @CirnoT
  • email.mime deps
  • Include all stdlib
  • Include pygame for notification plugin
  • Set GTK_CSD=0 in runtime hook
  • Host patched Twisted binary under Deluge

@cas--
Copy link
Member

cas-- commented Jan 13, 2022

Btw the job will not run by default on PRs but I added a condition if labelled with 'windows' so can try that out to see if it works...

@DjLegolas
Copy link
Contributor

About the stdlib, why not use the Windows embeddable package available from python?
It exists exactly for this purpose.

@doadin
Copy link
Contributor Author

doadin commented Jan 13, 2022

@cas-- Sorry, I just woke up was going to make a commit with some of the requested changes, was just commenting and saw you merged.

@CirnoT
Copy link
Contributor

CirnoT commented Jan 13, 2022

About the stdlib, why not use the Windows embeddable package available from python?
It exists exactly for this purpose.

That is not compatible with pyinstaller, or are you suggesting that we should abandon pyinstaller and create our own stub executables using embeddable package?

cas-- added a commit to cas--/Deluge that referenced this pull request Jan 21, 2022
CirnoT reported how they felt that GTK3 is not reliable on Windows.
Seeing some weird issues where clicking Deluge icon on taskbar does
bring window to front but doing so again does not minimize it as one
would expect. By using GTK_CSD=0 this would reduce these problems.

> If changed to 0, this disables the default use of client-side
decorations on GTK windows, thus making the window manager responsible
for drawing the decorations of windows that do not have a custom
titlebar widget.

This can be overridden by a global env var.

Ref: deluge-torrent#331 (comment)
Ref: https://docs.gtk.org/gtk3/running.html
cas-- added a commit that referenced this pull request Jan 21, 2022
CirnoT reported how they felt that GTK3 is not reliable on Windows.
Seeing some weird issues where clicking Deluge icon on taskbar does
bring window to front but doing so again does not minimize it as one
would expect. By using GTK_CSD=0 this would reduce these problems.

> If changed to 0, this disables the default use of client-side
decorations on GTK windows, thus making the window manager responsible
for drawing the decorations of windows that do not have a custom
titlebar widget.

This can be overridden by a global env var.

Ref: #331 (comment)
Ref: https://docs.gtk.org/gtk3/running.html
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
6 participants