DO NOT MERGE Idf tools stderr#872
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a transitional CLI shim (eim idf-tools) to expose the legacy idf_tools.py interface, allowing users to manage ESP-IDF tools without Python. It also redirects console logging from stdout to stderr across multiple modules. The review feedback points out several issues and optimization opportunities: fixing the --outdated flag logic to properly filter out up-to-date tools, correcting a logic bug where --no-constraints disables the PyPI mirror, removing a redundant collection conversion, avoiding a potential panic by safely handling non-UTF-8 paths, and removing an unused variable clone inside a closure.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| Ok(ToolStatus::Correct { .. }) => "up-to-date", | ||
| Ok(ToolStatus::DifferentVersion { installed, .. }) => { | ||
| println!( | ||
| "{:<25} {:<12} {:<8} {} (installed: {})", | ||
| tool.name, version_str, version_status, tool.description, installed | ||
| ); | ||
| continue; | ||
| } | ||
| Ok(ToolStatus::Missing) => "missing", | ||
| Err(_) => "error", | ||
| } |
There was a problem hiding this comment.
The --outdated flag is intended to only list tools that are installed but at a different version than recommended. However, if a tool is up-to-date (ToolStatus::Correct), the loop currently assigns "up-to-date" to install_marker and continues to print it at the end of the loop.
To fix this, the loop should continue when ToolStatus::Correct is returned, so that up-to-date tools are omitted from the output when --outdated is active.
Ok(ToolStatus::Correct { .. }) => {
continue;
}
Ok(ToolStatus::DifferentVersion { installed, .. }) => {
println!(
"{:<25} {:<12} {:<8} {} (installed: {})",
tool.name, version_str, version_status, tool.description, installed
);
continue;
}
Ok(ToolStatus::Missing) => "missing",
Err(_) => "error",| let feature_list: Vec<String> = features | ||
| .map(|f| f.split(',').map(|s| s.trim().to_string()).collect()) | ||
| .unwrap_or_default(); | ||
| let mirror = if no_constraints { None } else { pypi_mirror }; |
There was a problem hiding this comment.
There is a logic bug in how the --no-constraints flag is handled. Currently, let mirror = if no_constraints { None } else { pypi_mirror }; sets mirror to None if no_constraints is true, and then passes it as the pypi_mirror argument to install_python_env. This incorrectly disables the PyPI mirror when --no-constraints is passed, and does not actually disable constraints (since install_python_env always downloads and applies the constraints file regardless of this parameter).
To fix this, install_python_env needs to be refactored to support skipping constraints, and the pypi_mirror should not be overridden by the no_constraints flag.
| let mirror = if no_constraints { None } else { pypi_mirror }; | |
| let mirror = pypi_mirror; |
| let installed: HashMap<String, (String, idf_im_lib::idf_tools::Download)> = | ||
| get_list_of_tools_to_download(tools_file.clone(), targets.to_vec(), None) | ||
| .into_iter() | ||
| .collect(); |
There was a problem hiding this comment.
The call to get_list_of_tools_to_download already returns a HashMap<String, (String, Download)>. Collecting it again via .into_iter().collect() is redundant and can be removed.
| let installed: HashMap<String, (String, idf_im_lib::idf_tools::Download)> = | |
| get_list_of_tools_to_download(tools_file.clone(), targets.to_vec(), None) | |
| .into_iter() | |
| .collect(); | |
| let installed = get_list_of_tools_to_download(tools_file.clone(), targets.to_vec(), None); |
| .ok_or_else(|| anyhow!("invalid URL for {tool_name}: {}", dl.url))?; | ||
| let dest = ctx.tools_dist_path.join(filename); | ||
| println!(" -> {tool_name} {version}"); | ||
| idf_im_lib::download_file(&dl.url, dest.to_str().unwrap(), None).await?; |
There was a problem hiding this comment.
Using dest.to_str().unwrap() can cause a panic if the destination path contains non-UTF-8 characters. It is safer to handle the conversion gracefully.
let dest_str = dest.to_str().ok_or_else(|| anyhow!("Destination path is not valid UTF-8"))?;
idf_im_lib::download_file(&dl.url, dest_str, None).await?;| let ctx_clone = ctx.clone(); | ||
| let progress = move |p: DownloadProgress| { | ||
| use DownloadProgress::*; | ||
| match p { | ||
| Start(url) => println!(" -> starting {url}"), | ||
| Progress(cur, total) => { | ||
| if total > 0 { | ||
| println!(" {} / {} bytes", cur, total); | ||
| } | ||
| } | ||
| Indeterminate(cur) => println!(" {cur} bytes"), | ||
| Downloaded(url) => println!(" downloaded {url}"), | ||
| Verified(url) => println!(" verified {url}"), | ||
| Extracted(url, dest) => println!(" extracted {url} -> {dest}"), | ||
| Complete => {} | ||
| Error(e) => eprintln!(" error: {e}"), | ||
| } | ||
| let _ = &ctx_clone; | ||
| }; |
There was a problem hiding this comment.
The ctx_clone variable is cloned and captured by the progress closure, but it is never used except for a dummy reference let _ = &ctx_clone;. Since the closure does not need any context from ctx, both the clone and the dummy reference can be safely removed.
let progress = move |p: DownloadProgress| {
use DownloadProgress::*;
match p {
Start(url) => println!(" -> starting {url}"),
Progress(cur, total) => {
if total > 0 {
println!(" {} / {} bytes", cur, total);
}
}
Indeterminate(cur) => println!(" {cur} bytes"),
Downloaded(url) => println!(" downloaded {url}"),
Verified(url) => println!(" verified {url}"),
Extracted(url, dest) => println!(" extracted {url} -> {dest}"),
Complete => {}
Error(e) => eprintln!(" error: {e}"),
}
};7334bdd to
cfc5917
Compare
test branch