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

Fix Errors Due to Deps #433

Closed
wants to merge 2 commits into from
Closed

Conversation

doadin
Copy link
Contributor

@doadin doadin commented Sep 9, 2023

No description provided.

@doadin
Copy link
Contributor Author

doadin commented Sep 9, 2023

The pillow change fixes pip trying to compile pillow from source but is missing zlib headers(maybe others), you could set these up on your system/CI/CD or 9.5.0 works. They also recommend using the wheel if you don't want setup the headers.

The change to pyinstaller fixes a typelib error in freeze on windows saying freetype 2.0 is missing.

@doadin doadin changed the title Fix Building On Windows Fix Errors Due to Deps Sep 9, 2023
@doadin
Copy link
Contributor Author

doadin commented Sep 9, 2023

I don't know if you can do both greater and less then in requirements but also not sure that it matters in this case as I think the versions of the deps are old enough that versions older are not likely to be used with the versions of python deluge supports, but idk.

@doadin
Copy link
Contributor Author

doadin commented Sep 9, 2023

So I thought zope.interface change fixes python 3.7 but apparently not test still pass on 3.10, so I guess this is a python 3.7 issue?

The other two deps changes I believe are valid.

@mhertz
Copy link
Contributor

mhertz commented Sep 10, 2023

It's same issue also for py3.9(zope.interface), and I just reverted pyopenssl and cryptography to earlier versions(22.0.0/37.0.4) for quick fix(trial/error) when I ran into it last week.

Anyway, I looked little into it now, and don't have more time now, but found seemingly for some reason an older setuptools(47.1.0) is installed for python's under 3.10(tested 3.7, 3.9 and 3.10), and when upgrading setuptools afterwards to newest, then works. Probably need pin setuptools to 68.0.0, as latter two versions are py3.8 and up.

Also, in requirements.txt, then I believe twisted should be pinned to 22.10(or below), like in CI, as else: "Nameerror fdesc is not defined", when newest twisted installed.

Thanks! :)

Edit: Setuptools version variances because included already(as dep) with pip/python.

Edit2(last): In quick test here(not CI though), then pyinstaller 4.10 doesn't fail(py3.9.8), though have as always displayed a bunch missing typelib errors, originating from one of the hooks being buggy, though not important for us, as a misunderstanding those messages. Pyinstaller 5 broke freezing completely(hence 4.10 pin added), as respected those errors instead of just forward/log and ignore, but they gracefully fixed it for us in next version(maybe next again, can't remember, and I made PR to update pyinstaller and drop 4.10 pin, but I closed it later, and Cas thought not important to update at the time). Sorry if misunderstanding and 4.10 has actual error blocking on CI now and not just cosmetic, or that you already knew these things. Granted might as well update pyinstaller as well, and is confusing to end-users looking at, and just elaborating. Sorry for wordyness, and butting in - please let me know if I can help with something, and i'll but out again now.

@doadin
Copy link
Contributor Author

doadin commented Sep 12, 2023

seems related: zopefoundation/zope.interface#255

@doadin
Copy link
Contributor Author

doadin commented Sep 12, 2023

@mhertz the pyinstaller issue is when running deluge after packaging not in the packaging process sorry I was not clear.

@mhertz
Copy link
Contributor

mhertz commented Sep 13, 2023

seems related: zopefoundation/zope.interface#255

Good catch, yeah must've been fixed here presumably then: pypa/setuptools#3153 for setuptools 62. Just strange because first thing I tried myself was mess with that(change install name), to no avail, and see requirements.txt and setup.py has the canonical name regardless, but whatever.

I guess the setuptools change should be added little up too in CI file, as still old version in log.

Sorry didn't ment to butt in here again, unless called, but will adhere to now promise ;) Thin line to walk for guessing if beeing perceived helpful VS annoying you know :) Thanks as said.

@doadin doadin force-pushed the up-dep branch 4 times, most recently from 1148931 to 195447f Compare September 13, 2023 13:34
3.7 is eol 3.8 is oldest still in support 3.10 is newest libtorrent provides wheels for.
@doadin
Copy link
Contributor Author

doadin commented Sep 13, 2023

@mhertz @cas-- I think this is all the changes I am going to put forth. The team over at twisted are looking into the issue with v23 and looking to improve their CI to prevent issues in the future. For now v22.8 is latest working.

@cas-- cas-- closed this in 7082d9c Sep 18, 2023
@cas--
Copy link
Member

cas-- commented Sep 18, 2023

Thanks for the work identifying the build issues, I prefer that we identify and resolve each issue separately to properly understand the issue that is being fixed, as such I referenced your changes in new commits.

Just to note that the versions specified in requirements are usually there as a project minimum/maximum rather than for CI/CD and shouldn't be removed without checking the commit history. There are likely some improvements to how we handle the dependencies for project, packaging and CI/CD.

Also until we specifically drop support for Python 3.7 I still want the runners for it. The total number of Python versions to test against should also be small so we don't have to wait too long for them all to finish. There could be an improvement where we run a single Python version for the PR/tests and then run against all versions once merged to develop branch.

@doadin
Copy link
Contributor Author

doadin commented Sep 18, 2023

@cas-- Thank you for making the required changes. I am sorry though I might have not been clear maybe? Or I just didn't notice a reference to it in your commits. However the change for pillow is not just for compatibility with python versions but there is also missing header files for compiling from source.(and part of why move to wheel) So maybe a comment in a commit mentioning zlib source headers missing from pillow source code for compiling pillow from source? idk. Again sorry either way if you did or didn't. And big thanks for making the changes!

@cas--
Copy link
Member

cas-- commented Sep 18, 2023

Ah yeah I did find the other Pillow issue which is isolated to Windows 32-bit builds which as you say is due to missing wheels and trying build from source: 6c9b058

doadin pushed a commit to doadin/deluge that referenced this pull request Jun 4, 2024
The latest Pillow 10 does not support Py3.7 therefore wheels are no
longer available and we need to specify previous major version.

Older versions of setuptools do not correctly determine the Twisted
requirement for zope.interface>5 on Python 3.7 so ensure latest
installed. For the CD builds we don't want any surprises so keep the
setuptools version pinned.

Refs: https://pillow.readthedocs.io/en/stable/installation.html
Closes: deluge-torrent#433
doadin pushed a commit to doadin/deluge that referenced this pull request Sep 21, 2024
The latest Pillow 10 does not support Py3.7 therefore wheels are no
longer available and we need to specify previous major version.

Older versions of setuptools do not correctly determine the Twisted
requirement for zope.interface>5 on Python 3.7 so ensure latest
installed. For the CD builds we don't want any surprises so keep the
setuptools version pinned.

Refs: https://pillow.readthedocs.io/en/stable/installation.html
Closes: deluge-torrent#433
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.

3 participants