Skip to content

perf(pm): skip binary mirror work when scripts are ignored#2841

Closed
killagu wants to merge 1 commit into
nextfrom
agent/egg-dev/3bb639ad-1777304512
Closed

perf(pm): skip binary mirror work when scripts are ignored#2841
killagu wants to merge 1 commit into
nextfrom
agent/egg-dev/3bb639ad-1777304512

Conversation

@killagu
Copy link
Copy Markdown
Contributor

@killagu killagu commented Apr 27, 2026

Summary

  • skip binary mirror package rewrites when install scripts are ignored
  • skip binary mirror config loading for the official npm registry
  • add focused tests for the skip predicates

Verification

  • cargo fmt
  • git diff --check
  • cargo test -p utoo-pm test_should_ -- --nocapture (blocked: next.js/turbopack submodule files missing; submodule fetch/checkout did not populate next.js in this workspace)

Risk

  • Binary mirror rewriting now only runs when lifecycle scripts will run and the package declares install scripts. This should preserve scriptful installs while removing work from --ignore-scripts benchmark paths.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a 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 implements conditional binary mirror skipping for the official npm registry and refactors the installation service to only update package binaries when install scripts are enabled. Feedback was provided to handle errors gracefully for optional dependencies during the binary update process to ensure that failures in non-essential packages do not interrupt the overall installation.

Comment on lines +175 to +177
if update_binary {
update_package_binary(&target_path, &name).await?;
}
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);
                                }
                            }
                        }

@elrrrrrrr
Copy link
Copy Markdown
Contributor

Closing as stale: this draft is a one-off agent experiment from 2026-04-27 with no follow-up, and overlaps with sibling PRs exploring the same optimization. Reopen if revisited.

@elrrrrrrr elrrrrrrr closed this May 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants