EIM-395 the offline installer should now fail if there are no wheels #330
Conversation
Summary of ChangesHello @Hahihula, 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 of the offline installer by implementing more rigorous error handling and failure conditions. The installer will now correctly identify and report when critical steps, such as downloading Python wheels or processing specific ESP-IDF versions, fail. This prevents silent failures and ensures that the build process terminates with an error if an incomplete or non-functional installer would otherwise be produced, leading to a more reliable and predictable build outcome. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
The pull request aims to make the offline installer fail if it cannot download any Python wheels. The changes introduce logic to track failures during wheel downloads for different Python and ESP-IDF versions. If all Python wheel downloads fail for a given ESP-IDF version, that version is marked as failed. If any ESP-IDF version fails, the entire process now exits with an error code. This correctly implements the intended behavior.
However, I've found a few critical issues that will cause compilation errors, such as a non-mutable variable being modified and the use of an undefined system module. There's also a logic bug that would cause an error when no Python versions are specified for wheel generation, and a minor issue with an unused import. I've provided suggestions to fix these issues.
| }; | ||
|
|
||
| // ITERATE OVER EACH VERSION AND BUILD SEPARATE ARCHIVE | ||
| let failed_versions = vec![]; |
There was a problem hiding this comment.
The failed_versions vector is modified later in the code (e.g., line 681), so it needs to be declared as mutable. This will cause a compilation error as it is.
| let failed_versions = vec![]; | |
| let mut failed_versions = vec![]; |
| return; | ||
| } else { | ||
| error!("Some versions failed to process: {:?}", failed_versions); | ||
| system.exit(1); |
There was a problem hiding this comment.
The system alias is not defined. This will cause a compilation error. You should use std::process::exit(1) directly.
| system.exit(1); | |
| std::process::exit(1); |
| if versions_failed.len() >= python_versions.len() { | ||
| Err(format!( | ||
| "Failed to download wheels for Python versions: {:?}", | ||
| versions_failed.join(", ") | ||
| )) | ||
| } else { | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
There are a couple of issues with this error handling logic:
- Edge Case with Empty
python_versions: Ifpython_versionsis empty,versions_failed.len()will be 0, andpython_versions.len()will also be 0. The condition0 >= 0will be true, causing the function to incorrectly return an error when it should succeed. - Error Message Formatting: The format specifier
{:?}is used withversions_failed.join(", "), which returns aString. This will wrap the list of versions in quotes in the final error message (e.g.,"3.10, 3.11"). Using{}would be more appropriate for cleaner output.
I suggest modifying the condition to handle the empty case correctly and adjusting the format specifier. It's also good practice to check if versions_failed is not empty before comparing lengths.
| if versions_failed.len() >= python_versions.len() { | |
| Err(format!( | |
| "Failed to download wheels for Python versions: {:?}", | |
| versions_failed.join(", ") | |
| )) | |
| } else { | |
| Ok(()) | |
| } | |
| if !versions_failed.is_empty() && versions_failed.len() == python_versions.len() { | |
| Err(format!( | |
| "Failed to download wheels for all specified Python versions: {}", | |
| versions_failed.join(", ") | |
| )) | |
| } else { | |
| Ok(()) | |
| } |
| use log::error; | ||
| use log::info; | ||
| use log::warn; | ||
| use tauri::http::version; |
There was a problem hiding this comment.
This import of tauri::http::version appears to be unused in the file and can be removed to keep the code clean.
Hahihula
force-pushed
the
EIM-395-offline-installer-builder-now-fails-if-no-wheels
branch
from
a543591 to
40780fb
Compare
No description provided.