Skip to content

install: bind peer edges in the hoister, the way loading bun.lock binds them #38767

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
robobun wants to merge 1 commit into
base: main
Choose a base branch
from
Open

install: bind peer edges in the hoister, the way loading bun.lock binds them #38767

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
10 changes: 10 additions & 0 deletions src/install/lockfile.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1734,6 +1734,16 @@ impl Lockfile {
debug_assert!(
SemverStringBuilder::string_hash(self.str(&package.name)) == package.name_hash
);
// The hoister binds peers by scanning `package_index` under the package name.
debug_assert!(
match self.package_index.get(&package.name_hash) {
Some(PackageIndexEntry::Id(id)) => *id as usize == i,
Some(PackageIndexEntry::Ids(ids)) => ids.iter().any(|&id| id as usize == i),
None => false,
},
"package {} is not in package_index under its own name",
i
);
debug_assert!(
package
.dependencies
Expand Down
24 changes: 23 additions & 1 deletion src/install/lockfile/Tree.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ use bun_core::ZStr;
use bun_paths::{MAX_PATH_BYTES, PathBuffer, SEP};

use crate::lockfile::package::PackageColumns as _;
use crate::lockfile::{DepSorter, DependencyIDList, DependencyIDSlice, Lockfile};
use crate::lockfile::{DepSorter, DependencyIDList, DependencyIDSlice, Lockfile, bun_lock};
use crate::package_manager::{PackageManager, WorkspaceFilter};
use crate::{
Dependency, DependencyID, PackageID, PackageNameHash, Resolution, invalid_dependency_id,
Expand Down Expand Up @@ -493,6 +493,27 @@ impl<'a, const METHOD: BuilderMethod> Builder<'a, METHOD> {
self.lockfile().buffers.string_bytes.as_slice()
}

/// Binds a peer edge the way loading `bun.lock` does, whatever the resolver, a previous
/// lockfile or a migration left it bound to, so every tree built from these packages agrees.
Comment thread
robobun marked this conversation as resolved.
fn bind_peer(&mut self, dep_id: DependencyID) {
let dependency: &Dependency = &self.dependencies[dep_id as usize];
if !dependency.behavior.is_peer() {
return;
}
let lockfile_ref = self.lockfile;
let lockfile: &Lockfile = lockfile_ref.get();
if let Some(pkg_id) = bun_lock::resolve_peer_dep_version_based(
dependency,
&lockfile.catalogs,
&lockfile.package_index,
&lockfile.overrides,
lockfile.packages.items_resolution(),
lockfile.buffers.string_bytes.as_slice(),
) {
self.resolutions[dep_id as usize] = pkg_id;
}
}

/// Flatten the multi-dimensional ArrayList of package IDs into a single easily serializable array
pub(crate) fn clean(&mut self) -> Result<CleanResult, AllocError> {
let mut total: u32 = 0;
Expand Down Expand Up @@ -698,6 +719,7 @@ impl Tree {
let sort_buf_len = builder.sort_buf.len();
'dep: for sort_idx in 0..sort_buf_len {
let dep_id = builder.sort_buf[sort_idx];
builder.bind_peer(dep_id);
let pkg_id = builder.resolutions[dep_id as usize];

// filter out disabled dependencies
Expand Down
3 changes: 3 additions & 0 deletions src/install/lockfile/bun.lock.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3353,6 +3353,9 @@ fn deferred_peer_range<'a>(
/// re-keys isolated-linker store entries (and global-store entry hashes)
/// on warm installs.
///
/// The hoister applies the same binding to every peer edge it processes
/// (`tree::Builder::bind_peer`), so a saved tree is the tree its reload rebuilds.
///
Comment thread
robobun marked this conversation as resolved.
Comment thread
robobun marked this conversation as resolved.
/// Peers whose name matches a workspace package need no special casing
/// even though the fresh resolver binds them to the workspace before any
/// deferral (`'resolve_from_workspace`): the version scan below picks an
Expand Down
223 changes: 5 additions & 218 deletions src/install/yarn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use std::borrow::Cow;
use std::io::Write as _;

use crate::Error;
use bun_collections::{HashMap, StringHashMap, index_sort};
use bun_collections::StringHashMap;
use bun_install::bin::Bin;
use bun_install::dependency::{self, Dependency, DependencyExt as _};
use bun_install::install::{self, DependencyID, PackageID, PackageManager};
Expand All @@ -26,7 +26,7 @@ use bun_install::npm;
// `bun_install::resolution` stub keeps `Value` as a struct-of-fields and has no `init`.
use crate::bun_json;
use crate::repository::Repository;
use crate::resolution_real::{Resolution, Tag as ResolutionTag, TaggedValue as ResolutionValue};
use crate::resolution_real::{Resolution, TaggedValue as ResolutionValue};
use crate::versioned_url::VersionedURL;
use bun_core::strings;
use bun_paths::PathBuffer;
Expand Down Expand Up @@ -1380,222 +1380,9 @@ pub(crate) fn migrate_yarn_lockfile<'a>(
}
}

for (base_name, versions) in scoped_packages.iter_mut() {
let base_name: &[u8] = base_name.as_ref();

index_sort::sort_slice_by(versions, |a, b| a.package_id.cmp(&b.package_id));

let original_name_hash = string_hash(base_name);
// `remove` drops the value (and thus the `Ids` Vec) automatically.
let _ = this.package_index.remove(&original_name_hash);
}

for (base_name, versions) in scoped_packages.iter() {
let base_name: &[u8] = base_name.as_ref();

for version_info in versions.iter() {
let package_id = version_info.package_id;

let mut found_in_index = false;
for (_, index_value) in this.package_index.iter() {
match index_value {
lockfile::PackageIndexEntry::Id(id) => {
if *id == package_id {
found_in_index = true;
break;
}
}
lockfile::PackageIndexEntry::Ids(ids) => {
for id in ids.iter() {
if *id == package_id {
found_in_index = true;
break;
}
}
if found_in_index {
break;
}
}
}
}

if !found_in_index {
let mut fallback_name = Vec::new();
write!(
&mut fallback_name,
"{}#{}",
bstr::BStr::new(base_name),
package_id
)
.expect("unreachable");

let fallback_hash = string_hash(&fallback_name);
this.get_or_put_id(package_id, fallback_hash)?;
}
}
}

let mut package_names: Vec<&[u8]> = vec![b"".as_slice(); next_package_id as usize];

for (yarn_idx, entry) in yarn_lock.entries.iter().enumerate() {
let package_id = yarn_entry_to_package_id[yarn_idx];
if package_names[package_id as usize].is_empty() {
package_names[package_id as usize] = Entry::get_name_from_spec(entry.specs[0]);
}
}

let mut root_packages: StringHashMap<PackageID> = StringHashMap::new();

let mut usage_count: StringHashMap<u32> = StringHashMap::new();
for entry_idx in 0..yarn_lock.entries.len() {
let package_id = yarn_entry_to_package_id[entry_idx];
if package_id == install::INVALID_PACKAGE_ID {
continue;
}
let base_name = package_names[package_id as usize];

for dep_entry in yarn_lock.entries.iter() {
if let Some(deps) = &dep_entry.dependencies {
for (dep_name_key, _) in deps.iter() {
if dep_name_key.as_ref() == base_name {
let count = usage_count.get(base_name).copied().unwrap_or(0);
usage_count.put(base_name, count + 1)?;
}
}
}
}
}

for entry_idx in 0..yarn_lock.entries.len() {
let package_id = yarn_entry_to_package_id[entry_idx];
if package_id == install::INVALID_PACKAGE_ID {
continue;
}
let base_name = package_names[package_id as usize];

if root_packages.get(base_name).is_none() {
root_packages.put(base_name, package_id)?;
let name_hash = string_hash(base_name);
this.get_or_put_id(package_id, name_hash)?;
}
}

let mut scoped_names: HashMap<PackageID, Vec<u8>> = HashMap::new();
let mut scoped_count: u32 = 0;
for entry_idx in 0..yarn_lock.entries.len() {
let package_id = yarn_entry_to_package_id[entry_idx];
if package_id == install::INVALID_PACKAGE_ID {
continue;
}
let base_name = package_names[package_id as usize];

if let Some(root_pkg_id) = root_packages.get(base_name).copied() {
if root_pkg_id == package_id {
continue;
}
} else {
continue;
}

let mut scoped_name: Option<Vec<u8>> = None;
for (dep_entry_idx, dep_entry) in yarn_lock.entries.iter().enumerate() {
let dep_package_id = yarn_entry_to_package_id[dep_entry_idx];
if dep_package_id == install::INVALID_PACKAGE_ID {
continue;
}

if let Some(deps) = &dep_entry.dependencies {
for (dep_name_key, _) in deps.iter() {
if dep_name_key.as_ref() == base_name {
if dep_package_id != package_id {
let parent_name = package_names[dep_package_id as usize];

let mut potential_name = Vec::new();
write!(
&mut potential_name,
"{}/{}",
bstr::BStr::new(parent_name),
bstr::BStr::new(base_name)
)
.expect("unreachable");

let mut name_already_used = false;
for existing_name in scoped_names.values() {
if existing_name.as_slice() == potential_name.as_slice() {
name_already_used = true;
break;
}
}

if !name_already_used {
scoped_name = Some(potential_name);
break;
}
// else: potential_name dropped
}
}
}
if scoped_name.is_some() {
break;
}
}
}

if scoped_name.is_none() {
let pkg_resolution = this.packages.get(package_id as usize).resolution;
let version_str: Vec<u8> = match pkg_resolution.tag {
ResolutionTag::Npm => 'brk: {
let mut version_buf = [0u8; 64];
let mut cursor = &mut version_buf[..];
let npm_version = pkg_resolution.npm().version;
let _ = write!(
&mut cursor,
"{}",
npm_version.fmt(this.buffers.string_bytes.as_slice())
);
let written = 64 - cursor.len();
break 'brk version_buf[..written].to_vec();
}
_ => b"unknown".to_vec(),
};
let mut name = Vec::new();
write!(
&mut name,
"{}@{}",
bstr::BStr::new(base_name),
bstr::BStr::new(&version_str)
)
.expect("unreachable");
scoped_name = Some(name);
}

if let Some(final_scoped_name) = scoped_name {
let name_hash = string_hash(&final_scoped_name);
this.get_or_put_id(package_id, name_hash)?;
scoped_names.put(package_id, final_scoped_name)?;
scoped_count += 1;
}
}
let _ = scoped_count;

for (yarn_idx, entry) in yarn_lock.entries.iter().enumerate() {
let package_id = yarn_entry_to_package_id[yarn_idx];
if package_id == install::INVALID_PACKAGE_ID {
continue;
}

if let Some(resolved) = entry.resolved.as_deref() {
if let Some(real_name) = Entry::get_package_name_from_resolved_url(resolved) {
for spec in entry.specs.iter() {
let alias_name = Entry::get_name_from_spec(spec);

if alias_name != real_name {
let alias_hash = string_hash(alias_name);
this.get_or_put_id(package_id, alias_hash)?;
}
}
}
}
for id in 0..this.packages.len() {
let name_hash = this.packages.items_name_hash()[id];
this.get_or_put_id(id as PackageID, name_hash)?;
}

this.buffers.trees[0].dependencies = lockfile::DependencyIDSlice::new(0, 0);
Expand Down
68 changes: 67 additions & 1 deletion test/cli/install/hoist.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,8 @@
import { afterAll, beforeAll, test } from "bun:test";
import { file, write } from "bun";
import { afterAll, beforeAll, expect, test } from "bun:test";
import { exists, rm } from "fs/promises";
import { VerdaccioRegistry, bunEnv, runBunInstall } from "harness";
import { join } from "path";

const registry = new VerdaccioRegistry();

Expand Down Expand Up @@ -28,3 +31,66 @@ test("should handle resolving optional peer from multiple instances of same pack
// this shouldn't hit an assertion
await runBunInstall(bunEnv, packageDir);
});

test("tree written after a ranged peer gains a higher candidate is the tree the next install lays out", async () => {
// `peer-deps-fixed` has a peer on `no-deps@^1.0.0`. As a devDependency it is
// hoisted before the root's `dependencies`, so whatever its peer edge is bound
// to is the `no-deps` that lands at the root of node_modules. Loading bun.lock
// binds such an edge to the highest satisfying version in the lockfile, so
// the install that adds `one-dep` (no-deps@1.0.1, next to one-fixed-dep's
// 1.0.0) has to bind it the same way before hoisting. Otherwise it writes a
// lockfile keyed with 1.0.0 at the root and the very next `bun install`
// relinks node_modules with 1.0.1 at the root, without touching bun.lock.
const { packageDir, packageJson } = await registry.createTestDir({ bunfigOpts: { linker: "hoisted" } });
const noDepsVersion = async (...segments: string[]) => {
const pkg = join(packageDir, "node_modules", ...segments, "no-deps", "package.json");
return (await exists(pkg)) ? ((await file(pkg).json()) as { version: string }).version : null;
};
const layout = async () => ({
root: await noDepsVersion(),
"one-dep": await noDepsVersion("one-dep", "node_modules"),
"one-fixed-dep": await noDepsVersion("one-fixed-dep", "node_modules"),
});

await write(
packageJson,
JSON.stringify({
name: "ranged-peer-roundtrip",
dependencies: { "one-fixed-dep": "1.0.0" },
devDependencies: { "peer-deps-fixed": "1.0.0" },
}),
);
await runBunInstall(bunEnv, packageDir);
expect(await layout()).toEqual({ root: "1.0.0", "one-dep": null, "one-fixed-dep": null });

await write(
packageJson,
JSON.stringify({
name: "ranged-peer-roundtrip",
dependencies: { "one-dep": "1.0.0", "one-fixed-dep": "1.0.0" },
devDependencies: { "peer-deps-fixed": "1.0.0" },
}),
);
await runBunInstall(bunEnv, packageDir);
const written = await layout();
const lockfile = await file(join(packageDir, "bun.lock")).text();

// the tree on disk is the tree the lockfile describes, so reinstalling from it is a no-op
const { out, err } = await runBunInstall(bunEnv, packageDir, { savesLockfile: false });
expect(out).toContain("(no changes)");
expect(err).not.toContain("Saved lockfile");
expect(await layout()).toEqual(written);
expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile);

// the peer is bound to the highest satisfying version, and peer-deps-fixed hoists it first
expect(written).toEqual({ root: "1.0.1", "one-dep": null, "one-fixed-dep": "1.0.0" });
expect(lockfile).toContain('"no-deps": ["no-deps@1.0.1"');
expect(lockfile).toContain('"one-fixed-dep/no-deps": ["no-deps@1.0.0"');

// a fresh resolve of the same package.json binds the peer the same way
await rm(join(packageDir, "node_modules"), { recursive: true, force: true });
await rm(join(packageDir, "bun.lock"));
await runBunInstall(bunEnv, packageDir);
expect(await layout()).toEqual(written);
expect(await file(join(packageDir, "bun.lock")).text()).toBe(lockfile);
});
Loading
Loading