The normal PDF.js build only officially supports the latest Chromium, so things
might break every once in a while with QtWebEngine (e.g. #8199, #7335).
Let's instead bundle and recommend the legacy build.
Closes#8332Closes#7721 (reworded)
Also see #7135
This re-enables the pylint too-many-positional-arguments for the main
application code. It's still disabled for tests because that's how you pull in
pytlint fixtures, and I don't think we want to push people into being creative
with fixtures just to get around that.
When functions are called with many positional arguments the reader has to do
a bit of heavy lifting to figure out in what position a value is being passed,
and it's easier to make mistakes. So I would like to encourage using keyword
arguments for long argument lists.
I've set the `max-positional-arguments` to a completely arbitrary 7, from a
completely arbitrary 5, because there were many more violations under 7. If
like 99% of our functions fit under 7 it's probably fine.
Regarding the exceptions:
* objreg.register: I grepped it and it looks like everything is only passing
the first two args as positional already, lucky!
* `_get_color_percentage`: only one usage of it, but I was in "add directive
comment" mode
* update_3rdparty.py: only one usage, already using kwargs
* pyqtProperty: idk
* commands.py: "its complicated". Many methods in this file map to commands
used in qutebrowser's command mode. In that case it's usual for them to be
called as flags, rather than positional. But it could be complicated to wade
into that, and having one file excluded isn't so bad.
I just searched for qt5 and deleted stuff. EZ.
Will leave on a branch for a bit and see if I feel like testing this at
all, otherwise maybe leave this stuff in here and make it not called.
Not 100% sure that we need to remove all this stuff when we just want
the CI to go green. But tbh if we don't need to make Qt5 releases then
we don't need it. Better to be bold and pull it out than have to work
around it in the future. And we can always revert the commit.
This commit takes a screenshot of the active browser window when an
end2end test fails. When running on CI a zip file of screenshots will be
attached to the run summary as an artifact. When run locally screenshots
will be left in /$TMPDIR/pytest-screenshots/.
The screenshot is of the Xvfb screen that the tests are running under.
If there are multiple windows open it will likely only show the active
window because a) we aren't running with a window manager b) the Xvfb
display is, by default, the same size as the browser window.
I'm not sure if xvfb is used on the Window runs in CI. We could fall
back to trying to take screenshots if not running under xvfb but I'm a
bit wary of an automatic feature that takes screenshots of people's
desktops when running locally. Even if they just to to /tmp/ it might be
surprising. We can change it later if it turns out we need to try to
take screenshots in more cases.
I'm using pillow ImageGrab, the same as pyvirtualdisplay.smartdisplay. I'm
getting the display number from the pytest-xvfb plugin and formatting it
appropriately (pyvirtualdisplay has an already formatted one which is used by
the smartdisplay, but that's not accessible).
Pillow is now a requirement for running the tests. I thought about making
it gracefully not required but I'm not sure how to inform the user with
a warning from pytest, or if they would even want one. Maybe we could
add a config thing to allow not taking screenshots?
I had to bump the colordepth config for pytest-xvfb otherwise pillow
complained that the default 16bit color depth wasn't supported as it
only supports 24bit, see https://github.com/python-pillow/Pillow/blob/1138ea5370cbda5eb328ec949
8c314d376c81265/src/display.c#L898
I'm saving screenshots to a temp dir because I don't want to put them in
my workdir when running locally. I want to clear the directory for each
run so that you don't get confused by looking at old images. I'm not
100% sure about the lifecycle of the process classes though. Eg if we
have two processes they might both try to create the output directory.
I'm using pytest.session.stash to save the directory so perhaps the
lifecycle of the stash will handle that? Not sure.
Ideally the images would be uploaded somewhere where we could click
through and open them in the browser without having to download a zip
file, but I'm not sure how to achieve that.
It would be nice to print in the test logs that a screenshot was saved
and where to. Just so you could copy paste the filename instead of
having to match the sanitized filename against failing test names. But I
don't know how to log stuff from this stage in the pytest lifecycle.
TODO:
* I'm not sure that the screenshot captures the whole browser window?
Maybe the browser windows is bigger than the X11 display?
Closes: #7625
Few new vendored packages showing up from setuptools for environments
where pkg_resources is being imported for some reasons.
I don't think these requirements should be in our requirements files,
they aren't direct dependancies and they aren't declared as dependancies
of setuptools (and we are currently excluding setuptools from our
requirements files anyway, although apparently that is not the right
thing to do these days). These are actually not installed as normal
packages by are vendored packages shipped with setuptools.
Options I see to deal with them:
1. suck it up and add them to the compiled requirements files
* not ideal, but should be harmless. They are real packages that the
setuptools authors have chose to use
2. exclude these new packages using the markers in comments
* maybe, seems like it could lead to issues in the future if any of
these packages start getting declared as proper dependancies
3. find out where pkg_resources is being imported and stop it
* I don't seem to be able to reproduce this behaviour locally, even
when using a py3.8 docker container. And we are literally only
running `pip freeze` via subprocess, what could the difference be?
* I don't particularly want to delve into the arcane python packaging
stuff, it seems to be layers and layers of very specific issues and
old vendored packages
4. stop using pip freeze to compile requirements files and just compute
them based off of the raw files themselves
* Don't give us the chance to use stuff that we don't depend on but
happens to be installed. We get other nice things with this too
This commit does (1). I'll open a follow up PR to do (4).
Starting with the upgrade to Hypothesis 6.103.4 we got hangs when pytest exits.
This is caused by:
https://github.com/HypothesisWorks/hypothesis/pull/4013
combined with:
https://github.com/python/cpython/issues/102126
which was fixed in Python 3.10.11, but the latest 3.10 packaged by Archlinux was
3.10.10.
Thus, we instead build a newer 3.10 from the AUR.
This bumps the build time up to about 20 minutes on my machine, which is
probably acceptable since those are nightly builds only anyways. We could
probably half that by disabling --enable-optimization, but that would be at the
cost of making the actual test runs (which run more often) slower.
Closes#8247
With GitHub Actions now providing macOS 14 runners with M1 chips, we can
build a separate Apple Silicon release there and upload it.
Universal wheels are currently not possible, see #8229 for details.
Closes#6478
I'm trying to update pdf.js in the bleeding edge CI jobs. It complains
that either it can't find PyQt or it can't find yaml depending on how I
invoke tox. Joy. Since dict stuff isn't run by default in this script
hopefully that is the only broken import path and moving it into the
function lets the pdfjs (and ace) bit of the script work fine.
Actually, looking at the stack traces below, both of them are from dict
related code!
tox exec -re bleeding -- python scripts/dev/update_3rdparty.py --gh-token ***
Traceback (most recent call last):
File "/__w/qutebrowser/qutebrowser/scripts/dev/update_3rdparty.py", line 20, in <module>
from scripts import dictcli
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/dictcli.py", line 25, in <module>
from qutebrowser.browser.webengine import spell
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/browser/webengine/spell.py", line 14, in <module>
from qutebrowser.utils import log, message, standarddir
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/utils/message.py", line 15, in <module>
from qutebrowser.qt.core import pyqtSignal, pyqtBoundSignal, QObject
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/qt/core.py", line 17, in <module>
machinery.init_implicit()
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/qt/machinery.py", line 278, in init_implicit
raise NoWrapperAvailableError(info)
qutebrowser.qt.machinery.NoWrapperAvailableError: No Qt wrapper was importable.
python scripts/dev/update_3rdparty.py --gh-token ***
Traceback (most recent call last):
File "/__w/qutebrowser/qutebrowser/scripts/dev/update_3rdparty.py", line 20, in <module>
from scripts import dictcli
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/dictcli.py", line 25, in <module>
from qutebrowser.browser.webengine import spell
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/browser/webengine/spell.py", line 14, in <module>
from qutebrowser.utils import log, message, standarddir
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/utils/message.py", line 17, in <module>
from qutebrowser.utils import usertypes, log
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/utils/usertypes.py", line 16, in <module>
from qutebrowser.utils import log, qtutils, utils
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/utils/qtutils.py", line 39, in <module>
from qutebrowser.utils import usertypes, utils
File "/__w/qutebrowser/qutebrowser/scripts/dev/../../scripts/../qutebrowser/utils/utils.py", line 29, in <module>
import yaml
ModuleNotFoundError: No module named 'yaml'
This installs pdf.js in a selection of CI jobs. Previously the PDF.js
tests (in qutescheme.feature) were skipped in CI because it wasn't
installed anywhere. There has been a couple of recent cases where pdf.js
started depending on javascript features that are too new for even the
most recent QtWebEngine to support. The aim of this commit is to catch
that case. This doesn't add coverage for older webengine releases.
This also incidentally updates the ace editor in these test jobs, since that is
also updated by default by the update_3rdparty script. Hopefully that
doesn't cause issues.
The reasoning for installing on each type of job:
*ubuntu jobs*: not installed - while our main test runs are on ubuntu
there is an upstream issue where many assets used by pdf.js (like icons
used in the toolbar) aren't packaged, see #7904. This causes warning
messages because assets requested via qutescheme can't be found, which
causes the tests to fail. We could a) install pdf.js from source instead
of using the ubuntu one b) ignore the warning logs c) skip this
environment and rely on tests elsewhere. I've chosen to do (c). I don't
see a huge benefit in testing pdf.js across multiple environments if we
aren't using it installed from the OS anyway. We could install from
source but currently all the Qt < 6.5 tests are failing from some other
JS error, and I think fixing that is out of scope of this issue.
*docker Qt6*: installed - the archlinux pdfjs package works fine and we are
only testing the most recent Qt versions because arch users are expected
to stay up to date.
*docker Qt5*: not installed - doesn't support JS features required by
PDF.js. I guess we could install the legacy build from source here. I'm
mostly worried about catching new breakages for this commit though
*windows*: installed - we install pdf.js from source when making a
release so it would be nice to do that in tests too.
*macos*: not installed - the tests that were catching the breakages are
end2end tests which we don't run on mac. And I think there was an
error from the :versions tests here, don't remember.
*bleeding edge*: installed - from source
pdf.js tests fail on Qt < 6.5 with `Uncaught TypeError: Cannot read
properties of null (reading 'mainContainer')`
The `TestPDFJSVersion.test_real_file` unit tests currently fails because
`version._pdfjs_version()` returns `unknown (bundled)`, not sure why. I
think this is pre-existing and it also wasn't being run on CI.
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.
Easier fix instead of 6c4be8ef03.
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.
Florian Bruhin
Jun Chen
Ruben Gonzalez
toofar