Skip to content

Commit

Permalink
fix(install): allow unresolvable optionalDependencies (#11977)
Browse files Browse the repository at this point in the history
  • Loading branch information
@dylan-conway
dylan-conway authored Jun 19, 2024
1 parent 3ff2995 commit b23ba1f
Show file tree
Hide file tree
Showing 6 changed files with 150 additions and 77 deletions.
57 changes: 56 additions & 1 deletion src/install/install.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -13441,7 +13441,7 @@ pub const PackageManager = struct {
return error.InstallFailed;
}
}
manager.lockfile.verifyResolutions(manager.options.local_package_features, manager.options.remote_package_features, log_level);
manager.verifyResolutions(log_level);
}

// append scripts to lockfile before generating new metahash
Expand Down Expand Up @@ -13782,6 +13782,61 @@ pub const PackageManager = struct {
}
}

pub fn verifyResolutions(this: *PackageManager, comptime log_level: PackageManager.Options.LogLevel) void {
const lockfile = this.lockfile;
const resolutions_lists: []const Lockfile.DependencyIDSlice = lockfile.packages.items(.resolutions);
const dependency_lists: []const Lockfile.DependencySlice = lockfile.packages.items(.dependencies);
const dependencies_buffer = lockfile.buffers.dependencies.items;
const resolutions_buffer = lockfile.buffers.resolutions.items;
const end: PackageID = @truncate(lockfile.packages.len);

var any_failed = false;
const string_buf = lockfile.buffers.string_bytes.items;

const root_list = resolutions_lists[0];
for (resolutions_lists, dependency_lists, 0..) |resolution_list, dependency_list, parent_id| {
for (resolution_list.get(resolutions_buffer), dependency_list.get(dependencies_buffer)) |package_id, failed_dep| {
if (package_id < end) continue;

// TODO lockfile rewrite: remove this and make non-optional peer dependencies error if they did not resolve.
// Need to keep this for now because old lockfiles might have a peer dependency without the optional flag set.
if (failed_dep.behavior.isPeer()) continue;

// even if optional dependencies are enabled, it's still allowed to fail
if (failed_dep.behavior.optional or !failed_dep.behavior.isEnabled(
if (root_list.contains(@truncate(parent_id)))
this.options.local_package_features
else
this.options.remote_package_features,
)) continue;
if (log_level != .silent) {
if (failed_dep.name.isEmpty() or strings.eqlLong(failed_dep.name.slice(string_buf), failed_dep.version.literal.slice(string_buf), true)) {
Output.errGeneric("<b>{}<r><d> failed to resolve<r>", .{
failed_dep.version.literal.fmt(string_buf),
});
} else {
Output.errGeneric("<b>{s}<r><d>@<b>{}<r><d> failed to resolve<r>", .{
failed_dep.name.slice(string_buf),
failed_dep.version.literal.fmt(string_buf),
});
}
}
// track this so we can log each failure instead of just the first
any_failed = true;
}
}

if (any_failed) this.crash();
}

pub fn crash(this: *const PackageManager) noreturn {
if (this.options.log_level != .silent) {
this.log.printForLogLevel(Output.errorWriter()) catch {};
}

Global.crash();
}

pub fn spawnPackageLifecycleScripts(
this: *PackageManager,
ctx: Command.Context,
Expand Down
46 changes: 0 additions & 46 deletions src/install/lockfile.zig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1988,52 +1988,6 @@ pub fn verifyData(this: *const Lockfile) !void {
}
}

pub fn verifyResolutions(this: *Lockfile, local_features: Features, remote_features: Features, comptime log_level: PackageManager.Options.LogLevel) void {
const resolutions_lists: []const DependencyIDSlice = this.packages.items(.resolutions);
const dependency_lists: []const DependencySlice = this.packages.items(.dependencies);
const dependencies_buffer = this.buffers.dependencies.items;
const resolutions_buffer = this.buffers.resolutions.items;
const end = @as(PackageID, @truncate(this.packages.len));

var any_failed = false;
const string_buf = this.buffers.string_bytes.items;

const root_list = resolutions_lists[0];
for (resolutions_lists, dependency_lists, 0..) |resolution_list, dependency_list, parent_id| {
for (resolution_list.get(resolutions_buffer), dependency_list.get(dependencies_buffer)) |package_id, failed_dep| {
if (package_id < end) continue;
if (failed_dep.behavior.isPeer() or !failed_dep.behavior.isEnabled(
if (root_list.contains(@truncate(parent_id)))
local_features
else
remote_features,
)) continue;
if (log_level != .silent) {
if (failed_dep.name.isEmpty() or strings.eqlLong(failed_dep.name.slice(string_buf), failed_dep.version.literal.slice(string_buf), true)) {
Output.prettyErrorln(
"<r><red>error<r><d>:<r> <b>{}<r><d> failed to resolve<r>\n",
.{
failed_dep.version.literal.fmt(string_buf),
},
);
} else {
Output.prettyErrorln(
"<r><red>error<r><d>:<r> <b>{s}<r><d>@<b>{}<r><d> failed to resolve<r>\n",
.{
failed_dep.name.slice(string_buf),
failed_dep.version.literal.fmt(string_buf),
},
);
}
}
// track this so we can log each failure instead of just the first
any_failed = true;
}
}

if (any_failed) Global.crash();
}

pub fn saveToDisk(this: *Lockfile, filename: stringZ) void {
if (comptime Environment.allow_assert) {
this.verifyData() catch |err| {
Expand Down
Binary file modified test/bun.lockb
View file
Binary file not shown.
83 changes: 53 additions & 30 deletions test/cli/install/registry/bun-install-registry.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -66,38 +66,61 @@ registry = "http://localhost:${port}/"
);
});

for (const optional of [true, false]) {
test(`exit code is ${optional ? 0 : 1} when ${optional ? "optional" : ""} dependency fails to install`, async () => {
await write(
join(packageDir, "package.json"),
JSON.stringify({
name: "foo",
[optional ? "optionalDependencies" : "dependencies"]: {
"missing-tarball": "1.0.0",
"uses-what-bin": "1.0.0",
},
"trustedDependencies": ["uses-what-bin"],
}),
);
describe("optionalDependencies", () => {
for (const optional of [true, false]) {
test(`exit code is ${optional ? 0 : 1} when ${optional ? "optional" : ""} dependency tarball is missing`, async () => {
await write(
join(packageDir, "package.json"),
JSON.stringify({
name: "foo",
[optional ? "optionalDependencies" : "dependencies"]: {
"missing-tarball": "1.0.0",
"uses-what-bin": "1.0.0",
},
"trustedDependencies": ["uses-what-bin"],
}),
);

const { exited, err } = await runBunInstall(env, packageDir, {
[optional ? "allowWarnings" : "allowErrors"]: true,
expectedExitCode: optional ? 0 : 1,
savesLockfile: false,
const { exited, err } = await runBunInstall(env, packageDir, {
[optional ? "allowWarnings" : "allowErrors"]: true,
expectedExitCode: optional ? 0 : 1,
savesLockfile: false,
});
expect(err).toContain(
`${optional ? "warn" : "error"}: GET http://localhost:${port}/missing-tarball/-/missing-tarball-1.0.0.tgz - `,
);
expect(await exited).toBe(optional ? 0 : 1);
expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual([
".bin",
".cache",
"uses-what-bin",
"what-bin",
]);
expect(await exists(join(packageDir, "node_modules", "uses-what-bin", "what-bin.txt"))).toBeTrue();
});
expect(err).toContain(
`${optional ? "warn" : "error"}: GET http://localhost:${port}/missing-tarball/-/missing-tarball-1.0.0.tgz - 500`,
);
expect(await exited).toBe(optional ? 0 : 1);
expect(await readdirSorted(join(packageDir, "node_modules"))).toEqual([
".bin",
".cache",
"uses-what-bin",
"what-bin",
]);
expect(await exists(join(packageDir, "node_modules", "uses-what-bin", "what-bin.txt"))).toBeTrue();
});
}
}

for (const rootOptional of [true, false]) {
test(`exit code is 0 when ${rootOptional ? "root" : ""} optional dependency does not exist in registry`, async () => {
await write(
join(packageDir, "package.json"),
JSON.stringify({
name: "foo",
[rootOptional ? "optionalDependencies" : "dependencies"]: {
[rootOptional ? "this-package-does-not-exist-in-the-registry" : "has-missing-optional-dep"]: "||",
},
}),
);

const { err } = await runBunInstall(env, packageDir, {
allowWarnings: true,
savesLockfile: !rootOptional,
});

expect(err).toContain("warn: GET http://localhost:4873/this-package-does-not-exist-in-the-registry - ");
});
}
});

describe.each(["--production", "without --production"])("%s", flag => {
const prod = flag === "--production";
Expand Down
Binary file added ...cli/install/registry/packages/has-missing-optional-dep/has-missing-optional-dep-1.0.0.tgz
View file
Binary file not shown.
41 changes: 41 additions & 0 deletions test/cli/install/registry/packages/has-missing-optional-dep/package.json
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
{
"name": "has-missing-optional-dep",
"versions": {
"1.0.0": {
"name": "has-missing-optional-dep",
"version": "1.0.0",
"optionalDependencies": {
"this-package-does-not-exist-in-the-registry": "||"
},
"_id": "has-missing-optional-dep@1.0.0",
"_nodeVersion": "22.3.0",
"_npmVersion": "10.8.1",
"dist": {
"integrity": "sha512-XQr2TvafpmE+2L588mG6lp816cNczIgYrbXnH03UhDDqHvgzV5pltTum9YgjtcKAScM4KpgIliEUvgf9pxlmTg==",
"shasum": "589b96e2db2cd83ad0595b03f4cf225251e44afd",
"tarball": "http://localhost:4873/has-missing-optional-dep/-/has-missing-optional-dep-1.0.0.tgz"
},
"contributors": []
}
},
"time": {
"modified": "2024-06-19T19:28:09.181Z",
"created": "2024-06-19T19:28:09.181Z",
"1.0.0": "2024-06-19T19:28:09.181Z"
},
"users": {},
"dist-tags": {
"latest": "1.0.0"
},
"_uplinks": {},
"_distfiles": {},
"_attachments": {
"has-missing-optional-dep-1.0.0.tgz": {
"shasum": "589b96e2db2cd83ad0595b03f4cf225251e44afd",
"version": "1.0.0"
}
},
"_rev": "",
"_id": "has-missing-optional-dep",
"readme": "ERROR: No README data found!"
}

0 comments on commit b23ba1f

Please sign in to comment.