Skip to content
Open
Show file tree
Hide file tree
Changes from 4 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
16 changes: 14 additions & 2 deletions src/install/PackageInstall.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2276,8 +2276,20 @@ 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 workspace
// member's internal node_modules is not reset by wiping the
// root node_modules. Replace it and retry, like the Windows
// branch above.
Comment thread
robobun marked this conversation as resolved.
Outdated
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 @@ impl Package<u64> {
.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();
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
145 changes: 133 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()
});
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 @@ -2882,10 +2936,10 @@
pkg_resolutions,
string_buf,
)
} else {
None
};
let Some(res_id) = peer_res_id.or_else(|| {
let Some(res_id) = peer_res_id.or(nested_res_id).or_else(|| {

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

View check run for this annotation

Claude / Claude Code Review

Displaced member's hoisted dep can bind to the npm package's nested dep on reload

For a displaced member, `workspace_node_modules` = `"<member_name>/<dep>"` probes the *npm package's* nested-dep key space (the root key `<member_name>` now belongs to the npm package). When the member's dep is dedup-hoisted to root (e.g. root also depends on the same version), `nested_res_id` misses, then this probe hits the npm package's nested `<member_name>/<dep>` and binds the wrong version — the correct root `<dep>` is never reached. Fix: when `pkg_id` appears in `member_tree_keys`, skip t
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,43 @@
.expect("infallible: is_string checked");
let name_hash = StringBuilder::string_hash(name);

// A member whose root `packages` key is owned by another
// package had its workspace dependency replaced when the
// lockfile was written; honor that recorded shape rather
// than re-deriving it from the current config, so a
// `linkWorkspacePackages` flip re-resolves through the
// normal changes diff instead of binding 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.
Outdated
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