-
Notifications
You must be signed in to change notification settings - Fork 4
EIM-334: prerequisities check enhancement #342
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: master
Are you sure you want to change the base?
Conversation
Summary of ChangesHello @alirana01, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly improves the robustness and user experience of the prerequisite checking system. Previously, users could get stuck if the system failed to execute shell commands or encountered errors during verification. The changes introduce a more detailed prerequisite check result, including an explicit check for basic shell functionality. This allows the installer to gracefully handle verification failures by offering users the option to skip the check and proceed, thereby enhancing system resilience and preventing installation roadblocks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
alirana01
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.
Initial self review
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.
Code Review
This pull request significantly enhances the prerequisite checking system by adding robust handling for shell execution failures and other verification errors. The backend changes in Rust introduce a more detailed PrerequisitesCheckResult struct, which is a great improvement for communicating verification status to the frontend. The Vue components have been updated effectively to use this new information, providing a better user experience by allowing users to skip the check when verification fails. The consolidation of localization strings is also a welcome improvement for maintainability. My review focuses on further improving code clarity and robustness by addressing some typos, a misleading return value, a hardcoded fallback, and some code duplication.
|
|
||
| /// Checks which prerequisites are missing | ||
| #[tauri::command] | ||
| pub fn check_prequisites(app_handle: AppHandle) -> Vec<String> { |
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.
@Hahihula
The check_prequisites() function returns an empty Vec for both "all prerequisites satisfied" and "verification failed" cases.
In installation.rs (lines 873, 887, 898, 1543), this means if shell verification fails, the code proceeds as if prerequisites are satisfied (a warning is shown to the user, but installation continues).
Is this silent pass-through acceptable for the installation flow? Or should we update these callers to use check_prerequisites_detailed() which distinguishes between "success" and "can't verify"?
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.
In case it's really working as you described than it's definitely bug, but this task was filed because when the check failed there was no way to proceed so i am not sure it's actually working the way you are describing. I will take a closer look.
But you are completely right. we should not silent pass.
Hahihula
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.
Ok, as you correctly found we had a bug there probably. We should not alow silent pass when the prerequisities check fail.
but the same way we are offering the user in the gui wizard in. the case of failed check to continue, we should do the same i nthe cli interactive wizard.
| /// * `Ok(Vec<&'static str>)` - If the function completes successfully, returns a vector of unsatisfied tools. | ||
| /// * `Err(String)` - If an error occurs, returns an error message. | ||
| pub fn check_prerequisites() -> Result<Vec<&'static str>, String> { | ||
| // pub fn check_prerequisites() -> Result<Vec<&'static str>, String> { |
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.
what is this commented out code?
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.
Removed
|
|
||
| /// Checks which prerequisites are missing | ||
| #[tauri::command] | ||
| pub fn check_prequisites(app_handle: AppHandle) -> Vec<String> { |
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.
In case it's really working as you described than it's definitely bug, but this task was filed because when the check failed there was no way to proceed so i am not sure it's actually working the way you are describing. I will take a closer look.
But you are completely right. we should not silent pass.
| /// system prerequisites, including both the list of missing tools and whether | ||
| /// the system was able to run verification commands at all. | ||
| #[derive(Debug, Clone)] | ||
| pub struct PrerequisitesCheckResult { |
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.
aren't the can_verify and shell_failed mutualy exclusive ? maybe we do not need the shell_failed because that is what can_verify=false means ?
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.
shell_failed distinguishes why can_verify is false:
can_verify=false, shell_failed=true→ Shell commands don't work on this systemcan_verify=false, shell_failed=false→ Shell works, but another error occurred (e.g., unsupported package manager)
This allows the UI to show appropriate messages:
- Shell failed → "Basic shell execution failed"
- Other error → "A system error occurred: {specific error}"
Without shell_failed, we couldn't differentiate between these two failure modes.
So basically to relay proper information to user
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.
but than your logic is flawed ... you return
verify_shell_execution -> false for both unsupported OS as well as for failed shell execution
in current implementation they are mutualy exclusive
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.
Yep fixed now need your expert review again please. Callers are already handling this properly
|
@Hahihula can you please review again some more changes around critical parts just need to be sure if I understand this correctly. |
src/components/SimpleSetup.vue
Outdated
| console.log('Default installation path:', installPath.value) | ||
| const versions = await invoke('get_idf_versions', { includeUnstable: false }) | ||
| selectedVersion.value = versions?.[0]?.name || 'v5.5.1' |
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.
i know it was here already, but as you are refactoring this function toss this hardcoded default out. if we could not get_idf_versions, it should fail.
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.
Done
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.
under current logic the can_verify and shell_failed are mutually exclusive, either your logic is flawed or their are redundant.
Also could you please finally stick to the branch naming convention in this repository in next PRs ?
Hahihula
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.
Thanks!
Description
This PR enhances the prerequisites checking system to handle scenarios where the system cannot execute shell commands or encounters errors during verification. Previously, if the shell failed or a system error occurred, users were stuck with no way to proceed.

Changes Summary
Backend (Rust)
verify_shell_execution()to test basic shell execution on each platform:sh -c "echo test"zsh -c "echo test"cmd /c echo testPrerequisitesCheckResultstruct with:missing: Vec<&'static str>— list of missing prerequisitescan_verify: bool— whether the system can run verification commandsshell_failed: bool— whether shell execution specifically failedcheck_prerequisites_with_result()to return detailed verification status--skip-prerequisites-checkflagcan_verifyandshell_failedfieldsFrontend (Vue)
SimpleSetup.vue(Easy Installation) to show a skip button on verification failurePrerequisitesCheck.vue(Expert Installation) to stop auto-navigation and show a skip optionmissingarray)Localization
app.ymlfor shell verification failurescommon.prerequisitesinen.jsonandcn.jsonChecklist
Before submitting a Pull Request, please ensure the following: