Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 26 additions & 2 deletions crates/pm/src/service/binary.rs
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,10 @@ async fn handle_cypress(
}

pub async fn update_package_binary(dir: &Path, name: &str) -> Result<()> {
if should_skip_binary_mirror() {
return Ok(());
}

let config = load_config().await?;

let mirrors = config["mirrors"]["china"]
Expand Down Expand Up @@ -285,14 +289,21 @@ pub async fn update_package_binary(dir: &Path, name: &str) -> Result<()> {
fn should_skip_binary_mirror() -> bool {
*SKIP_BINARY_MIRROR.get_or_init(|| {
let registry = get_registry();
let skip = is_npm_registry(&registry);
let skip = should_skip_binary_mirror_for_registry(&registry);
if skip {
tracing::debug!("Skipping binary mirror envs for npm registry: {}", registry);
tracing::debug!(
"Skipping binary mirror config for npm registry: {}",
registry
);
}
skip
})
}

fn should_skip_binary_mirror_for_registry(registry: &str) -> bool {
is_npm_registry(registry)
}

pub async fn get_envs() -> Option<&'static Map<String, Value>> {
// Skip binary mirror envs when using official npm registry
if should_skip_binary_mirror() {
Expand Down Expand Up @@ -391,6 +402,19 @@ mod tests {
);
}

#[test]
fn test_should_skip_binary_mirror_for_npm_registry() {
assert!(should_skip_binary_mirror_for_registry(
"https://registry.npmjs.org"
));
assert!(should_skip_binary_mirror_for_registry(
"https://registry.npmjs.org/"
));
assert!(!should_skip_binary_mirror_for_registry(
"https://registry.npmmirror.com"
));
}

#[tokio::test]
async fn test_replace_with_regex() {
let content = "Visit old.com and old.com";
Expand Down
38 changes: 36 additions & 2 deletions crates/pm/src/service/install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -62,10 +62,15 @@ fn should_omit_package(package: &Package, omit: &HashSet<OmitType>) -> bool {
false
}

fn should_update_package_binary(package: &Package, scripts: ScriptPolicy) -> bool {
scripts == ScriptPolicy::Run && package.has_install_scripts()
}

pub async fn install_packages(
groups: &HashMap<usize, Vec<(String, Package)>>,
cwd: &Path,
omit: &HashSet<OmitType>,
scripts: ScriptPolicy,
) -> Result<()> {
use crate::util::cloner::clone_package_once;

Expand Down Expand Up @@ -152,6 +157,7 @@ pub async fn install_packages(
// Check if this is an optional dependency
let is_optional =
package.optional == Some(true) || package.dev_optional == Some(true);
let update_binary = should_update_package_binary(&package, scripts);

let task = tokio::spawn(async move {
if let Err(e) =
Expand All @@ -166,7 +172,10 @@ pub async fn install_packages(
}
PROGRESS_BAR.inc(1);
log_progress(&format!("{name} resolved"));
update_package_binary(&target_path, &name).await
if update_binary {
update_package_binary(&target_path, &name).await?;
}
Comment on lines +175 to +177
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

When an optional dependency fails to update its binary, the error should be handled gracefully to maintain consistency with the cloning step. Currently, a failure in update_package_binary will propagate an error via ?, causing the entire installation task to fail even if is_optional is true. It is recommended to wrap this call in a check that logs a warning for optional dependencies instead of returning an error.

                        if update_binary {
                            if let Err(e) = update_package_binary(&target_path, &name).await {
                                if is_optional {
                                    tracing::warn!("Optional dependency {name} binary update failed (ignored): {e}");
                                } else {
                                    return Err(e);
                                }
                            }
                        }

Ok(())
});
clone_tasks.push(task);
} else {
Expand Down Expand Up @@ -271,7 +280,7 @@ impl InstallService {
}

let link_start = Instant::now();
install_packages(&groups, root_path, omit)
install_packages(&groups, root_path, omit, scripts)
.await
.context("Failed to install packages")?;

Expand Down Expand Up @@ -412,6 +421,31 @@ mod tests {
assert!(should_omit_package(&dev_optional_pkg, &omit_dev_optional));
}

#[test]
fn test_should_update_package_binary() {
let package_with_script = Package {
has_install_script: Some(true),
..Package::default()
};
let package_without_script = Package {
has_install_script: Some(false),
..Package::default()
};

assert!(should_update_package_binary(
&package_with_script,
ScriptPolicy::Run
));
assert!(!should_update_package_binary(
&package_with_script,
ScriptPolicy::Ignore
));
assert!(!should_update_package_binary(
&package_without_script,
ScriptPolicy::Run
));
}

#[test]
fn test_is_optional_dependency() {
// Test helper to verify is_optional detection logic
Expand Down
Loading