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

[#10200] Force using GTK+3 in twisted.internet.gireactor #1600

Closed
wants to merge 3 commits into from

Conversation

mgorny
Copy link
Contributor

@mgorny mgorny commented May 21, 2021

Scope and purpose

Fixes #10200

Twisted uses incompatible GTK+4 when installed. This patch forces GTK+3 to avoid the problems.

It does not cover adding GTK+4 compatibility ;-).

Contributor Checklist:

  • The associated ticket in Trac is here: https://twistedmatrix.com/trac/ticket/10200
  • I ran tox -e lint to format my patch to meet the Twisted Coding Standard
  • I have created a newsfragment in src/twisted/newsfragments/ (see: News files)
  • The title of the PR starts with the associated Trac ticket number (without the # character).
  • I have updated the automated tests and checked that all checks for the PR are green.
  • I have submitted the associated Trac ticket for review by adding the word review to the keywords field in Trac, and putting a link to this PR in the comment; it shows up in https://twisted.reviews/ now.
  • The merge commit will use the below format
    The first line is automatically generated by GitHub based on PR ID and branch name.
    The other lines generated by GitHub should be replaced.
Merge pull request #123 from twisted/4356-branch-name-with-trac-id

Author: <github_username>, <github_usernames_if_more_authors>
Reviewer: <github_username>, <github_username_if_more_reviewers>
Fixes: ticket:<trac_ticket_number>, ticket:<another_if_more_in_one_PR>

Long description providing a summary of these changes.
(as long as you wish)

@mgorny
Copy link
Contributor Author

mgorny commented May 21, 2021

I don't think I can improve the coverage on this, really (or at least not within the scope of this PR).

@glyph
Copy link
Member

glyph commented Jul 22, 2021

I don't think I can improve the coverage on this, really (or at least not within the scope of this PR).

It's likely that, particularly with an overflowing review queue, reviewers are likely to skip over this unless it has a passing commit status. If you don't think that coverage on these lines are important, could you put # pragma: no-cover lines to make codecov chill out? The reviewer can then see it's intentional and potentially disagree, but the checkmark will be green at least :). (And you can include a comment next to it explaining why coverage is difficult.)

@mgorny mgorny force-pushed the 10200-mgorny-force-gtk3 branch 4 times, most recently from 16e924b to eec6b60 Compare July 29, 2021 06:58
@mgorny
Copy link
Contributor Author

mgorny commented Jul 29, 2021

I don't think I can improve the coverage on this, really (or at least not within the scope of this PR).

It's likely that, particularly with an overflowing review queue, reviewers are likely to skip over this unless it has a passing commit status. If you don't think that coverage on these lines are important, could you put # pragma: no-cover lines to make codecov chill out? The reviewer can then see it's intentional and potentially disagree, but the checkmark will be green at least :). (And you can include a comment next to it explaining why coverage is difficult.)

Thanks for the suggestion. However, no matter how hard I try, it doesn't seem to work :-(. Maybe the no cover pragma doesn't work for diffs?

Modify twisted.internet.gireactor to explicitly request GTK+ version 3.
Otherwise it uses the newest version installed which could be GTK+4
that gireactor is currently incompatible with.
@adiroiban
Copy link
Member

I am updating the branch.

We now run some GTK reactors tests as part of the CI.

@adiroiban adiroiban changed the title 10200: Force using GTK+3 in twisted.internet.gireactor [#10200] Force using GTK+3 in twisted.internet.gireactor Sep 8, 2022
@adiroiban
Copy link
Member

I think that this needs-changes

See #10200 (comment) comment from here

There is a report that the code still needs changes

@glyph
Copy link
Member

glyph commented Oct 6, 2022

I'm working on a fix in #11706 that somewhat obviates the need for this; there is now only one place we import Gtk, and it's in the test suite, which can be controlled with an env var if so desired, but defaulting to 4.0 (which, hey, has actual compatibility guarantees, supposedly!).

We will need some updates in the docs, though, since we still refer to truly ancient gtk2-style code in a lot of places.

@glyph
Copy link
Member

glyph commented Nov 29, 2022

I think that #11705 fixed the underlying problem here. If Gtk3 systems still need help, please open a new PR.

@glyph glyph closed this Nov 29, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Test failures and hang when GTK+4 is installed in the system
5 participants