mirror of
https://github.com/pnpm/pnpm.git
synced 2026-05-30 03:26:43 -04:00
fix(pacquet): address review round-3 findings in run/dlx
run:
- A failing `test` script now prints "[ELIFECYCLE] Test failed. See above
for more details." instead of the generic exit-code line, matching
pnpm's reportLifecycleError test special-case (reportError.ts:371-378).
- Empty script bodies are skipped in run_stage, matching pnpm's
`!m.scripts[stage]` no-op (runLifecycleHook.ts:100) and the truthiness
gate on pre/post hooks (run.ts:403,411).
dlx:
- Build a fresh build-script allow-list for the cache install (the dlx
packages' aliases + CLI --allow-build) instead of inheriting the
caller project's allow_builds / dangerously_allow_all_builds. Mirrors
pnpm's `allowBuilds = {...resolvedPkgAliases, ...allowBuild}`
(dlx.ts:168). This also makes the cache key (which hashes only pkgs +
CLI allow_build) fully capture the effective build policy.
- Select the default bin among many by the installed package's own
`name` field (scopeless), not the dependency alias it was installed
under — they differ for aliased specs (dlx.ts:286).
- Remove the partially-installed prepare dir when the cache install
fails, instead of leaking it across failed runs (pnpm fs.rm on error).
https://claude.ai/code/session_01PPwhryEFfN4iyZkVsjy2Hf
This commit is contained in:
1
Cargo.lock
generated
1
Cargo.lock
generated
@@ -2087,6 +2087,7 @@ dependencies = [
|
||||
"pacquet-package-manifest",
|
||||
"pacquet-registry",
|
||||
"pacquet-reporter",
|
||||
"pacquet-resolving-parse-wanted-dependency",
|
||||
"pacquet-store-dir",
|
||||
"pacquet-tarball",
|
||||
"pacquet-testing-utils",
|
||||
|
||||
@@ -27,6 +27,7 @@ pacquet-package-manager = { workspace = true }
|
||||
pacquet-package-is-installable = { workspace = true }
|
||||
pacquet-registry = { workspace = true }
|
||||
pacquet-reporter = { workspace = true }
|
||||
pacquet-resolving-parse-wanted-dependency = { workspace = true }
|
||||
pacquet-tarball = { workspace = true }
|
||||
pacquet-diagnostics = { workspace = true }
|
||||
|
||||
|
||||
@@ -8,6 +8,7 @@ use pacquet_crypto_hash::create_short_hash;
|
||||
use pacquet_fs::force_symlink_dir;
|
||||
use pacquet_package_manifest::DependencyGroup;
|
||||
use pacquet_reporter::Reporter;
|
||||
use pacquet_resolving_parse_wanted_dependency::parse_wanted_dependency;
|
||||
use serde_json::{Value, json};
|
||||
use std::{
|
||||
collections::BTreeMap,
|
||||
@@ -163,7 +164,16 @@ impl DlxArgs {
|
||||
None => {
|
||||
let prepare_dir =
|
||||
get_prepare_dir(&dlx_command_cache_dir, SystemTime::now(), std::process::id());
|
||||
install_into_cache::<Reporter>(&prepare_dir, &pkgs, &allow_build, config).await?;
|
||||
if let Err(error) =
|
||||
install_into_cache::<Reporter>(&prepare_dir, &pkgs, &allow_build, config).await
|
||||
{
|
||||
// Don't leave a half-installed prepare dir behind to
|
||||
// accumulate across failed runs. Mirrors pnpm's
|
||||
// `fs.rm(cachedDir, { recursive: true, force: true })`
|
||||
// on install failure (dlx.ts). Best-effort cleanup.
|
||||
let _ = fs::remove_dir_all(&prepare_dir);
|
||||
return Err(error);
|
||||
}
|
||||
// Best-effort: a parallel dlx process may have raced
|
||||
// us to the link. Either link is equally fresh, so
|
||||
// ignore the failure and run from our own prepare dir.
|
||||
@@ -210,8 +220,22 @@ async fn install_into_cache<Reporter: self::Reporter + 'static>(
|
||||
// The cache install is always fresh, so no lockfile is loaded from
|
||||
// the process working directory.
|
||||
config.lockfile = false;
|
||||
// Allow requested packages to run build scripts during the install,
|
||||
// mirroring pnpm's dlx `allowBuilds` handling.
|
||||
// Build a *fresh* allow-list for the throwaway install — the dlx
|
||||
// packages themselves plus the CLI `--allow-build` entries — rather
|
||||
// than inheriting the caller project's `allow_builds` /
|
||||
// `dangerously_allow_all_builds`. Mirrors pnpm's
|
||||
// `allowBuilds = Object.fromEntries([...resolvedPkgAliases, ...allowBuild])`
|
||||
// (dlx.ts:168). Inheriting the caller's policy would run build
|
||||
// scripts the dlx invocation never opted into, and would also leave
|
||||
// the cache key (which hashes only pkgs + CLI allow_build) unable to
|
||||
// distinguish two callers with different policies.
|
||||
config.dangerously_allow_all_builds = false;
|
||||
config.allow_builds.clear();
|
||||
for spec in pkgs {
|
||||
if let Some(alias) = parse_wanted_dependency(spec).alias {
|
||||
config.allow_builds.insert(alias, true);
|
||||
}
|
||||
}
|
||||
for name in allow_build {
|
||||
config.allow_builds.insert(name.clone(), true);
|
||||
}
|
||||
@@ -359,8 +383,14 @@ fn get_bin_name(cached_dir: &Path) -> Result<String, DlxError> {
|
||||
[] => Err(DlxError::NoBin { package: pkg_name }),
|
||||
[bin] => Ok(bin.name.clone()),
|
||||
bins => {
|
||||
let scopeless = scopeless(&pkg_name);
|
||||
if let Some(bin) = bins.iter().find(|bin| bin.name == scopeless) {
|
||||
// The default bin is the one named after the installed
|
||||
// package's own `name` field (scopeless), not the dependency
|
||||
// alias it was installed under. Mirrors `scopeless(manifest.name)`
|
||||
// at dlx.ts:286 — they differ for aliased specs such as
|
||||
// `foo@npm:@scope/realtool`.
|
||||
let manifest_name = manifest.get("name").and_then(Value::as_str).unwrap_or(&pkg_name);
|
||||
let scopeless_name = scopeless(manifest_name);
|
||||
if let Some(bin) = bins.iter().find(|bin| bin.name == scopeless_name) {
|
||||
return Ok(bin.name.clone());
|
||||
}
|
||||
let names = bins.iter().map(|bin| bin.name.as_str()).collect::<Vec<_>>().join(", ");
|
||||
|
||||
@@ -143,6 +143,22 @@ fn get_bin_name_picks_the_scopeless_match_among_many() {
|
||||
assert_eq!(get_bin_name(dir.path()).expect("bin name"), "tool");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn get_bin_name_uses_installed_manifest_name_not_alias() {
|
||||
// The dependency key (`alias`) differs from the installed package's
|
||||
// own `name`. The default bin among many is selected by the manifest
|
||||
// name (`scopeless("@scope/realtool")` == "realtool"), not the alias.
|
||||
let dir = cached_dir_with(
|
||||
"alias",
|
||||
serde_json::json!({
|
||||
"name": "@scope/realtool",
|
||||
"version": "1.0.0",
|
||||
"bin": { "realtool": "r.js", "other": "o.js" },
|
||||
}),
|
||||
);
|
||||
assert_eq!(get_bin_name(dir.path()).expect("bin name"), "realtool");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn get_bin_name_errors_when_no_dependency() {
|
||||
let dir = tempdir().expect("temp dir");
|
||||
|
||||
@@ -200,6 +200,13 @@ fn run_stage(
|
||||
if args.is_empty() && script == "npx only-allow pnpm" {
|
||||
return Ok(());
|
||||
}
|
||||
// An empty script body is a no-op under pnpm, which skips any stage
|
||||
// whose (post-arg) command is falsy (runLifecycleHook.ts:100). pnpm
|
||||
// also gates pre/post on the body being truthy (run.ts:403,411), so
|
||||
// an empty `pre<name>`/`post<name>` never runs.
|
||||
if script.is_empty() {
|
||||
return Ok(());
|
||||
}
|
||||
|
||||
let status = run_script(RunScript {
|
||||
manifest: ctx.manifest.value(),
|
||||
@@ -223,7 +230,14 @@ fn run_stage(
|
||||
|
||||
if !status.success() {
|
||||
let code = status.code().unwrap_or(1);
|
||||
eprintln!("[ELIFECYCLE] Command failed with exit code {code}.");
|
||||
// pnpm's reportLifecycleError special-cases the `test` stage
|
||||
// (reportError.ts:371-378): "Test failed. See above for more
|
||||
// details." rather than the exit-code line.
|
||||
if stage == "test" {
|
||||
eprintln!("[ELIFECYCLE] Test failed. See above for more details.");
|
||||
} else {
|
||||
eprintln!("[ELIFECYCLE] Command failed with exit code {code}.");
|
||||
}
|
||||
std::process::exit(code);
|
||||
}
|
||||
Ok(())
|
||||
|
||||
@@ -228,6 +228,32 @@ fn run_propagates_failing_script_exit_code() {
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// A failing `test` script prints pnpm's stage-specific lifecycle error
|
||||
/// (`Test failed. See above for more details.`) rather than the generic
|
||||
/// exit-code line, matching reportLifecycleError's `test` special case.
|
||||
#[cfg_attr(target_os = "windows", ignore = "uses a POSIX shell `exit` builtin")]
|
||||
#[test]
|
||||
fn run_failing_test_script_prints_test_failed_message() {
|
||||
let CommandTempCwd { pacquet, root, workspace, .. } = CommandTempCwd::init();
|
||||
let manifest = json!({
|
||||
"name": "test",
|
||||
"version": "0.0.0",
|
||||
"scripts": { "test": "exit 1" },
|
||||
})
|
||||
.to_string();
|
||||
fs::write(workspace.join("package.json"), manifest).expect("write package.json");
|
||||
|
||||
let output = pacquet.with_arg("run").with_arg("test").output().expect("spawn pacquet run");
|
||||
assert_eq!(output.status.code(), Some(1), "the script's exit code must propagate");
|
||||
let stderr = String::from_utf8_lossy(&output.stderr);
|
||||
assert!(
|
||||
stderr.contains("Test failed. See above for more details."),
|
||||
"test-stage failure should print pnpm's test message:\n{stderr}",
|
||||
);
|
||||
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// A script that invokes a locally-installed binary resolves it through
|
||||
/// `node_modules/.bin`, which `pnpm run` prepends to `PATH`.
|
||||
#[cfg(unix)]
|
||||
|
||||
Reference in New Issue
Block a user