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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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.

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