Switch packaging from PyInstaller to Nuitka#518
Conversation
Reviewer's GuideSwitches the build system from PyInstaller to Nuitka for both Linux and Windows GitHub Actions workflows, updates dev dependencies accordingly, adds manual workflow triggers, adjusts system/compiler prerequisites, and hardens a destructor against TypeError during temp-file cleanup. Flow diagram for hardened temp file cleanup in destructorflowchart TD
A[WindMainWindow___del__] --> B[Call_kill_temp_file]
B --> C{TypeError_raised?}
C -->|No| D[Destructor_completes_normally]
C -->|Yes| E[Catch_TypeError]
E --> F[Ignore_error_and_continue]
F --> D
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- In the Qt window destructor, catching
TypeErrorand silentlypass-ing risks hiding real bugs; consider either narrowing the condition (e.g., checking the state before callingkill_temp_file) or at least logging the exception so unexpected failures are visible. - The GNU/Linux workflow now assumes a
dnf-based environment for system dependencies; if this runs on standard GitHub-hosted Ubuntu runners, you may want to switch toaptor gate this step to avoid failures on non-Fedora images.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In the Qt window destructor, catching `TypeError` and silently `pass`-ing risks hiding real bugs; consider either narrowing the condition (e.g., checking the state before calling `kill_temp_file`) or at least logging the exception so unexpected failures are visible.
- The GNU/Linux workflow now assumes a `dnf`-based environment for system dependencies; if this runs on standard GitHub-hosted Ubuntu runners, you may want to switch to `apt` or gate this step to avoid failures on non-Fedora images.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
There was a problem hiding this comment.
Code Review
This pull request replaces PyInstaller with Nuitka as a development dependency and updates the lock file. It also adds a try-except block to the del method in gi_loadouts/face/wind/main.py to handle TypeErrors during cleanup. Feedback suggests that using del for resource management is unreliable and recommends refactoring the cleanup logic to use the atexit module instead.
| def __del__(self) -> None: | ||
| kill_temp_file() | ||
| try: | ||
| kill_temp_file() | ||
| except TypeError: | ||
| pass |
There was a problem hiding this comment.
Using __del__ for resource cleanup is generally discouraged as its execution is not guaranteed, especially during interpreter shutdown. This can lead to resource leaks.
A more robust approach is to use the atexit module to register cleanup functions. This ensures that kill_temp_file is called reliably when the program exits.
I recommend this refactoring:
- Add
import atexitat the top ofgi_loadouts/face/wind/main.py. - In the
MainWindow.__init__method, addatexit.register(kill_temp_file). - Remove this
__del__method entirely.
This will make resource cleanup more predictable and robust.
e30ce29 to
392d910
Compare
4d5617a to
59cc050
Compare
Signed-off-by: Akashdeep Dhar <akashdeep.dhar@gmail.com> Assisted-by: Claude Opus 4.6 <noreply@anthropic.com>
sdglitched
left a comment
There was a problem hiding this comment.
LGTM 🚀
I need to know more about Nuitka before commenting on its usability. As the workflow was fine and the compiled object is running, I'll merge this.
Switch packaging from PyInstaller to Nuitka
Assisted-by: Claude Opus 4.6 noreply@anthropic.com
Summary by Sourcery
Switch project packaging and CI workflows from PyInstaller to Nuitka for building distributable binaries.
Enhancements:
Build:
Chores: