Thanks for the report.
I see that we try to run the tests, but they are skipped

GObject Introspection `gi` module not importable

twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_cantRegisterAfterRun
twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_cantRegisterTwice
twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_gApplicationActivate
twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_gtkAliases
twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_gtkApplicationActivate
twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_noQuit
twisted.internet.test.test_gireactor.GApplicationRegistrationTests.test_portable
===============================================================================
[SKIPPED]
gi reactor not available

twisted.internet.test.test_glibbase.GlibReactorBaseTests.test_simulate

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

https://github.com/deluge-torrent/deluge/blob/a459e78268ae8002a9cfc8caf6d410fa45b1d805/.github/workflows/cd.yml#L54-L62

      - name: Install GTK
        run: |
          $WebClient = New-Object System.Net.WebClient
          $WebClient.DownloadFile("https://github.com/deluge-torrent/gvsbuild-release/releases/download/latest/gvsbuild-py${{ matrix.python }}-vs16-${{ matrix.arch }}.zip","C:\GTK.zip")
          7z x C:\GTK.zip -oc:\GTK
          echo "C:\GTK\release\lib" | Out-File -FilePath $env:GITHUB_PATH -Append
          echo "C:\GTK\release\bin" | Out-File -FilePath $env:GITHUB_PATH -Append
          echo "C:\GTK\release" | Out-File -FilePath $env:GITHUB_PATH -Append
          python -m pip install --no-index --find-links="C:\GTK\release\python" pycairo PyGObject

so maybe we can have something like this

I tried making this commit but it fails with gi missing error.
6d1d919

@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 --find-links= option down to the pip invoked by tox somehow. I don't know how to do this off the top of my head, but if you can inject the wheels into the place that's actually doing the testing, you should be good.

@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.

@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 PIP_FIND_LINKS environment variable: https://pip.pypa.io/en/stable/topics/configuration/#environment-variables

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 gi reactor.

The tests can be executed with select reactor and the gi unit tests are also execute and they fail

@adiroiban I just put back the windows GTK build, which you seemed to have added and then removed. Executing the tests with the gi reactor is a bit of a red herring, the GTK tests run with ReactorBuilder, which doesn't care which reactor Trial is running.

OK, I think that running the tests, now that it's not tripping on the fdesc import error, I think it's segfaulting, or doing the windows equivalent. Not sure where the mismatch is, and my first attempt for tmate debugging failed, so I'll have to look a different day

I have create the PR just as a proof of concept for having gobjects installed.
I have never used gi / gtk reactors , so I don't know how they work.
I am just trying to solve the CI part here.

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.
I know that for Windows, we do a separate run for the iocp reactor.
This is something that was migrated from the old Buildbot configuration.

The gi / gt reactor part can be solved by someone who is using that code and knows how it's expected to behave

I have create the PR just as a proof of concept for having gobjects installed. I have never used gi / gtk reactors , so I don't know how they work. I am just trying to solve the CI part here.

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.

Actually, the long term goal of ReactorBuilder was for Twisted to not need to use trial --reactor ... as part of its CI at all. If there are important behaviors of reactors, they should be tested by a ReactorBuilder-based test. Then they're run against all the reactors and as part of every normal run (so long as the dependencies are satisfied).

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.

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.
With that in mind, running all the tests can help and act as a failsafe.

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
#11988

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
looks like there was a change here 13c5e3a to "Make _signals importable on Windows" but this change broke part of the code for signal as the import was removed but not refferences to the import.

@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.

@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.

looks like there was a change here 13c5e3a to "Make _signals importable on Windows" but this change broke part of the code for signal as the import was removed but not refferences to the import.

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

As far as I know, currently, there is no maintainer for Twisted GTK support.

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.

In principle I'd love to have better support for Windows & GTK as well, but the PR to set up that test environment within github actions is pretty challenging.

I think I do have a Windows/PyGTK development environment as well, and if someone can put together the fix that makes it importable again, I can manually validate it, but we are likely to have this type of regression again unless someone can figure out the automated incantation to get it into the build.

Edit: oops, Github was hiding some comments from me. I see we do have some CI in place in a PR, yay!

@adiroiban reapAllProcesses is our only process dependency, which is not necessary on Windows, so I've conditional-ized the import in your branch. Perhaps that will move us a bit further in the direction of a solution.

@adiroiban sorry, I was mistaken. The code has unfortunatly changed a LOT since this regression.