Skip to content
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions src/install/PackageInstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2276,8 +2276,19 @@ impl<'a> PackageInstall<'a> {
dest_name_buf[..dest.len()].copy_from_slice(dest);
// SAFETY: zero-initialized; NUL at [dest.len()].
let dest_z = ZStr::from_buf(&dest_name_buf, dest.len());
if let Err(err) = sys::symlinkat(target_z, dest_dir.fd(), dest_z) {
return InstallResult::fail(err.into(), Step::LinkingDependency, None);
if let Err(first_err) = sys::symlinkat(target_z, dest_dir.fd(), dest_z) {
// A stale entry can survive `skip_delete` (a member's
// internal node_modules outlives a root node_modules wipe);
// replace it and retry, like the Windows branch above.
Comment thread
robobun marked this conversation as resolved.
let retry = first_err.get_errno() == sys::E::EEXIST;
let mut result = Err(first_err);
if retry {
self.uninstall_before_install(destination_dir);
result = sys::symlinkat(target_z, dest_dir.fd(), dest_z);
}
if let Err(err) = result {
return InstallResult::fail(err.into(), Step::LinkingDependency, None);
}
}
}

Expand Down
10 changes: 10 additions & 0 deletions src/install/isolated_install/Installer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -534,6 +534,16 @@ impl<'a> Installer<'a> {
break 'state (node_id, CompleteState::Skipped);
}

// A member displaced by a root dependency is reachable only
// through non-workspace edges, and workspace store tasks re-run
// every install, so a completed one is not an install.
Comment thread
robobun marked this conversation as resolved.
let pkg_id = nodes.items_pkg_id()[node_id.get() as usize];
if self.lockfile().packages.slice().items_resolution()[pkg_id as usize].tag
== ResolutionTag::Workspace
{
break 'state (node_id, CompleteState::Skipped);
}

break 'state (node_id, state);
};

Expand Down
56 changes: 33 additions & 23 deletions src/install/lockfile/Package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1750,31 +1750,41 @@
.append::<String>(if relative.is_empty() { b"." } else { relative });
}
dependency::version::Tag::Npm => {
if let Some(workspace_version) = workspace_version {
let satisfies =
dependency_version
.npm()
.version
.satisfies(workspace_version, buf, buf);
if workspace_path.is_some() {
let satisfies = match workspace_version {
Some(workspace_version) => {
dependency_version
.npm()
.version
.satisfies(workspace_version, buf, buf)
}
// A versionless member is only linkable by a wildcard
// range, matching `get_or_put_resolved_package`.
Comment thread
robobun marked this conversation as resolved.
None => dependency_version.npm().version.is_star(),
};
if pm.options.link_workspace_packages && satisfies {
// `String::sliced` takes `&'a self`; bind the unwrapped
// value so the borrow outlives the parse call.
let wp = workspace_path.unwrap();
let path = wp.sliced(buf);
if let Some(mut dep) = dependency::parse_with_tag(
external_alias.value,
Some(external_alias.hash),
path.slice,
dependency::version::Tag::Workspace,
&path,
Some(&mut *log),
Some(&mut *pm),
) {
// Whole-struct move so `Drop` frees the old npm
// chain; keep the existing `literal`.
dep.literal = dependency_version.literal;
dependency_version = dep;
if workspace_version.is_some() {
// `String::sliced` takes `&'a self`; bind the unwrapped
// value so the borrow outlives the parse call.
Comment thread
robobun marked this conversation as resolved.
Outdated
let wp = workspace_path.unwrap();

Check failure on line 1769 in src/install/lockfile/Package.rs

View workflow job for this annotation

GitHub Actions / cargo clippy

called `unwrap` on `workspace_path` after checking its variant with `is_some`
let path = wp.sliced(buf);
if let Some(mut dep) = dependency::parse_with_tag(
external_alias.value,
Some(external_alias.hash),
path.slice,
dependency::version::Tag::Workspace,
&path,
Some(&mut *log),
Some(&mut *pm),
) {
// Whole-struct move so `Drop` frees the old npm
// chain; keep the existing `literal`.
Comment thread
robobun marked this conversation as resolved.
dep.literal = dependency_version.literal;
dependency_version = dep;
}
}
// For a versionless member the dependency stays as-is;
// resolution links it to the workspace package.
Comment thread
robobun marked this conversation as resolved.
} else {
// It doesn't satisfy, but a workspace shares the same name. Override the workspace with the other dependency
for dep in &mut package_dependencies[0..dependencies_count as usize] {
Expand Down
163 changes: 147 additions & 16 deletions src/install/lockfile/bun.lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1604,6 +1604,10 @@ pub(crate) fn parse_into_binary_lockfile(
) -> Result<(), ParseError> {
lockfile.init_empty();

let link_workspace_packages = manager
.as_deref()
.is_none_or(|m| m.options.link_workspace_packages);

let Some(lockfile_version_expr) = root.get(b"lockfileVersion") else {
log.add_error(Some(source), root.loc, b"Missing lockfile version");
return Err(ParseError::InvalidLockfileVersion);
Expand Down Expand Up @@ -2091,6 +2095,7 @@ pub(crate) fn parse_into_binary_lockfile(
None
};

let root_pkgs_expr = root.get(b"packages");
let (off, len) = parse_append_dependencies::<false, true>(
lockfile,
&root_pkg_exr,
Expand All @@ -2100,6 +2105,8 @@ pub(crate) fn parse_into_binary_lockfile(
None,
None,
Some(&workspaces_obj),
root_pkgs_expr.as_ref(),
link_workspace_packages,
)?;

let mut root_pkg = Package::default();
Expand All @@ -2124,8 +2131,13 @@ pub(crate) fn parse_into_binary_lockfile(
let workspace_pkgs_off: u32 = 1;
let mut workspace_pkgs_len: u32 = 0;

// Workspace-name duplicate detection, decoupled from the conditional
// `pkg_map` claim below so it fires even for displaced members.
Comment thread
robobun marked this conversation as resolved.
let mut seen_workspace_names: PkgPathSet = PkgPathSet::init();

if lockfile_version != Version::V0 {
// these are the `workspaceOnly` packages
let pkgs_expr_for_claims = root.get(b"packages");
// snapshot the workspace-path handles up front so the loop
// body can take `&mut *lockfile` (`parse_append_dependencies`,
// `append_package_dedupe`) without conflicting with the
Expand Down Expand Up @@ -2156,6 +2168,16 @@ pub(crate) fn parse_into_binary_lockfile(
.expect("infallible: is_string checked");
let name_hash = StringBuilder::string_hash(name);

if seen_workspace_names.contains(name) {
log.add_error_fmt(
source,
row.key_loc,
format_args!("Duplicate workspace name: '{}'", bstr::BStr::new(name)),
);
return Err(ParseError::InvalidWorkspaceObject);
}
seen_workspace_names.put(name, ());

pkg.name = sbuf!(lockfile).append_with_hash(name, name_hash)?;
pkg.name_hash = name_hash;

Expand All @@ -2168,6 +2190,8 @@ pub(crate) fn parse_into_binary_lockfile(
None,
None,
None,
None,
link_workspace_packages,
)?;

pkg.dependencies = DependencySlice::new(off, len);
Expand All @@ -2185,21 +2209,24 @@ pub(crate) fn parse_into_binary_lockfile(
}

// there should be no duplicates
let pkgs_len_before = lockfile.packages.len();
let pkg_id = lockfile.append_package_dedupe(&mut pkg)?;

let entry = pkg_map.get_or_put(name)?;
if entry.found_existing {
log.add_error_fmt(
source,
row.key_loc,
format_args!("Duplicate workspace name: '{}'", bstr::BStr::new(name)),
);
return Err(ParseError::InvalidWorkspaceObject);
// A member displaced by a root dependency only appears
// nested; pre-claiming its name would falsely collide with
// the root `packages` key the other package owns.
Comment thread
robobun marked this conversation as resolved.
if member_owns_packages_key(pkgs_expr_for_claims.as_ref(), name, path) {
let entry = pkg_map.get_or_put(name)?;
debug_assert!(!entry.found_existing, "caught by seen_workspace_names");
*entry.value_ptr = pkg_id;
}

*entry.value_ptr = pkg_id;

workspace_pkgs_len += 1;
// `workspace_pkgs_len` sizes the 1..1+len package-id range
// resolved below; it must count appended packages, not loop
// iterations.
Comment thread
robobun marked this conversation as resolved.
if lockfile.packages.len() > pkgs_len_before {
workspace_pkgs_len += 1;
}
continue 'workspaces;
}
}
Expand Down Expand Up @@ -2263,6 +2290,11 @@ pub(crate) fn parse_into_binary_lockfile(
bundled_pkgs.put(pkg_path, ());
}

// Workspace packages claimed under a `packages` key other than their
// name (nested under a dependent, or aliased). Their own dependency
// keys are written under that placement, e.g. "beta/member/dep".
Comment thread
robobun marked this conversation as resolved.
let mut member_tree_keys: Vec<(PackageID, &[u8])> = Vec::new();

'next_pkg_key: for row in object_rows(&pkgs_expr) {
let key_loc = row.key_loc;
let pkg_path = row.key.slice();
Expand Down Expand Up @@ -2454,6 +2486,7 @@ pub(crate) fn parse_into_binary_lockfile(
// "another-pkg1": "workspaces:packages/pkg1",
// },
*entry.value_ptr = workspace_pkg_id;
member_tree_keys.push((workspace_pkg_id, pkg_path));
continue 'next_pkg_key;
}
}
Expand Down Expand Up @@ -2521,6 +2554,8 @@ pub(crate) fn parse_into_binary_lockfile(
Some(pkg_path),
Some(&bundled_pkgs),
None,
None,
link_workspace_packages,
)?;

pkg.dependencies = DependencySlice::new(off, len);
Expand Down Expand Up @@ -2847,12 +2882,45 @@ pub(crate) fn parse_into_binary_lockfile(

seen_deps.clear_retaining_capacity();

// `"<name>/<dep>"` keys belong to whichever package owns the
// root `<name>` entry; for a displaced member that is the
// npm package that replaced it.
Comment thread
robobun marked this conversation as resolved.
let owns_name_node = pkg_map.get(workspace_name).copied() == Some(pkg_id);

let deps = pkg_deps[pkg_id as usize];
for _dep_id in deps.begin()..deps.end() {
let dep_id: DependencyID = _dep_id;
let dep = &mut dependencies[dep_id as usize];
let dep_name = dep.name.slice(string_buf);

// A displaced member's node lives under its recorded
// `packages` key (e.g. "beta/member"). Walk that
// placement up like hoisting would: "beta/member/dep",
// then "beta/dep"; the bare "dep" probe below covers the
// root level.
Comment thread
robobun marked this conversation as resolved.
let nested_res_id = member_tree_keys.iter().find_map(|&(id, key)| {
if id != pkg_id {
return None;
}
let mut prefix = key;
loop {
let needed = prefix.len() + 1 + dep_name.len();
let buf_slice = &mut path_buf[..];
if needed <= buf_slice.len() {
buf_slice[..prefix.len()].copy_from_slice(prefix);
buf_slice[prefix.len()] = b'/';
buf_slice[prefix.len() + 1..needed].copy_from_slice(dep_name);
if let Some(&found) = pkg_map.get(&buf_slice[..needed]) {
return Some(found);
}
}
match strings::last_index_of_char(prefix, b'/') {
Some(i) => prefix = &prefix[..i as usize],
Comment thread
claude[bot] marked this conversation as resolved.
Outdated
None => return None,
}
}
});
Comment thread
claude[bot] marked this conversation as resolved.
Comment thread
coderabbitai[bot] marked this conversation as resolved.

let workspace_node_modules = {
let buf_slice = &mut path_buf[..];
let needed = workspace_name.len() + 1 + dep_name.len();
Expand Down Expand Up @@ -2885,11 +2953,13 @@ pub(crate) fn parse_into_binary_lockfile(
} else {
None
};
let Some(res_id) = peer_res_id.or_else(|| {
pkg_map
.get(workspace_node_modules)
.or_else(|| pkg_map.get(dep_name))
.copied()
let Some(res_id) = peer_res_id.or(nested_res_id).or_else(|| {
Comment thread
robobun marked this conversation as resolved.
let by_name = if owns_name_node {
pkg_map.get(workspace_node_modules)
} else {
None
};
by_name.or_else(|| pkg_map.get(dep_name)).copied()
}) else {
if dep.behavior.contains(Behavior::OPTIONAL) {
continue;
Expand Down Expand Up @@ -3153,6 +3223,33 @@ fn map_dep_to_pkg(
}
}

/// A workspace member normally owns the root `packages` key matching its
/// name. When a root dependency replaced the member's workspace dependency
/// (`Package::parse_dependency`), that key holds the other package and the
/// member only appears nested, so its name must not pre-claim the key.
Comment thread
robobun marked this conversation as resolved.
fn member_owns_packages_key(pkgs_expr: Option<&Expr>, name: &[u8], path: &[u8]) -> bool {
let Some(pkgs) = pkgs_expr else {
return true;
};
let Some(value) = pkgs.get(name) else {
return true;
};
if !value.is_array() {
return true;
}
let Some(first) = array_items(&value).first().and_then(|item| item.as_str()) else {
return true;
};
// a member's own entry is written as "<name>@workspace:<path>"
let Some(rest) = first.get(name.len() + 1..) else {
return false;
};
strings::has_prefix(first, name)
&& first[name.len()] == b'@'
&& strings::has_prefix(rest, b"workspace:")
&& strings::eql(&rest[b"workspace:".len()..], path)
}

fn dependency_resolution_failure(
dep: &Dependency,
pkg_path: Option<&[u8]>,
Expand Down Expand Up @@ -3213,6 +3310,9 @@ fn parse_append_dependencies<const CHECK_FOR_BUNDLED: bool, const IS_ROOT: bool>
pkg_path: Option<&[u8]>,
bundled_pkgs: Option<&PkgPathSet>,
workspaces_obj: Option<&Expr>,
// Only meaningful when `IS_ROOT`.
pkgs_expr: Option<&Expr>,
link_workspace_packages: bool,
) -> Result<(u32, u32), ParseError> {
// Clearing on entry is equivalent to clearing on every exit path for all
// callers (none read the buf between calls) and also covers early-error exits.
Expand Down Expand Up @@ -3362,6 +3462,37 @@ fn parse_append_dependencies<const CHECK_FOR_BUNDLED: bool, const IS_ROOT: bool>
.expect("infallible: is_string checked");
let name_hash = StringBuilder::string_hash(name);

// The key-ownership check honors the shape the lockfile was
// written in (a linkWorkspacePackages flip must re-resolve,
// not bind a workspace edge to the npm package); the range
// scan mirrors `Package::parse_dependency` for aliased
// dependencies, whose root key is the alias.
Comment thread
robobun marked this conversation as resolved.
let overridden = {
let bytes = lockfile.buffers.string_bytes.as_slice();
!member_owns_packages_key(pkgs_expr, name, path) || {
let member_version = lockfile.workspace_versions.get(&name_hash).copied();
lockfile.buffers.dependencies.as_slice()[off..]
.iter()
.any(|dep| {
if dep.version.tag != DependencyVersionTag::Npm {
return false;
}
let npm = dep.version.npm();
if StringBuilder::string_hash(npm.name.slice(bytes)) != name_hash {
return false;
}
let satisfies = match member_version {
Some(version) => npm.version.satisfies(version, bytes, bytes),
None => npm.version.is_star(),
};
!(link_workspace_packages && satisfies)
})
}
};
if overridden {
continue 'workspaces;
}

let dep = Dependency {
name: sbuf!(lockfile).append_with_hash(name, name_hash)?,
name_hash,
Expand Down
Loading
Loading