-
-
Notifications
You must be signed in to change notification settings - Fork 948
feat(node): suggest setting node.flavor if the flavor is not found in mirror #8206
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ use crate::build_time::built_info; | |||||||||||||||||||||||||||||||||||
| use crate::cache::CacheManagerBuilder; | ||||||||||||||||||||||||||||||||||||
| use crate::cli::args::BackendArg; | ||||||||||||||||||||||||||||||||||||
| use crate::cmd::CmdLineRunner; | ||||||||||||||||||||||||||||||||||||
| use crate::config::settings::DEFAULT_NODE_MIRROR_URL; | ||||||||||||||||||||||||||||||||||||
| use crate::config::{Config, Settings}; | ||||||||||||||||||||||||||||||||||||
| use crate::file::{TarFormat, TarOptions}; | ||||||||||||||||||||||||||||||||||||
| use crate::http::{HTTP, HTTP_FETCH}; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -157,15 +158,27 @@ impl NodePlugin { | |||||||||||||||||||||||||||||||||||
| ) -> Result<()> { | ||||||||||||||||||||||||||||||||||||
| debug!("{:?}: we will fetch the source and compile", self); | ||||||||||||||||||||||||||||||||||||
| let tarball_name = &opts.source_tarball_name; | ||||||||||||||||||||||||||||||||||||
| self.fetch_tarball( | ||||||||||||||||||||||||||||||||||||
| ctx, | ||||||||||||||||||||||||||||||||||||
| tv, | ||||||||||||||||||||||||||||||||||||
| ctx.pr.as_ref(), | ||||||||||||||||||||||||||||||||||||
| &opts.source_tarball_url, | ||||||||||||||||||||||||||||||||||||
| &opts.source_tarball_path, | ||||||||||||||||||||||||||||||||||||
| &opts.version, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| .await?; | ||||||||||||||||||||||||||||||||||||
| if let Err(err) = self | ||||||||||||||||||||||||||||||||||||
| .fetch_tarball( | ||||||||||||||||||||||||||||||||||||
| ctx, | ||||||||||||||||||||||||||||||||||||
| tv, | ||||||||||||||||||||||||||||||||||||
| ctx.pr.as_ref(), | ||||||||||||||||||||||||||||||||||||
| &opts.source_tarball_url, | ||||||||||||||||||||||||||||||||||||
| &opts.source_tarball_path, | ||||||||||||||||||||||||||||||||||||
| &opts.version, | ||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||
| .await | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| if let Some(reqwest_err) = err.root_cause().downcast_ref::<reqwest::Error>() | ||||||||||||||||||||||||||||||||||||
| && reqwest_err.status() == Some(reqwest::StatusCode::NOT_FOUND) | ||||||||||||||||||||||||||||||||||||
| && let Ok(Some(msg)) = self | ||||||||||||||||||||||||||||||||||||
| .suggest_available_flavors(&opts.version, &Settings::get()) | ||||||||||||||||||||||||||||||||||||
| .await | ||||||||||||||||||||||||||||||||||||
| { | ||||||||||||||||||||||||||||||||||||
| return Err(eyre::eyre!("{err}\n{msg}")); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| return Err(err); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
| ctx.pr.next_operation(); | ||||||||||||||||||||||||||||||||||||
| ctx.pr.set_message(format!("extract {tarball_name}")); | ||||||||||||||||||||||||||||||||||||
| file::remove_all(&opts.build_dir)?; | ||||||||||||||||||||||||||||||||||||
|
|
@@ -399,6 +412,64 @@ impl NodePlugin { | |||||||||||||||||||||||||||||||||||
| .join(&format!("v{v}/SHASUMS256.txt"))?; | ||||||||||||||||||||||||||||||||||||
| Ok(url) | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| async fn suggest_available_flavors( | ||||||||||||||||||||||||||||||||||||
| &self, | ||||||||||||||||||||||||||||||||||||
| v: &str, | ||||||||||||||||||||||||||||||||||||
| settings: &Settings, | ||||||||||||||||||||||||||||||||||||
| ) -> Result<Option<String>> { | ||||||||||||||||||||||||||||||||||||
| let base = settings.node.mirror_url(); | ||||||||||||||||||||||||||||||||||||
| // If using default mirror, we don't need to suggest anything as it's likely a real 404 | ||||||||||||||||||||||||||||||||||||
| if base.to_string() == DEFAULT_NODE_MIRROR_URL { | ||||||||||||||||||||||||||||||||||||
| return Ok(None); | ||||||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| let versions: Vec<NodeVersion> = HTTP_FETCH | ||||||||||||||||||||||||||||||||||||
| .json(base.join("index.json")?) | ||||||||||||||||||||||||||||||||||||
| .await | ||||||||||||||||||||||||||||||||||||
| .unwrap_or_default(); | ||||||||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This call to
Suggested change
|
||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||
| if let Some(version) = versions.iter().find(|nv| { | ||||||||||||||||||||||||||||||||||||
|
Comment on lines
+429
to
+432
|
||||||||||||||||||||||||||||||||||||
| .await | |
| .unwrap_or_default(); | |
| if let Some(version) = versions.iter().find(|nv| { | |
| let versions: Vec<NodeVersion> = match HTTP_FETCH | |
| .json(base.join("index.json")?) | |
| .await | |
| { | |
| Ok(versions) => versions, | |
| Err(err) => { | |
| eprintln!( | |
| "Warning: failed to fetch Node mirror index.json from {}: {err}", | |
| base | |
| ); | |
| Vec::new() | |
| } | |
| }; |
Copilot
AI
Feb 17, 2026
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.
The version comparison logic includes format!("v{v}.") which appears to be checking for a version string ending with a period. This seems incorrect as version strings typically don't end with a period. This condition would match versions like "v20.0.0." which is unlikely to be a valid version format. Consider removing this comparison or clarifying the intended behavior if there's a specific use case for versions ending with periods.
| let os = os(); | |
| nv.version == format!("v{v}") || nv.version == v |
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.
The URL comparison uses string comparison which may not be reliable for URLs. The URL
base.to_string()could have variations like trailing slashes or different representations that would still be semantically equivalent. Consider using URL comparison directly or normalizing both URLs before comparison. For example, if one has a trailing slash and the other doesn't, they would be considered different even though they represent the same resource.