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

Update requirements and CI for PyQt6.7 #8175

Closed
wants to merge 21 commits into from
Closed

Conversation

toofar
Copy link
Member

@toofar toofar commented Apr 28, 2024

(This is on top of #8173, so ignore the first two commits if they are still present.)

This is the usual "add support for new Qt/PyQt version" change that normally happens in the background where the dripping pipes and greasy rags are. I'm raising this PR for visibility in case anyone has an opinion on how to deal with the macOS WebEngine binaries not being available yet.

See commit ce562b7 for the PyQt6.7 related updates to requirements files and CI definitions for the actual version upgrade.

Upgrading to Qt6.7 doesn't seem to have had any user visible impact (apart from the Vulkan backend not working, and primary selection not being filled from the copy action anymore), which is a relief. It did cause some hard to debug issues in the tests, but hopefully that's the last we'll hear of that. (Also looks like there is one issue with IPC sockets closing differently on application shutdown on windows that still needs to be resolved, in the tests).

We've wanted to get a new release out with Qt6.7 for a while. Unfortunately the corresponding PyQt release has been delayed for a longer time than usual, but it's available now! Which means we can cut a new release! BUT, as mentioned in the announce email there are no binaries available for macOS. It seems PyQt is waiting on this support request to PyPI: pypi/support#3949

As I see it our options are:

  1. delay the release until the PyQt6.7 WebEngine binaries for macOS are available
  2. release now1 with macOS shipping with Qt 6.6 - and then make a new one when the binaries become available so mac users can get a new chromium base
  3. release now1 but finagle the release job so it pulls QtWebEngine from riverbank's PyPI servers, as suggested in their email

Footnotes

  1. "now" probably means some time next week, we might want to add a fix for https://github.com/qutebrowser/qutebrowser/issues/8170 in too 2

toofar and others added 2 commits April 28, 2024 13:59
6.7 is released now, some distros are already shipping it!

This commit:
1. adds a new 6.7 requirements file (the plain 6 one has already been
   updated by the bot)
2. adds a new tox env referring to the new requirements file
3. updates the mac and windows installer jobs to run with pyqt67 with the
   assumption we'll be including that in our next release
4. adds two new CI environments for 6.7, one each for python 3.11 and 3.12
   (3.12 only came out like 6 months ago)
5. updates a couple of references to the py37 tox env that looked like they
   were missed, 3.7 support was dropped in 93c7fdd
6. updates various ubuntu containers to the latest LTS instead of the previous
   related one - this is quite unrelated to this change but I thought I would
   give it a go, no need to use the old one unless we are specifically testing
   on it?
   - linters - they use tox but probably use system libraries
   - these all run in nested containers anyway, should be fully isolated
   - codeql - eh, uses a third party action, check the docs if it fails
   - irc - as above
@The-Compiler
Copy link
Member

Ugh. I thought it would be easy and we'd just need to set commands_pre for tox. Turns out it wasn't easy, because installing dependencies happens before that....

I attempted the required finagling with requirement files and tox.ini and mkvenv.py and whatnot to get this to work.

On a second thought, I wonder if I could just have dumped --extra-index-url https://www.riverbankcomputing.com/pypi/simple/ into the requirements files, because pip should support this in theory.

This reverts commit 6c4be8e.

Possibly easier solution in next commit.
@The-Compiler The-Compiler added this to the v3.2.0 milestone Apr 30, 2024
Easier fix instead of 6c4be8e.
Seems to get picked up just fine, and shouldn't hurt when it's not needed, as we
don't use --pre. Thus, no development releases should be installed.
@toofar
Copy link
Member Author

toofar commented May 1, 2024

✔️ cool, I'm okay with that approach. I think this is a safe use of --extra-index-url since the riverbank pypi is a pretty tightly controlled repo, and I think they use pre-release builds for experimental stuff (we've been warned a lot about --extra-index-url at work but that's regarding someone shadowing internal libraries on the public pypi repo, not really applicable here).

Regarding the failure on windows, I got as far as seeing that it looks like a socket "times out" in like 200ms when the timeout is set to 5s. I was going to compare the logs for a single test on windows to linux and see if I can spot a difference. I did install a nightly from this branch in a VM and it shut down fine at least, didn't check with userscripts because I forgot they aren't bundled :)

Eg regarding the timing there are logs like this (wait, is that the ID of the ephemeral socket for a client connection or the listening one for the server?):

02:35:05.191 DEBUG    ipc        ipc:handle_connection:255 Client connected (socket 0x1d2da57d3b0).
02:35:05.223 ERROR    ipc        ipc:on_timeout:394 IPC connection timed out (socket 0x1d2da57d3b0).

Edit: heres the nightly built off of this branch, it works!:

image

@toofar

This comment was marked as duplicate.

@The-Compiler
Copy link
Member

Taking the IPC timeout thing to #8191. I'm taking a look and need a place for notes.

The-Compiler and others added 16 commits May 21, 2024 08:02
Windows seems to struggle with the dash
Since we added some sanity checking in usertypes.Timer() around
QTBUG-124496 it would be convenient if there was a reminder for future
timer users to use our Timer object instead. Here's one!

It's looking for QTimer initialisations, we are still allowing
QTimer.singleShot(), although that probably can hit the same issue.

It uses an end-of-line anchor in the regex so you can put a comment (any
comment) on the end of the line to ignore the check.
Since we are now checking for early timer firings in usertype.Timer I
figured why do it in two places, lets call the check method on Timer in
the handler in the IPC class too.

I also removed the platform and version check because while the bug only
happens under those conditions the check to ensure a timer event was
valid should work everywhere. Right? We can add them back of someone
wants but I'm not sure I see a point.

It would be nice if we could filter out invalid timer events in
usertypes.Timer and not require the consuming code to have to do
anything. But that would be more work, so lets wait until more places of
code actually need to care about it, chances are it'll be fixed in Qt
soon enough anyway.

I also think usertypes.Timer should be restarting the time so it gets
called at the proper time. But if we aren't filtering the events that
would mean it gets called twice.
The `test_cleanup()` test for guiprocess was triggering the early timer
warning messages because it was using a VeryCourse timer with a 100ms
interval. Changed it to to a normal Course timer (the default) for that
test.

For the `ipc-timeout` timer we know that is happening in the tests on
windows. It's being logged in lost of e2e tests, the elapsed times it's
logging are between 0 and 0.020s. I'm not sure if it's the right thing
to be changing the log level in production code or marking the messages
as expected in test code.
Workaround IPC connection closing due to misfiring QTimers on Windows
@The-Compiler The-Compiler requested a review from rcorre as a code owner May 22, 2024 19:42
@The-Compiler
Copy link
Member

Conflicts resolved locally (I added Python 3.13 to tox.ini) and merged - thanks!

@toofar toofar deleted the feat/pyqt67_enablement branch May 24, 2024 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants