Skip to content
Open
Show file tree
Hide file tree
Changes from 8 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
139 changes: 127 additions & 12 deletions src/install/lockfile/bun.lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1604,6 +1604,10 @@
) -> 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 @@
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 @@
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 @@
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 @@
.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 @@
None,
None,
None,
None,
link_workspace_packages,
)?;

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

// 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 @@
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 @@
// "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 @@
Some(pkg_path),
Some(&bundled_pkgs),
None,
None,
link_workspace_packages,
)?;

pkg.dependencies = DependencySlice::new(off, len);
Expand Down Expand Up @@ -2853,6 +2888,25 @@
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"), so its own
// dependency keys are "beta/member/dep", not
// "member/dep".
Comment thread
robobun marked this conversation as resolved.
Outdated
let nested_res_id = member_tree_keys.iter().find_map(|&(id, key)| {
if id != pkg_id {
return None;
}
let needed = key.len() + 1 + dep_name.len();
let buf_slice = &mut path_buf[..];
if needed > buf_slice.len() {
return None;
}
buf_slice[..key.len()].copy_from_slice(key);
buf_slice[key.len()] = b'/';
buf_slice[key.len() + 1..needed].copy_from_slice(dep_name);
pkg_map.get(&buf_slice[..needed]).copied()
});

Check failure on line 2908 in src/install/lockfile/bun.lock.rs

View check run for this annotation

Claude / Claude Code Review

nested_res_id single-level probe misses intermediate-hoisted deps for displaced members

`nested_res_id` only probes the exact `"<tree_key>/<dep>"` and does not walk up the tree — so a displaced member's dep that hoisted to an *intermediate* ancestor (e.g. root has `a-dep@1.0.2`, `beta` has `a-dep@1.0.1`, member under `beta` also wants `1.0.1` → written only at `"beta/a-dep"`) misses here, misses `workspace_node_modules`, and binds root's wrong version. This is a distinct trigger from the comment on line 2942, and that comment's suggested fix ("skip `workspace_node_modules` → go `ne
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,7 +2939,7 @@
} else {
None
};
let Some(res_id) = peer_res_id.or_else(|| {
let Some(res_id) = peer_res_id.or(nested_res_id).or_else(|| {
Comment thread
robobun marked this conversation as resolved.
pkg_map
.get(workspace_node_modules)
.or_else(|| pkg_map.get(dep_name))
Expand Down Expand Up @@ -3153,6 +3207,33 @@
}
}

/// 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 +3294,9 @@
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 +3446,37 @@
.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