-
-
Notifications
You must be signed in to change notification settings - Fork 56
Add flatpak update plugin for system-wide flatpaks #200
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
base: main
Are you sure you want to change the base?
Conversation
5646bd3 to
b1b49de
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #200 +/- ##
=======================================
Coverage 72.98% 72.98%
=======================================
Files 10 10
Lines 1155 1155
=======================================
Hits 843 843
Misses 312 312 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
b1b49de to
a22d5b2
Compare
marmarek
left a comment
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.
Besides the two specific comments below, this should be a bit better integrated. At the very least regarding outcome reporting - it does make a difference if there were some updates or not (for example, if there were no updates, updater will not propose restarting relevant app qubes). Right now update plugins don't return anything (implicit None). Maybe consider return value similar to normal updater if a plugin returns not None?
| Distro agnostic plugin to update system-wide flatpaks | ||
| """ | ||
|
|
||
| if not Path("/usr/bin/flatpak"): |
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.
Did you mean .exists()? but then, I don't think you can assume it's always /usr/bin, especially with NixOS existing out there...
We use distutils.spawn.find_executable() in various places
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.
We use
distutils.spawn.find_executable()in various places
Where? distutils is deprecated since version 3.10, removed in version 3.12.
Anyways, I will replace it with appropriate supported lib (i.e. shutil.which).
| if not subprocess.Popen( | ||
| ["flatpak", "remote-ls", "--system", "--updates"], | ||
| stdout=subprocess.PIPE | ||
| ).stdout.read().decode().strip(): |
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.
This should consider exit code (and report as update failure, not "no updates". subprocess.check_output may be helpful.
|
And one more thing, plugins run before normal update. It might be better to run flatpak updates after updating flatpak itself... Idk how to do that best. Either not use plugin for this (which will make progress reporting later easier), or add some option to run plugins after update (plugins_post dir?) |
improves: QubesOS/qubes-issues#2766
requires: QubesOS/qubes-core-agent-linux#604