Refactor InstallUpdate.read_output() and consolidate dialog windows#69
Merged
Refactor InstallUpdate.read_output() and consolidate dialog windows#69
Conversation
This commit simplifies the package update installation process and standardizes window close behavior across the application.
Frontend refactoring (frontend.py):
- Moved subprocess creation from frontend to backend via command_output()
- Extracted read_output() into focused helper methods:
* process_output() - runs commands and reads stdout line-by-line
* log_failure() - writes error details to update.failed file
* needs_reboot() - checks if installed packages require reboot
* is_pkg_only_update() - detects pkg-only updates
* install_packages() - handles install with retry logic for temp file failures
* fetch_packages() - downloads package updates
* bootstrap_major_upgrade() - bootstraps pkg for major version upgrades
* prepare_backup() - creates ZFS boot environment backups
- Improved returncode==3 retry logic: delete failed packages, collect for
reinstall after upgrade completes (max 5 retries)
- Fixed race condition: replaced `if proc.poll() is not None` with
`if not line: break` for EOF detection, then proc.wait()
- Moved update_progress() from common.py (only used here)
- Fixed bug where clicking "Install update" quit the app in check-now mode
by changing from "destroy" to "delete-event" signal
Dialog consolidation (dialog.py):
- Created BaseDialog class with common initialization and close handling
- All 9 dialog classes now inherit from BaseDialog:
FailedUpdate, RestartSystem, UpdateCompleted, NoUpdateAvailable,
UpdateStationOpen, MirrorSyncing, ServerUnreachable, SomethingIsWrong,
NotRoot
- Standardized close behavior using Data.close_session flag
- Return True from on_close() to prevent GTK double-destroy
- Removed common.py (no longer needed)
Backend additions (backend.py):
- Added command_output() - creates Popen process for real-time output
reading (follows software-station pattern)
Notification fixes (notification.py):
- Fixed MajorUpgradeWindow to use "delete-event" instead of "destroy"
to prevent unexpected app quit when buttons clicked
All windows now use "delete-event" consistently and won't unexpectedly
quit when destroyed programmatically. Code is cleaner, more maintainable,
and follows consistent patterns throughout.
Contributor
Reviewer's GuideRefactors the update installation flow to use backend-managed subprocesses and smaller helper methods, introduces robust retry logic and reboot detection, and consolidates dialog window behavior on a shared base class with consistent GTK delete-event handling, while adding a backend command_output helper and a custom setup.py clean command. Sequence diagram for updated package install and dialog flowsequenceDiagram
actor User
participant UpdateWindow
participant Backend as backend_command_output
participant Pkg as pkg_static
participant Dialog as Dialogs
User->>UpdateWindow: Click Install update
UpdateWindow->>UpdateWindow: read_output(progress)
Note over UpdateWindow: Initialize env, reboot flag, pkg-only flag, option, fraction
alt Backup enabled
UpdateWindow->>UpdateWindow: prepare_backup(progress, fraction)
UpdateWindow->>bectl: get_be_list / destroy_be / create_be
end
alt Major upgrade
UpdateWindow->>UpdateWindow: bootstrap_major_upgrade(env, progress, fraction)
UpdateWindow->>Backend: command_output("env ... pkg bootstrap -f")
Backend-->>UpdateWindow: Popen process
loop Read bootstrap output
UpdateWindow->>Pkg: Read stdout line
UpdateWindow->>UpdateWindow: process_output updates progress
end
alt Bootstrap failed
UpdateWindow->>UpdateWindow: fail = True
UpdateWindow->>UpdateWindow: stop_tread(True, update_pkg, reboot)
UpdateWindow->>Dialogs: FailedUpdate()
UpdateWindow->>UpdateWindow: win.destroy()
UpdateWindow->>UpdateWindow: return
end
end
UpdateWindow->>UpdateWindow: fetch_packages(env, option, packages, progress, fraction)
UpdateWindow->>Backend: command_output("pkg-static upgrade -Fy...")
Backend-->>UpdateWindow: Popen process
loop Read fetch output
UpdateWindow->>Pkg: Read stdout line
UpdateWindow->>UpdateWindow: process_output updates progress
end
alt Fetch failed
UpdateWindow->>UpdateWindow: log_failure(stdout + stderr)
UpdateWindow->>UpdateWindow: fail = True
else Fetch succeeded
UpdateWindow->>UpdateWindow: install_packages(env, option, packages, progress, fraction)
loop Up to 5 retries
UpdateWindow->>Backend: command_output("pkg-static upgrade -y...")
Backend-->>UpdateWindow: Popen process
loop Read install output
UpdateWindow->>Pkg: Read stdout line
UpdateWindow->>UpdateWindow: process_output updates progress
end
alt returncode == 3 and temp file error
UpdateWindow->>Backend: command_output("pkg-static rquery ...")
UpdateWindow->>Backend: command_output("pkg-static delete ...")
alt Delete failed
UpdateWindow->>UpdateWindow: log_failure(delete_stdout + stderr)
UpdateWindow->>UpdateWindow: fail = True
UpdateWindow->>UpdateWindow: break
else Delete succeeded
UpdateWindow->>UpdateWindow: Record package for reinstall
UpdateWindow->>UpdateWindow: Update progress "Removed ... will reinstall"
end
else returncode != 0
UpdateWindow->>UpdateWindow: log_failure(install_stdout + stderr)
UpdateWindow->>UpdateWindow: fail = True
UpdateWindow->>UpdateWindow: break
else Success
UpdateWindow->>UpdateWindow: Update progress "Software packages upgrade completed"
UpdateWindow->>UpdateWindow: break
end
end
alt Packages recorded for reinstall
loop For each package
UpdateWindow->>Backend: command_output("pkg-static install -y package")
Backend-->>UpdateWindow: Popen process
alt Reinstall failed
UpdateWindow->>UpdateWindow: log_failure(reinstall_stdout + stderr)
UpdateWindow->>UpdateWindow: fail = True
UpdateWindow->>UpdateWindow: break
end
end
end
end
UpdateWindow->>UpdateWindow: win.destroy()
UpdateWindow->>UpdateWindow: stop_tread(fail, update_pkg, reboot)
alt fail == True
UpdateWindow->>Dialogs: FailedUpdate()
else fail == False and reboot == True
UpdateWindow->>Dialogs: RestartSystem()
else fail == False and reboot == False
UpdateWindow->>Dialogs: UpdateCompleted()
end
Class diagram for dialog hierarchy and update window helpersclassDiagram
class Data {
}
class BaseDialog {
+Gtk.Window window
+BaseDialog(title: str)
+on_close(_widget: Gtk.Widget, _event)
}
class FailedUpdate {
+FailedUpdate()
}
class RestartSystem {
+RestartSystem()
}
class UpdateCompleted {
+UpdateCompleted()
}
class NoUpdateAvailable {
+NoUpdateAvailable()
}
class UpdateStationOpen {
+UpdateStationOpen()
}
class MirrorSyncing {
+MirrorSyncing()
}
class ServerUnreachable {
+ServerUnreachable()
}
class SomethingIsWrong {
+SomethingIsWrong()
}
class NotRoot {
+NotRoot()
}
class MajorUpgradeWindow {
+MajorUpgradeWindow()
+on_clicked(widget: Gtk.Widget)
+on_close(_widget: Gtk.Widget, _event)
}
class UpdateWindow {
+Gtk.Window window
+delete_event(_widget: Gtk.Widget, _event)
+read_output(progress: Gtk.ProgressBar)
+install_packages(env: str, option: str, packages: str, progress: Gtk.ProgressBar, fraction: float) bool
+fetch_packages(env: str, option: str, packages: str, progress: Gtk.ProgressBar, fraction: float) bool
+bootstrap_major_upgrade(env: str, progress: Gtk.ProgressBar, fraction: float) bool
+prepare_backup(progress: Gtk.ProgressBar, fraction: float)
+stop_tread(fail: bool, update_pkg: bool, reboot: bool)
+should_destroy_be(be_line: str, today_str: str) bool
+log_failure(text: str)
+process_output(command: str, progress: Gtk.ProgressBar, fraction: float) tuple
+needs_reboot() bool
+is_pkg_only_update() tuple
}
class NotificationManager {
+notify()
+on_activated(notification, _action_name)
}
class MajorUpgradeStatusIcon {
+left_click(status_icon: Gtk.StatusIcon)
}
class Backend {
+run_command(command: str, check: bool) CompletedProcess
+command_output(command: str) Popen
+check_for_update() bool
+get_default_repo_url() str
+get_abi_upgrade() str
+get_current_abi() str
+get_pkg_upgrade(option: str) str
}
class Utils {
+update_progress(progress: Gtk.ProgressBar, fraction: float, text: str)
}
BaseDialog <|-- FailedUpdate
BaseDialog <|-- RestartSystem
BaseDialog <|-- UpdateCompleted
BaseDialog <|-- NoUpdateAvailable
BaseDialog <|-- UpdateStationOpen
BaseDialog <|-- MirrorSyncing
BaseDialog <|-- ServerUnreachable
BaseDialog <|-- SomethingIsWrong
BaseDialog <|-- NotRoot
UpdateWindow ..> Backend : uses
UpdateWindow ..> Utils : uses
FailedUpdate ..> Backend : uses get_detail
RestartSystem ..> Backend : uses on_reboot
NotificationManager ..> MajorUpgradeWindow : creates
NotificationManager ..> UpdateWindow : creates via StartCheckUpdate
MajorUpgradeWindow ..> Data : reads flags
BaseDialog ..> Data : reads close_session
UpdateWindow ..> Data : reads and writes update state
Backend ..> Data : reads and writes update metadata
Flow diagram for install_packages retry and reinstall logicflowchart TD
A_Start["install_packages(env, option, packages, progress, fraction)"] --> B_Init["Show 'Package updates downloaded' and 'Installing package updates'"]
B_Init --> C_Setup["packages_to_reinstall = []\nmax_retries = 5"]
C_Setup --> D_LoopStart{Retry < max_retries}
D_LoopStart -->|Yes| E_RunInstall["process_output('pkg-static upgrade -y' + option + packages)"]
E_RunInstall --> F_CheckRC{return_code == 3
& temp file error}
F_CheckRC -->|Yes| G_ParseFailed["Extract failed_package from install_text"]
G_ParseFailed --> H_QueryName["command_output('pkg-static rquery ...')"]
H_QueryName --> I_Delete["process_output('pkg-static delete -y package_name')"]
I_Delete --> J_DeleteOK{return_code == 0}
J_DeleteOK -->|No| K_LogDeleteFailure["log_failure(delete_text + stderr_text)"]
K_LogDeleteFailure --> L_Fail["fail = True\nreturn False"]
J_DeleteOK -->|Yes| M_RecordReinstall["packages_to_reinstall.append(package_name)"]
M_RecordReinstall --> N_UpdateMsg["Update progress: 'Removed failed_package, will reinstall after upgrade'"]
N_UpdateMsg --> O_NextRetry["retry += 1"]
O_NextRetry --> D_LoopStart
F_CheckRC -->|No| P_CheckNonZero{return_code != 0}
P_CheckNonZero -->|Yes| Q_LogInstallFailure["log_failure(install_text + stderr_text)"]
Q_LogInstallFailure --> L_Fail
P_CheckNonZero -->|No_Success| R_SuccessMsg["Update progress: 'Software packages upgrade completed'"]
R_SuccessMsg --> S_Break["Break retry loop"]
D_LoopStart -->|No_max_retries| T_MaxRetries["log_failure(install_text + stderr_text)"]
T_MaxRetries --> L_Fail
S_Break --> U_ReinstallLoopStart{packages_to_reinstall not empty}
U_ReinstallLoopStart -->|Yes| V_ForEach["For each package_name in packages_to_reinstall"]
V_ForEach --> W_ReinstallMsg["Update progress: 'Reinstalling package_name'"]
W_ReinstallMsg --> X_RunReinstall["process_output('pkg-static install -y package_name')"]
X_RunReinstall --> Y_ReinstallOK{return_code == 0}
Y_ReinstallOK -->|No| Z_LogReinstallFailure["log_failure(reinstall_text + stderr_text)"]
Z_LogReinstallFailure --> L_Fail
Y_ReinstallOK -->|Yes| AA_NextPackage["Next package"]
AA_NextPackage --> U_ReinstallLoopStart
U_ReinstallLoopStart -->|No| AB_Return["return True if last return_code == 0 else False"]
L_Fail --> AC_End["Return False"]
AB_Return --> AD_End["End install_packages"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 security issue, 1 other issue, and left some high level feedback:
Security issues:
- Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- The new
install_packages()retry flow makes it hard to reason about failure cases: if the delete of a failed package returns a non‑zero code youbreakthe retry loop but still proceed into the reinstall loop and finally returnreturn_code == 0(wherereturn_coderefers to the lastpkg-static upgradecall, not the delete); consider explicitly treating delete failures as a hard error (setfail/returnFalse) and skipping the reinstall loop in that case. process_output()always appends stderr to stdout and returns both, but the callers sometimes only log the concatenation without preserving which phase failed (bootstrap vs fetch vs install); if you intend to keep a singleupdate.failedfile, consider prefixing sections or including the command that failed so that downstream troubleshooting can distinguish which step produced which output.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `install_packages()` retry flow makes it hard to reason about failure cases: if the delete of a failed package returns a non‑zero code you `break` the retry loop but still proceed into the reinstall loop and finally return `return_code == 0` (where `return_code` refers to the last `pkg-static upgrade` call, not the delete); consider explicitly treating delete failures as a hard error (set `fail`/return `False`) and skipping the reinstall loop in that case.
- `process_output()` always appends stderr to stdout and returns both, but the callers sometimes only log the concatenation without preserving which phase failed (bootstrap vs fetch vs install); if you intend to keep a single `update.failed` file, consider prefixing sections or including the command that failed so that downstream troubleshooting can distinguish which step produced which output.
## Individual Comments
### Comment 1
<location> `update_station/backend.py:77-81` </location>
<code_context>
+ )
+
+
def check_for_update() -> bool:
"""
Check if there is an update.
+
:return: True if there is an update else False.
"""
kernel_version_change()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** The check_for_update return type/docstring suggest a strict bool, but callers rely on a third "None" state.
In `frontend.StartCheckUpdate.check_for_update`, the code treats `check_for_update()` as returning `True`, `False`, or `None` (`elif update_available is not None:` + `else`). This conflicts with the `-> bool` signature and docstring that promise only a boolean. Please either (a) change the return type/docstring to something like `Optional[bool]` with `None` meaning error/unknown, or (b) adjust the implementation/callers so the function truly always returns a `bool` and handles errors separately.
Suggested implementation:
```python
def check_for_update() -> "Optional[bool]":
"""
Check if there is an update.
:return: True if there is an update, False if there is no update, or None if
the update status could not be determined (e.g. due to an error).
"""
kernel_version_change()
```
1. Ensure `Optional` is imported from `typing` in this module, e.g. `from typing import Optional`. If this file already uses `from __future__ import annotations`, you can keep the forward reference `"Optional[bool]"`; otherwise, you may replace it with `Optional[bool]` after adding the import.
2. If there are any type hints or stubs (e.g. in `.pyi` files or type-checking-only blocks) that declare `check_for_update` as returning `bool`, update them to `Optional[bool]` as well.
3. If this function is re-exported or documented elsewhere with a hard `bool` return type, those references should be updated to describe the tri-state (`True`/`False`/`None`) behavior.
</issue_to_address>
### Comment 2
<location> `update_station/backend.py:67-74` </location>
<code_context>
return Popen(
command,
shell=True,
stdout=PIPE,
stderr=PIPE,
close_fds=True,
universal_newlines=True
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'Popen' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This commit simplifies the package update installation process and standardizes window close behavior across the application.
Frontend refactoring (frontend.py):
if proc.poll() is not Nonewithif not line: breakfor EOF detection, then proc.wait()Dialog consolidation (dialog.py):
Backend additions (backend.py):
Notification fixes (notification.py):
All windows now use "delete-event" consistently and won't unexpectedly quit when destroyed programmatically. Code is cleaner, more maintainable, and follows consistent patterns throughout.
Summary by Sourcery
Refactor the update installation flow and dialog handling to centralize subprocess management, improve robustness of package upgrades, and standardize window close behavior across the application.
New Features:
Bug Fixes:
Enhancements:
Build: