From 01f3e260bff7d94283b81c19819249fc6eeae99f Mon Sep 17 00:00:00 2001 From: Rakibul Yeasin Date: Tue, 28 Apr 2026 17:01:32 +0600 Subject: [PATCH 1/5] feat(package-manager): support version range in `pacquet add` --- crates/cli/src/cli_args/add.rs | 2 +- crates/package-manager/src/add.rs | 114 ++++++++++++++++++++++++++--- crates/registry/src/package_tag.rs | 5 +- 3 files changed, 107 insertions(+), 14 deletions(-) diff --git a/crates/cli/src/cli_args/add.rs b/crates/cli/src/cli_args/add.rs index 18ee1b8c4..84195cafd 100644 --- a/crates/cli/src/cli_args/add.rs +++ b/crates/cli/src/cli_args/add.rs @@ -66,7 +66,7 @@ impl AddDependencyOptions { #[derive(Debug, Args)] pub struct AddArgs { /// Name of the package - pub package_name: String, // TODO: 1. support version range, 2. multiple arguments, 3. name this `packages` + pub package_name: String, // TODO: 1. multiple arguments, 2. name this `packages` /// --save-prod, --save-dev, --save-optional, --save-peer #[clap(flatten)] pub dependency_options: AddDependencyOptions, diff --git a/crates/package-manager/src/add.rs b/crates/package-manager/src/add.rs index f755a71fe..ba2c9a86a 100644 --- a/crates/package-manager/src/add.rs +++ b/crates/package-manager/src/add.rs @@ -1,12 +1,13 @@ use crate::{Install, InstallError, ResolvedPackages}; use derive_more::{Display, Error}; use miette::Diagnostic; +use node_semver::{Range, Version}; use pacquet_lockfile::Lockfile; use pacquet_network::ThrottledClient; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::PackageManifestError; use pacquet_package_manifest::{DependencyGroup, PackageManifest}; -use pacquet_registry::{PackageTag, PackageVersion}; +use pacquet_registry::{PackageTag, PackageVersion, RegistryError}; use pacquet_tarball::MemCache; /// This subroutine does everything `pacquet add` is supposed to do. @@ -23,13 +24,15 @@ where pub manifest: &'a mut PackageManifest, pub lockfile: Option<&'a Lockfile>, pub list_dependency_groups: ListDependencyGroups, // must be a function because it is called multiple times - pub package_name: &'a str, // TODO: 1. support version range, 2. multiple arguments, 3. name this `packages` + pub package_name: &'a str, // TODO: 1. multiple arguments, 2. name this `packages` pub save_exact: bool, // TODO: add `save-exact` to `.npmrc`, merge configs, and remove this } /// Error type of [`Add`]. #[derive(Debug, Display, Error, Diagnostic)] pub enum AddError { + #[display("Failed to fetch version for package: {_0}")] + FetchVersion(#[error(source)] RegistryError), #[display("Failed to add package to manifest: {_0}")] AddDependencyToManifest(#[error(source)] PackageManifestError), #[display("Failed save the manifest file: {_0}")] @@ -38,6 +41,20 @@ pub enum AddError { Install(#[error(source)] InstallError), } +/// Split a package argument into name and version specifier. +/// +/// Handles scoped packages: `@scope/name@1.0.0` splits into `("@scope/name", "1.0.0")`. +fn parse_pkg_arg(arg: &str) -> (&str, &str) { + let start = usize::from(arg.starts_with('@')); + match arg[start..].find('@') { + Some(pos) => { + let split = start + pos; + (&arg[..split], &arg[split + 1..]) + } + None => (arg, ""), + } +} + impl<'a, ListDependencyGroups, DependencyGroupList> Add<'a, ListDependencyGroups, DependencyGroupList> where @@ -57,19 +74,52 @@ where resolved_packages, } = self; - let latest_version = PackageVersion::fetch_from_registry( - package_name, - PackageTag::Latest, // TODO: add support for specifying tags - http_client, - &config.registry, - ) - .await - .expect("resolve latest tag"); // TODO: properly propagate this error + let (name, specifier) = parse_pkg_arg(package_name); + + // Resolve the version specifier to a range string to save in package.json. + // For tags (no specifier or dist-tag), we fetch the resolved version first so + // we can save a pinned semver range rather than a mutable tag name. + let version_to_save = if specifier.is_empty() || specifier == "latest" { + let version = PackageVersion::fetch_from_registry( + name, + PackageTag::Latest, + http_client, + &config.registry, + ) + .await + .map_err(AddError::FetchVersion)?; + version.serialize(save_exact) + } else if let Ok(v) = specifier.parse::() { + // Exact semver version: fetch to validate, then save with ^ unless --save-exact. + PackageVersion::fetch_from_registry( + name, + PackageTag::Version(v), + http_client, + &config.registry, + ) + .await + .map_err(AddError::FetchVersion)?; + if save_exact { specifier.to_owned() } else { format!("^{specifier}") } + } else if specifier.parse::().is_ok() { + // Semver range (e.g. `^18`, `~1.0.0`, `>=1 <2`): save as-is and let + // the install step resolve the best matching version. + specifier.to_owned() + } else { + // Named dist-tag (e.g. `next`, `beta`): resolve to a concrete version. + let version = PackageVersion::fetch_from_registry( + name, + PackageTag::Tag(specifier.to_owned()), + http_client, + &config.registry, + ) + .await + .map_err(AddError::FetchVersion)?; + version.serialize(save_exact) + }; - let version_range = latest_version.serialize(save_exact); for dependency_group in list_dependency_groups() { manifest - .add_dependency(package_name, &version_range, dependency_group) + .add_dependency(name, &version_to_save, dependency_group) .map_err(AddError::AddDependencyToManifest)?; } @@ -92,3 +142,43 @@ where Ok(()) } } + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn parse_pkg_arg_no_specifier() { + assert_eq!(parse_pkg_arg("react"), ("react", "")); + } + + #[test] + fn parse_pkg_arg_with_version() { + assert_eq!(parse_pkg_arg("react@18.2.0"), ("react", "18.2.0")); + } + + #[test] + fn parse_pkg_arg_with_range() { + assert_eq!(parse_pkg_arg("react@^18"), ("react", "^18")); + } + + #[test] + fn parse_pkg_arg_with_tag() { + assert_eq!(parse_pkg_arg("react@next"), ("react", "next")); + } + + #[test] + fn parse_pkg_arg_scoped_no_specifier() { + assert_eq!(parse_pkg_arg("@scope/pkg"), ("@scope/pkg", "")); + } + + #[test] + fn parse_pkg_arg_scoped_with_version() { + assert_eq!(parse_pkg_arg("@scope/pkg@1.0.0"), ("@scope/pkg", "1.0.0")); + } + + #[test] + fn parse_pkg_arg_scoped_with_range() { + assert_eq!(parse_pkg_arg("@scope/pkg@^1.0.0"), ("@scope/pkg", "^1.0.0")); + } +} diff --git a/crates/registry/src/package_tag.rs b/crates/registry/src/package_tag.rs index 680a72f01..7874e2c2b 100644 --- a/crates/registry/src/package_tag.rs +++ b/crates/registry/src/package_tag.rs @@ -5,11 +5,14 @@ use std::str::FromStr; /// Version or tag that is attachable to a registry URL. #[derive(Debug, Display, From, TryInto)] pub enum PackageTag { - /// Literally `latest``. + /// Literally `latest`. #[display("latest")] Latest, /// Pinned version. Version(Version), + /// Named distribution tag (for example `next` or `beta`). + #[display("{_0}")] + Tag(String), } impl FromStr for PackageTag { From 9faa8a0dc511c2c939f11841246018b208ff0426 Mon Sep 17 00:00:00 2001 From: Rakibul Yeasin Date: Tue, 28 Apr 2026 17:09:01 +0600 Subject: [PATCH 2/5] feat(package-manager): support multiple packages in `pacquet add` --- crates/cli/src/cli_args/add.rs | 10 +-- crates/package-manager/src/add.rs | 102 +++++++++++++++--------------- 2 files changed, 58 insertions(+), 54 deletions(-) diff --git a/crates/cli/src/cli_args/add.rs b/crates/cli/src/cli_args/add.rs index 84195cafd..97438ed65 100644 --- a/crates/cli/src/cli_args/add.rs +++ b/crates/cli/src/cli_args/add.rs @@ -65,8 +65,9 @@ impl AddDependencyOptions { #[derive(Debug, Args)] pub struct AddArgs { - /// Name of the package - pub package_name: String, // TODO: 1. multiple arguments, 2. name this `packages` + /// Names of the packages to add. + #[clap(required = true)] + pub packages: Vec, /// --save-prod, --save-dev, --save-optional, --save-peer #[clap(flatten)] pub dependency_options: AddDependencyOptions, @@ -88,6 +89,7 @@ impl AddArgs { let State { tarball_mem_cache, http_client, config, manifest, lockfile, resolved_packages } = &mut state; + let packages: Vec<&str> = self.packages.iter().map(String::as_str).collect(); Add { tarball_mem_cache, http_client, @@ -95,13 +97,13 @@ impl AddArgs { manifest, lockfile: lockfile.as_ref(), list_dependency_groups: || self.dependency_options.dependency_groups(), - package_name: &self.package_name, + packages: &packages, save_exact: self.save_exact, resolved_packages, } .run() .await - .wrap_err("adding a new package") + .wrap_err("adding packages") } } diff --git a/crates/package-manager/src/add.rs b/crates/package-manager/src/add.rs index ba2c9a86a..21c0510be 100644 --- a/crates/package-manager/src/add.rs +++ b/crates/package-manager/src/add.rs @@ -24,8 +24,8 @@ where pub manifest: &'a mut PackageManifest, pub lockfile: Option<&'a Lockfile>, pub list_dependency_groups: ListDependencyGroups, // must be a function because it is called multiple times - pub package_name: &'a str, // TODO: 1. multiple arguments, 2. name this `packages` - pub save_exact: bool, // TODO: add `save-exact` to `.npmrc`, merge configs, and remove this + pub packages: &'a [&'a str], + pub save_exact: bool, // TODO: add `save-exact` to `.npmrc`, merge configs, and remove this } /// Error type of [`Add`]. @@ -69,58 +69,60 @@ where manifest, lockfile, list_dependency_groups, - package_name, + packages, save_exact, resolved_packages, } = self; - let (name, specifier) = parse_pkg_arg(package_name); - - // Resolve the version specifier to a range string to save in package.json. - // For tags (no specifier or dist-tag), we fetch the resolved version first so - // we can save a pinned semver range rather than a mutable tag name. - let version_to_save = if specifier.is_empty() || specifier == "latest" { - let version = PackageVersion::fetch_from_registry( - name, - PackageTag::Latest, - http_client, - &config.registry, - ) - .await - .map_err(AddError::FetchVersion)?; - version.serialize(save_exact) - } else if let Ok(v) = specifier.parse::() { - // Exact semver version: fetch to validate, then save with ^ unless --save-exact. - PackageVersion::fetch_from_registry( - name, - PackageTag::Version(v), - http_client, - &config.registry, - ) - .await - .map_err(AddError::FetchVersion)?; - if save_exact { specifier.to_owned() } else { format!("^{specifier}") } - } else if specifier.parse::().is_ok() { - // Semver range (e.g. `^18`, `~1.0.0`, `>=1 <2`): save as-is and let - // the install step resolve the best matching version. - specifier.to_owned() - } else { - // Named dist-tag (e.g. `next`, `beta`): resolve to a concrete version. - let version = PackageVersion::fetch_from_registry( - name, - PackageTag::Tag(specifier.to_owned()), - http_client, - &config.registry, - ) - .await - .map_err(AddError::FetchVersion)?; - version.serialize(save_exact) - }; - - for dependency_group in list_dependency_groups() { - manifest - .add_dependency(name, &version_to_save, dependency_group) - .map_err(AddError::AddDependencyToManifest)?; + for &pkg in packages { + let (name, specifier) = parse_pkg_arg(pkg); + + // Resolve the version specifier to a range string to save in package.json. + // For tags (no specifier or dist-tag), we fetch the resolved version first so + // we can save a pinned semver range rather than a mutable tag name. + let version_to_save = if specifier.is_empty() || specifier == "latest" { + let version = PackageVersion::fetch_from_registry( + name, + PackageTag::Latest, + http_client, + &config.registry, + ) + .await + .map_err(AddError::FetchVersion)?; + version.serialize(save_exact) + } else if let Ok(v) = specifier.parse::() { + // Exact semver version: fetch to validate, then save with ^ unless --save-exact. + PackageVersion::fetch_from_registry( + name, + PackageTag::Version(v), + http_client, + &config.registry, + ) + .await + .map_err(AddError::FetchVersion)?; + if save_exact { specifier.to_owned() } else { format!("^{specifier}") } + } else if specifier.parse::().is_ok() { + // Semver range (e.g. `^18`, `~1.0.0`, `>=1 <2`): save as-is and let + // the install step resolve the best matching version. + specifier.to_owned() + } else { + // Named dist-tag (e.g. `next`, `beta`): resolve to a concrete version. + let version = PackageVersion::fetch_from_registry( + name, + PackageTag::Tag(specifier.to_owned()), + http_client, + &config.registry, + ) + .await + .map_err(AddError::FetchVersion)?; + version.serialize(save_exact) + }; + + for dependency_group in list_dependency_groups() { + manifest + .add_dependency(name, &version_to_save, dependency_group) + .map_err(AddError::AddDependencyToManifest)?; + } } Install { From 1932a8a9dd560923351cb6361f514d7b16d770f8 Mon Sep 17 00:00:00 2001 From: Rakibul Yeasin Date: Wed, 29 Apr 2026 10:56:20 +0600 Subject: [PATCH 3/5] feat(package-manager): link dependency binaries into node_modules/.bin MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Implements two passes that mirror pnpm's linkBinsOfPackages + linkAllBins from the headless restorer (3f37d17b23): 1. Root node_modules/.bin/ — symlinks for direct dependencies with has_bin. 2. Per-package node_modules/.bin/ inside each virtual-store slot — symlinks for that slot's own dependencies so lifecycle scripts can call them. Reads the bin field from each package's package.json (string and object forms), validates command names are URL-safe, guards against path traversal via lexical .. resolution, and ensures executable bits on targets. Upstream: https://github.com/pnpm/pnpm/blob/3f37d17b23/installing/deps-restorer/src/index.ts --- Cargo.lock | 1 + crates/package-manager/Cargo.toml | 1 + .../src/install_frozen_lockfile.rs | 23 +- crates/package-manager/src/lib.rs | 2 + crates/package-manager/src/link_bins.rs | 440 ++++++++++++++++++ 5 files changed, 463 insertions(+), 4 deletions(-) create mode 100644 crates/package-manager/src/link_bins.rs diff --git a/Cargo.lock b/Cargo.lock index b13a3c537..3aff0fb37 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1897,6 +1897,7 @@ dependencies = [ "rayon", "reflink-copy", "serde-saphyr", + "serde_json", "ssri", "tempfile", "tokio", diff --git a/crates/package-manager/Cargo.toml b/crates/package-manager/Cargo.toml index ba2970592..b502ad22f 100644 --- a/crates/package-manager/Cargo.toml +++ b/crates/package-manager/Cargo.toml @@ -28,6 +28,7 @@ node-semver = { workspace = true } pipe-trait = { workspace = true } rayon = { workspace = true } reflink-copy = { workspace = true } +serde_json = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } miette = { workspace = true } diff --git a/crates/package-manager/src/install_frozen_lockfile.rs b/crates/package-manager/src/install_frozen_lockfile.rs index 983875661..22ca7ff7a 100644 --- a/crates/package-manager/src/install_frozen_lockfile.rs +++ b/crates/package-manager/src/install_frozen_lockfile.rs @@ -1,6 +1,6 @@ use crate::{ - CreateVirtualStore, CreateVirtualStoreError, SymlinkDirectDependencies, - SymlinkDirectDependenciesError, + CreateVirtualStore, CreateVirtualStoreError, LinkBins, LinkBinsError, + SymlinkDirectDependencies, SymlinkDirectDependenciesError, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -19,6 +19,7 @@ use std::collections::HashMap; /// * Import the files from the store dir to each `node_modules/.pacquet/{name}@{version}/node_modules/{name}/`. /// * Create dependency symbolic links in each `node_modules/.pacquet/{name}@{version}/node_modules/`. /// * Create a symbolic link at each `node_modules/{name}`. +/// * Create `.bin` symlinks for all installed packages with executables. #[must_use] pub struct InstallFrozenLockfile<'a, DependencyGroupList> where @@ -40,6 +41,9 @@ pub enum InstallFrozenLockfileError { #[diagnostic(transparent)] SymlinkDirectDependencies(#[error(source)] SymlinkDirectDependenciesError), + + #[diagnostic(transparent)] + LinkBins(#[error(source)] LinkBinsError), } impl<'a, DependencyGroupList> InstallFrozenLockfile<'a, DependencyGroupList> @@ -59,14 +63,25 @@ where // TODO: check if the lockfile is out-of-date + // Collect once so we can pass the same slice to both symlink and bin-link steps. + let dependency_groups: Vec = dependency_groups.into_iter().collect(); + CreateVirtualStore { http_client, config, packages, snapshots } .run() .await .map_err(InstallFrozenLockfileError::CreateVirtualStore)?; - SymlinkDirectDependencies { config, importers, dependency_groups } + SymlinkDirectDependencies { + config, + importers, + dependency_groups: dependency_groups.iter().copied(), + } + .run() + .map_err(InstallFrozenLockfileError::SymlinkDirectDependencies)?; + + LinkBins { config, importers, packages, snapshots, dependency_groups: &dependency_groups } .run() - .map_err(InstallFrozenLockfileError::SymlinkDirectDependencies)?; + .map_err(InstallFrozenLockfileError::LinkBins)?; Ok(()) } diff --git a/crates/package-manager/src/lib.rs b/crates/package-manager/src/lib.rs index 0d9289084..c75208b02 100644 --- a/crates/package-manager/src/lib.rs +++ b/crates/package-manager/src/lib.rs @@ -9,6 +9,7 @@ mod install_frozen_lockfile; mod install_package_by_snapshot; mod install_package_from_registry; mod install_without_lockfile; +mod link_bins; mod link_file; mod retry_config; mod store_init; @@ -26,6 +27,7 @@ pub use install_frozen_lockfile::*; pub use install_package_by_snapshot::*; pub use install_package_from_registry::*; pub use install_without_lockfile::*; +pub use link_bins::*; pub use link_file::*; pub use symlink_direct_dependencies::*; pub use symlink_package::*; diff --git a/crates/package-manager/src/link_bins.rs b/crates/package-manager/src/link_bins.rs new file mode 100644 index 000000000..bf574872d --- /dev/null +++ b/crates/package-manager/src/link_bins.rs @@ -0,0 +1,440 @@ +use std::{ + collections::HashMap, + fs, + io::{self, ErrorKind}, + path::{Path, PathBuf}, +}; + +use derive_more::{Display, Error}; +use miette::Diagnostic; +use pacquet_lockfile::{Lockfile, PackageKey, PackageMetadata, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry}; +use pacquet_npmrc::Npmrc; +use pacquet_package_manifest::DependencyGroup; +use serde_json::Value; + +/// Error type for [`LinkBins`]. +#[derive(Debug, Display, Error, Diagnostic)] +pub enum LinkBinsError { + #[display("Failed to read package.json at {path:?}: {error}")] + #[diagnostic(code(pacquet_package_manager::link_bins_read_manifest))] + ReadManifest { + path: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to parse package.json at {path:?}: {error}")] + #[diagnostic(code(pacquet_package_manager::link_bins_parse_manifest))] + ParseManifest { + path: PathBuf, + #[error(source)] + error: serde_json::Error, + }, + + #[display("Failed to create .bin directory at {dir:?}: {error}")] + #[diagnostic(code(pacquet_package_manager::link_bins_create_dir))] + CreateBinDir { + dir: PathBuf, + #[error(source)] + error: io::Error, + }, + + #[display("Failed to create bin symlink at {link:?}: {error}")] + #[diagnostic(code(pacquet_package_manager::link_bins_create_symlink))] + CreateSymlink { + link: PathBuf, + #[error(source)] + error: io::Error, + }, +} + +/// A resolved binary command from a package's `package.json`. +struct BinCmd { + name: String, + path: PathBuf, +} + +/// Create `.bin` symlinks for installed packages. +/// +/// Performs two passes that mirror pnpm's `linkBinsOfPackages` + +/// `linkAllBins` steps in the headless restorer: +/// +/// 1. **Root `node_modules/.bin/`** — bins exposed by direct dependencies +/// of the root project (filtered by `dependency_groups`). +/// 2. **Per-package `node_modules/.bin/`** inside each virtual-store slot — +/// bins of that slot's own dependencies, so that lifecycle scripts can +/// call their deps' executables. +/// +/// Upstream reference: +/// +#[must_use] +pub struct LinkBins<'a> { + pub config: &'static Npmrc, + pub importers: &'a HashMap, + pub packages: Option<&'a HashMap>, + pub snapshots: Option<&'a HashMap>, + pub dependency_groups: &'a [DependencyGroup], +} + +impl<'a> LinkBins<'a> { + /// Execute the subroutine. + pub fn run(self) -> Result<(), LinkBinsError> { + let LinkBins { config, importers, packages, snapshots, dependency_groups } = self; + + let Some(snapshots) = snapshots else { return Ok(()) }; + let Some(packages) = packages else { return Ok(()) }; + + let virtual_store_dir = &config.virtual_store_dir; + let modules_dir = &config.modules_dir; + + // Pass 1: root node_modules/.bin/ for direct dependencies. + if let Some(root_importer) = importers.get(Lockfile::ROOT_IMPORTER_KEY) { + let bins_dir = modules_dir.join(".bin"); + + for (dep_name, dep_spec) in root_importer.dependencies_by_groups(dependency_groups.iter().copied()) { + let pkg_key = PkgNameVerPeer::new(dep_name.clone(), dep_spec.version.clone()); + let metadata_key = pkg_key.without_peer(); + + if packages.get(&metadata_key).and_then(|m| m.has_bin) != Some(true) { + continue; + } + + let pkg_dir = virtual_store_dir + .join(pkg_key.to_virtual_store_name()) + .join("node_modules") + .join(dep_name.to_string()); + + let cmds = read_bin_cmds(&pkg_dir)?; + create_bin_symlinks(&cmds, &bins_dir)?; + } + } + + // Pass 2: per-package node_modules/.bin/ for each virtual-store slot. + // + // Each slot's .bin contains symlinks for its own dependencies' bins, + // so that lifecycle scripts can invoke those executables. Mirrors + // pnpm's `linkAllBins` in the deps-restorer. + for (snapshot_key, snapshot) in snapshots { + let snapshot_node_modules = virtual_store_dir + .join(snapshot_key.to_virtual_store_name()) + .join("node_modules"); + let bins_dir = snapshot_node_modules.join(".bin"); + + let all_deps = snapshot + .dependencies + .iter() + .flatten() + .chain(snapshot.optional_dependencies.iter().flatten()); + + let mut cmds_for_slot: Vec = Vec::new(); + + for (dep_alias, dep_ref) in all_deps { + let resolved = dep_ref.resolve(dep_alias); + let dep_metadata_key = resolved.without_peer(); + + if packages.get(&dep_metadata_key).and_then(|m| m.has_bin) != Some(true) { + continue; + } + + let dep_pkg_dir = virtual_store_dir + .join(resolved.to_virtual_store_name()) + .join("node_modules") + .join(resolved.name.to_string()); + + cmds_for_slot.extend(read_bin_cmds(&dep_pkg_dir)?); + } + + if !cmds_for_slot.is_empty() { + create_bin_symlinks(&cmds_for_slot, &bins_dir)?; + } + } + + Ok(()) + } +} + +/// Read the `bin` field from `{pkg_dir}/package.json` and return resolved commands. +/// +/// Returns an empty `Vec` if `package.json` is absent or has no `bin` field. +fn read_bin_cmds(pkg_dir: &Path) -> Result, LinkBinsError> { + let manifest_path = pkg_dir.join("package.json"); + let content = match fs::read_to_string(&manifest_path) { + Ok(c) => c, + Err(e) if e.kind() == ErrorKind::NotFound => return Ok(vec![]), + Err(error) => return Err(LinkBinsError::ReadManifest { path: manifest_path, error }), + }; + let manifest: Value = serde_json::from_str(&content) + .map_err(|error| LinkBinsError::ParseManifest { path: manifest_path, error })?; + + Ok(parse_bin_cmds(&manifest, pkg_dir)) +} + +/// Parse binary commands from a manifest value. +/// +/// Supports both the string form (`"bin": "./cli.js"`) and the object form +/// (`"bin": { "tsc": "./bin/tsc" }`), mirroring `commandsFromBin` in +/// `@pnpm/bins.resolver`. +/// +/// `directories.bin` is not yet implemented (rare in practice). +/// +/// Upstream reference: +/// +fn parse_bin_cmds(manifest: &Value, pkg_dir: &Path) -> Vec { + let pkg_name = manifest.get("name").and_then(Value::as_str).unwrap_or(""); + + match manifest.get("bin") { + Some(Value::String(rel_path)) => { + let cmd_name = strip_scope(pkg_name).to_string(); + if cmd_name.is_empty() || !is_bin_name_safe(&cmd_name) { + return vec![]; + } + let bin_path = pkg_dir.join(rel_path); + if !is_within(pkg_dir, &bin_path) { + return vec![]; + } + vec![BinCmd { name: cmd_name, path: bin_path }] + } + Some(Value::Object(map)) => map + .iter() + .filter_map(|(name, path_val)| { + let bin_name = if name.starts_with('@') { + name.split_once('/')?.1.to_string() + } else { + name.clone() + }; + if !is_bin_name_safe(&bin_name) { + return None; + } + let rel_path = path_val.as_str()?; + let bin_path = pkg_dir.join(rel_path); + if !is_within(pkg_dir, &bin_path) { + return None; + } + Some(BinCmd { name: bin_name, path: bin_path }) + }) + .collect(), + _ => vec![], + } +} + +/// Create symlinks in `bins_dir` for each command. +/// +/// Uses absolute targets (consistent with the rest of the pacquet codebase; +/// pnpm uses relative targets — tracked as a TODO). +/// +/// Skips creation if the symlink already points at the correct target +/// (idempotent on warm installs). +fn create_bin_symlinks(cmds: &[BinCmd], bins_dir: &Path) -> Result<(), LinkBinsError> { + if cmds.is_empty() { + return Ok(()); + } + + fs::create_dir_all(bins_dir).map_err(|error| LinkBinsError::CreateBinDir { + dir: bins_dir.to_path_buf(), + error, + })?; + + for cmd in cmds { + let link_path = bins_dir.join(&cmd.name); + + // Skip if already correct. + if matches!(fs::read_link(&link_path), Ok(t) if t == cmd.path) { + continue; + } + + // Remove stale link or file. + let _ = fs::remove_file(&link_path); + + std::os::unix::fs::symlink(&cmd.path, &link_path) + .map_err(|error| LinkBinsError::CreateSymlink { link: link_path.clone(), error })?; + + // Ensure the target has the executable bit set. + if let Ok(meta) = fs::metadata(&cmd.path) { + use std::os::unix::fs::PermissionsExt; + let mut perms = meta.permissions(); + let mode = perms.mode(); + if mode & 0o111 == 0 { + perms.set_mode(mode | 0o111); + let _ = fs::set_permissions(&cmd.path, perms); + } + } + } + + Ok(()) +} + +/// Strip the `@scope/` prefix from a package name. +/// +/// Used when a `"bin"` value is a string: the command name is derived from +/// the package name, minus any scope. +fn strip_scope(name: &str) -> &str { + if let Some(rest) = name.strip_prefix('@') { + rest.split_once('/').map(|x| x.1).unwrap_or(name) + } else { + name + } +} + +/// Return `true` if `candidate` is inside `base` (no path traversal). +/// +/// Lexically normalises `candidate` by resolving `..` components before +/// calling `starts_with`, so a path like `pkg/../../../etc/passwd` does not +/// falsely pass the prefix check. Mirrors the `isSubdir` check in +/// `@pnpm/bins.resolver`. +fn is_within(base: &Path, candidate: &Path) -> bool { + normalize_path(candidate).starts_with(base) +} + +/// Resolve `..` and `.` components lexically (without hitting the filesystem). +fn normalize_path(path: &Path) -> PathBuf { + use std::path::Component; + let mut components: Vec> = Vec::new(); + for c in path.components() { + match c { + Component::ParentDir => { + // Only pop a real component; ignore `..` at the root. + if matches!(components.last(), Some(Component::Normal(_))) { + components.pop(); + } + } + Component::CurDir => {} + other => components.push(other), + } + } + components.iter().collect() +} + +/// Return `true` if `name` is a safe bin name. +/// +/// Mirrors pnpm's check: valid if every character is unchanged by +/// `encodeURIComponent`, or the name is exactly `$`. +/// +/// `encodeURIComponent` leaves unreserved characters unencoded: +/// `A-Z a-z 0-9 - _ . ! ~ * ' ( )` +/// +/// Upstream reference: +/// +fn is_bin_name_safe(name: &str) -> bool { + if name == "$" { + return true; + } + name.chars().all(|c| { + matches!(c, + 'A'..='Z' | 'a'..='z' | '0'..='9' + | '-' | '_' | '.' | '!' | '~' | '*' | '\'' | '(' | ')' + ) + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use serde_json::json; + use tempfile::TempDir; + + fn touch(dir: &Path, rel: &str) -> PathBuf { + let p = dir.join(rel); + if let Some(parent) = p.parent() { + fs::create_dir_all(parent).unwrap(); + } + fs::write(&p, "#!/usr/bin/env node\n").unwrap(); + p + } + + #[test] + fn strip_scope_unscoped() { + assert_eq!(strip_scope("typescript"), "typescript"); + } + + #[test] + fn strip_scope_scoped() { + assert_eq!(strip_scope("@babel/cli"), "cli"); + } + + #[test] + fn is_bin_name_safe_valid() { + assert!(is_bin_name_safe("tsc")); + assert!(is_bin_name_safe("jest")); + assert!(is_bin_name_safe("create-react-app")); + assert!(is_bin_name_safe("$")); + } + + #[test] + fn is_bin_name_safe_invalid() { + assert!(!is_bin_name_safe("../evil")); + assert!(!is_bin_name_safe("cmd with space")); + assert!(!is_bin_name_safe("cmd;injection")); + } + + #[test] + fn parse_bin_string_form() { + let tmp = TempDir::new().unwrap(); + touch(tmp.path(), "bin/tsc"); + let manifest = json!({ "name": "typescript", "bin": "bin/tsc" }); + let cmds = parse_bin_cmds(&manifest, tmp.path()); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0].name, "typescript"); + assert_eq!(cmds[0].path, tmp.path().join("bin/tsc")); + } + + #[test] + fn parse_bin_string_form_scoped() { + let tmp = TempDir::new().unwrap(); + touch(tmp.path(), "cli.js"); + let manifest = json!({ "name": "@babel/cli", "bin": "cli.js" }); + let cmds = parse_bin_cmds(&manifest, tmp.path()); + assert_eq!(cmds.len(), 1); + assert_eq!(cmds[0].name, "cli"); + } + + #[test] + fn parse_bin_object_form() { + let tmp = TempDir::new().unwrap(); + touch(tmp.path(), "bin/tsc"); + touch(tmp.path(), "bin/tsserver"); + let manifest = json!({ + "name": "typescript", + "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } + }); + let mut cmds = parse_bin_cmds(&manifest, tmp.path()); + cmds.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(cmds.len(), 2); + assert_eq!(cmds[0].name, "tsc"); + assert_eq!(cmds[1].name, "tsserver"); + } + + #[test] + fn parse_bin_path_traversal_rejected() { + let tmp = TempDir::new().unwrap(); + let manifest = json!({ "name": "evil", "bin": "../../../etc/passwd" }); + let cmds = parse_bin_cmds(&manifest, tmp.path()); + assert!(cmds.is_empty()); + } + + #[test] + fn create_bin_symlinks_creates_link() { + let tmp = TempDir::new().unwrap(); + let pkg_dir = tmp.path().join("pkg"); + let bin_target = touch(&pkg_dir, "bin/cli.js"); + let bins_dir = tmp.path().join("node_modules/.bin"); + + let cmds = vec![BinCmd { name: "mycli".to_string(), path: bin_target.clone() }]; + create_bin_symlinks(&cmds, &bins_dir).unwrap(); + + let link = bins_dir.join("mycli"); + assert!(link.exists() || link.symlink_metadata().is_ok()); + assert_eq!(fs::read_link(&link).unwrap(), bin_target); + } + + #[test] + fn create_bin_symlinks_idempotent() { + let tmp = TempDir::new().unwrap(); + let pkg_dir = tmp.path().join("pkg"); + let bin_target = touch(&pkg_dir, "bin/cli.js"); + let bins_dir = tmp.path().join("node_modules/.bin"); + + let cmds = vec![BinCmd { name: "mycli".to_string(), path: bin_target.clone() }]; + create_bin_symlinks(&cmds, &bins_dir).unwrap(); + create_bin_symlinks(&cmds, &bins_dir).unwrap(); // second call must not error + } +} From bd65f2b746da7e6aa46c4daea21aa67bffe1cc7c Mon Sep 17 00:00:00 2001 From: Rakibul Yeasin Date: Wed, 29 Apr 2026 11:28:43 +0600 Subject: [PATCH 4/5] fix(package-manager): skip platform-incompatible optional snapshots Implements platform filtering for optional dependencies during frozen lockfile install, fixing issue #266. Packages with os/cpu/libc constraints in the lockfile packages: section are now skipped when the constraints do not match the current system and the package is only reachable via optional dependency edges. Two passes: 1. BFS from root importer tracks which snapshots are required vs. optional-only (required beats optional: a package reached via both paths is treated as required and always installed). 2. Optional-only snapshots that fail is_platform_supported() are added to skipped_snapshots, which CreateVirtualStore and SymlinkDirectDependencies both honour. Mirrors pnpm's checkPlatform in @pnpm/config.package-is-installable and the packageIsInstallable flow in the headless restorer: https://github.com/pnpm/pnpm/blob/3f37d17b23/config/package-is-installable/src/checkPlatform.ts supportedArchitectures (pnpm-workspace.yaml) is not yet implemented. --- crates/package-manager/src/check_platform.rs | 189 ++++++++++++++++++ .../src/create_virtual_store.rs | 21 +- .../src/install_frozen_lockfile.rs | 109 +++++++++- crates/package-manager/src/lib.rs | 2 + .../src/symlink_direct_dependencies.rs | 14 +- 5 files changed, 325 insertions(+), 10 deletions(-) create mode 100644 crates/package-manager/src/check_platform.rs diff --git a/crates/package-manager/src/check_platform.rs b/crates/package-manager/src/check_platform.rs new file mode 100644 index 000000000..3391b07b5 --- /dev/null +++ b/crates/package-manager/src/check_platform.rs @@ -0,0 +1,189 @@ +// Platform compatibility check for optional packages. +// +// Mirrors pnpm's `checkPlatform` from `@pnpm/config.package-is-installable`: +// https://github.com/pnpm/pnpm/blob/3f37d17b23/config/package-is-installable/src/checkPlatform.ts +// +// Only the current-platform path is implemented (no `supportedArchitectures` +// expansion — that is a separate `pnpm-workspace.yaml` feature). + +/// Return the OS string as pnpm sees it. +/// +/// Maps Rust `target_os` values to the Node.js `process.platform` strings +/// that appear in `package.json` `os` fields. +pub fn current_os() -> &'static str { + if cfg!(target_os = "linux") { + "linux" + } else if cfg!(target_os = "macos") { + "darwin" + } else if cfg!(target_os = "windows") { + "win32" + } else if cfg!(target_os = "freebsd") { + "freebsd" + } else if cfg!(target_os = "openbsd") { + "openbsd" + } else { + "unknown" + } +} + +/// Return the CPU architecture string as pnpm sees it. +/// +/// Maps Rust `target_arch` values to the Node.js `process.arch` strings +/// that appear in `package.json` `cpu` fields. +pub fn current_cpu() -> &'static str { + if cfg!(target_arch = "x86_64") { + "x64" + } else if cfg!(target_arch = "aarch64") { + "arm64" + } else if cfg!(target_arch = "x86") { + "ia32" + } else if cfg!(target_arch = "arm") { + "arm" + } else if cfg!(target_arch = "riscv64") { + "riscv64" + } else if cfg!(target_arch = "s390x") { + "s390x" + } else if cfg!(target_arch = "powerpc64") { + "ppc64" + } else { + "unknown" + } +} + +/// Return the C library identifier as pnpm sees it. +/// +/// Only meaningful on Linux: returns `"glibc"` for GNU/glibc targets and +/// `"musl"` for musl targets. Returns `"unknown"` on all other platforms, +/// which causes the libc check to be skipped (mirrors pnpm's behaviour when +/// `detect-libc` returns `null`). +pub fn current_libc() -> &'static str { + if cfg!(all(target_os = "linux", target_env = "musl")) { + "musl" + } else if cfg!(target_os = "linux") { + "glibc" + } else { + "unknown" + } +} + +/// Return `true` if a package with the given platform constraints is +/// installable on the current system. +/// +/// `None` means "no constraint" (always allowed). An empty slice defaults to +/// `["any"]` to match pnpm's runtime fallback. +/// +/// Upstream reference: +/// +pub fn is_platform_supported( + os: Option<&[String]>, + cpu: Option<&[String]>, + libc: Option<&[String]>, +) -> bool { + if let Some(os_list) = os + && !check_list(current_os(), os_list) + { + return false; + } + if let Some(cpu_list) = cpu + && !check_list(current_cpu(), cpu_list) + { + return false; + } + if let Some(libc_list) = libc { + let c = current_libc(); + // Skip the libc check when we cannot determine the current libc + // (mirrors pnpm: `if (wantedPlatform.libc && currentLibc !== 'unknown')`). + if c != "unknown" && !check_list(c, libc_list) { + return false; + } + } + true +} + +/// Check whether `value` satisfies `list`. +/// +/// Rules (mirrors pnpm's `checkList`): +/// * `["any"]` → always true. +/// * Entries starting with `!` are exclusions: if `value` matches, return false. +/// * Positive entries: if any matches `value`, the check passes. +/// * If every entry is an exclusion and none matched `value`, the check passes. +fn check_list(value: &str, list: &[String]) -> bool { + if list.len() == 1 && list[0] == "any" { + return true; + } + let mut any_match = false; + let mut blacklist_count: usize = 0; + for entry in list { + if let Some(excluded) = entry.strip_prefix('!') { + if excluded == value { + return false; + } + blacklist_count += 1; + } else if entry == value { + any_match = true; + } + } + any_match || blacklist_count == list.len() +} + +#[cfg(test)] +mod tests { + use super::*; + + fn sl(items: &[&str]) -> Vec { + items.iter().map(|s| s.to_string()).collect() + } + + #[test] + fn any_always_passes() { + assert!(check_list("linux", &sl(&["any"]))); + assert!(check_list("darwin", &sl(&["any"]))); + } + + #[test] + fn positive_match() { + assert!(check_list("linux", &sl(&["linux"]))); + assert!(!check_list("darwin", &sl(&["linux"]))); + } + + #[test] + fn multiple_positive() { + assert!(check_list("linux", &sl(&["linux", "darwin"]))); + assert!(check_list("darwin", &sl(&["linux", "darwin"]))); + assert!(!check_list("win32", &sl(&["linux", "darwin"]))); + } + + #[test] + fn negation_excludes() { + // !darwin means "not darwin" — linux passes + assert!(check_list("linux", &sl(&["!darwin"]))); + assert!(!check_list("darwin", &sl(&["!darwin"]))); + } + + #[test] + fn all_negations_with_no_match_passes() { + // ["!darwin", "!win32"] — linux passes (all entries are exclusions, none matched) + assert!(check_list("linux", &sl(&["!darwin", "!win32"]))); + assert!(!check_list("darwin", &sl(&["!darwin", "!win32"]))); + } + + #[test] + fn is_platform_supported_no_constraints() { + assert!(is_platform_supported(None, None, None)); + } + + #[test] + fn is_platform_supported_os_constraint() { + // Package that claims to support only darwin should fail on linux + #[cfg(target_os = "linux")] + assert!(!is_platform_supported(Some(&sl(&["darwin"])), None, None)); + + #[cfg(target_os = "macos")] + assert!(!is_platform_supported(Some(&sl(&["linux"])), None, None)); + } + + #[test] + fn is_platform_supported_all_os_allowed() { + assert!(is_platform_supported(Some(&sl(&["any"])), None, None)); + } +} diff --git a/crates/package-manager/src/create_virtual_store.rs b/crates/package-manager/src/create_virtual_store.rs index dad48f396..804310649 100644 --- a/crates/package-manager/src/create_virtual_store.rs +++ b/crates/package-manager/src/create_virtual_store.rs @@ -10,7 +10,7 @@ use pacquet_npmrc::Npmrc; use pacquet_store_dir::{StoreIndex, StoreIndexWriter, store_index_key}; use pacquet_tarball::prefetch_cas_paths; use pipe_trait::Pipe; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; /// This subroutine generates filesystem layout for the virtual store at `node_modules/.pacquet`. #[must_use] @@ -19,6 +19,10 @@ pub struct CreateVirtualStore<'a> { pub config: &'static Npmrc, pub packages: Option<&'a HashMap>, pub snapshots: Option<&'a HashMap>, + /// Snapshot keys that should be skipped because they are optional + /// dependencies that are incompatible with the current platform. + /// Computed by [`InstallFrozenLockfile`] before calling this subroutine. + pub skipped_snapshots: &'a HashSet, } /// Error type of [`CreateVirtualStore`]. @@ -43,7 +47,8 @@ pub enum CreateVirtualStoreError { impl<'a> CreateVirtualStore<'a> { /// Execute the subroutine. pub async fn run(self) -> Result<(), CreateVirtualStoreError> { - let CreateVirtualStore { http_client, config, packages, snapshots } = self; + let CreateVirtualStore { http_client, config, packages, snapshots, skipped_snapshots } = + self; let Some(snapshots) = snapshots else { // No snapshots to install. If the lockfile also has no project deps @@ -162,6 +167,18 @@ impl<'a> CreateVirtualStore<'a> { type SnapshotWithCacheKey<'a> = (&'a PackageKey, &'a SnapshotEntry, Option); let snapshot_entries: Vec> = snapshots .iter() + .filter(|(snapshot_key, _)| { + if skipped_snapshots.contains(snapshot_key) { + tracing::debug!( + target: "pacquet::install", + %snapshot_key, + "skipping optional snapshot: incompatible with current platform", + ); + false + } else { + true + } + }) .map(|(snapshot_key, snapshot)| { snapshot_cache_key(snapshot_key, packages).map(|key| (snapshot_key, snapshot, key)) }) diff --git a/crates/package-manager/src/install_frozen_lockfile.rs b/crates/package-manager/src/install_frozen_lockfile.rs index 22ca7ff7a..469665767 100644 --- a/crates/package-manager/src/install_frozen_lockfile.rs +++ b/crates/package-manager/src/install_frozen_lockfile.rs @@ -1,14 +1,16 @@ use crate::{ CreateVirtualStore, CreateVirtualStoreError, LinkBins, LinkBinsError, - SymlinkDirectDependencies, SymlinkDirectDependenciesError, + SymlinkDirectDependencies, SymlinkDirectDependenciesError, check_platform::is_platform_supported, }; use derive_more::{Display, Error}; use miette::Diagnostic; -use pacquet_lockfile::{PackageKey, PackageMetadata, ProjectSnapshot, SnapshotEntry}; +use pacquet_lockfile::{ + Lockfile, PackageKey, PackageMetadata, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry, +}; use pacquet_network::ThrottledClient; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::DependencyGroup; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet, VecDeque}; /// This subroutine installs dependencies from a frozen lockfile. /// @@ -63,10 +65,18 @@ where // TODO: check if the lockfile is out-of-date - // Collect once so we can pass the same slice to both symlink and bin-link steps. + // Collect once so we can pass the same slice to symlink and bin-link steps. let dependency_groups: Vec = dependency_groups.into_iter().collect(); - CreateVirtualStore { http_client, config, packages, snapshots } + // Compute which optional snapshots are incompatible with the current + // platform before touching the filesystem. Snapshots in this set are + // skipped by CreateVirtualStore, SymlinkDirectDependencies, and + // LinkBins. Mirrors pnpm's `packageIsInstallable` + optional-path + // tracking in the headless restorer. + let skipped_snapshots = + compute_skipped_snapshots(importers, snapshots, packages.unwrap_or(&HashMap::new())); + + CreateVirtualStore { http_client, config, packages, snapshots, skipped_snapshots: &skipped_snapshots } .run() .await .map_err(InstallFrozenLockfileError::CreateVirtualStore)?; @@ -75,6 +85,7 @@ where config, importers, dependency_groups: dependency_groups.iter().copied(), + skipped_snapshots: &skipped_snapshots, } .run() .map_err(InstallFrozenLockfileError::SymlinkDirectDependencies)?; @@ -86,3 +97,91 @@ where Ok(()) } } + +/// Compute the set of snapshot keys that should be skipped. +/// +/// A snapshot is skipped when **both** conditions hold: +/// 1. It is only reachable via optional dependency edges (if it is also +/// reachable via a required edge it must be installed even if the platform +/// check fails, and a separate diagnostic should be emitted — not yet +/// implemented). +/// 2. Its `os` / `cpu` / `libc` constraints (from the `packages:` section) +/// are incompatible with the current platform. +/// +/// Algorithm: BFS from the root importer, tracking whether each reachable +/// snapshot was reached via at least one non-optional path (`required`) or +/// only via optional paths (`optional_only`). Required beats optional: once a +/// snapshot is in `required` it cannot be demoted. +/// +/// Upstream reference: +/// +fn compute_skipped_snapshots( + importers: &HashMap, + snapshots: Option<&HashMap>, + packages: &HashMap, +) -> HashSet { + let Some(snapshots) = snapshots else { return HashSet::new() }; + + let mut required: HashSet = HashSet::new(); + let mut optional_only: HashSet = HashSet::new(); + // (key, is_on_optional_path) + let mut queue: VecDeque<(PackageKey, bool)> = VecDeque::new(); + + let enqueue = |key: PackageKey, is_optional: bool, required: &mut HashSet, optional_only: &mut HashSet, queue: &mut VecDeque<(PackageKey, bool)>| { + if is_optional { + if !required.contains(&key) && optional_only.insert(key.clone()) { + queue.push_back((key, true)); + } + } else { + let newly_required = required.insert(key.clone()); + let was_optional = optional_only.remove(&key); + if newly_required || was_optional { + queue.push_back((key, false)); + } + } + }; + + // Seed from root importer's direct dependencies. + if let Some(root) = importers.get(Lockfile::ROOT_IMPORTER_KEY) { + for (name, spec) in root.dependencies_by_groups([ + DependencyGroup::Prod, + DependencyGroup::Dev, + ]) { + let key = PkgNameVerPeer::new(name.clone(), spec.version.clone()); + enqueue(key, false, &mut required, &mut optional_only, &mut queue); + } + for (name, spec) in root.dependencies_by_groups([DependencyGroup::Optional]) { + let key = PkgNameVerPeer::new(name.clone(), spec.version.clone()); + enqueue(key, true, &mut required, &mut optional_only, &mut queue); + } + } + + // BFS through snapshot dependency edges. + while let Some((key, is_optional_path)) = queue.pop_front() { + let Some(snapshot) = snapshots.get(&key) else { continue }; + + for (dep_name, dep_ref) in snapshot.dependencies.iter().flatten() { + let dep_key = dep_ref.resolve(dep_name); + enqueue(dep_key, is_optional_path, &mut required, &mut optional_only, &mut queue); + } + for (dep_name, dep_ref) in snapshot.optional_dependencies.iter().flatten() { + let dep_key = dep_ref.resolve(dep_name); + // Optional deps are always on an optional path regardless of parent. + enqueue(dep_key, true, &mut required, &mut optional_only, &mut queue); + } + } + + // A snapshot is skipped when it is optional-only AND platform-incompatible. + optional_only + .into_iter() + .filter(|key| { + let metadata_key = key.without_peer(); + let Some(meta) = packages.get(&metadata_key) else { return false }; + !is_platform_supported( + meta.os.as_deref(), + meta.cpu.as_deref(), + meta.libc.as_deref(), + ) + }) + .collect() +} diff --git a/crates/package-manager/src/lib.rs b/crates/package-manager/src/lib.rs index c75208b02..2a4fe8821 100644 --- a/crates/package-manager/src/lib.rs +++ b/crates/package-manager/src/lib.rs @@ -1,5 +1,6 @@ mod add; mod build_snapshot; +mod check_platform; mod create_cas_files; mod create_symlink_layout; mod create_virtual_dir_by_snapshot; @@ -18,6 +19,7 @@ mod symlink_package; pub use add::*; pub use build_snapshot::*; +pub use check_platform::*; pub use create_cas_files::*; pub use create_symlink_layout::*; pub use create_virtual_dir_by_snapshot::*; diff --git a/crates/package-manager/src/symlink_direct_dependencies.rs b/crates/package-manager/src/symlink_direct_dependencies.rs index 7c08b1e44..72983b9a5 100644 --- a/crates/package-manager/src/symlink_direct_dependencies.rs +++ b/crates/package-manager/src/symlink_direct_dependencies.rs @@ -1,11 +1,11 @@ use crate::symlink_package; use derive_more::{Display, Error}; use miette::Diagnostic; -use pacquet_lockfile::{Lockfile, PkgName, PkgNameVerPeer, ProjectSnapshot}; +use pacquet_lockfile::{Lockfile, PackageKey, PkgName, PkgNameVerPeer, ProjectSnapshot}; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::DependencyGroup; use rayon::prelude::*; -use std::collections::HashMap; +use std::collections::{HashMap, HashSet}; /// This subroutine creates symbolic links in the `node_modules` directory for /// the direct dependencies. The targets of the link are the virtual directories. @@ -21,6 +21,9 @@ where pub config: &'static Npmrc, pub importers: &'a HashMap, pub dependency_groups: DependencyGroupList, + /// Snapshot keys that were skipped due to platform incompatibility. + /// Symlinks to these packages are not created. + pub skipped_snapshots: &'a HashSet, } /// Error type of [`SymlinkDirectDependencies`]. @@ -39,7 +42,8 @@ where { /// Execute the subroutine. pub fn run(self) -> Result<(), SymlinkDirectDependenciesError> { - let SymlinkDirectDependencies { config, importers, dependency_groups } = self; + let SymlinkDirectDependencies { config, importers, dependency_groups, skipped_snapshots } = + self; let project_snapshot = importers.get(Lockfile::ROOT_IMPORTER_KEY).ok_or_else(|| { SymlinkDirectDependenciesError::MissingRootImporter { @@ -49,6 +53,10 @@ where project_snapshot .dependencies_by_groups(dependency_groups) + .filter(|(name, spec)| { + let key = PkgNameVerPeer::new(PkgName::clone(name), spec.version.clone()); + !skipped_snapshots.contains(&key) + }) .collect::>() .par_iter() .for_each(|(name, spec)| { From 3d6dbddc6fb5a3be18689b4a3b8764908eb6115e Mon Sep 17 00:00:00 2001 From: Rakibul Yeasin Date: Wed, 29 Apr 2026 17:06:49 +0600 Subject: [PATCH 5/5] fix(package-manager): complete link_bins implementation - Thread skipped_snapshots into LinkBins so platform-incompatible optional packages are skipped explicitly rather than relying on implicit NotFound. - Add directories.bin support via walkdir (mirrors getBinsFromPackageManifest). - Switch bin symlinks from absolute to relative targets, matching pnpm's symlink-dir behaviour. - Add bin conflict deduplication (deduplicateCommands): group by name, pick winner by ownership (BIN_OWNER_OVERRIDES), then alphabetical pkg name, then higher semver version. Mirrors @pnpm/bins.linker. - Add pkg_name/pkg_version to BinCmd for conflict resolution. Upstream references: - https://github.com/pnpm/pnpm/blob/3f37d17b23/bins/resolver/src/index.ts - https://github.com/pnpm/pnpm/blob/3f37d17b23/bins/linker/src/index.ts --- crates/package-manager/Cargo.toml | 2 +- .../src/install_frozen_lockfile.rs | 49 +- crates/package-manager/src/link_bins.rs | 452 ++++++++++++++---- 3 files changed, 397 insertions(+), 106 deletions(-) diff --git a/crates/package-manager/Cargo.toml b/crates/package-manager/Cargo.toml index b502ad22f..9a7e4f858 100644 --- a/crates/package-manager/Cargo.toml +++ b/crates/package-manager/Cargo.toml @@ -32,6 +32,7 @@ serde_json = { workspace = true } tokio = { workspace = true } tracing = { workspace = true } miette = { workspace = true } +walkdir = { workspace = true } [dev-dependencies] pacquet-registry-mock = { workspace = true } @@ -43,4 +44,3 @@ pretty_assertions = { workspace = true } serde-saphyr = { workspace = true } ssri = { workspace = true } tempfile = { workspace = true } -walkdir = { workspace = true } diff --git a/crates/package-manager/src/install_frozen_lockfile.rs b/crates/package-manager/src/install_frozen_lockfile.rs index 469665767..6423512bb 100644 --- a/crates/package-manager/src/install_frozen_lockfile.rs +++ b/crates/package-manager/src/install_frozen_lockfile.rs @@ -1,6 +1,7 @@ use crate::{ CreateVirtualStore, CreateVirtualStoreError, LinkBins, LinkBinsError, - SymlinkDirectDependencies, SymlinkDirectDependenciesError, check_platform::is_platform_supported, + SymlinkDirectDependencies, SymlinkDirectDependenciesError, + check_platform::is_platform_supported, }; use derive_more::{Display, Error}; use miette::Diagnostic; @@ -76,10 +77,16 @@ where let skipped_snapshots = compute_skipped_snapshots(importers, snapshots, packages.unwrap_or(&HashMap::new())); - CreateVirtualStore { http_client, config, packages, snapshots, skipped_snapshots: &skipped_snapshots } - .run() - .await - .map_err(InstallFrozenLockfileError::CreateVirtualStore)?; + CreateVirtualStore { + http_client, + config, + packages, + snapshots, + skipped_snapshots: &skipped_snapshots, + } + .run() + .await + .map_err(InstallFrozenLockfileError::CreateVirtualStore)?; SymlinkDirectDependencies { config, @@ -90,9 +97,16 @@ where .run() .map_err(InstallFrozenLockfileError::SymlinkDirectDependencies)?; - LinkBins { config, importers, packages, snapshots, dependency_groups: &dependency_groups } - .run() - .map_err(InstallFrozenLockfileError::LinkBins)?; + LinkBins { + config, + importers, + packages, + snapshots, + dependency_groups: &dependency_groups, + skipped_snapshots: &skipped_snapshots, + } + .run() + .map_err(InstallFrozenLockfileError::LinkBins)?; Ok(()) } @@ -127,7 +141,11 @@ fn compute_skipped_snapshots( // (key, is_on_optional_path) let mut queue: VecDeque<(PackageKey, bool)> = VecDeque::new(); - let enqueue = |key: PackageKey, is_optional: bool, required: &mut HashSet, optional_only: &mut HashSet, queue: &mut VecDeque<(PackageKey, bool)>| { + let enqueue = |key: PackageKey, + is_optional: bool, + required: &mut HashSet, + optional_only: &mut HashSet, + queue: &mut VecDeque<(PackageKey, bool)>| { if is_optional { if !required.contains(&key) && optional_only.insert(key.clone()) { queue.push_back((key, true)); @@ -143,10 +161,9 @@ fn compute_skipped_snapshots( // Seed from root importer's direct dependencies. if let Some(root) = importers.get(Lockfile::ROOT_IMPORTER_KEY) { - for (name, spec) in root.dependencies_by_groups([ - DependencyGroup::Prod, - DependencyGroup::Dev, - ]) { + for (name, spec) in + root.dependencies_by_groups([DependencyGroup::Prod, DependencyGroup::Dev]) + { let key = PkgNameVerPeer::new(name.clone(), spec.version.clone()); enqueue(key, false, &mut required, &mut optional_only, &mut queue); } @@ -177,11 +194,7 @@ fn compute_skipped_snapshots( .filter(|key| { let metadata_key = key.without_peer(); let Some(meta) = packages.get(&metadata_key) else { return false }; - !is_platform_supported( - meta.os.as_deref(), - meta.cpu.as_deref(), - meta.libc.as_deref(), - ) + !is_platform_supported(meta.os.as_deref(), meta.cpu.as_deref(), meta.libc.as_deref()) }) .collect() } diff --git a/crates/package-manager/src/link_bins.rs b/crates/package-manager/src/link_bins.rs index bf574872d..5ccbbf511 100644 --- a/crates/package-manager/src/link_bins.rs +++ b/crates/package-manager/src/link_bins.rs @@ -1,16 +1,34 @@ use std::{ - collections::HashMap, + collections::{HashMap, HashSet}, fs, io::{self, ErrorKind}, - path::{Path, PathBuf}, + path::{Component, Path, PathBuf}, }; use derive_more::{Display, Error}; use miette::Diagnostic; -use pacquet_lockfile::{Lockfile, PackageKey, PackageMetadata, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry}; +use node_semver::Version; +use pacquet_lockfile::{ + Lockfile, PackageKey, PackageMetadata, PkgNameVerPeer, ProjectSnapshot, SnapshotEntry, +}; use pacquet_npmrc::Npmrc; use pacquet_package_manifest::DependencyGroup; use serde_json::Value; +use walkdir::WalkDir; + +// Maps a bin name to all packages that are legitimate owners of it beyond the +// default rule that a package named `X` owns the `X` bin. +// Mirrors pnpm's `BIN_OWNER_OVERRIDES` in `@pnpm/bins.resolver`. +// +// Upstream reference: +// +static BIN_OWNER_OVERRIDES: &[(&str, &[&str])] = &[ + ("npx", &["npm"]), + ("pn", &["pnpm", "@pnpm/exe"]), + ("pnpm", &["@pnpm/exe"]), + ("pnpx", &["pnpm", "@pnpm/exe"]), + ("pnx", &["pnpm", "@pnpm/exe"]), +]; /// Error type for [`LinkBins`]. #[derive(Debug, Display, Error, Diagnostic)] @@ -52,6 +70,8 @@ pub enum LinkBinsError { struct BinCmd { name: String, path: PathBuf, + pkg_name: String, + pkg_version: String, } /// Create `.bin` symlinks for installed packages. @@ -74,12 +94,21 @@ pub struct LinkBins<'a> { pub packages: Option<&'a HashMap>, pub snapshots: Option<&'a HashMap>, pub dependency_groups: &'a [DependencyGroup], + /// Snapshot keys skipped by platform check; their bins are not linked. + pub skipped_snapshots: &'a HashSet, } impl<'a> LinkBins<'a> { /// Execute the subroutine. pub fn run(self) -> Result<(), LinkBinsError> { - let LinkBins { config, importers, packages, snapshots, dependency_groups } = self; + let LinkBins { + config, + importers, + packages, + snapshots, + dependency_groups, + skipped_snapshots, + } = self; let Some(snapshots) = snapshots else { return Ok(()) }; let Some(packages) = packages else { return Ok(()) }; @@ -91,8 +120,16 @@ impl<'a> LinkBins<'a> { if let Some(root_importer) = importers.get(Lockfile::ROOT_IMPORTER_KEY) { let bins_dir = modules_dir.join(".bin"); - for (dep_name, dep_spec) in root_importer.dependencies_by_groups(dependency_groups.iter().copied()) { + let mut cmds: Vec = Vec::new(); + for (dep_name, dep_spec) in + root_importer.dependencies_by_groups(dependency_groups.iter().copied()) + { let pkg_key = PkgNameVerPeer::new(dep_name.clone(), dep_spec.version.clone()); + + if skipped_snapshots.contains(&pkg_key) { + continue; + } + let metadata_key = pkg_key.without_peer(); if packages.get(&metadata_key).and_then(|m| m.has_bin) != Some(true) { @@ -104,9 +141,9 @@ impl<'a> LinkBins<'a> { .join("node_modules") .join(dep_name.to_string()); - let cmds = read_bin_cmds(&pkg_dir)?; - create_bin_symlinks(&cmds, &bins_dir)?; + cmds.extend(read_bin_cmds(&pkg_dir)?); } + create_bin_symlinks(cmds, &bins_dir)?; } // Pass 2: per-package node_modules/.bin/ for each virtual-store slot. @@ -115,9 +152,12 @@ impl<'a> LinkBins<'a> { // so that lifecycle scripts can invoke those executables. Mirrors // pnpm's `linkAllBins` in the deps-restorer. for (snapshot_key, snapshot) in snapshots { - let snapshot_node_modules = virtual_store_dir - .join(snapshot_key.to_virtual_store_name()) - .join("node_modules"); + if skipped_snapshots.contains(snapshot_key) { + continue; + } + + let snapshot_node_modules = + virtual_store_dir.join(snapshot_key.to_virtual_store_name()).join("node_modules"); let bins_dir = snapshot_node_modules.join(".bin"); let all_deps = snapshot @@ -130,6 +170,11 @@ impl<'a> LinkBins<'a> { for (dep_alias, dep_ref) in all_deps { let resolved = dep_ref.resolve(dep_alias); + + if skipped_snapshots.contains(&resolved) { + continue; + } + let dep_metadata_key = resolved.without_peer(); if packages.get(&dep_metadata_key).and_then(|m| m.has_bin) != Some(true) { @@ -144,18 +189,17 @@ impl<'a> LinkBins<'a> { cmds_for_slot.extend(read_bin_cmds(&dep_pkg_dir)?); } - if !cmds_for_slot.is_empty() { - create_bin_symlinks(&cmds_for_slot, &bins_dir)?; - } + create_bin_symlinks(cmds_for_slot, &bins_dir)?; } Ok(()) } } -/// Read the `bin` field from `{pkg_dir}/package.json` and return resolved commands. +/// Read the `bin` / `directories.bin` field from `{pkg_dir}/package.json` and +/// return resolved commands. /// -/// Returns an empty `Vec` if `package.json` is absent or has no `bin` field. +/// Returns an empty `Vec` if `package.json` is absent or has no bin field. fn read_bin_cmds(pkg_dir: &Path) -> Result, LinkBinsError> { let manifest_path = pkg_dir.join("package.json"); let content = match fs::read_to_string(&manifest_path) { @@ -171,81 +215,120 @@ fn read_bin_cmds(pkg_dir: &Path) -> Result, LinkBinsError> { /// Parse binary commands from a manifest value. /// -/// Supports both the string form (`"bin": "./cli.js"`) and the object form -/// (`"bin": { "tsc": "./bin/tsc" }`), mirroring `commandsFromBin` in -/// `@pnpm/bins.resolver`. -/// -/// `directories.bin` is not yet implemented (rare in practice). +/// Supports the string form, object form, and `directories.bin`, mirroring +/// `getBinsFromPackageManifest` + `commandsFromBin` in `@pnpm/bins.resolver`. /// /// Upstream reference: /// fn parse_bin_cmds(manifest: &Value, pkg_dir: &Path) -> Vec { let pkg_name = manifest.get("name").and_then(Value::as_str).unwrap_or(""); + let pkg_version = manifest.get("version").and_then(Value::as_str).unwrap_or(""); - match manifest.get("bin") { - Some(Value::String(rel_path)) => { - let cmd_name = strip_scope(pkg_name).to_string(); - if cmd_name.is_empty() || !is_bin_name_safe(&cmd_name) { - return vec![]; - } - let bin_path = pkg_dir.join(rel_path); - if !is_within(pkg_dir, &bin_path) { - return vec![]; - } - vec![BinCmd { name: cmd_name, path: bin_path }] + if let Some(bin) = manifest.get("bin") { + return commands_from_bin(bin, pkg_name, pkg_version, pkg_dir); + } + + // directories.bin: enumerate files in the named sub-directory. + // Mirrors pnpm's `findFiles` + directory path in `getBinsFromPackageManifest`. + if let Some(Value::String(rel_dir)) = manifest.get("directories").and_then(|d| d.get("bin")) { + let bin_dir = pkg_dir.join(rel_dir); + if !is_within(pkg_dir, &bin_dir) { + return vec![]; } - Some(Value::Object(map)) => map - .iter() - .filter_map(|(name, path_val)| { - let bin_name = if name.starts_with('@') { - name.split_once('/')?.1.to_string() - } else { - name.clone() - }; - if !is_bin_name_safe(&bin_name) { - return None; - } - let rel_path = path_val.as_str()?; - let bin_path = pkg_dir.join(rel_path); - if !is_within(pkg_dir, &bin_path) { + return WalkDir::new(&bin_dir) + .follow_links(false) + .into_iter() + .filter_map(|e| e.ok()) + .filter(|e| e.file_type().is_file()) + .filter_map(|e| { + let name = e.file_name().to_string_lossy().into_owned(); + if !is_bin_name_safe(&name) { return None; } - Some(BinCmd { name: bin_name, path: bin_path }) + Some(BinCmd { + name, + path: e.path().to_path_buf(), + pkg_name: pkg_name.to_string(), + pkg_version: pkg_version.to_string(), + }) }) - .collect(), - _ => vec![], + .collect(); } + + vec![] +} + +/// Build commands from a `bin` field value (string or object form). +/// +/// When `bin` is a string the command name is derived from `pkg_name` (with +/// any `@scope/` prefix stripped), matching pnpm's `commandsFromBin`. +fn commands_from_bin( + bin: &Value, + pkg_name: &str, + pkg_version: &str, + pkg_dir: &Path, +) -> Vec { + let pairs: Vec<(&str, &str)> = match bin { + Value::String(rel_path) => vec![(pkg_name, rel_path.as_str())], + Value::Object(map) => { + map.iter().filter_map(|(k, v)| v.as_str().map(|p| (k.as_str(), p))).collect() + } + _ => return vec![], + }; + + pairs + .into_iter() + .filter_map(|(cmd_name, rel_path)| { + let bin_name = + if cmd_name.starts_with('@') { cmd_name.split_once('/')?.1 } else { cmd_name }; + if bin_name.is_empty() || !is_bin_name_safe(bin_name) { + return None; + } + let bin_path = pkg_dir.join(rel_path); + if !is_within(pkg_dir, &bin_path) { + return None; + } + Some(BinCmd { + name: bin_name.to_string(), + path: bin_path, + pkg_name: pkg_name.to_string(), + pkg_version: pkg_version.to_string(), + }) + }) + .collect() } /// Create symlinks in `bins_dir` for each command. /// -/// Uses absolute targets (consistent with the rest of the pacquet codebase; -/// pnpm uses relative targets — tracked as a TODO). +/// Deduplicates commands by name before creating symlinks, mirroring pnpm's +/// `deduplicateCommands` call in `_linkBins`. Uses relative symlink targets +/// (matching pnpm's behaviour via `symlink-dir`). /// -/// Skips creation if the symlink already points at the correct target -/// (idempotent on warm installs). -fn create_bin_symlinks(cmds: &[BinCmd], bins_dir: &Path) -> Result<(), LinkBinsError> { +/// Idempotent: skips creation when the symlink already points at the correct +/// relative target. +fn create_bin_symlinks(cmds: Vec, bins_dir: &Path) -> Result<(), LinkBinsError> { if cmds.is_empty() { return Ok(()); } - fs::create_dir_all(bins_dir).map_err(|error| LinkBinsError::CreateBinDir { - dir: bins_dir.to_path_buf(), - error, - })?; + let cmds = deduplicate_commands(cmds, bins_dir); - for cmd in cmds { + fs::create_dir_all(bins_dir) + .map_err(|error| LinkBinsError::CreateBinDir { dir: bins_dir.to_path_buf(), error })?; + + for cmd in &cmds { let link_path = bins_dir.join(&cmd.name); + let rel_target = relative_path(bins_dir, &cmd.path); // Skip if already correct. - if matches!(fs::read_link(&link_path), Ok(t) if t == cmd.path) { + if matches!(fs::read_link(&link_path), Ok(t) if t == rel_target) { continue; } // Remove stale link or file. let _ = fs::remove_file(&link_path); - std::os::unix::fs::symlink(&cmd.path, &link_path) + std::os::unix::fs::symlink(&rel_target, &link_path) .map_err(|error| LinkBinsError::CreateSymlink { link: link_path.clone(), error })?; // Ensure the target has the executable bit set. @@ -263,16 +346,108 @@ fn create_bin_symlinks(cmds: &[BinCmd], bins_dir: &Path) -> Result<(), LinkBinsE Ok(()) } -/// Strip the `@scope/` prefix from a package name. +/// Deduplicate commands by name, keeping one winner per name. +/// +/// Mirrors pnpm's `deduplicateCommands` + `resolveCommandConflicts` in +/// `@pnpm/bins.linker`. +/// +/// Upstream reference: +/// +fn deduplicate_commands(cmds: Vec, bins_dir: &Path) -> Vec { + let mut groups: HashMap> = HashMap::new(); + for cmd in cmds { + groups.entry(cmd.name.clone()).or_default().push(cmd); + } + groups.into_values().map(|group| resolve_command_conflicts(group, bins_dir)).collect() +} + +fn resolve_command_conflicts(group: Vec, bins_dir: &Path) -> BinCmd { + group + .into_iter() + .reduce(|a, b| { + if compare_commands(&a, &b).is_ge() { + tracing::debug!( + target: "pacquet::install", + binary_name = %b.name, + bins_dir = %bins_dir.display(), + linked_pkg = %a.pkg_name, + linked_pkg_version = %a.pkg_version, + skipped_pkg = %b.pkg_name, + skipped_pkg_version = %b.pkg_version, + "bin conflict resolved", + ); + a + } else { + tracing::debug!( + target: "pacquet::install", + binary_name = %a.name, + bins_dir = %bins_dir.display(), + linked_pkg = %b.pkg_name, + linked_pkg_version = %b.pkg_version, + skipped_pkg = %a.pkg_name, + skipped_pkg_version = %a.pkg_version, + "bin conflict resolved", + ); + b + } + }) + .unwrap() // group is non-empty by construction +} + +/// Compare two commands competing for the same bin name. +/// +/// Priority (highest to lowest): +/// 1. Package that "owns" the bin name (name matches or in `BIN_OWNER_OVERRIDES`). +/// 2. Alphabetically later package name (arbitrary but deterministic tiebreaker, +/// matching pnpm's `localeCompare` — see upstream). +/// 3. Higher semver version (same package, different versions). +/// +/// Returns `Greater` when `a` should be chosen over `b`. +fn compare_commands(a: &BinCmd, b: &BinCmd) -> std::cmp::Ordering { + use std::cmp::Ordering; + let a_owns = pkg_owns_bin(&a.name, &a.pkg_name); + let b_owns = pkg_owns_bin(&b.name, &b.pkg_name); + if a_owns && !b_owns { + return Ordering::Greater; + } + if !a_owns && b_owns { + return Ordering::Less; + } + if a.pkg_name != b.pkg_name { + return a.pkg_name.cmp(&b.pkg_name); + } + match (a.pkg_version.parse::(), b.pkg_version.parse::()) { + (Ok(av), Ok(bv)) => av.cmp(&bv), + _ => Ordering::Equal, + } +} + +/// Return `true` if `pkg_name` is the legitimate owner of `bin_name`. /// -/// Used when a `"bin"` value is a string: the command name is derived from -/// the package name, minus any scope. -fn strip_scope(name: &str) -> &str { - if let Some(rest) = name.strip_prefix('@') { - rest.split_once('/').map(|x| x.1).unwrap_or(name) - } else { - name +/// Mirrors pnpm's `pkgOwnsBin` in `@pnpm/bins.resolver`. +fn pkg_owns_bin(bin_name: &str, pkg_name: &str) -> bool { + if bin_name == pkg_name { + return true; + } + BIN_OWNER_OVERRIDES.iter().any(|(b, owners)| *b == bin_name && owners.contains(&pkg_name)) +} + +/// Compute a relative path from `from_dir` to `to` (both absolute). +/// +/// Equivalent to `path.relative(from_dir, to)` in Node.js, used so that +/// bin symlinks are portable (matching pnpm's `symlink-dir` behaviour). +fn relative_path(from_dir: &Path, to: &Path) -> PathBuf { + let from: Vec> = from_dir.components().collect(); + let to: Vec> = to.components().collect(); + let common = from.iter().zip(to.iter()).take_while(|(a, b)| a == b).count(); + let mut rel = PathBuf::new(); + for _ in 0..(from.len() - common) { + rel.push(".."); + } + for c in &to[common..] { + rel.push(c.as_os_str()); } + rel } /// Return `true` if `candidate` is inside `base` (no path traversal). @@ -287,7 +462,6 @@ fn is_within(base: &Path, candidate: &Path) -> bool { /// Resolve `..` and `.` components lexically (without hitting the filesystem). fn normalize_path(path: &Path) -> PathBuf { - use std::path::Component; let mut components: Vec> = Vec::new(); for c in path.components() { match c { @@ -341,14 +515,13 @@ mod tests { p } - #[test] - fn strip_scope_unscoped() { - assert_eq!(strip_scope("typescript"), "typescript"); - } - - #[test] - fn strip_scope_scoped() { - assert_eq!(strip_scope("@babel/cli"), "cli"); + fn make_bin(name: &str, path: PathBuf) -> BinCmd { + BinCmd { + name: name.to_string(), + path, + pkg_name: name.to_string(), + pkg_version: "1.0.0".to_string(), + } } #[test] @@ -370,18 +543,20 @@ mod tests { fn parse_bin_string_form() { let tmp = TempDir::new().unwrap(); touch(tmp.path(), "bin/tsc"); - let manifest = json!({ "name": "typescript", "bin": "bin/tsc" }); + let manifest = json!({ "name": "typescript", "version": "5.0.0", "bin": "bin/tsc" }); let cmds = parse_bin_cmds(&manifest, tmp.path()); assert_eq!(cmds.len(), 1); assert_eq!(cmds[0].name, "typescript"); assert_eq!(cmds[0].path, tmp.path().join("bin/tsc")); + assert_eq!(cmds[0].pkg_name, "typescript"); + assert_eq!(cmds[0].pkg_version, "5.0.0"); } #[test] fn parse_bin_string_form_scoped() { let tmp = TempDir::new().unwrap(); touch(tmp.path(), "cli.js"); - let manifest = json!({ "name": "@babel/cli", "bin": "cli.js" }); + let manifest = json!({ "name": "@babel/cli", "version": "7.0.0", "bin": "cli.js" }); let cmds = parse_bin_cmds(&manifest, tmp.path()); assert_eq!(cmds.len(), 1); assert_eq!(cmds[0].name, "cli"); @@ -394,6 +569,7 @@ mod tests { touch(tmp.path(), "bin/tsserver"); let manifest = json!({ "name": "typescript", + "version": "5.0.0", "bin": { "tsc": "bin/tsc", "tsserver": "bin/tsserver" } }); let mut cmds = parse_bin_cmds(&manifest, tmp.path()); @@ -412,18 +588,55 @@ mod tests { } #[test] - fn create_bin_symlinks_creates_link() { + fn parse_directories_bin() { + let tmp = TempDir::new().unwrap(); + touch(tmp.path(), "scripts/foo"); + touch(tmp.path(), "scripts/bar"); + let manifest = json!({ + "name": "mypkg", + "version": "1.0.0", + "directories": { "bin": "scripts" } + }); + let mut cmds = parse_bin_cmds(&manifest, tmp.path()); + cmds.sort_by(|a, b| a.name.cmp(&b.name)); + assert_eq!(cmds.len(), 2); + assert_eq!(cmds[0].name, "bar"); + assert_eq!(cmds[1].name, "foo"); + } + + #[test] + fn parse_directories_bin_path_traversal_rejected() { + let tmp = TempDir::new().unwrap(); + let manifest = json!({ + "name": "evil", + "directories": { "bin": "../../etc" } + }); + let cmds = parse_bin_cmds(&manifest, tmp.path()); + assert!(cmds.is_empty()); + } + + #[test] + fn create_bin_symlinks_creates_relative_link() { let tmp = TempDir::new().unwrap(); let pkg_dir = tmp.path().join("pkg"); let bin_target = touch(&pkg_dir, "bin/cli.js"); let bins_dir = tmp.path().join("node_modules/.bin"); - let cmds = vec![BinCmd { name: "mycli".to_string(), path: bin_target.clone() }]; - create_bin_symlinks(&cmds, &bins_dir).unwrap(); + let cmds = vec![make_bin("mycli", bin_target.clone())]; + create_bin_symlinks(cmds, &bins_dir).unwrap(); let link = bins_dir.join("mycli"); - assert!(link.exists() || link.symlink_metadata().is_ok()); - assert_eq!(fs::read_link(&link).unwrap(), bin_target); + let link_target = fs::read_link(&link).unwrap(); + // Target must be relative, not absolute. + assert!( + link_target.is_relative(), + "symlink target should be relative, got {link_target:?}" + ); + // Resolving from bins_dir must reach the actual file. + assert_eq!( + bins_dir.join(&link_target).canonicalize().unwrap(), + bin_target.canonicalize().unwrap() + ); } #[test] @@ -433,8 +646,73 @@ mod tests { let bin_target = touch(&pkg_dir, "bin/cli.js"); let bins_dir = tmp.path().join("node_modules/.bin"); - let cmds = vec![BinCmd { name: "mycli".to_string(), path: bin_target.clone() }]; - create_bin_symlinks(&cmds, &bins_dir).unwrap(); - create_bin_symlinks(&cmds, &bins_dir).unwrap(); // second call must not error + let cmds = || vec![make_bin("mycli", bin_target.clone())]; + create_bin_symlinks(cmds(), &bins_dir).unwrap(); + create_bin_symlinks(cmds(), &bins_dir).unwrap(); + } + + #[test] + fn deduplicate_keeps_owner() { + let tmp = TempDir::new().unwrap(); + touch(tmp.path(), "a.js"); + touch(tmp.path(), "b.js"); + // "tsc" is not owned by either package per BIN_OWNER_OVERRIDES, + // so alphabetically later pkg name wins. + let cmds = vec![ + BinCmd { + name: "tsc".to_string(), + path: tmp.path().join("a.js"), + pkg_name: "aaa".to_string(), + pkg_version: "1.0.0".to_string(), + }, + BinCmd { + name: "tsc".to_string(), + path: tmp.path().join("b.js"), + pkg_name: "zzz".to_string(), + pkg_version: "1.0.0".to_string(), + }, + ]; + let bins_dir = tmp.path().join(".bin"); + let result = deduplicate_commands(cmds, &bins_dir); + assert_eq!(result.len(), 1); + assert_eq!(result[0].pkg_name, "zzz"); + } + + #[test] + fn deduplicate_owner_override_wins() { + let tmp = TempDir::new().unwrap(); + touch(tmp.path(), "a.js"); + touch(tmp.path(), "b.js"); + // "pnpm" bin: "@pnpm/exe" is in BIN_OWNER_OVERRIDES, so it wins over "zzz". + let cmds = vec![ + BinCmd { + name: "pnpm".to_string(), + path: tmp.path().join("a.js"), + pkg_name: "@pnpm/exe".to_string(), + pkg_version: "9.0.0".to_string(), + }, + BinCmd { + name: "pnpm".to_string(), + path: tmp.path().join("b.js"), + pkg_name: "zzz".to_string(), + pkg_version: "9.0.0".to_string(), + }, + ]; + let bins_dir = tmp.path().join(".bin"); + let result = deduplicate_commands(cmds, &bins_dir); + assert_eq!(result.len(), 1); + assert_eq!(result[0].pkg_name, "@pnpm/exe"); + } + + #[test] + fn relative_path_sibling_dirs() { + let rel = relative_path(Path::new("/a/b/.bin"), Path::new("/a/b/pkg/bin/cli.js")); + assert_eq!(rel, PathBuf::from("../pkg/bin/cli.js")); + } + + #[test] + fn relative_path_up_and_across() { + let rel = relative_path(Path::new("/a/b/c"), Path::new("/a/d/e.js")); + assert_eq!(rel, PathBuf::from("../../d/e.js")); } }