Skip to content

install: skip yarn.lock entries the migration cannot build a resolution for #39001

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
+204 −81
Open

install: skip yarn.lock entries the migration cannot build a resolution for #39001

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
133 changes: 52 additions & 81 deletions src/install/yarn.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -900,6 +900,15 @@ pub(crate) fn migrate_yarn_lockfile<'a>(

let mut package_id_to_yarn_idx: Vec<usize> = vec![usize::MAX; next_package_id as usize];

// The ids handed out above count every distinct name@version, but an entry
// whose resolution cannot be built is not appended below, and a package's id
// has to be its index in `this.packages`. Maps the ids above to the appended
// ones; a skipped entry stays `INVALID_PACKAGE_ID`, which gives its dependents
// the same unresolved edge as a spec with no yarn.lock entry at all.
let mut appended_package_ids: Vec<PackageID> =
vec![install::INVALID_PACKAGE_ID; next_package_id as usize];
let silent = manager.options.log_level.is_silent();

let created_packages: StringHashMap<bool> = StringHashMap::new();
let _ = &created_packages; // never populated

Expand Down Expand Up @@ -1074,14 +1083,42 @@ pub(crate) fn migrate_yarn_lockfile<'a>(
}
};

if resolution.tag == ResolutionTag::Uninitialized {
if !silent {
let specs = bstr::join(", ", &entry.specs);
let specs = bstr::BStr::new(&specs);
if entry.version.is_empty() {
bun_core::warn!(
"skipped \"{}\" from yarn.lock: missing \"version\" field",
specs
);
} else if !Semver::Version::parse_utf8(entry.version).valid {
bun_core::warn!(
"skipped \"{}\" from yarn.lock: invalid version \"{}\"",
specs,
bstr::BStr::new(entry.version)
);
} else {
bun_core::warn!(
"skipped \"{}\" from yarn.lock: missing \"resolved\" field",
specs
);
}
}
continue;
}

let appended_id = PackageID::try_from(this.packages.len()).expect("int cast");
appended_package_ids[package_id as usize] = appended_id;

this.packages.append(LockfilePackage {
name: pkg_name,
name_hash,
resolution,
dependencies: Default::default(),
resolutions: Default::default(),
meta: PackageMeta {
id: package_id,
id: appended_id,
origin: Origin::Npm,
arch: if let Some(cpu_list) = &entry.cpu {
let mut arch = npm::Architecture::NONE.negatable();
Expand Down Expand Up @@ -1115,6 +1152,16 @@ pub(crate) fn migrate_yarn_lockfile<'a>(
})?;
}

for package_id in yarn_entry_to_package_id.iter_mut() {
*package_id = appended_package_ids[*package_id as usize];
}
for (_, versions) in scoped_packages.iter_mut() {
versions.retain_mut(|info| {
info.package_id = appended_package_ids[info.package_id as usize];
info.package_id != install::INVALID_PACKAGE_ID
});
}

// The derive's `&mut self` accessors can't alias, so we re-borrow per write
// below via `this.packages.items_*_mut()[idx] = …` instead of caching
// two field slices simultaneously.
Expand Down Expand Up @@ -1301,85 +1348,6 @@ pub(crate) fn migrate_yarn_lockfile<'a>(
dependencies: lockfile::DependencyIDSlice::new(0, 0),
});

let mut package_dependents: Vec<Vec<PackageID>> =
(0..next_package_id).map(|_| Vec::new()).collect();

for (yarn_idx, entry) in yarn_lock.entries.iter().enumerate() {
let parent_package_id = yarn_entry_to_package_id[yarn_idx];

let dep_maps: [Option<&StringHashMap<&[u8]>>; 4] = [
entry.dependencies.as_ref(),
entry.optional_dependencies.as_ref(),
entry.peer_dependencies.as_ref(),
entry.dev_dependencies.as_ref(),
];

for deps in dep_maps.iter().flatten() {
for (dep_name_key, dep_version_ref) in deps.iter() {
let dep_name: &[u8] = dep_name_key.as_ref();
let dep_version: &[u8] = *dep_version_ref;
let mut dep_spec = Vec::new();
write!(
&mut dep_spec,
"{}@{}",
bstr::BStr::new(dep_name),
bstr::BStr::new(dep_version)
)
.expect("unreachable");

let found_entry_idx: Option<usize> = yarn_lock
.entries
.iter()
.position(|e| e.specs.contains(&dep_spec.as_slice()));

if let Some(found_entry_idx) = found_entry_idx {
let dep_entry_specs = &yarn_lock.entries[found_entry_idx].specs;
for (idx, e) in yarn_lock.entries.iter().enumerate() {
let mut found = false;
for spec in e.specs.iter() {
for dep_spec_item in dep_entry_specs.iter() {
if *spec == *dep_spec_item {
found = true;
break;
}
}
if found {
break;
}
}

if found {
let dep_package_id = yarn_entry_to_package_id[idx];
package_dependents[dep_package_id as usize].push(parent_package_id);
break;
}
}
}
}
}
}

for dep in root_dependencies.iter() {
let mut dep_spec = Vec::new();
write!(
&mut dep_spec,
"{}@{}",
bstr::BStr::new(&dep.name),
bstr::BStr::new(&dep.version)
)
.expect("unreachable");

for (idx, entry) in yarn_lock.entries.iter().enumerate() {
for spec in entry.specs.iter() {
if *spec == dep_spec.as_slice() {
let dep_package_id = yarn_entry_to_package_id[idx];
package_dependents[dep_package_id as usize].push(0); // 0 is root package
break;
}
}
}
}

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

Expand Down Expand Up @@ -1435,10 +1403,13 @@ pub(crate) fn migrate_yarn_lockfile<'a>(
}
}

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

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 package_names[package_id as usize].is_empty() {
package_names[package_id as usize] = Entry::get_name_from_spec(entry.specs[0]);
}
Expand Down
152 changes: 152 additions & 0 deletions test/cli/install/migration/yarn-lock-migration.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1640,3 +1640,155 @@ fsevents@^2.3.2:
expect(bunLockContent).toContain("@esbuild/darwin-arm64");
});
});

describe.concurrent("yarn.lock entries the migration cannot build a resolution for", () => {
// After migrating, bun fetches the manifest of every migrated registry package (for bin
// metadata), so the registry has to answer for "pinned". Nothing below downloads a tarball.
function manifestOnlyRegistry() {
return Bun.serve({
port: 0,
fetch(req, server) {
const name = new URL(req.url).pathname.slice(1);
if (name !== "pinned") return new Response("not found", { status: 404 });
return Response.json({
name,
"dist-tags": { latest: "2.0.0" },
versions: { "2.0.0": { name, version: "2.0.0", dist: { tarball: `${server.url}pinned-2.0.0.tgz` } } },
});
},
});
}

const registryTarball = (name: string, version: string) =>
`https://registry.yarnpkg.com/${name}/-/${name}-${version}.tgz#0000000000000000000000000000000000000000`;

const header = "# yarn lockfile v1\n\n\n";
const pinnedEntry = (dependencies = "") =>
`pinned@^2.0.0:\n version "2.0.0"\n resolved "${registryTarball("pinned", "2.0.0")}"\n${dependencies}`;
const brokenEntry = `broken@^1.0.0:\n version "not-a-version"\n resolved "${registryTarball("broken", "1.0.0")}"\n`;
const brokenWarning = 'warn: skipped "broken@^1.0.0" from yarn.lock: invalid version "not-a-version"';

async function run(cwd: string, registry: string, ...args: string[]) {
await using proc = Bun.spawn({
cmd: [bunExe(), ...args],
cwd,
env: { ...bunEnv, BUN_CONFIG_REGISTRY: registry, BUN_INSTALL_CACHE_DIR: join(cwd, ".bun-cache") },
stdout: "pipe",
stderr: "pipe",
stdin: "ignore",
});
const [stdout, stderr, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]);
return { stdout, stderr, exitCode };
}

// A skipped entry leaves its dependents with the same unresolved edge as a spec that has no
// yarn.lock entry at all, which `bun install` reports instead of resolving. Before the entry was
// skipped, install crashed on the package's missing resolution (debug builds) or wrote a bun.lock
// that listed the dependency without a package for it.
test("bun install: a root dependency on a skipped entry is reported as unresolved", async () => {
using registry = manifestOnlyRegistry();
using dir = tempDir("yarn-migration-skipped-root-dep", {
"package.json": JSON.stringify({ name: "app", dependencies: { broken: "^1.0.0", pinned: "^2.0.0" } }),
"yarn.lock": header + brokenEntry + "\n" + pinnedEntry(),
});

const { stderr, exitCode } = await run(String(dir), registry.url.href, "install", "--lockfile-only");

expect(stderr).toContain(brokenWarning);
expect(stderr).toContain("error: broken@^1.0.0 failed to resolve");
expect(exitCode).toBe(1);
});

test("bun install: a transitive dependency on a skipped entry is reported as unresolved", async () => {
using registry = manifestOnlyRegistry();
using dir = tempDir("yarn-migration-skipped-transitive-dep", {
"package.json": JSON.stringify({ name: "app", dependencies: { pinned: "^2.0.0" } }),
"yarn.lock": header + brokenEntry + "\n" + pinnedEntry(' dependencies:\n broken "^1.0.0"\n'),
});

const { stderr, exitCode } = await run(String(dir), registry.url.href, "install", "--lockfile-only");

expect(stderr).toContain(brokenWarning);
expect(stderr).toContain("error: broken@^1.0.0 failed to resolve");
expect(exitCode).toBe(1);
});

const skippedEntries: [label: string, brokenRange: string, entry: string, warning: string][] = [
[
"version is not semver",
"^1.0.0",
brokenEntry,
'skipped "broken@^1.0.0" from yarn.lock: invalid version "not-a-version"',
],
[
"version is not semver and there is no resolved field",
"^1.0.0",
'broken@^1.0.0:\n version "not-a-version"\n',
'skipped "broken@^1.0.0" from yarn.lock: invalid version "not-a-version"',
],
[
"there is no version field",
"^1.0.0",
`broken@^1.0.0:\n resolved "${registryTarball("broken", "1.0.0")}"\n`,
'skipped "broken@^1.0.0" from yarn.lock: missing "version" field',
],
[
"npm: alias without a resolved field",
"npm:actual@^1.0.0",
'"broken@npm:actual@^1.0.0":\n version "1.0.0"\n',
'skipped "broken@npm:actual@^1.0.0" from yarn.lock: missing "resolved" field',
],
[
"tarball URL spec without a resolved field",
"https://example.com/broken-1.0.0.tgz",
'"broken@https://example.com/broken-1.0.0.tgz":\n version "1.0.0"\n',
'skipped "broken@https://example.com/broken-1.0.0.tgz" from yarn.lock: missing "resolved" field',
],
[
"entry shared by several specs",
"^1.0.0",
`broken@^1.0.0, broken@^1.2.0:\n version "not-a-version"\n resolved "${registryTarball("broken", "1.0.0")}"\n`,
'skipped "broken@^1.0.0, broken@^1.2.0" from yarn.lock: invalid version "not-a-version"',
],
];

test.each(skippedEntries)(
"bun pm migrate warns and keeps the other entries when %s",
async (_, range, entry, warning) => {
using registry = manifestOnlyRegistry();
using dir = tempDir("yarn-migration-skipped-entry", {
"package.json": JSON.stringify({ name: "app", dependencies: { broken: range, pinned: "^2.0.0" } }),
"yarn.lock": header + entry + "\n" + pinnedEntry(),
});

const { stderr, exitCode } = await run(String(dir), registry.url.href, "pm", "migrate", "-f");

expect(stderr).toContain(`warn: ${warning}`);
expect(stderr).toContain("migrated lockfile from yarn.lock");
expect(exitCode).toBe(0);

const bunLock = await Bun.file(join(String(dir), "bun.lock")).text();
expect(bunLock).toContain('"pinned": ["pinned@2.0.0", "", {}, ""]');
expect(bunLock).not.toContain('"broken": [');
},
);

test("bun pm migrate keeps a version of a package whose other version is skipped", async () => {
using registry = manifestOnlyRegistry();
using dir = tempDir("yarn-migration-skipped-sibling-version", {
"package.json": JSON.stringify({ name: "app", dependencies: { pinned: "^2.0.0" } }),
"yarn.lock":
header +
`pinned@^1.0.0:\n version "not-a-version"\n resolved "${registryTarball("pinned", "1.0.0")}"\n\n` +
pinnedEntry(),
});

const { stderr, exitCode } = await run(String(dir), registry.url.href, "pm", "migrate", "-f");

expect(stderr).toContain('warn: skipped "pinned@^1.0.0" from yarn.lock: invalid version "not-a-version"');
expect(exitCode).toBe(0);

const bunLock = await Bun.file(join(String(dir), "bun.lock")).text();
expect(bunLock).toContain('"pinned": ["pinned@2.0.0", "", {}, ""]');
});
});
Loading