-
Notifications
You must be signed in to change notification settings - Fork 29
bump AppImage and Snap Qt versions #449
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
Conversation
Reviewer's GuideUpdates Snap and AppImage build configurations to target newer Qt/core base stacks, modernizes dependency setup, and tweaks Qt high-DPI rounding behavior, while fixing a workflow label typo for Snap checksums. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 2 issues, and left some high level feedback:
- Changing the high DPI scale factor rounding policy from
PassThroughtoRoundalters rendering behavior on fractional scaling; consider making this configurable (e.g., via an environment variable or command-line flag) or at least gating it by platform/Qt version if some users rely on the previous behavior. - In the AppImage workflow, the KDE Neon repository is hardcoded to
jammy; if the GitHub runner base image changes, this may become incompatible, so consider tying the suite to the runner OS version (e.g., via a matrix variable) to avoid future mismatch issues.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Changing the high DPI scale factor rounding policy from `PassThrough` to `Round` alters rendering behavior on fractional scaling; consider making this configurable (e.g., via an environment variable or command-line flag) or at least gating it by platform/Qt version if some users rely on the previous behavior.
- In the AppImage workflow, the KDE Neon repository is hardcoded to `jammy`; if the GitHub runner base image changes, this may become incompatible, so consider tying the suite to the runner OS version (e.g., via a matrix variable) to avoid future mismatch issues.
## Individual Comments
### Comment 1
<location> `.github/workflows/build-appimage.yml:29` </location>
<code_context>
- - name: Install Build Dependencies
+ - name: Add KDE Neon repository
run: |
+ wget -qO - http://archive.neon.kde.org/public.key | sudo gpg --dearmor -o /usr/share/keyrings/kde-neon.gpg
+ echo "deb [signed-by=/usr/share/keyrings/kde-neon.gpg] http://archive.neon.kde.org/user jammy main" | sudo tee /etc/apt/sources.list.d/kde-neon.list
sudo apt update -qq
</code_context>
<issue_to_address>
**🚨 issue (security):** Fetch the Neon key over HTTPS instead of HTTP to avoid a man-in-the-middle risk when importing the signing key.
Because this step bootstraps trust in the Neon repository, fetching the key over plain HTTP allows a network attacker to swap in a malicious key. The host already supports TLS, so please change the URL to `https://archive.neon.kde.org/public.key` to avoid this risk.
</issue_to_address>
### Comment 2
<location> `.github/workflows/build-appimage.yml:30` </location>
<code_context>
+ - name: Add KDE Neon repository
run: |
+ wget -qO - http://archive.neon.kde.org/public.key | sudo gpg --dearmor -o /usr/share/keyrings/kde-neon.gpg
+ echo "deb [signed-by=/usr/share/keyrings/kde-neon.gpg] http://archive.neon.kde.org/user jammy main" | sudo tee /etc/apt/sources.list.d/kde-neon.list
sudo apt update -qq
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Mixing a Jammy-based Neon repo on non-Jammy runners could introduce dependency or ABI mismatches.
Since `ubuntu-latest` now maps to 24.04 (noble), enabling a Jammy Neon repo there risks pulling in incompatible libraries or toolchains. Please either constrain this workflow to a Jammy runner (`runs-on: ubuntu-22.04`) so the suite matches the base distro, or add an apt preferences file to pin specific Neon packages and prevent core system libraries from being upgraded from that repo.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| - name: Add KDE Neon repository | ||
| run: | | ||
| wget -qO - http://archive.neon.kde.org/public.key | sudo gpg --dearmor -o /usr/share/keyrings/kde-neon.gpg | ||
| echo "deb [signed-by=/usr/share/keyrings/kde-neon.gpg] http://archive.neon.kde.org/user jammy main" | sudo tee /etc/apt/sources.list.d/kde-neon.list |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Mixing a Jammy-based Neon repo on non-Jammy runners could introduce dependency or ABI mismatches.
Since ubuntu-latest now maps to 24.04 (noble), enabling a Jammy Neon repo there risks pulling in incompatible libraries or toolchains. Please either constrain this workflow to a Jammy runner (runs-on: ubuntu-22.04) so the suite matches the base distro, or add an apt preferences file to pin specific Neon packages and prevent core system libraries from being upgraded from that repo.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #449 +/- ##
==========================================
+ Coverage 66.48% 75.72% +9.24%
==========================================
Files 85 94 +9
Lines 4186 4355 +169
Branches 255 295 +40
==========================================
+ Hits 2783 3298 +515
+ Misses 1403 1057 -346 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Summary by Sourcery
Update Snap and AppImage configurations to use newer Qt and base runtimes while adjusting build and DPI settings accordingly.
Enhancements:
Build:
CI: