mirror of
https://github.com/pnpm/pnpm.git
synced 2026-07-14 01:32:40 -04:00
fix(cli): accept top-level --if-present and stop workspace installs scaffolding a root manifest (#12919)
Accept --if-present ahead of the script name, the way pnpm's option parser does. The repo's own test-pkgs-branch script uses that spelling, so every PR TS CI job died at argument parsing under the pacquet-based pnpm v12. Declared top-level like the other run-scoped options but not clap-global (run/stop/restart declare their own --if-present), merged with the subcommand flags, validated like --resume-from/--no-bail. Rejected for exec like pnpm, where dispatch would drop it silently. Never scaffold a missing manifest inside a workspace. Gate State::init's no-scaffold path on workspace membership (config.workspace_dir) rather than a pnpm-workspace.yaml beside the missing manifest, so installs in workspace member directories can't scaffold a package.json either — pnpm never creates one anywhere in a workspace. Keep an empty object as the in-memory stand-in instead of the init template: pnpm's add at a rootless workspace root persists a manifest holding only the added dependencies, and the template's failing test script would otherwise ride along on any explicit save. Together with pnpm/pnpm#12914 this unblocks the ci:test-branch job once a pnpm v12 alpha carrying both is released and packageManager is bumped.
This commit is contained in:
@@ -182,6 +182,15 @@ pub struct CliArgs {
|
||||
/// Recursive only: keep going after a project fails.
|
||||
#[clap(long = "no-bail", global = true, hide = true)]
|
||||
pub no_bail: bool,
|
||||
|
||||
/// Avoid exiting with a non-zero exit code when the script is
|
||||
/// undefined, accepted ahead of the script name
|
||||
/// (`pnpm --if-present test`) the way pnpm's option parser does.
|
||||
/// Deliberately not `global = true`: `run` / `stop` / `restart`
|
||||
/// declare their own `--if-present`, and a propagated global flag
|
||||
/// would collide with theirs.
|
||||
#[clap(long = "if-present", hide = true)]
|
||||
pub if_present: bool,
|
||||
}
|
||||
|
||||
fn parse_store_dir(value: &str) -> Result<PathBuf, std::convert::Infallible> {
|
||||
@@ -199,6 +208,9 @@ impl CliArgs {
|
||||
if self.no_bail {
|
||||
self.validate_run_scoped_global_option("--no-bail")?;
|
||||
}
|
||||
if self.if_present {
|
||||
self.validate_if_present_top_level_option()?;
|
||||
}
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -216,6 +228,20 @@ impl CliArgs {
|
||||
}
|
||||
}
|
||||
|
||||
/// `restart` also runs scripts and accepts its own `--if-present`,
|
||||
/// so the top-level spelling is valid for it too — unlike the
|
||||
/// recursive-only flags, which `restart` rejects. `exec` is the
|
||||
/// reverse: it takes the recursive-only flags but runs arbitrary
|
||||
/// commands rather than scripts, so pnpm rejects `--if-present`
|
||||
/// for it and pacquet must too.
|
||||
fn validate_if_present_top_level_option(&self) -> Result<(), clap::Error> {
|
||||
match self.command {
|
||||
CliCommand::Restart(_) => Ok(()),
|
||||
CliCommand::Exec(_) => Err(Self::unexpected_argument_error("--if-present")),
|
||||
_ => self.validate_run_scoped_global_option("--if-present"),
|
||||
}
|
||||
}
|
||||
|
||||
fn validate_run_scoped_global_option(&self, option: &str) -> Result<(), clap::Error> {
|
||||
if matches!(
|
||||
self.command,
|
||||
@@ -228,8 +254,12 @@ impl CliArgs {
|
||||
) {
|
||||
return Ok(());
|
||||
}
|
||||
Err(Self::command()
|
||||
.error(ErrorKind::UnknownArgument, format!("unexpected argument '{option}' found")))
|
||||
Err(Self::unexpected_argument_error(option))
|
||||
}
|
||||
|
||||
fn unexpected_argument_error(option: &str) -> clap::Error {
|
||||
Self::command()
|
||||
.error(ErrorKind::UnknownArgument, format!("unexpected argument '{option}' found"))
|
||||
}
|
||||
|
||||
fn validate_report_summary_global_option(&self) -> Result<(), clap::Error> {
|
||||
|
||||
@@ -36,6 +36,9 @@ pub(crate) struct RunCtx<'a> {
|
||||
pub(crate) recursive_report_summary: bool,
|
||||
pub(crate) recursive_no_bail: bool,
|
||||
pub(crate) recursive_sort: bool,
|
||||
/// The top-level `--if-present` spelling (`pnpm --if-present test`);
|
||||
/// merged with the flag the script subcommands declare themselves.
|
||||
pub(crate) if_present: bool,
|
||||
pub(crate) config: &'a (dyn Fn() -> miette::Result<&'static mut Config> + Sync),
|
||||
/// Like [`Self::config`] but anchored at the pnpm home dir instead of
|
||||
/// `--dir`, so a `-g` install can't inherit the caller project's
|
||||
@@ -139,6 +142,7 @@ impl CliArgs {
|
||||
resume_from,
|
||||
report_summary,
|
||||
no_bail,
|
||||
if_present,
|
||||
} = self;
|
||||
|
||||
// Canonicalize `--dir` so the bunyan-envelope `prefix` emitted by
|
||||
@@ -259,6 +263,7 @@ impl CliArgs {
|
||||
recursive_report_summary: report_summary,
|
||||
recursive_no_bail: no_bail,
|
||||
recursive_sort: !no_sort,
|
||||
if_present,
|
||||
config: &config,
|
||||
global_config: &global_config,
|
||||
state: &state,
|
||||
|
||||
@@ -88,6 +88,7 @@ fn with_recursive_run_options(ctx: &RunCtx<'_>, mut args: RunArgs) -> RunArgs {
|
||||
args.report_summary = ctx.recursive_report_summary;
|
||||
args.no_bail = ctx.recursive_no_bail;
|
||||
args.sort = ctx.recursive_sort;
|
||||
args.if_present |= ctx.if_present;
|
||||
args
|
||||
}
|
||||
|
||||
@@ -103,7 +104,8 @@ pub(super) fn start<'a>(ctx: &RunCtx<'a>) -> miette::Result<CommandFuture<'a>> {
|
||||
run(ctx, run_args_for_script("start", false))
|
||||
}
|
||||
|
||||
pub(super) fn stop<'a>(ctx: &RunCtx<'a>, args: StopArgs) -> miette::Result<CommandFuture<'a>> {
|
||||
pub(super) fn stop<'a>(ctx: &RunCtx<'a>, mut args: StopArgs) -> miette::Result<CommandFuture<'a>> {
|
||||
args.if_present |= ctx.if_present;
|
||||
if ctx.recursive {
|
||||
run(ctx, args.into_run_args())
|
||||
} else {
|
||||
@@ -114,8 +116,9 @@ pub(super) fn stop<'a>(ctx: &RunCtx<'a>, args: StopArgs) -> miette::Result<Comma
|
||||
|
||||
pub(super) fn restart<'a>(
|
||||
ctx: &RunCtx<'a>,
|
||||
args: RestartArgs,
|
||||
mut args: RestartArgs,
|
||||
) -> miette::Result<CommandFuture<'a>> {
|
||||
args.if_present |= ctx.if_present;
|
||||
args.run(ctx.dir, (ctx.config)()?, matches!(ctx.reporter, ReporterType::Silent))?;
|
||||
Ok(Box::pin(std::future::ready(Ok(()))))
|
||||
}
|
||||
|
||||
@@ -129,6 +129,61 @@ fn script_scoped_global_flags_parse_before_script_commands() {
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn if_present_flag_parses_before_script_commands() {
|
||||
for argv in [
|
||||
["pacquet", "--if-present", "run", "build"].as_slice(),
|
||||
["pacquet", "--if-present", "test"].as_slice(),
|
||||
["pacquet", "--if-present", "start"].as_slice(),
|
||||
["pacquet", "--if-present", "stop"].as_slice(),
|
||||
["pacquet", "--if-present", "restart"].as_slice(),
|
||||
] {
|
||||
let parsed = CliArgs::try_parse_from(argv).expect("parses top-level --if-present");
|
||||
assert!(parsed.if_present);
|
||||
parsed.validate_command_scoped_global_options().expect("script command accepts flag");
|
||||
}
|
||||
}
|
||||
|
||||
/// The exact shape of the repo's own `test-pkgs-branch` script.
|
||||
#[test]
|
||||
fn if_present_flag_parses_before_fallback_command() {
|
||||
let parsed = CliArgs::try_parse_from([
|
||||
"pacquet",
|
||||
"--workspace-concurrency=1",
|
||||
"--filter=...[origin/main]",
|
||||
"--no-sort",
|
||||
"--if-present",
|
||||
".test",
|
||||
])
|
||||
.expect("parses top-level --if-present with a fallback script");
|
||||
assert!(parsed.if_present);
|
||||
assert!(
|
||||
matches!(&parsed.command, CliCommand::External(command) if command.as_slice() == [".test"]),
|
||||
);
|
||||
parsed.validate_command_scoped_global_options().expect("fallback command accepts flag");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn if_present_flag_rejects_non_script_commands() {
|
||||
for argv in [
|
||||
["pacquet", "--if-present", "install"].as_slice(),
|
||||
["pacquet", "--if-present", "publish"].as_slice(),
|
||||
["pacquet", "--if-present", "exec", "ls"].as_slice(),
|
||||
] {
|
||||
let parsed =
|
||||
CliArgs::try_parse_from(argv).expect("global parser accepts compatibility flag");
|
||||
let err = parsed
|
||||
.validate_command_scoped_global_options()
|
||||
.expect_err("non-script command rejects flag");
|
||||
assert_eq!(err.kind(), clap::error::ErrorKind::UnknownArgument);
|
||||
}
|
||||
// Not `global = true` (the script subcommands declare their own
|
||||
// `--if-present`), so after a non-script subcommand it fails at
|
||||
// parse time instead of validation.
|
||||
CliArgs::try_parse_from(["pacquet", "install", "--if-present"])
|
||||
.expect_err("install rejects --if-present at parse time");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn report_summary_global_flag_parses_for_publish() {
|
||||
for argv in [
|
||||
|
||||
@@ -76,7 +76,7 @@ impl State {
|
||||
};
|
||||
Ok(State {
|
||||
config,
|
||||
manifest: load_or_create_manifest(manifest_path)?,
|
||||
manifest: load_or_create_manifest(manifest_path, config)?,
|
||||
lockfile,
|
||||
http_client: std::sync::Arc::new(
|
||||
ThrottledClient::for_installs(
|
||||
@@ -104,7 +104,16 @@ impl State {
|
||||
/// — pnpm reads every manifest base name. Alternate manifests stay
|
||||
/// read-only (see `pacquet_workspace::project_manifest`); commands
|
||||
/// that write the manifest back still require `package.json`.
|
||||
fn load_or_create_manifest(manifest_path: PathBuf) -> Result<PackageManifest, InitStateError> {
|
||||
///
|
||||
/// Inside a workspace, a missing root manifest is tolerated rather
|
||||
/// than scaffolded: pnpm installs such a workspace with no root
|
||||
/// importer and never creates a `package.json` there, so the stand-in
|
||||
/// manifest stays in memory only. Commands that save the manifest
|
||||
/// (`add`) still persist it through their explicit save.
|
||||
fn load_or_create_manifest(
|
||||
manifest_path: PathBuf,
|
||||
config: &Config,
|
||||
) -> Result<PackageManifest, InitStateError> {
|
||||
if !manifest_path.exists() {
|
||||
let project_dir = manifest_path.parent().expect("manifest path always has a parent dir");
|
||||
if let Some((_, manifest)) = pacquet_workspace::try_read_project_manifest(project_dir)
|
||||
@@ -112,15 +121,8 @@ fn load_or_create_manifest(manifest_path: PathBuf) -> Result<PackageManifest, In
|
||||
{
|
||||
return Ok(manifest);
|
||||
}
|
||||
// A workspace root defined by `pnpm-workspace.yaml` alone is legal
|
||||
// without a root manifest, and pnpm never scaffolds one there. A
|
||||
// scaffolded root `package.json` would turn the root into a project —
|
||||
// with the init template's failing `test` script — that recursive
|
||||
// selection (e.g. a `[<since>]` filter picking up a root-level change)
|
||||
// would then run. Use the scaffold in memory without persisting it.
|
||||
if project_dir.join("pnpm-workspace.yaml").exists() {
|
||||
let value = PackageManifest::init_value_for(&manifest_path);
|
||||
return Ok(PackageManifest::from_value(manifest_path, value));
|
||||
if config.workspace_dir.is_some() {
|
||||
return Ok(PackageManifest::from_value(manifest_path, serde_json::json!({})));
|
||||
}
|
||||
}
|
||||
manifest_path.pipe(PackageManifest::create_if_needed).map_err(InitStateError::Manifest)
|
||||
|
||||
@@ -177,6 +177,26 @@ fn run_with_if_present_is_a_noop_for_missing_script() {
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// pnpm also accepts `--if-present` ahead of the script name
|
||||
/// (`pnpm --if-present <script>`), where the script dispatches through
|
||||
/// the shorthand fallback instead of an explicit `run`. The missing
|
||||
/// script must be the same clean no-op — not an exec fallback error.
|
||||
#[test]
|
||||
fn top_level_if_present_is_a_noop_for_missing_script() {
|
||||
let CommandTempCwd { pacquet, root, workspace, .. } = CommandTempCwd::init();
|
||||
let manifest = json!({
|
||||
"name": "test",
|
||||
"version": "0.0.0",
|
||||
"scripts": { "build": "echo built" },
|
||||
})
|
||||
.to_string();
|
||||
fs::write(workspace.join("package.json"), manifest).expect("write package.json");
|
||||
|
||||
pacquet.with_arg("--if-present").with_arg("nonexistent").assert().success();
|
||||
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// `pnpm run` with no script name lists the available scripts, grouped
|
||||
/// into lifecycle scripts and others. Mirrors pnpm's `printProjectCommands`.
|
||||
#[test]
|
||||
|
||||
@@ -1041,6 +1041,27 @@ fn recursive_run_if_present_is_a_noop_when_no_package_has_the_script() {
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// The top-level `--if-present` spelling with a shorthand script — the
|
||||
/// shape the repo's own `test-pkgs-branch` script uses
|
||||
/// (`pnpm --workspace-concurrency=1 --no-sort --if-present <script>`) —
|
||||
/// is the same clean no-op when no package defines the script.
|
||||
#[test]
|
||||
fn recursive_top_level_if_present_is_a_noop_when_no_package_has_the_script() {
|
||||
let CommandTempCwd { pacquet, root, workspace, .. } = CommandTempCwd::init();
|
||||
write_workspace(&workspace, &[("project-1", build_writes_marker("project-1"))]);
|
||||
|
||||
pacquet
|
||||
.with_arg("--workspace-concurrency=1")
|
||||
.with_arg("--no-sort")
|
||||
.with_arg("--if-present")
|
||||
.with_arg("-r")
|
||||
.with_arg("lint")
|
||||
.assert()
|
||||
.success();
|
||||
|
||||
drop(root);
|
||||
}
|
||||
|
||||
/// Recursive `run` must resolve each package's `node_modules/.bin` on
|
||||
/// PATH so locally-installed bins (e.g. `tsc`, `eslint`) work, for every
|
||||
/// project. Without it, `pacquet -r run build` would fail with
|
||||
|
||||
Reference in New Issue
Block a user