Ci fixes and updates #454

Lord-Kamina · 2024-06-28T14:51:37Z

This branch updates the CI stack a bit in different ways.

It updates the workflow version of several actions that were crying due to Node 16 actions being deprecated.
It adds builds newer versions of python, so now it's built on 3.7, 3.9 and 3.10 (not using newer versions due to unavailability of newer libtorrent wheels on pypi)
Uses an updated gvsbuild-release for newer python versions.
Fixes console tests on test_ui_entry.py hanging, which allows us to unpin the pytest version used.

Notes:

Point 3 above makes this PR dependent on Update gvsbuild stack and allow releases using both a newer stack, as well as using the latest versions that still support python 3.7. gvsbuild-release#3
I updated libtorrent but we're still currently pinned to <2.0.9. This is because of point 2 above, as well as the fact that a change in libtorrent 2.0.9 makes one of the deluge tests fail (this should be addressed by fixing/removing that functionality or removing that test, I guess)

cas-- · 2024-08-26T19:52:20Z

I'm afraid there are too many changes here and not enough details in the commits about why the change is required. If possible, can you split these up into more concise PRs it will help reviewing and merging

Lord-Kamina · 2024-09-02T14:51:51Z

I'm afraid there are too many changes here and not enough details in the commits about why the change is required. If possible, can you split these up into more concise PRs it will help reviewing and merging

I hadn't seen this comment, sorry for not answering before.

I'd rather not split it (although it IS true that there are some not-strictly related things here) because then we'll never get it merged. However, I'm more than happy to amend commit messages and/or explain why I've made each change.

Lord-Kamina · 2024-09-02T14:54:26Z

deluge/ui/gtk3/gtkui.py

 from twisted.internet.error import ReactorAlreadyInstalledError
 from twisted.internet.task import LoopingCall

 try:
    # Install twisted reactor, before any other modules import reactor.
-    reactor = gtk3reactor.install()
+    reactor = gireactor.install()


This is just to avoid using a deprecated class name.

We support a minimum version of Twisted and afaik this would break that so needs to be reverted, I don't think they are actually removing it anytime soon. We could add a Twisted version check for the import but would need a comment to explain why gireactor is being used since the Twisted docs are now contradictory about not using it for UI.

Does it though? the current requirement spec is:

twisted[tls]>=17.1; sys_platform != 'win32' twisted[tls]<23,>=17.1; sys_platform == 'win32'

And the Twisted api documentation for 16.4.1, a full major version earlier (https://twisted.org/documents/16.4.1/api/twisted.internet.gireactor.html), already has the gireactor, with an API that I think is identical?

Lord-Kamina · 2024-09-02T14:55:02Z

requirements-ci.txt

@@ -1,4 +1,4 @@
 -r requirements.txt
 -r requirements-tests.txt
-libtorrent==2.0.7
-pytest==7.4.2
+pytest


Currently, pytest is pinned due to console tests failing, which is fixed by this PR.

Lord-Kamina · 2024-09-02T14:55:47Z

requirements-ci.txt

-libtorrent==2.0.7
-pytest==7.4.2
+pytest
+libtorrent<2.0.9


libtorrent is pinned also due to a test failure, but the change was introduced in 2.0.9. We should eventually fix our test rather than keep pinning, but we can still update libtorrent even if just a bit.

These are pinned to avoid breaking the CI over time so can be version bumped but must still be pinned

These are pinned to avoid breaking the CI over time so can be version bumped but must still be pinned

For now, I'm happy to change it to libtorrent==2.0.8, but I think there's a discussion to be had about this (not in the context of this PR): I can understand the desire for the CI to not break easily, especially if you have little time as-is, but IMO a CI shouldn't easily break (if at all) due to a dependency publishing new bug-fix level releases.

You're pinned to a two-year old version of what is quite probably your most important dependency. There has to be a better way to approach it (I have a few different ideas, but I don't really know where is the best place to start a discussion such as this?)

Lord-Kamina · 2024-09-02T14:58:32Z

deluge/tests/test_web_api.py

@@ -76,7 +76,7 @@ def test_set_config(self):
        assert config['pwd_sha1'] != web_config['pwd_sha1']
        assert config['sessions'] != web_config['sessions']

-    @defer.inlineCallbacks
+    @pytest_twisted.inlineCallbacks


These changes are due to the pytest developers recommending that twisted not be used directly in tests with fixtures. From the pytest-twisted docs:

Using twisted.internet.defer.inlineCallbacks as a decorator for test functions, which use fixtures, does not work. Please use pytest_twisted.inlineCallbacks instead.

Lord-Kamina · 2024-09-02T14:59:29Z

deluge/tests/test_ui_entry.py

    def test_console_command_config_set_download_location(self):
        fd = StringFileDescriptor(sys.stdout)
        self.patch_arg_command(['config --set download_location /downloads'])
        self.patch(sys, 'stdout', fd)

-        yield self.exec_command()
+        with mock.patch('deluge.ui.console.main.ConsoleUI.quit', autospec=True):


This was calling the real ConsoleUI.quit(), which was stopping the reactor and thus making the tests go boom.

Lord-Kamina · 2024-09-02T15:00:47Z

deluge/core/alertmanager.py

@@ -174,7 +174,7 @@ def handle_alerts(self):
                d = task.deferLater(reactor, 0, handler, alert)
                on_handler_timeout = partial(
                    self.on_delayed_call_timeout,
-                    handler=handler.__qualname__,
+                    handler=handler.__class__.__qualname__,


This flat-out produces an error, because __qualname__ is an attribute that classes (not their instances) have.

When does it raise an error? Handlers are functions or methods not class instances so handle.__class__.__qualname__ does not make seem to make sense here?

When does it raise an error? Handlers are functions or methods not class instances so handle.__class__.__qualname__ does not make seem to make sense here?

See this log, for an example:

https://github.com/Lord-Kamina/deluge/actions/runs/10050873417/job/27779525108

EDIT (copied the error here for ease):

deluge/tests/test_alertmanager.py::TestAlertManager::test_pop_alert /opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/_pytest/threadexception.py:82: PytestUnhandledThreadExceptionWarning: Exception in thread alert-poller Traceback (most recent call last): File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/threading.py", line 1016, in _bootstrap_inner self.run() File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/threading.py", line 953, in run self._target(*self._args, **self._kwargs) File "/home/runner/work/deluge/deluge/deluge/core/alertmanager.py", line 93, in wait_for_alert_in_thread threads.blockingCallFromThread(reactor, self.maybe_handle_alerts) File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/twisted/internet/threads.py", line 135, in blockingCallFromThread result.raiseException() File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/twisted/python/failure.py", line 505, in raiseException raise self.value.with_traceback(self.tb) File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/site-packages/twisted/internet/defer.py", line 212, in maybeDeferred result = f(*args, **kwargs) File "/home/runner/work/deluge/deluge/deluge/core/alertmanager.py", line 113, in maybe_handle_alerts self.handle_alerts() File "/home/runner/work/deluge/deluge/deluge/core/alertmanager.py", line 177, in handle_alerts handler=handler.__qualname__, File "/opt/hostedtoolcache/Python/3.10.14/x64/lib/python3.10/unittest/mock.py", line 645, in __getattr__ raise AttributeError(name) AttributeError: __qualname__

Although now... reading that again, it might be an issue specific to running the tests.

@cas-- I have reverted that change and implemented a proper fix in the actual test now.

Lord-Kamina · 2024-09-04T19:57:14Z

@cas-- Using libtorrent==2.0.8fails because for some reason that version is missing from pip. I think that's why I had done <2.0.9

zakkarry · 2024-09-06T06:09:08Z

@cas-- Using libtorrent==2.0.8fails because for some reason that version is missing from pip. I think that's why I had done <2.0.9

The publishing to pip seems to be intermittent, I've made an issue on libtorrent a little over a week ago about pip builds, but I haven't seen any update. There is a open PR that seems to address some of the builds/CI for libtorrent, but I'm not sure if this directly address this concern.

You can see my issue and the linked PR here

DjLegolas · 2024-09-06T06:41:59Z

@zakkarry there were several issues with the pypi publish process for libtorrent...
i hope the fixes will work on the next release, as py3.13 is about to be released

@Lord-Kamina please revert the change to Install GTK step as it will always fail until we have a newer release for it.

zakkarry · 2024-09-06T06:44:13Z

@zakkarry there were several issues with the pypi publish process for libtorrent... i hope the fixes will work on the next release, as py3.13 is about to be released

@Lord-Kamina please revert the change to Install GTK step as it will always fail until we have a newer release for it.

As I've found out recently heh.

A user on the forums was asking about 3.13 support, and I told him to make an issue or PR, and he made #462 for it - it's failing some tests though it seems.

Lord-Kamina · 2024-09-06T11:26:50Z

@zakkarry there were several issues with the pypi publish process for libtorrent...

i hope the fixes will work on the next release, as py3.13 is about to be released

@Lord-Kamina please revert the change to Install GTK step as it will always fail until we have a newer release for it.

The change to gvsbuild-release was already commited, all that's missing is for the workflow to run, which is only on a manual trigger. However, that would break the current CI. The idea would be to run it just before this PR is ready to be merged.

Lord-Kamina · 2024-09-06T11:32:20Z

@cas-- Using libtorrent==2.0.8fails because for some reason that version is missing from pip. I think that's why I had done <2.0.9

The publishing to pip seems to be intermittent, I've made an issue on libtorrent a little over a week ago about pip builds, but I haven't seen any update. There is a open PR that seems to address some of the builds/CI for libtorrent, but I'm not sure if this directly address this concern.

You can see my issue and the linked PR here

I know and I was inquiring about it as well a while ago. Thing is, even if that's fixed, I doubt we'll get retroactive releases for an old version, so the point stands in that we need to be able to 1) update libtorrent and 2) have at least some CI workflow that fails anf alerts us when newer versions break, because else we're putting ourselves in the same position the folks at twisted did, which is essentially forgetting it's an issue.

Of course, I'm willing to help however I can but I have essentially no experience working with libtorrent so I'm not sure that really amounts to much.

What do you think @DjLegolas?

Lord-Kamina · 2024-09-08T00:43:04Z

@zakkarry there were several issues with the pypi publish process for libtorrent... i hope the fixes will work on the next release, as py3.13 is about to be released
@Lord-Kamina please revert the change to Install GTK step as it will always fail until we have a newer release for it.

As I've found out recently heh.

A user on the forums was asking about 3.13 support, and I told him to make an issue or PR, and he made #462 for it - it's failing some tests though it seems.

I don't think that's at all related?
What was merged (and still is not really reflecting anywhere) is a change in the way gvsbuild is built for newer releases to be used in Deluge.

zakkarry · 2024-09-08T00:45:48Z

I wasn't saying it was related necessarily, but you mentioned 3.13, so I informed you of a relevant PR to the version, that's all.

Clarified a little what this commit actually does.

Lord-Kamina · 2024-09-18T13:46:25Z

Closing this as per #466

zakkarry mentioned this pull request Jul 19, 2024

[CI] Update actions to latest version (node 16 deprecation) #453

Closed

Lord-Kamina force-pushed the ci_fixes branch 3 times, most recently from e1c41ad to dfabeb2 Compare July 23, 2024 01:08

Lord-Kamina force-pushed the ci_fixes branch 4 times, most recently from a0f2a10 to 264039a Compare August 24, 2024 20:59

Lord-Kamina force-pushed the ci_fixes branch 2 times, most recently from 1e247aa to 4e1acbe Compare September 2, 2024 14:44

Lord-Kamina commented Sep 2, 2024

View reviewed changes

Lord-Kamina force-pushed the ci_fixes branch 3 times, most recently from 76c9772 to a26d731 Compare September 8, 2024 00:34

Update GTK stack for use in CI/CD

ead0088

Clarified a little what this commit actually does.

Lord-Kamina added 2 commits September 9, 2024 11:39

Change gtk3reactor to gireactor

7a9d7c0

Fix console tests hanging so we can unpin pytest

8525b5f

Lord-Kamina force-pushed the ci_fixes branch from a26d731 to 8525b5f Compare September 9, 2024 14:40

Lord-Kamina mentioned this pull request Sep 18, 2024

Update GTK stack for use in CI/CD #466

Open

Lord-Kamina closed this Sep 18, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ci fixes and updates #454

Ci fixes and updates #454

Lord-Kamina commented Jun 28, 2024

cas-- commented Aug 26, 2024

Lord-Kamina commented Sep 2, 2024

Lord-Kamina Sep 2, 2024

cas-- Sep 4, 2024

Lord-Kamina Sep 4, 2024 •

edited

Loading

Lord-Kamina Sep 2, 2024

Lord-Kamina Sep 2, 2024

cas-- Sep 4, 2024

Lord-Kamina Sep 4, 2024

Lord-Kamina Sep 2, 2024

Lord-Kamina Sep 2, 2024 •

edited

Loading

Lord-Kamina Sep 2, 2024 •

edited

Loading

cas-- Sep 4, 2024

Lord-Kamina Sep 4, 2024 •

edited

Loading

Lord-Kamina Sep 4, 2024

Lord-Kamina Sep 8, 2024 •

edited

Loading

Lord-Kamina commented Sep 4, 2024

zakkarry commented Sep 6, 2024

DjLegolas commented Sep 6, 2024

zakkarry commented Sep 6, 2024

Lord-Kamina commented Sep 6, 2024

Lord-Kamina commented Sep 6, 2024

Lord-Kamina commented Sep 8, 2024

zakkarry commented Sep 8, 2024

Lord-Kamina commented Sep 18, 2024

Ci fixes and updates #454

Ci fixes and updates #454

Conversation

Lord-Kamina commented Jun 28, 2024

cas-- commented Aug 26, 2024

Lord-Kamina commented Sep 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lord-Kamina Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lord-Kamina Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Lord-Kamina Sep 2, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lord-Kamina Sep 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lord-Kamina Sep 8, 2024 • edited Loading

Choose a reason for hiding this comment

Lord-Kamina commented Sep 4, 2024

zakkarry commented Sep 6, 2024

DjLegolas commented Sep 6, 2024

zakkarry commented Sep 6, 2024

Lord-Kamina commented Sep 6, 2024

Lord-Kamina commented Sep 6, 2024

Lord-Kamina commented Sep 8, 2024

zakkarry commented Sep 8, 2024

Lord-Kamina commented Sep 18, 2024

Lord-Kamina Sep 4, 2024 •

edited

Loading

Lord-Kamina Sep 2, 2024 •

edited

Loading

Lord-Kamina Sep 2, 2024 •

edited

Loading

Lord-Kamina Sep 4, 2024 •

edited

Loading

Lord-Kamina Sep 8, 2024 •

edited

Loading