diff --git a/.changeset/cuddly-spoons-link.md b/.changeset/cuddly-spoons-link.md new file mode 100644 index 0000000000..ffc2b0f093 --- /dev/null +++ b/.changeset/cuddly-spoons-link.md @@ -0,0 +1,5 @@ +--- +"pacquet": patch +--- + +`pnpm deploy` now links commands exposed by workspace dependencies into the deployed project's `node_modules/.bin` directory [pnpm/pnpm#14899](https://github.com/pnpm/pnpm/issues/14899). diff --git a/pnpm/crates/cli/src/cli_args/deploy.rs b/pnpm/crates/cli/src/cli_args/deploy.rs index 8eaadb2b22..5908b840f0 100644 --- a/pnpm/crates/cli/src/cli_args/deploy.rs +++ b/pnpm/crates/cli/src/cli_args/deploy.rs @@ -26,7 +26,9 @@ use pnpm_lockfile::{ WantedLockfileSelection, }; use pnpm_lockfile_preferred_versions::get_preferred_versions_from_lockfile_and_manifests; -use pnpm_package_manager::{ImportIndexedDirOpts, apply_deploy_manifest_hook, import_indexed_dir}; +use pnpm_package_manager::{ + ImportIndexedDirOpts, apply_deploy_manifest_hook, import_indexed_dir, manifest_has_bin, +}; use pnpm_package_manifest::{DependencyGroup, PackageManifest}; use pnpm_reporter::{LogEvent, LogLevel, PnpmLog, Reporter}; use pnpm_resolving_resolver_base::PreferredVersions; @@ -125,6 +127,7 @@ enum DeployError { #[derive(Clone)] struct ProjectInfo { name: Option, + has_bin: bool, peer_dependencies: Vec, /// Names the project declares as prod or optional dependencies. A peer it /// depends on itself is already bound by that edge, whether or not the @@ -137,6 +140,16 @@ struct SelectedProject { projects_by_path: HashMap, } +impl SelectedProject { + fn project_info(&self, root: &Path) -> Option<&ProjectInfo> { + self.projects_by_path.get(&ProjectPathKey::new(root)) + } + + fn has_bin(&self, root: &Path) -> Option { + self.project_info(root).map(|project| project.has_bin) + } +} + struct DeployWorkspaceConfig { patched_dependencies: Option>, allow_builds: HashMap, @@ -347,6 +360,7 @@ fn index_projects(projects: &[Project]) -> HashMap .get("name") .and_then(Value::as_str) .map(str::to_string), + has_bin: manifest_has_bin(Some(project.manifest.value())).is_some(), peer_dependencies: manifest_dependency_names( &project.manifest, &["peerDependencies"], diff --git a/pnpm/crates/cli/src/cli_args/deploy/lockfile.rs b/pnpm/crates/cli/src/cli_args/deploy/lockfile.rs index 32ca619b20..396fdad397 100644 --- a/pnpm/crates/cli/src/cli_args/deploy/lockfile.rs +++ b/pnpm/crates/cli/src/cli_args/deploy/lockfile.rs @@ -219,7 +219,7 @@ fn convert_deploy_packages( os: None, libc: None, deprecated: None, - has_bin: None, + has_bin: selected.has_bin(&project_root), prepare: None, bundled_dependencies: None, peer_dependencies: None, @@ -258,7 +258,7 @@ fn convert_deploy_snapshots( validate_lockfile_local_path(&lockfile_dir.join(importer_path), lockfile_dir)?; let bases = ResolveBases { file_base: lockfile_dir, link_base: &project_root }; let package_key = create_file_url_key(&project_root, "", &selected.projects_by_path, None)?; - if let Some(project) = selected.projects_by_path.get(&ProjectPathKey::new(&project_root)) + if let Some(project) = selected.project_info(&project_root) && !project.peer_dependencies.is_empty() { linked_workspace_projects.insert(package_key.clone(), project.clone()); diff --git a/pnpm/crates/cli/src/cli_args/deploy/tests.rs b/pnpm/crates/cli/src/cli_args/deploy/tests.rs index 869f774bd9..a355b14efb 100644 --- a/pnpm/crates/cli/src/cli_args/deploy/tests.rs +++ b/pnpm/crates/cli/src/cli_args/deploy/tests.rs @@ -154,6 +154,7 @@ fn create_file_url_key_prefers_workspace_project_name() { ProjectPathKey::new(&project_root), ProjectInfo { name: Some("workspace-name".to_string()), + has_bin: false, peer_dependencies: Vec::new(), declared_dependencies: HashSet::default(), }, diff --git a/pnpm/crates/cli/tests/suite/deploy.rs b/pnpm/crates/cli/tests/suite/deploy.rs index 4c133cd5a5..a526a10aaf 100644 --- a/pnpm/crates/cli/tests/suite/deploy.rs +++ b/pnpm/crates/cli/tests/suite/deploy.rs @@ -113,6 +113,78 @@ fn deploy_from_shared_lockfile_installs_selected_project() { drop((root, mock_instance)); } +#[test] +fn deploy_links_workspace_dependency_bins() { + let CommandTempCwd { + pacquet, + root, + workspace, + npmrc_info, + .. + } = CommandTempCwd::init().add_mocked_registry(); + let AddMockedRegistry { mock_instance, .. } = npmrc_info; + write_workspace(&workspace, true); + write_project( + &workspace, + "lib", + &serde_json::json!({ + "name": "lib", + "version": "1.0.0", + "files": ["index.js", "bin.js"], + "bin": { "lib-cli": "bin.js" }, + "dependencies": { "binless": "workspace:*" }, + }), + ); + write_project( + &workspace, + "binless", + &serde_json::json!({ + "name": "binless", + "version": "1.0.0", + "files": ["index.js"], + }), + ); + fs::write(workspace.join("packages/lib/bin.js"), "#!/usr/bin/env node\n").unwrap(); + + pacquet + .with_arg("install") + .assert() + .success(); + for (deploy_dir, extra_args) in + [("deploy", [].as_slice()), ("legacy-deploy", ["--legacy"].as_slice())] + { + pacquet_cmd(&workspace) + .with_args(["--filter", "app", "deploy", "--prod"]) + .with_args(extra_args) + .with_arg(deploy_dir) + .assert() + .success(); + assert!( + workspace + .join(deploy_dir) + .join("node_modules/.bin/lib-cli") + .exists(), + "{deploy_dir} must link the workspace dependency's bin", + ); + if deploy_dir == "deploy" { + let lockfile = + Lockfile::load_wanted_from_dir(&workspace.join(deploy_dir)).unwrap().unwrap(); + assert_eq!(package_has_bin(&lockfile, "lib"), Some(Some(true))); + assert_eq!(package_has_bin(&lockfile, "binless"), Some(Some(false))); + } + } + + drop((root, mock_instance)); +} + +fn package_has_bin(lockfile: &Lockfile, name: &str) -> Option> { + lockfile.packages + .as_ref()? + .iter() + .find(|(key, _)| key.name.to_string() == name) + .map(|(_, metadata)| metadata.has_bin) +} + /// A pinned `lockfileDir` moves the shared lockfile `deploy` reads and /// the importer id naming the selected project in it. Reading either from /// the workspace root instead drops the deploy to its "shared lockfile not diff --git a/pnpm/crates/deps-restorer/src/link_bins.rs b/pnpm/crates/deps-restorer/src/link_bins.rs index 49da3dd789..bba4b31142 100644 --- a/pnpm/crates/deps-restorer/src/link_bins.rs +++ b/pnpm/crates/deps-restorer/src/link_bins.rs @@ -167,8 +167,8 @@ impl LinkVirtualStoreBins<'_> { } } -/// Pre-compute the set of package keys whose lockfile metadata sets -/// `hasBin: true`. Most packages don't declare a bin, so +/// Pre-compute the set of package keys whose lockfile metadata says they +/// may expose bins. Most packages don't declare a bin, so /// short-circuiting the per-child manifest lookup with this set is /// the cheapest win on warm-cache installs. /// @@ -180,7 +180,9 @@ impl LinkVirtualStoreBins<'_> { /// linker falls back to the conservative "process every child" /// path and lets the per-package bin resolver sort it out. /// - `Some(set)` — the section was present and we used it. The -/// `set` contains only entries with `hasBin == Some(true)`; an +/// `set` contains entries with `hasBin == Some(true)`, runtimes whose +/// bin metadata lives in their resolution, and local directories whose +/// generated metadata does not carry `hasBin`; an /// *empty* `Some(set)` is authoritative: the lockfile says no /// package has a bin, and every slot should short-circuit /// immediately. Conflating this case with `None` (the bug flagged @@ -199,6 +201,8 @@ fn build_has_bin_set( meta.resolution, LockfileResolution::Binary(_) | LockfileResolution::Variations(_), ) + || (meta.has_bin.is_none() + && matches!(meta.resolution, LockfileResolution::Directory(_))) }) .map(|(key, _)| key.clone()) .collect(), diff --git a/pnpm/crates/deps-restorer/src/link_bins/tests.rs b/pnpm/crates/deps-restorer/src/link_bins/tests.rs index f2d55fdc41..f686ad6f4a 100644 --- a/pnpm/crates/deps-restorer/src/link_bins/tests.rs +++ b/pnpm/crates/deps-restorer/src/link_bins/tests.rs @@ -624,17 +624,12 @@ fn dummy_binary_resolution() -> BinaryResolution { } } -/// `build_has_bin_set` must include runtime resolutions -/// (`Binary` / `Variations`) unconditionally, *regardless of* -/// `meta.has_bin`. Pnpm v11 doesn't emit `hasBin: true` for -/// runtime entries in `pnpm-lock.yaml` (the bin info lives on -/// `resolution.bin`, not at the metadata level), so the existing -/// `has_bin == Some(true)` filter would drop runtime slots from -/// the bin-link dispatch and the synthesized `package.json` -/// (from `install_package_by_snapshot::synthesize_runtime_manifest_bytes`) -/// would go unread. +/// Runtime resolutions must be included without `has_bin` because pnpm v11 +/// records their bin in the resolution instead of emitting `hasBin: true`. +/// Directory resolutions need the same fallback because deploy lockfiles can +/// omit `hasBin` for workspace packages. #[test] -fn build_has_bin_set_includes_runtime_resolutions_even_when_has_bin_is_absent() { +fn build_has_bin_set_includes_resolutions_with_implicit_bin_metadata() { let registry_with_bin: PackageKey = "react@18.0.0".parse().expect("parse react key"); let registry_no_bin: PackageKey = "lodash@4.17.0".parse().expect("parse lodash key"); let runtime_binary: PackageKey = "node@22.0.0".parse().expect("parse node key"); @@ -664,8 +659,6 @@ fn build_has_bin_set_includes_runtime_resolutions_even_when_has_bin_is_absent() ); packages.insert( runtime_binary.clone(), - // Runtime entry without `has_bin: true` (pnpm v11 does - // not emit it for runtimes). Must still land in the set. metadata_with_resolution(LockfileResolution::Binary(dummy_binary_resolution()), None), ); packages.insert( @@ -702,7 +695,7 @@ fn build_has_bin_set_includes_runtime_resolutions_even_when_has_bin_is_absent() set.contains(&runtime_variations), "Variations runtime must be in the set unconditionally", ); - assert!(!set.contains(&directory), "directory without has_bin must be filtered out"); + assert!(set.contains(&directory), "directory without has_bin must be probed for bins"); } #[test] diff --git a/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile.rs b/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile.rs index 79f30defcf..2ba3532d31 100644 --- a/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile.rs +++ b/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile.rs @@ -7,8 +7,7 @@ //! fanning it out on write. pub(crate) use importers::manifest_publish_config; -pub use packages::PackageMetadataSources; -pub(crate) use packages::manifest_has_bin; +pub use packages::{PackageMetadataSources, manifest_has_bin}; mod packages; diff --git a/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile/packages.rs b/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile/packages.rs index e8db8a53d4..3d80a9b177 100644 --- a/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile/packages.rs +++ b/pnpm/crates/package-manager/src/dependencies_graph_to_lockfile/packages.rs @@ -279,7 +279,7 @@ pub(super) fn read_string_or_list( } /// `Some(true)` when the manifest declares executable files, recorded as /// the `hasBin: true` signal; the field is dropped entirely when absent. -pub(crate) fn manifest_has_bin(manifest: Option<&Value>) -> Option { +pub fn manifest_has_bin(manifest: Option<&Value>) -> Option { let manifest = manifest?; let has_bin = manifest .get("bin")