Update GTK stack for use in CI/CD #466

Lord-Kamina · 2024-09-18T13:44:10Z

Ok @cas--, you win; I'll split it.

I also opted to make a new PR that will supersede #454, to make it easier to keep track of the discussion, were there to be any.

After this, I will make another PR for unpinning pytest (I might include the change from gtkreactor to gireactor in this latter one because I genuinely think it would make everybody's lives easier, but you tell me)

This is just the bit updating the GTK stack. As you can see, I've reverted the changes to libtorrent and such (although I maintain it should be somewhat of a priority to address this sooner rather than later)

Lord-Kamina · 2025-02-02T15:31:29Z

@cas-- anything that needs to change here before this can be merged? (The CI fails should be resolved by triggering a manual run at the gvsbuild-release repo)

cas-- · 2025-02-02T15:52:24Z

.github/workflows/ci.yml

@@ -89,6 +97,24 @@ jobs:
          cache: "pip"
          cache-dependency-path: "requirements*.txt"

+      - name: Determine gvsbuild release URL


If we are not running the gtkui tests then we should not be installing gtk here

The idea is to enable the gtk tests though. I think that might have gotten cut onto another PR though.

If we are not using it let's leave it out of this PR, the code is always going to be in CD.yml. Also it doesn't address the Linux GTK tests

cas-- · 2025-02-02T15:55:11Z

.github/workflows/docs.yml

@@ -29,7 +29,7 @@ jobs:
        run: |
          pip install --upgrade pip wheel
          pip install tox
-          sudo apt-get install enchant-2
+          sudo apt-get install enchant-2 libgirepository1.0-dev


Why are we installing this for docs?

I honestly can't remember 100% but it is possible that it failed somewhere without it. Would have to look it up.

Removed as well.

cas-- · 2025-02-02T15:57:21Z

requirements-tests.txt

@@ -1,4 +1,4 @@
-libtorrent
+libtorrent==2.0.7


This shouldn't need pinned?

Yes because things in newer libtorrent made tests fail, so until tests are updated, it has to be.

So for CI we already have this pinned so this only affects local testing, perhaps there shouldn't be a difference but that is also a question about pinning the entire test/dev stack to known working dependencies. I really do want to fix the tests for libtorrent > 2.0.7 so keeping it unpinned serves as a reminder

cas-- · 2025-02-02T15:58:16Z

.github/workflows/cd.yml

-        python: ["3.9"]
-        libtorrent: [2.0.7, 1.2.19]
+        python: ["3.7", "3.10"]
+        libtorrent: [2.0.8, 1.2.19]


Is 2.0.8 available?

Actually I think this was a typo or possibly I missed this when reverting. I can't find it now so it should go back to 2.0.7 (for now)

Changed back to 2.0.7

cas-- · 2025-02-02T16:00:07Z

.github/workflows/ci.yml

@@ -36,6 +42,7 @@ jobs:

      - name: Install dependencies
        run: |
+          sudo apt-get install libcairo2-dev libgirepository1.0-dev


This doesn't seem right to be installing these dev dependencies, everything should have been built already?

I removed it for now.

cas-- · 2025-02-02T16:01:32Z

.github/workflows/ci.yml

@@ -74,7 +81,8 @@ jobs:
    runs-on: windows-2022
    strategy:
      matrix:
-        python-version: ["3.7", "3.10"]
+        python-version: ["3.7", "3.9", "3.10"]


Why are we including 3.9 here and removing it from package build?

I'm including it due to some differences in the importlib module. It's not relevant now but I think it will be to test against with some PR I've yet to submit.

Let's leave that to the other PR then

cas-- · 2025-02-02T16:09:45Z

.github/workflows/cd.yml

@@ -77,7 +88,6 @@ jobs:
        working-directory: deluge_src
        run: |
          python -m pip install .
-          python setup.py install_scripts


Are you sure removing this is correct?

Nope. I think it might be with something I did on another branch but I'll cross that bridge when I get there.
Reverted for now.

.github/workflows/cd.yml

cas-- · 2025-02-02T16:12:13Z

.github/workflows/ci.yml

-    runs-on: ubuntu-22.04
    strategy:
      matrix:
-        python-version: ["3.7", "3.10"]
-
+        python-version: ["3.9", "3.10"]
+        os: ["ubuntu-24.04"]
+        include:
+          - os: ubuntu-22.04
+            python-version: 3.7
+    runs-on: ${{ matrix.os }}


Let's keep these Linux CI changes separate from the Windows GTK changes

That'll just prolong this whole thing. The linux changes are just: keep 22.04 (which needs an older python) and add a build on a newer version of ubuntu.

Can we keep the scope of this PR to Win GTK only please

Clarified a little what this commit actually does.

Lord-Kamina · 2025-02-03T10:39:42Z

BTW @cas-- I have pinned twisted here because of the issue with the maybe_coroutine test, until we can figure out what changed/is going on.
Other than that, I think most comments have been addressed.

cas--

Can we please keep PRs to a single feature/fix as outlined here: https://testing.googleblog.com/2024/07/in-praise-of-small-pull-requests.html?m=1

cas-- · 2025-02-11T19:05:24Z

requirements.txt

@@ -1,5 +1,5 @@
 libtorrent
-twisted[tls]>=17.1
+twisted[tls]>=17.1,<=24.7


I have resolved the problem with Twisted 24.10 so this can be removed

cas-- · 2025-02-11T19:31:46Z

.github/workflows/cd.yml

+        shell: bash
+        run: |
+          test -z "${{ github.event.inputs.gvsbuild-tag }}" && tag=latest || tag="${{ github.event.inputs.gvsbuild-tag }}"
+          if [[ "$tag" == "latest" ]]; then URL="https://github.com/${{ github.repository_owner }}/gvsbuild-release/releases/$tag/download"; else URL="https://github.com/${{ github.repository_owner }}/gvsbuild-release/releases/download/$tag" ; fi


This should be able to split over multiple lines to make it more readable?

cas-- · 2025-02-11T19:32:30Z

.github/workflows/ci.yml

-    runs-on: ubuntu-22.04
    strategy:
      matrix:
-        python-version: ["3.7", "3.10"]
-
+        python-version: ["3.9", "3.10"]
+        os: ["ubuntu-24.04"]
+        include:
+          - os: ubuntu-22.04
+            python-version: 3.7
+    runs-on: ${{ matrix.os }}


Can we keep the scope of this PR to Win GTK only please

cas-- · 2025-02-11T19:33:19Z

.github/workflows/ci.yml

@@ -74,7 +81,8 @@ jobs:
    runs-on: windows-2022
    strategy:
      matrix:
-        python-version: ["3.7", "3.10"]
+        python-version: ["3.7", "3.9", "3.10"]


Let's leave that to the other PR then

cas-- · 2025-02-11T19:34:57Z

.github/workflows/ci.yml

@@ -89,6 +97,24 @@ jobs:
          cache: "pip"
          cache-dependency-path: "requirements*.txt"

+      - name: Determine gvsbuild release URL


If we are not using it let's leave it out of this PR, the code is always going to be in CD.yml. Also it doesn't address the Linux GTK tests

cas-- · 2025-02-11T19:37:04Z

requirements-tests.txt

@@ -1,4 +1,4 @@
-libtorrent
+libtorrent==2.0.7


So for CI we already have this pinned so this only affects local testing, perhaps there shouldn't be a difference but that is also a question about pinning the entire test/dev stack to known working dependencies. I really do want to fix the tests for libtorrent > 2.0.7 so keeping it unpinned serves as a reminder

Lord-Kamina · 2025-02-12T12:56:52Z

Can we please keep PRs to a single feature/fix as outlined here: https://testing.googleblog.com/2024/07/in-praise-of-small-pull-requests.html?m=1

So here's my problem with this...

I understand the concept and motivation for wanting small PRs, and IMO I've been nothing if not accommodating. But other issues with it aside, this "nothing" PR has been open for months. This is not a dig on you or anyone else, because I know everyone working on this project does so as a volunteer freely contributing their time.

My point is this: These commits were meant as the first-step in getting a more modern CI pipeline as the foundation of other rather large and potentially breaking changes to deluge that require using newer versions of python (this is, dropping pkg_resources entirely and moving the plug-in system to using wheels and importlib).
That code is also already written and tested (rebasing, cleaning-up and addressing other issues aside).

And, that itself is just a preparatory step for something I've not yet done, which is make a better and easily-reproducible process for creating macOS packages and eventually, adding macOS to the CD.

With this in mind, and considering the snail pace at which this project moves (again, not a dig, but rather a fact), at some point you maybe gotta start wondering why don't you make it easier for everyone, yourself included? This was originally a much larger PR that involved fixing tests and also updating the python runtimes, which is why the there is an update on the linux workflow as well. I already split it and excluded the fixes to the testing suite, but at this point it's gone well beyond "small PRs are good". The change to the linux workflow is absolutely immaterial to the working of deluge or the CI, and is directly related to the intent of this PR.

It's incredibly frustrating because it seems like we're gettint fixated with dividing what should have been a simple task into an ever-increasing number of steps ad nauseam.

So, to recap: I understand and fully support the spirit of advocating for small PRs, but I think you're being maybe a tad excessive on this matter.

Lord-Kamina · 2025-02-12T13:05:38Z

Can we please keep PRs to a single feature/fix as outlined here: https://testing.googleblog.com/2024/07/in-praise-of-small-pull-requests.html?m=1

And finally,

Let me quote how that same article ends:

Small pull requests are not always possible. In particular:
Frequent pull requests require reviewers to respond quickly to code review requests. If it takes multiple hours to get a pull request reviewed, developers spend more time blocked. Small pull requests often work better when reviewers are co-located (ideally within Nerf gun range for gentle reminders).

Some features cannot safely be committed in partial states. If this is a concern, try to put the new feature behind a flag.
Refactorings such as changing an argument type in a public method may require modifying many dozens of files at once.

Nonetheless, even if a pull request can’t be small, it can still be focused, e.g., fixing one bug, adding one feature or UI element, or refactoring one method.

And let me once again remind you that we're not talking here about "several hours" but rather months.

kenstir · 2025-02-12T16:58:06Z

And let me once again remind you that we're not talking here about "several hours" but rather months.

To pile on just a little here, this project feels "stale" to me, a word which here means

PRs are not reviewed promptly. This one is obviously a redo of an old one, and it took 4 months to get a comment.
Releases are not packaged routinely. The last release was 2 years ago, and the previous one 2 years before that.
Prospective developers are not encouraged. I asked for help on Discord getting the tests passing on Ubuntu and didn't get help. I am an experienced developer and I offered my help with anything. I was answered with, "you can always clone the git repo", which of course I had already because I submitted a PR.

So as not only to complain, let me make some specific proposals:

Add developers to the project. Not me, but people you trust. Allow them to do a preliminary (and timely) review of PRs and if necessary ask them not to merge for now if you want to maintain control.
Create a new #dev-general channel on Discord for development support. Autobrr does this and it was helpful to me when I was submitting my first PRs. If you aren't in that channel regularly, designate some specific and regular time to be present in that channel.
Decide what it is you want help with and what you want to control. For instance, you might want help with migrating the bug database from Trac. Write it up in an Issue and announce in #dev-general. I could help here. Maybe you want help with macOS packaging. I could help here. I wouldn't enjoy it, but I would be willing to do it if I knew it would at least be reviewed.

Lord-Kamina mentioned this pull request Sep 18, 2024

Ci fixes and updates #454

Closed

cas-- added the package A label to force the CI packaging job label Feb 2, 2025

cas-- reviewed Feb 2, 2025

View reviewed changes

Lord-Kamina force-pushed the update_ci branch 7 times, most recently from 3bdf92d to 4401d33 Compare February 2, 2025 20:11

Update GTK stack for use in CI/CD

404edd9

Clarified a little what this commit actually does.

Lord-Kamina force-pushed the update_ci branch from 4401d33 to 404edd9 Compare February 2, 2025 20:48

cas-- requested changes Feb 11, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update GTK stack for use in CI/CD #466

Update GTK stack for use in CI/CD #466

Lord-Kamina commented Sep 18, 2024

Lord-Kamina commented Feb 2, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 11, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 11, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 11, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 2, 2025

Lord-Kamina Feb 2, 2025

cas-- Feb 11, 2025

Lord-Kamina commented Feb 3, 2025

cas-- left a comment

cas-- Feb 11, 2025

cas-- Feb 11, 2025

cas-- Feb 11, 2025

cas-- Feb 11, 2025

cas-- Feb 11, 2025

cas-- Feb 11, 2025

Lord-Kamina commented Feb 12, 2025

Lord-Kamina commented Feb 12, 2025

kenstir commented Feb 12, 2025

Update GTK stack for use in CI/CD #466

Are you sure you want to change the base?

Update GTK stack for use in CI/CD #466

Conversation

Lord-Kamina commented Sep 18, 2024

Lord-Kamina commented Feb 2, 2025

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

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

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

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 commented Feb 3, 2025

cas-- left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Lord-Kamina commented Feb 12, 2025

Lord-Kamina commented Feb 12, 2025

kenstir commented Feb 12, 2025