Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
2 changes: 1 addition & 1 deletion docs/pm/workspaces.mdx
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ For that workspace `bun install` then behaves as a hoisting barrier — nothing
directly or transitively (including through other workspaces it depends on), is placed above
`apps/desktop/node_modules`, so that directory is a complete tree — and materializes those
packages as real copies instead of hardlinks / clones from the cache, so tools that rewrite them
cannot affect the cache or other projects. All other workspaces keep hoisting to the root as usual. The setting is recorded in `bun.lock` for the workspace, so installs from the lockfile reproduce the same layout.
cannot affect the cache or other projects. All other workspaces keep hoisting to the root as usual. The lockfile does not record the setting. Bun reads it from `package.json` on every install, `--frozen-lockfile` included, so the lockfile is the same with and without it.
This setting has no effect with the isolated linker, where every package already resolves only its
own dependencies.

Expand Down
5 changes: 1 addition & 4 deletions src/install/PackageManager/install_with_manager.rs
Original file line number Diff line number Diff line change
Expand Up @@ -235,10 +235,7 @@ pub fn install_with_manager(
};

had_any_diffs = manager.summary.has_diffs();
// Which workspaces asked for a self-contained node_modules is a property
// of their manifests, not of the dependency graph: mirror the freshly
// parsed manifests whether or not anything else changed, so the copy
// loaded from bun.lock never goes stale.
// The lockfile does not store the set. Every install takes it from the manifests.
manager
.lockfile
.self_contained_workspaces
Expand Down
47 changes: 9 additions & 38 deletions src/install/lockfile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -180,11 +180,7 @@ pub struct Lockfile {
pub(crate) scripts: Scripts,
pub(crate) workspace_paths: NameHashMap,
pub workspace_versions: VersionHashMap,
/// Workspaces (by name hash) that must get a self-contained node_modules: those
/// listed in the root manifest's `workspaces.selfContained` and those declaring
/// `installConfig.hoistingLimits = "workspaces"`. Mirrored from the manifests on
/// every install and persisted per workspace in bun.lock (`"hoistingLimits"`), so a
/// tree rebuilt from the lockfile is hoisted the same way.
/// Name hashes of the self-contained workspaces, from the manifests. Not saved.
pub self_contained_workspaces: ArrayHashMap<PackageNameHash, (), ArrayIdentityContextU64>,

/// Optional because `trustedDependencies` in package.json might be an
Expand Down Expand Up @@ -823,7 +819,7 @@ impl Lockfile {
/// root manifest's `workspaces.selfContained` (by path or name) or declaring
/// `"installConfig": { "hoistingLimits": "workspaces" }` in their own manifest.
/// Both are recorded in `self_contained_workspaces` while the workspaces are
/// parsed (and persisted per workspace in bun.lock).
/// parsed.
pub(crate) fn self_contained_workspace_ids(&self) -> Vec<PackageID> {
if self.self_contained_workspaces.count() == 0 {
return Vec::new();
Expand Down Expand Up @@ -1365,13 +1361,19 @@ impl Lockfile {
) -> Result<bool, tree::SubtreeError> {
let slice = self.packages.slice();

// Only the install applies the barrier, so the saved tree does not depend on it.
let self_contained = if METHOD == tree::BuilderMethod::Filter {
self.self_contained_workspace_ids()
} else {
Vec::new()
};
Comment on lines +1364 to +1369

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔴 Dropping the barrier from the Resolvable tree makes remove_collapsed_copies delete parts of a self-contained workspace's node_modules: after bun update/bun dedupe/bun audit fix with the hoisted linker, any package in <ws>/node_modules that is also installed at the root is treated as a collapsed duplicate and removed, so the workspace is no longer self-contained (base kept it because both trees carried the barrier). Fix: make the post-install collapsed-copy pass reason about the barrier-applied layout — e.g. skip workspaces in self_contained_workspaces (and their subtrees) in prune::remove_collapsed_copies, or compare against a Filter-built tree there.

Extended reasoning...

Repo: root and workspace web depend on bar@ 0.0.2; workspace desktop (self-contained via installConfig.hoistingLimits) transitively depends on bar@ 0.0.2 (same test fixture in bun-workspaces-self-contained.test.ts). bun install lays out node_modules/bar and apps/desktop/node_modules/bar. Run bun update. install_hoisted_packages runs filter() (barrier applied) so both copies are (re)installed, then its scopeguard restores manager.lockfile.buffers.trees to the resolve() output from Cloner::flush, which after this change is built with self_contained = Vec::new(). remove_collapsed_copies (install_with_manager.rs:899, gated on Dedupe/Audit/Update) therefore builds new from a barrier-less tree: no tree exists at node_modules/desktop/node_modules (surviving = None) and old_rows is empty. Its workspace pass (prune.rs:1378-1398) scans apps/desktop/node_modules; for bar, was_row = false, so it evaluates !new.collapsed_into_ancestor(0, "bar"). bar is expected at root in the barrier-less new and verified_installed finds node_modules/bar…

Verification: normal — the diff at src/install/lockfile.rs:1364-1369 gates self_contained_workspace_ids() to BuilderMethod::Filter only, so resolve() (line 1329, Resolvable) now builds a barrier-less tree. The base version (git show e26b4e1:src/install/lockfile.rs, let self_contained = self.self_contained_workspace_ids(); was unconditional) applied the barrier to both. Trace on `bun update… | normal…


// `tree::Builder` stores `lockfile: ParentRef<Lockfile>` so
// the `&mut buffers.resolutions` split-borrow below can coexist with
// the read-only lockfile view inside the builder (see Tree.rs note).
// `ParentRef::new` captures `SharedReadOnly` provenance from `&*self`,
// which is exactly what `Builder` needs (it only ever `Deref`s); the
// `Builder` does not outlive this `&mut self` borrow.
let self_contained = self.self_contained_workspace_ids();
let lockfile_ref = bun_ptr::ParentRef::<Lockfile>::new(&*self);
let mut builder = tree::Builder::<METHOD> {
self_contained,
Expand Down Expand Up @@ -2974,30 +2976,6 @@ impl Lockfile {
string_builder.count(SCRIPTS_END);
}

// Self-contained workspaces change the hoisted layout without changing any
// resolution; include them (sorted, only when present) so bun.lockb's
// frozen-lockfile check notices. Text lockfiles compare the tree itself.
const SELF_CONTAINED_BEGIN: &[u8] = b"\n-- BEGIN SELF-CONTAINED WORKSPACES --\n";
let mut self_contained_names: Vec<&[u8]> = Vec::new();
if self.self_contained_workspaces.count() > 0 {
for i in 0..packages_len {
if resolutions[i].tag == crate::resolution::Tag::Workspace
&& self
.self_contained_workspaces
.contains(&self.packages.items_name_hash()[i])
{
self_contained_names.push(names[i].slice(bytes));
}
}
self_contained_names.sort_unstable();
if !self_contained_names.is_empty() {
string_builder.count(SELF_CONTAINED_BEGIN);
for n in &self_contained_names {
string_builder.fmt_count(format_args!("{}\n", bstr::BStr::new(n)));
}
}
}

{
let alphabetizer = package::Alphabetizer::<u64> {
names: names.into(),
Expand Down Expand Up @@ -3036,13 +3014,6 @@ impl Lockfile {
let _ = string_builder.append(SCRIPTS_END);
}

if !self_contained_names.is_empty() {
let _ = string_builder.append(SELF_CONTAINED_BEGIN);
for n in &self_contained_names {
let _ = string_builder.fmt(format_args!("{}\n", bstr::BStr::new(n)));
}
}

let _ = string_builder.append(HASH_SUFFIX);

let len = string_builder.len;
Expand Down
28 changes: 0 additions & 28 deletions src/install/lockfile/bun.lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,6 @@ impl Stringifier {
extern_strings,
deps_buf,
&lockfile.workspace_versions,
&lockfile.self_contained_workspaces,
&mut optional_peers_buf,
&pkg_map,
b"",
Expand Down Expand Up @@ -450,7 +449,6 @@ impl Stringifier {
extern_strings,
deps_buf,
&lockfile.workspace_versions,
&lockfile.self_contained_workspaces,
&mut optional_peers_buf,
&pkg_map,
pkg_names[workspace_pkg_id as usize].slice(buf),
Expand Down Expand Up @@ -1244,11 +1242,6 @@ impl Stringifier {
extern_strings: &[ExternalString],
deps_buf: &[Dependency],
workspace_versions: &VersionHashMap,
self_contained_workspaces: &bun_collections::ArrayHashMap<
PackageNameHash,
(),
bun_collections::ArrayIdentityContextU64,
>,
optional_peers_buf: &mut Vec<String>,
pkg_map: &PkgMap<()>,
relative_path: &[u8],
Expand Down Expand Up @@ -1301,12 +1294,6 @@ impl Stringifier {
write!(writer, "\"version\": \"{}\"", version.fmt(buf))?;
}

if self_contained_workspaces.contains(&pkg_name_hashes[pkg_id as usize]) {
writer.write_all(b",\n")?;
Self::write_indent(writer, *indent)?;
writer.write_all(b"\"hoistingLimits\": \"workspaces\"")?;
}

if pkg_bins[pkg_id as usize].tag != BinTag::None {
let bin = &pkg_bins[pkg_id as usize];
writer.write_all(b",\n")?;
Expand Down Expand Up @@ -2360,21 +2347,6 @@ pub(crate) fn parse_into_binary_lockfile(
.workspace_versions
.insert(name_hash, parsed.version.min());
}

// `installConfig.hoistingLimits` mirrored from the workspace manifest, so the
// tree is hoisted the same way when it is rebuilt from this lockfile
if let Some(h) = value.get(b"hoistingLimits") {
if h.as_utf8_string_literal() == Some(b"workspaces".as_slice()) {
lockfile.self_contained_workspaces.insert(name_hash, ());
} else {
log.add_error(
Some(source),
value_loc_of(source, h.loc),
b"Expected \"workspaces\" for hoistingLimits",
);
return Err(ParseError::InvalidWorkspaceObject);
}
}
}

let mut optional_peers_buf: HashMap<u64, ()> = HashMap::default();
Expand Down
9 changes: 9 additions & 0 deletions src/install/prune.rs
Original file line number Diff line number Diff line change
Expand Up @@ -596,6 +596,15 @@ pub(crate) fn exit_unless_lockfile_matches_package_json(
}
};

// The lockfile does not store the set. Take it from the manifests, as an install does.
manager
.lockfile
.self_contained_workspaces
.clear_retaining_capacity();
for key in to_lockfile.self_contained_workspaces.keys() {
manager.lockfile.self_contained_workspaces.put(*key, ())?;
}

if summary.changes_dependencies() {
if !quiet {
Output::err_generic(
Expand Down
109 changes: 107 additions & 2 deletions test/cli/install/bun-workspaces-self-contained.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -51,11 +51,11 @@ async function install(cwd: string, args: string[] = []) {
return { out, err, code };
}

async function writeProject(desktopExtra: object, workspacesExtra: object) {
async function writeProject(desktopExtra: object, workspacesExtra: object, saveTextLockfile = true) {
await writeFile(
join(package_dir, "bunfig.toml"),
Bun.TOML.stringify({
install: { cache: false, registry: root_url + "/", saveTextLockfile: true, linker: "hoisted" },
install: { cache: false, registry: root_url + "/", saveTextLockfile, linker: "hoisted" },
}),
);
await mkdir(join(package_dir, "apps", "desktop"), { recursive: true });
Expand Down Expand Up @@ -176,3 +176,108 @@ it("without either setting the workspace is hoisted normally", async () => {
expect(existsSync(join(package_dir, "node_modules", "@barn", "moo", "package.json"))).toBeTrue();
expect(existsSync(join(package_dir, "node_modules", "baz", "package.json"))).toBeTrue();
});

// The lockfile does not record the setting. Bun 1.4.0 ignored
// `installConfig.hoistingLimits`, so a repo that upgrades keeps its lockfile, and every
// install, a frozen one too, reads the setting from the manifests.
const spellings = {
"installConfig.hoistingLimits": [{ installConfig: { hoistingLimits: "workspaces" } }, {}],
"workspaces.selfContained": [{}, { selfContained: ["apps/desktop"] }],
} as const;

describe.each([
["hoisted", "bun.lock", "installConfig.hoistingLimits"],
["hoisted", "bun.lock", "workspaces.selfContained"],
["isolated", "bun.lock", "installConfig.hoistingLimits"],
["hoisted", "bun.lockb", "installConfig.hoistingLimits"],
["hoisted", "bun.lockb", "workspaces.selfContained"],
] as const)("with the %s linker, %s, and %s", (linker, lockfileName, spelling) => {
const [desktopExtra, workspacesExtra] = spellings[spelling];
const text = lockfileName === "bun.lock";
const readLockfile = () => Bun.file(join(package_dir, lockfileName)).bytes();
const desktopNm = () => join(package_dir, "apps", "desktop", "node_modules");
const cleanTree = async () => {
await rm(join(package_dir, "node_modules"), { recursive: true, force: true });
await rm(desktopNm(), { recursive: true, force: true });
};
// the isolated linker has no hoisting to limit, so only the hoisted layout differs
const expectLayout = async (layout: "hoisted" | "self-contained") => {
if (linker !== "hoisted") return;
if (layout === "hoisted") {
expect(existsSync(desktopNm())).toBeFalse();
expect(existsSync(join(package_dir, "node_modules", "@barn", "moo", "package.json"))).toBeTrue();
} else {
expect(await readdirSorted(desktopNm())).toEqual(["@barn", "bar", "baz", "shared"]);
if (!isWindows) expect(statSync(join(desktopNm(), "bar", "package.json")).nlink).toBe(1);
}
};

it("the setting does not change the lockfile", async () => {
await writeProject({}, {}, text);
let r = await install(package_dir, [`--linker=${linker}`]);
expect(r.err).not.toContain("error:");
expect(r.code).toBe(0);
const without = await readLockfile();

await writeProject(desktopExtra, workspacesExtra, text);
await cleanTree();
r = await install(package_dir, [`--linker=${linker}`]);
expect(r.err).not.toContain("error:");
expect(r.code).toBe(0);
expect(await readLockfile()).toEqual(without);
await expectLayout("self-contained");
});

it("a frozen install applies the manifest's setting to the same lockfile", async () => {
// the lockfile that bun 1.4.0 writes for this project
await writeProject({}, {}, text);
let r = await install(package_dir, [`--linker=${linker}`]);
expect(r.err).not.toContain("error:");
expect(r.code).toBe(0);
const lockfile = await readLockfile();

// the manifests declare the setting (the yarn key was there all along; 1.4.0 ignored it)
await writeProject(desktopExtra, workspacesExtra, text);
await cleanTree();
r = await install(package_dir, [`--linker=${linker}`, "--frozen-lockfile"]);
expect(r.err).not.toContain("error:");
expect(r.code).toBe(0);
expect(await readLockfile()).toEqual(lockfile);
await expectLayout("self-contained");

// the manifest drops the setting, and the workspace hoists again
await writeProject({}, {}, text);
await cleanTree();
r = await install(package_dir, [`--linker=${linker}`, "--frozen-lockfile"]);
expect(r.err).not.toContain("error:");
expect(r.code).toBe(0);
expect(await readLockfile()).toEqual(lockfile);
await expectLayout("hoisted");
});
});

it.each(Object.keys(spellings) as (keyof typeof spellings)[])(
"bun prune keeps the packages of a workspace made self-contained by %s",
async spelling => {
const [desktopExtra, workspacesExtra] = spellings[spelling];
await writeProject(desktopExtra, workspacesExtra);
const r = await install(package_dir);
expect(r.err).not.toContain("error:");
expect(r.code).toBe(0);
const desktopNm = join(package_dir, "apps", "desktop", "node_modules");
expect(await readdirSorted(desktopNm)).toEqual(["@barn", "bar", "baz", "shared"]);

await using proc = spawn({
cmd: [bunExe(), "prune"],
cwd: package_dir,
env,
stdout: "pipe",
stderr: "pipe",
});
const [, err, code] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
expect(err).not.toContain("error:");
expect(code).toBe(0);
expect(await readdirSorted(desktopNm)).toEqual(["@barn", "bar", "baz", "shared"]);
expect(existsSync(join(desktopNm, "bar", "package.json"))).toBeTrue();
},
);
Loading