Add support for PyQt5 #610
Conversation
There was a problem hiding this comment.
Thanks @eloquence, tested in both Qubes 4.1 and 4.0.3, and very pleased to report that the changes changes work as expected 🎉
Changes work well in Qubes 4.0. In 4.1 I did observe a popup (see below) , but these changes are very likely unrelated to the changes introduced here. I will add a note in #600
A couple of comments inline
Test plan
Qubes 4.0
- Copy this branch into
dom0in a provisioned Qubes 4.0 SDW development or staging environment - Run the updater via
python3 launcher/sdw-launcher.py --skip-delta 0
- Observe that the updater runs as before.
- Run the updater via
SDW_UPDATER_QT=5 python3 launcher/sdw-launcher.py --skip-delta 0
- Observe that the updater fails to run because Qt 5 is not installed.
- Remove the file
~/.securedrop_launcher/sdw-last-updated - Ensure that your system uptime is at least 30 minutes.
- Run the notify script via
python3 launcher/sdw-notify.py
- Observe that the update alert is shown as before.
Qubes 4.1 (nightly)
- Copy this branch into
dom0in a provisioned Qubes 4.1-nightly SDW development or staging environment - Run the updater via
python3 launcher/sdw-launcher.py --skip-delta 0
- Observe that the updater runs to completion with no graphical errors.
- Remove the file
~/.securedrop_launcher/sdw-last-updated - Ensure that your system uptime is at least 30 minutes.
- Run the notify script via
python3 launcher/sdw-notify.py
- Observe that the update alert is shown.
Development environment
- Activate a virtualenv and install the Qt5 requirements via
pip install --require-hashes -r launcher/qt5-requirements.txt - Run the updater via
SDW_UPDATER_QT=5 python3 launcher/sdw-launcher.py --skip-delta 0
- Observe that the updater starts (you won't be able to run it), and that your terminal indicates that Qt5 is used.
- Remove the file
~/.securedrop_launcher/sdw-last-updated - Ensure that your system uptime is at least 30 minutes.
- Run the notify script via
SDW_UPDATER_QT=5 python3 launcher/sdw-notify.py
- Observe that the update alert is shown. (NB: The notifier checks for process contention with
make, so if nothing appears, that may be because you have a dev env running -- checksdw-notify.login~/.securedrop_launcherif in doubt)
launcher/README.md
Outdated
| Activate a virtualenv and intall the requirements using the following | ||
| command: | ||
|
|
||
| pip install --require-hashes -r qt5-requirements.txt |
There was a problem hiding this comment.
are these requirements strictly required here? In my dev vm (Debian 10) I can run the launcher with QT4 and QT5 outside a virtualenv with no dependencies, using system-level qt packages. Is there a reason we should treat qt5 as a special case and track requirements ?
There was a problem hiding this comment.
The main reason I think this may make sense is so that we all use a predictable version of PyQt during development, with a defined hash, which could be important during debugging. (For now it will fetch the same version as we use with securedrop-client, but ideally it should match whatever is shipped with Qubes 4.1.) This was not an option for PyQt4, which is no longer available via pypi.org, but it is an option for PyQt5.
We could consolidate test-requirements.txt and qt5-requirements.txt into a single dev-requirements.txt.
There was a problem hiding this comment.
Since we aren't shipping/packaging/locking the PyQT dependency in dom0, I don't see the value in locking for development environment consistency (especially since we are using OS level packages)
However, locking for testing consistency makes sense here (since it will run both on developer machines but also in CI). Adding it to test-requirements (and optionally renaming, no strong preference here) sounds like a good approach to me
There was a problem hiding this comment.
I've refactored accordingly. I cheated by not updating all requirements but only inserting the up-to-date PyQt5 requirements into dev-requirements.txt. (Updating all requirements will pull in a new version of black that'll require formatting changes.) Once this PR lands I think it'd be good idea to do a requirements refresh for both preflight updater & main repo requirements.
| except ValueError: | ||
| continue |
There was a problem hiding this comment.
pytest is telling me that these two lines don't have test coverage
There was a problem hiding this comment.
I'll add a bad-file-format fixture for this case.
Also adds PyQt5 to shared requirements file and renames it to dev-requirements.txt for consistency.
There was a problem hiding this comment.
Thanks for the changes @eloquence . Successfully went though another test plan in both 4.0.3 (Fedora 25) and 4.1 (Fedora 32) using RPMs (opened #611 to track F32 build issues). I didn't see the error I initially reported, but we are tracking that issue separately anyways.
Given the changes here and the comprehensive test coverage, the impact to the Fedora 25 logic is very minimal.
| @@ -0,0 +1,114 @@ | |||
| # -*- coding: utf-8 -*- | |||
There was a problem hiding this comment.
For packaging, this file is handled by https://github.com/freedomofpress/securedrop-workstation/blob/main/rpm-build/SPECS/securedrop-workstation-dom0-config.spec#L82-L83
Status
Ready for review
Description of Changes
Fixes #607.
Uses the appropriate Qt version:
SDW_UPDATER_QT=5is setAdds PyQt5 to
dev-requirements.txt(renamed fromtest-requirements.txtfor consistency with https://github.com/freedomofpress/securedrop-workstation/blob/main/dev-requirements.txt).NOTE: Once this PR is merged, the Qt5 version of the UI will need to be regenerated along with the Qt4 version every time
sdw_updater.uiis modified, usingpyuic5.Test plan
Qubes 4.0
dom0in a provisioned Qubes 4.0 SDW development or staging environmentpython3 launcher/sdw-launcher.py --skip-delta 0SDW_UPDATER_QT=5 python3 launcher/sdw-launcher.py --skip-delta 0~/.securedrop_launcher/sdw-last-updatedpython3 launcher/sdw-notify.pyQubes 4.1 (nightly)
dom0in a provisioned Qubes 4.1-nightly SDW development or staging environmentpython3 launcher/sdw-launcher.py --skip-delta 0~/.securedrop_launcher/sdw-last-updatedpython3 launcher/sdw-notify.pyDevelopment environment
pip install --require-hashes -r launcher/qt5-requirements.txtSDW_UPDATER_QT=5 python3 launcher/sdw-launcher.py --skip-delta 0~/.securedrop_launcher/sdw-last-updatedSDW_UPDATER_QT=5 python3 launcher/sdw-notify.pymake, so if nothing appears, that may be because you have a dev env running -- checksdw-notify.login~/.securedrop_launcherif in doubt)