-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
gi/gtk3reactor fail to import on Windows: NameError: name 'fdesc' is not defined #11987
Comments
Thanks for the report.
I guess the first step is to see how to get GI/GTK available on Python on Windows on the GitHub Actions VMs |
Looking into how Deluge runs the tests I see
so maybe we can have something like this |
I tried making this commit but it fails with gi missing error. |
@doadin Thanks for a report, and sorry for the regression. Can you include a link to a draft pull request so the logs are visible? I can't see the error you're talking about. |
@glyph https://github.com/doadin/twisted/actions/runs/6162326017/job/16723577496?pr=3 is this what you mean? |
@doadin Yeah. The problem you're having there is that you're installing pycairo and PyGObject globally, then tox makes a fresh virtual environment for testing without pycairo and PyGObject installed. You need to smuggle the |
@glyph yea, i have no idea lol I think everything is in place to at least get the tox running EXCEPT getting those installed into tox env and idk how to do that. |
Try using the |
I think that we got the CI working https://github.com/twisted/twisted/actions/runs/6166274452/job/16735515393?pr=11988#step:11:14884 it's a hack , but for now it works Since gi reactor is broken, we can't start the tests with the The tests can be executed with |
@adiroiban I just put back the windows GTK build, which you seemed to have added and then removed. Executing the tests with the |
OK, I think that running the tests, now that it's not tripping on the |
I have create the PR just as a proof of concept for having gobjects installed. I saw that we have the ReactorBuilder for unit testing. But I guess that for the long term, it helps to run all the tests with the gi reactor (or the reactor) used by Deluge, to make sure we have more integration tests. Maybe the gi/gtk reactors are super simple, and they don't need a separate test run. The gi / gt reactor part can be solved by someone who is using that code and knows how it's expected to behave |
Actually, the long term goal of There is no legitimate difference in behavior between select and epoll and gtk that should matter to application code like Twisted Web so there should be no good reason to run the Twisted Web test suite against 9 different reactors. |
Agree. I am not sure if the iocp / cfreactor / gi unit tests are good enough. I am happy not to have to run all the Windows tests using the gi / gtk reactors. |
sorry, i have not been able to help, and I know there have been several changes but this still seems to be an issue. I know you guys were working on CI to help which is a complicated issue, an chance for just a fix for the original issue? :) |
Hi. The issue was reproduced using GitHub Actions as part of As far as I know, currently, there is no maintainer for Twisted GTK support. I don't use Twisted with GTK and I don't have time to look into this. I tried to help with the CI / build / automated testing part. We need someone with experience with GTK to help fix this issue. Cheers |
@adiroiban well, I know fdesc was imported in the last working version, and it didn't seam to cause any error, I think the simplest "fix" would be to just revert the change to not import fdesc in windows, however I don't know why it was changed to not import on windows in the first place. Essentially it seems who ever made that change created this regression I guess not manually testing in windows, and as you mentioned only just recently was CI worked on for GTK to see this regression without manually testing. |
@adiroiban @exarkun |
@adiroiban @exarkun It looks like https://github.com/twisted/twisted/blob/trunk/src/twisted/internet/fdesc.py#L42 though fdesc is made for posix, it is windows compatible or has 1 of 2 functions used in _signals, I don't know if something in fdesc changed that broke windows compatibility or what. |
Sorry for the trouble. That work was part of a larger effort to dramatically improve the internal factoring of the reactors in Twisted but my motivation to complete the work was crushed by the vagaries of how macOS applications work and my lack of access to a macOS development environment. In retrospect, I should have just built a whole new set of reactors from scratch instead of trying to fix the existing ones in-place. Regardless, I'm not in a position to pick up any of this work again in the foreseeable future. |
That change should not be reverted. If you revert it whole Windows support breaks. As a test, I have made that change in #11988 PR and you can see the test failing |
I'm happy to look at GTK fixes, I have a Linux/GTK development environment and I care about eventually making apps that use it.
Edit: oops, Github was hiding some comments from me. I see we do have some CI in place in a PR, yay! |
@adiroiban |
@adiroiban sorry, I was mistaken. The code has unfortunatly changed a LOT since this regression. |
Describe the incorrect behavior you saw
A clear and concise description of what the bug is.
13c5e3a seems related this change doesnt replace use of fdesc later in code but gates the import under "posix"
Describe how to cause this behavior
What did you do to get it to happen?
Does it happen every time you follow these steps, sometimes, or only one time?
On Windows
from twisted.internet import defer, gtk3reactor
reactor = gtk3reactor.install()
Preferable a Short, Self Contained, Correct (Compilable), Example on a branch or on a gist.
Automated tests that are demonstrating the failure would be awesome.
Describe the correct behavior you'd like to see
A clear and concise description of what you expected to happen, or what you believe should be happening instead.
Testing environment
uname -a ; cat /etc/lsb-release
systeminfo | Findstr /i "OS"
sw_vers
twist --version
andpip --freeze
OS Name: Microsoft Windows 11 Pro
OS Version: 10.0.22621 N/A Build 22621
OS Manufacturer: Microsoft Corporation
OS Configuration: Standalone Workstation
OS Build Type: Multiprocessor Free
Twisted 22.10+ (<22.10 works)
Reactor gi/gtk3reactor
Additional context
Add any other context about the problem here.
The text was updated successfully, but these errors were encountered: