From 2b01cb89d555cfa04ea0a154803a52ca18af8e33 Mon Sep 17 00:00:00 2001 From: robobun <117481402+robobun@users.noreply.github.com> Date: Mon, 19 Oct 2026 23:50:11 +0000 Subject: [PATCH] install: keep an optional git, GitHub or tarball dependency on its locked package while it is read again `bun update` reads the source of a git, GitHub or tarball dependency again, and `bun install --force` does so for a tarball. The dependency had no package during that read. A failed read of an optional dependency is a warning, so the package left bun.lock and node_modules at exit 0, and a later `bun install --frozen-lockfile` passed without it. The dependency now keeps the package that bun.lock has for it. A bit per dependency, `Scratch::refreshing`, tells the resolver that it still has to resolve the dependency, and `assign_resolution` clears it. `enqueue::refreshes_in_place` decides which dependencies qualify, at the four places that start such a read: the root after the differ, a workspace that is read again, the dependencies of a git, GitHub or tarball package that is read again, and a named update. A dependency qualifies if it is optional, if bun.lock has the same entry with a package from the source that the entry names, if no `overrides` rule exists for its key or for the name of that package, and if the resolver would not refuse it. A rule for the name that the source has after the read ends the keep. A git ref that the repository no longer has is an answer, and the package leaves as before. `bun update` prints `warn: @ was not checked for updates` for each package that was kept. `enqueue_tarball_for_reading` now answers `AlreadyFailed` for a `file:` tarball whose read has failed, as `enqueue_download` does for a download. The isolated linker waited for such a read forever. On main that happens with `bun install --force` and an empty cache when the root and a workspace have the same optional `file:` tarball and the file is gone. --- docs/pm/cli/update.mdx | 1 + src/install/PackageInstaller.rs | 19 +- .../PackageManager/PackageManagerEnqueue.rs | 158 +++- .../PackageManagerResolution.rs | 1 + .../PackageManager/PopulatePackageCache.rs | 3 +- .../PackageManager/install_with_manager.rs | 38 +- .../PackageManager/processDependencyList.rs | 8 +- src/install/PackageManager/runTasks.rs | 11 +- src/install/isolated_install.rs | 3 +- src/install/lockfile.rs | 82 +- src/install/lockfile/Package.rs | 65 +- src/install/update_transitive.rs | 34 + test/cli/install/bun-install-retry.test.ts | 744 +++++++++++++++++- 13 files changed, 1100 insertions(+), 67 deletions(-) diff --git a/docs/pm/cli/update.mdx b/docs/pm/cli/update.mdx index c96ffcfdfe0b..39342f147ceb 100644 --- a/docs/pm/cli/update.mdx +++ b/docs/pm/cli/update.mdx @@ -42,6 +42,7 @@ Updated packages appear in the install summary as `↑ name old → new`, with ` - Bun does not add a version that another one covers. Two dependencies declaring `foo@^1` and `foo@*` share one `1.x` copy, as on `bun install`. Versions already in `bun.lock` and your own `package.json` entries are never moved to save a copy. - Versions in `patchedDependencies` stay put as long as their range allows. Bun reports them as `kept name@version (patched, v1.2.3 available)`. `--latest` and [`bun audit fix`](/pm/cli/audit#bun-audit-fix) do move them; re-create the patch with [`bun patch`](/pm/cli/patch) afterwards. - If a registry request for a transitive package fails, that package keeps its locked version and Bun prints a warning. A failed request for a direct dependency is an error. +- An optional dependency on a git repository, a GitHub repository, or a tarball keeps its locked package when Bun cannot fetch that source. Bun prints `warn: name@version was not checked for updates`. This does not apply to a dependency that an [`overrides`](/pm/overrides) rule or a catalog sets. If you pass the `package.json` key of the dependency, or if `--prod` or `--depth 0` selects it, `bun update` exits with code 1 and writes nothing. ## `--interactive` diff --git a/src/install/PackageInstaller.rs b/src/install/PackageInstaller.rs index 0cb992357bf4..24775418293f 100644 --- a/src/install/PackageInstaller.rs +++ b/src/install/PackageInstaller.rs @@ -2084,17 +2084,14 @@ impl<'a> PackageInstaller<'a> { context, ) } - resolution::Tag::LocalTarball => { - package_manager::enqueue_tarball_for_reading( - self.manager_mut(), - dependency_id, - package_id, - alias.slice(string_buf!()), - resolution, - context, - ); - Ok(()) - } + resolution::Tag::LocalTarball => package_manager::enqueue_tarball_for_reading( + self.manager_mut(), + dependency_id, + package_id, + alias.slice(string_buf!()), + resolution, + context, + ), resolution::Tag::RemoteTarball => { package_manager::enqueue_tarball_for_download( self.manager_mut(), diff --git a/src/install/PackageManager/PackageManagerEnqueue.rs b/src/install/PackageManager/PackageManagerEnqueue.rs index 97e5ca021193..99f89625db7b 100644 --- a/src/install/PackageManager/PackageManagerEnqueue.rs +++ b/src/install/PackageManager/PackageManagerEnqueue.rs @@ -316,10 +316,7 @@ fn enqueue_download( task_context: TaskCallbackContext, authorization: Authorization, ) -> Result<(), ForTarballError> { - if this.network_task_has_failed(task_id) { - if is_asked_by_linker(&task_context) { - report_failed_download_as_required(this, task_id, dependency_id, package_id); - } + if has_already_failed(this, task_id, dependency_id, package_id, &task_context) { return Err(ForTarballError::AlreadyFailed); } if this.options.offline == OfflineMode::Offline { @@ -363,12 +360,15 @@ pub fn enqueue_tarball_for_reading( alias: &[u8], resolution: &Resolution, task_context: TaskCallbackContext, -) { +) -> Result<(), ForTarballError> { // `enqueue_local_tarball` copies `path` into the filename store before `string_bytes` can grow. let path = this.lockfile.str_detached(resolution.local_tarball()); let task_id = Task::Id::for_tarball(path); + if has_already_failed(this, task_id, dependency_id, package_id, &task_context) { + return Err(ForTarballError::AlreadyFailed); + } if !push_task_callback(this, task_id, task_context) { - return; + return Ok(()); } let integrity = this.lockfile.packages.items_meta()[package_id as usize].integrity; @@ -383,6 +383,7 @@ pub fn enqueue_tarball_for_reading( Some(&integrity), ); this.task_batch.push(ThreadPool::Batch::from(task)); + Ok(()) } /// Returns `Err(Offline)` if the repository isn't cached. Nothing is queued then, and the caller @@ -456,6 +457,23 @@ fn is_asked_by_linker(context: &TaskCallbackContext) -> bool { ) } +/// Returns true if `task_id` has failed, which means that nothing would answer a request for it. +fn has_already_failed( + this: &mut PackageManager, + task_id: Task::Id, + dependency_id: DependencyID, + package_id: PackageID, + context: &TaskCallbackContext, +) -> bool { + if !this.network_task_has_failed(task_id) { + return false; + } + if is_asked_by_linker(context) { + report_failed_download_as_required(this, task_id, dependency_id, package_id); + } + true +} + /// The download failed with only a warning, because only optional dependencies needed it at the /// time. A linker that requires the package reports an error. #[cold] @@ -986,6 +1004,87 @@ fn locked_repository( this.locked_tarball(res) } +/// Only a package.json read from the project may name a tarball in the project. +fn may_name_local_tarball( + lockfile: &Lockfile::Lockfile, + declarer: PackageID, + dependency: &Dependency, +) -> bool { + lockfile.is_local_declarer(declarer) + || lockfile.has_equal_root_dependency(dependency) + || lockfile.packages.items_resolution()[declarer as usize].tag + == ResolutionTag::LocalTarball +} + +/// Returns false for a dependency that never `refreshes_in_place`, whatever bun.lock has for it. +#[inline] +pub(crate) fn may_refresh_in_place(dependency: &Dependency) -> bool { + use dependency::version::Tag; + dependency.behavior.is_optional() + && matches!( + dependency.version.tag, + Tag::Git | Tag::Github | Tag::Tarball + ) +} + +/// Returns true if `dependency` of `declarer` keeps `locked` while this run reads its source again. +pub(crate) fn refreshes_in_place( + this: &PackageManager, + declarer: PackageID, + dependency: &Dependency, + locked: PackageID, +) -> bool { + use dependency::version::Tag; + if !may_refresh_in_place(dependency) { + return false; + } + let lockfile = &*this.lockfile; + let Some(locked_res) = lockfile.packages.items_resolution().get(locked as usize) else { + return false; + }; + let buf = lockfile.buffers.string_bytes.as_slice(); + // An older bun.lock binds a dependency that never resolved to any package of its name. + let is_from_source = match dependency.version.tag { + Tag::Github => { + let asks = dependency.version.github(); + locked_res.tag == ResolutionTag::Github + && locked_res.github().owner.eql(asks.owner, buf, buf) + && locked_res.github().repo.eql(asks.repo, buf, buf) + } + Tag::Git => { + let asks = dependency.version.git().repo.slice(buf); + // bun.lock has `host:path` as a URL. + let recorded = crate::repository_real::locked_scp_like(asks); + locked_res.tag == ResolutionTag::Git + && locked_res.git().repo.slice(buf) == recorded.as_deref().unwrap_or(asks) + } + _ => match &dependency.version.tarball().uri { + dependency::tarball::Uri::Remote(url) => { + locked_res.tag == ResolutionTag::RemoteTarball + && locked_res.remote_tarball().eql(*url, buf, buf) + } + dependency::tarball::Uri::Local(path) => { + locked_res.tag == ResolutionTag::LocalTarball + && locked_res.local_tarball().eql(*path, buf, buf) + && may_name_local_tarball(lockfile, declarer, dependency) + } + }, + }; + let has_rule = |name_hash: PackageNameHash| { + lockfile.overrides.has_rule_for_name(name_hash) + || this + .summary + .overridden_names + .binary_search(&name_hash) + .is_ok() + }; + is_from_source + // A rule replaces what the dependency declares, and a changed rule can replace `locked`. + && !has_rule(dependency.name_hash) + && !has_rule(lockfile.packages.items_name_hash()[locked as usize]) + && unsafe_alias(lockfile, dependency).is_none() +} + /// Returns the `npm:` alias that the project declares for `name`, if any. A dependency of the root /// package.json only follows an alias that the root package.json itself declares. fn followed_npm_alias( @@ -1258,7 +1357,7 @@ fn request_remote_package_ahead( asked.version.tarball().uri, dependency::tarball::Uri::Local(_) ); - if resolution != invalid_package_id + if !this.lockfile.awaits_resolve(id) || this.options.runtime_auto_install // A deferred peer doesn't request anything, because the tree may provide it. || (dependency.behavior.is_peer() && !install_peer) @@ -1810,15 +1909,7 @@ fn unsafe_name<'a>( dependency: &'a Dependency, asked: &'a Asked<'_>, ) -> Option<&'a [u8]> { - let alias = lockfile.str(&dependency.name); - let alias_is_safe = if alias == lockfile.str(&dependency.version.literal) { - // Until `assign_resolution` runs, the alias of `bun add ` is the specifier - // itself. It never becomes a folder name, but it is printed. - !dependency::contains_control_character(alias) - } else { - alias.is_empty() || crate::package_installer::alias_is_safe_install_target(alias) - }; - if !alias_is_safe { + if let Some(alias) = unsafe_alias(lockfile, dependency) { return Some(alias); } let registry_name: Option<&[u8]> = match asked.version.tag { @@ -1832,6 +1923,23 @@ fn unsafe_name<'a>( registry_name.filter(|name| !crate::package_installer::alias_is_safe_install_target(name)) } +/// The alias of a dependency, which is a future folder, if it is unsafe. +#[inline(always)] +fn unsafe_alias<'a>( + lockfile: &'a Lockfile::Lockfile, + dependency: &'a Dependency, +) -> Option<&'a [u8]> { + let alias = lockfile.str(&dependency.name); + let alias_is_safe = if alias == lockfile.str(&dependency.version.literal) { + // Until `assign_resolution` runs, the alias of `bun add ` is the specifier + // itself. It never becomes a folder name, but it is printed. + !dependency::contains_control_character(alias) + } else { + alias.is_empty() || crate::package_installer::alias_is_safe_install_target(alias) + }; + (!alias_is_safe).then_some(alias) +} + /// Resolves one dependency: binds it to a package that the lockfile has, or appends the package. pub(crate) fn bind_row( this: &mut PackageManager, @@ -1880,6 +1988,13 @@ pub(crate) fn bind_row( version, version_was_replaced, } = asked; + // A rule for the name that its source has now replaces what the dependency asks for. + let resolution = if version_was_replaced && this.lockfile.is_refreshing(id) { + this.lockfile.unbind_refreshing(id); + invalid_package_id + } else { + resolution + }; let mut loaded_manifest: Option = None; match version.tag { @@ -2170,8 +2285,7 @@ fn bind_row_without_manifest( let clone_id = Task::Id::for_git_clone(url); if let Some(repo_fd) = this.git_repositories.get(&clone_id).copied() { - let needs_ctx = - this.lockfile.buffers.resolutions[id as usize] == invalid_package_id; + let needs_ctx = this.lockfile.awaits_resolve(id); // Waits on a `git log` task; `run_tasks` fills `git_commits` and re-enters here. let committish = this.lockfile.str_detached(&dep.committish); @@ -2336,14 +2450,10 @@ fn bind_row_without_manifest( dependency::tarball::Uri::Remote(url) => ResolutionTagged::RemoteTarball(*url), }, }); - // Only a package.json read from the project may name a tarball in the project. if res.tag == ResolutionTag::LocalTarball && !version_was_replaced - && !this.lockfile.is_dependency_of_local_package(id) - && !this.lockfile.has_equal_root_dependency(dependency) && let Some(declarer) = this.lockfile.get_parent_pkg_of_dependency(id) - && this.lockfile.packages.items_resolution()[declarer as usize].tag - != ResolutionTag::LocalTarball + && !may_name_local_tarball(&this.lockfile, declarer, dependency) { if dependency.behavior.is_required() { reject_local_tarball_of_remote_package(this, declarer, dependency); @@ -2373,7 +2483,7 @@ fn bind_row_without_manifest( }; let task_id = Task::Id::for_tarball(url); - if this.lockfile.buffers.resolutions[id as usize] == invalid_package_id { + if this.lockfile.awaits_resolve(id) { if let Some(pkg_id) = resolve_from_appended_task(this, task_id, id) { success_fn(this, id, pkg_id); return Ok(()); diff --git a/src/install/PackageManager/PackageManagerResolution.rs b/src/install/PackageManager/PackageManagerResolution.rs index 29f6d2221e10..db59717463d7 100644 --- a/src/install/PackageManager/PackageManagerResolution.rs +++ b/src/install/PackageManager/PackageManagerResolution.rs @@ -78,6 +78,7 @@ impl PackageManager { ); debug_assert!((package_id as usize) < self.lockfile.packages.len()); // debug_assert!(self.lockfile.buffers.resolutions.as_slice()[dependency_id as usize] == invalid_package_id); + self.lockfile.take_refreshing(dependency_id); let buffers = &mut self.lockfile.buffers; buffers.resolutions.as_mut_slice()[dependency_id as usize] = package_id; let string_buf = buffers.string_bytes.as_slice(); diff --git a/src/install/PackageManager/PopulatePackageCache.rs b/src/install/PackageManager/PopulatePackageCache.rs index 1068d84dab61..b6ae3278277f 100644 --- a/src/install/PackageManager/PopulatePackageCache.rs +++ b/src/install/PackageManager/PopulatePackageCache.rs @@ -159,8 +159,7 @@ pub(crate) fn download_pinned_packages( enqueue_tarball_for_download(manager, dep_id, pkg_id, url, waiter) } ResolutionTag::LocalTarball => { - enqueue_tarball_for_reading(manager, dep_id, pkg_id, alias, &resolution, waiter); - Ok(()) + enqueue_tarball_for_reading(manager, dep_id, pkg_id, alias, &resolution, waiter) } ResolutionTag::Git => enqueue_git_for_checkout( manager, diff --git a/src/install/PackageManager/install_with_manager.rs b/src/install/PackageManager/install_with_manager.rs index fdf7de1a335b..12f16f4d0516 100644 --- a/src/install/PackageManager/install_with_manager.rs +++ b/src/install/PackageManager/install_with_manager.rs @@ -16,7 +16,7 @@ use crate::lockfile::{self, Lockfile, reachable}; use crate::resolution::Tag as ResolutionTag; use crate::update_transitive::{ DirectDependencies, TransitiveUpdate, enqueue_peer_rows, moved_targets_after_clean, - plannable_peer_rows, print_kept_patched, redirect_moved_edges, + plannable_peer_rows, print_kept_patched, redirect_moved_edges, warn_unchecked_sources, }; use crate::{ Dependency, DependencyID, Features, PackageID, PackageNameHash, PatchTask, Resolution, @@ -38,7 +38,7 @@ use bun_install_types::NodeLinker::NodeLinker; // Free-function "methods" on `PackageManager` hosted in sibling modules // to avoid one giant `impl PackageManager` block. -use crate::package_manager_real::enqueue::add_dependency_error; +use crate::package_manager_real::enqueue::{self, add_dependency_error}; use crate::package_manager_real::remove_stale_workspace_links::remove_stale_workspace_links; use crate::package_manager_real::run_tasks::{RunTasksCallbacks, run_tasks}; use crate::package_manager_real::{ @@ -513,6 +513,13 @@ pub fn install_with_manager( builder.clamp(); + manager + .summary + .overridden_names + .clone_from(&all_name_hashes); + let rows = lockfile::DependencySlice::new(off, len); + Diff::keep_while_refreshing(manager, 0, root.dependencies, rows); + let invalidates_rows = (manager.summary.overrides_changed && !all_name_hashes.is_empty()) || manager.summary.catalogs_changed; @@ -678,6 +685,7 @@ pub fn install_with_manager( manager.log_held(); transitive.print_plan(manager, &direct_deps_before, &named.moved); print_kept_patched(manager); + warn_unchecked_sources(manager); let had_errors_before_cleaning_lockfile = manager.log_mut().has_errors(); manager @@ -1811,8 +1819,14 @@ fn enqueue_named_updates( let mut peer_rows: Vec = Vec::new(); let dependencies_len = manager.lockfile.buffers.dependencies.len(); for dependency_i in 0..dependencies_len { + let row = dependency_i as DependencyID; let dependency = manager.lockfile.buffers.dependencies[dependency_i].clone(); - let package_id = manager.lockfile.buffers.resolutions[dependency_i]; + // A dependency that is refreshed is already queued, like one that has no package. + let package_id = if manager.lockfile.is_refreshing(row) { + invalid_package_id + } else { + manager.lockfile.buffers.resolutions[dependency_i] + }; let Some(request) = index_of_named_update(manager, &dependency, package_id) else { continue; }; @@ -1822,7 +1836,7 @@ fn enqueue_named_updates( } matched.set(request); if collect_latest_rows { - named.latest_rows.push(dependency_i as DependencyID); + named.latest_rows.push(row); } // The root's link to a workspace has no version to update. Other dependencies on that name // can only link to the workspace while it stays resolved. @@ -1834,13 +1848,21 @@ fn enqueue_named_updates( } if dependency.behavior.is_peer() { if plannable_peers.is_set(dependency_i) { - peer_rows.push(dependency_i as DependencyID); + peer_rows.push(row); } continue; } - manager.lockfile.buffers.resolutions[dependency_i] = invalid_package_id; - named.moved.push((dependency_i as DependencyID, package_id)); - enqueue_row(manager, dependency_i as DependencyID); + let refreshes_in_place = enqueue::may_refresh_in_place(&dependency) + && (manager.lockfile.get_parent_pkg_of_dependency(row)).is_some_and(|declarer| { + enqueue::refreshes_in_place(manager, declarer, &dependency, package_id) + }); + if refreshes_in_place { + manager.lockfile.mark_refreshing(row); + } else { + manager.lockfile.buffers.resolutions[dependency_i] = invalid_package_id; + } + named.moved.push((row, package_id)); + enqueue_row(manager, row); } reject_unknown_update_requests( diff --git a/src/install/PackageManager/processDependencyList.rs b/src/install/PackageManager/processDependencyList.rs index e0cbfae08021..eac4bc373978 100644 --- a/src/install/PackageManager/processDependencyList.rs +++ b/src/install/PackageManager/processDependencyList.rs @@ -7,7 +7,7 @@ use bun_semver::{ExternalString, String as SemverString}; use crate::bun_json::Expr; use crate::dependency::{TagExt as _, VersionTag}; -use crate::lockfile_real::package::{Package, PackageColumns, ResolverContext}; +use crate::lockfile_real::package::{Diff, Package, PackageColumns, ResolverContext}; use crate::lockfile_real::{DependencySlice, Lockfile, PackageIDSlice, QueuedRows, StringBuilder}; use crate::package_manager_real::options::LogLevel; use crate::package_manager_real::{ArrivedPackage, PackageManager, RemoteTurn, enqueue, run_tasks}; @@ -195,6 +195,12 @@ impl PackageManager { ), ); } + Diff::keep_while_refreshing( + self, + existing_id, + existing.dependencies, + package.dependencies, + ); existing.dependencies = package.dependencies; existing.resolutions = package.resolutions; existing.bin = package.bin; diff --git a/src/install/PackageManager/runTasks.rs b/src/install/PackageManager/runTasks.rs index bcb7c79f4e6d..c67c07736fd8 100644 --- a/src/install/PackageManager/runTasks.rs +++ b/src/install/PackageManager/runTasks.rs @@ -1316,7 +1316,7 @@ fn run_tasks_erased( _ => unreachable!(), } }; - // The mark is a no-op for `local_tarball` tasks (they never populate the map). + // A linker's `local_tarball` task is not in the map, so it gets no mark. let Some(is_required) = cb.dispatch_tarball_error( manager, extract_ctx, @@ -1563,7 +1563,12 @@ fn run_tasks_erased( if task.status == Task::Status::Fail { let err = task.err.unwrap_or(crate::Error::Failed); let is_required = manager.is_network_task_required(task.id); - let _ = forget_failed_git_task(manager, task.id, name); + // The repository (with `--offline`, its cached clone) has no such commit. + for waiter in forget_failed_git_task(manager, task.id, name).iter() { + if let &bun_install::TaskCallbackContext::Dependency(id) = waiter { + manager.lockfile.unbind_refreshing(id); + } + } if cb.has_on_package_manifest_error { (cb.on_package_manifest_error)(extract_ctx, name, err, url); } else { @@ -2011,7 +2016,9 @@ pub(crate) fn fail_update_requests( let resolutions = lockfile.buffers.resolutions.as_slice(); let waiters = this.task_queue.get(&task_id).map_or(&[][..], Vec::as_slice); // An npm tarball task has no waiters: the dependencies it was for already resolved to its package. + // A dependency that is refreshed still has the package of bun.lock, which the task was not for. let package_id = tarball_dependency_id + .filter(|&id| !lockfile.is_refreshing(id)) .and_then(|id| resolutions.get(id as usize).copied()) .filter(|&package_id| package_id != INVALID_PACKAGE_ID); let was_for = |id: DependencyID| { diff --git a/src/install/isolated_install.rs b/src/install/isolated_install.rs index bea3cdf5d555..236b626dbfa1 100644 --- a/src/install/isolated_install.rs +++ b/src/install/isolated_install.rs @@ -2610,8 +2610,7 @@ pub(crate) fn install_isolated_packages( dep.name.slice(string_buf), &pkg_res, ctx, - ); - Ok(()) + ) } _ => package_manager::enqueue_tarball_for_download( installer.manager_mut(), diff --git a/src/install/lockfile.rs b/src/install/lockfile.rs index 79ea9ae1c476..9c8c1ad6ed90 100644 --- a/src/install/lockfile.rs +++ b/src/install/lockfile.rs @@ -1045,14 +1045,16 @@ impl Lockfile { /// that one of them depends on or overrides to. This is checked rather than assumed, because a /// migrated lockfile can have dependencies for a folder that a registry package shipped. pub(crate) fn is_dependency_of_local_package(&self, id: DependencyID) -> bool { - let Some(parent_id) = self.get_parent_pkg_of_dependency(id) else { - return false; - }; - match self.packages.items_resolution()[parent_id as usize].tag { + self.get_parent_pkg_of_dependency(id) + .is_some_and(|parent_id| self.is_local_declarer(parent_id)) + } + + /// See `is_dependency_of_local_package`. + pub(crate) fn is_local_declarer(&self, id: PackageID) -> bool { + match self.packages.items_resolution()[id as usize].tag { ResolutionTag::Root | ResolutionTag::Workspace => true, ResolutionTag::Folder => { - self.is_workspace_declared_package(parent_id) - || self.is_override_selected_package(parent_id) + self.is_workspace_declared_package(id) || self.is_override_selected_package(id) } _ => false, } @@ -2515,6 +2517,71 @@ impl Lockfile { .is_set_allow_out_of_bound(id as usize, false) } + /// See `Scratch::refreshing`. + pub(crate) fn mark_refreshing(&mut self, id: DependencyID) { + let rows = self.buffers.dependencies.len(); + let refreshing = &mut self.scratch.refreshing; + if refreshing.bit_length() < rows { + bun_core::handle_oom(refreshing.resize(rows, false)); + } + refreshing.set(id as usize); + } + + #[inline] + pub(crate) fn is_refreshing(&self, id: DependencyID) -> bool { + self.scratch + .refreshing + .is_set_allow_out_of_bound(id as usize, false) + } + + /// The resolve has an answer for `id`. Returns false if `id` was not being refreshed. + #[inline] + pub(crate) fn take_refreshing(&mut self, id: DependencyID) -> bool { + let was_refreshing = self.is_refreshing(id); + if was_refreshing { + self.scratch.refreshing.unset(id as usize); + } + was_refreshing + } + + /// Returns true if `id` has no package, or keeps the one of bun.lock only until it is resolved. + #[inline] + pub(crate) fn awaits_resolve(&self, id: DependencyID) -> bool { + self.buffers.resolutions[id as usize] == invalid_package_id || self.is_refreshing(id) + } + + /// The source of `id` was read, and it has no package for it: the one of bun.lock goes. + pub(crate) fn unbind_refreshing(&mut self, id: DependencyID) { + if self.take_refreshing(id) { + self.buffers.resolutions[id as usize] = invalid_package_id; + } + } + + /// Once everything is resolved: the packages that stay only because a source could not be read. + pub(crate) fn kept_without_refresh(&self) -> Vec { + let mut kept = Vec::new(); + if self.scratch.refreshing.count() == 0 { + return kept; + } + let resolutions = self.buffers.resolutions.as_slice(); + let stays = reachable::packages(self, resolutions, reachable::Options::all(0)); + for (declarer, rows) in self.packages.items_dependencies().iter().enumerate() { + if !stays.is_set(declarer) { + continue; + } + for row in rows.begin()..rows.end() { + let package_id = resolutions[row as usize]; + if self.is_refreshing(row) + && (package_id as usize) < self.packages.len() + && !kept.contains(&package_id) + { + kept.push(package_id); + } + } + } + kept + } + pub(crate) fn get_package_id( &self, name_hash: u64, @@ -2724,6 +2791,8 @@ pub struct Scratch { pub(crate) dependency_list_queue: DependencyQueue, /// `bit[dependency_id]`: this resolve reached it below a dependency that the installers filter. pub(crate) unplaced_subtree: DynamicBitSet, + /// `bit[dependency_id]`: it keeps its bun.lock package until its source is read again. + pub(crate) refreshing: DynamicBitSet, } pub(crate) type DuplicateCheckerMap = @@ -2916,6 +2985,7 @@ impl Scratch { dependency_list_queue: DependencyQueue::init(), duplicate_checker_map: DuplicateCheckerMap::default(), unplaced_subtree: DynamicBitSet::default(), + refreshing: DynamicBitSet::default(), } } } diff --git a/src/install/lockfile/Package.rs b/src/install/lockfile/Package.rs index 90ecff8af174..3ad79600b140 100644 --- a/src/install/lockfile/Package.rs +++ b/src/install/lockfile/Package.rs @@ -874,6 +874,8 @@ pub struct DiffSummary { /// How many of the changes counted in `update` leave every dependency unchanged. pub(crate) script_only_updates: u32, pub(crate) overrides_changed: bool, + /// The sorted names with a rule in bun.lock or package.json, if `overrides_changed`. + pub(crate) overridden_names: Vec, pub(crate) catalogs_changed: bool, /// Set after the differ, by `Lockfile::reconcile_package_extensions`. pub(crate) package_extensions_changed: bool, @@ -1700,21 +1702,20 @@ impl Diff { /// `to` is about to replace `from_id`. Its dependencies keep the resolutions that `generate` /// would let the root's dependencies keep. pub(crate) fn keep_resolutions(pm: &mut PackageManager, from_id: PackageID, to: &Package) { - if pm.to_update || from_id >= pm.lockfile.loaded_package_count { + if from_id >= pm.lockfile.loaded_package_count { return; } let from = pm.lockfile.packages.items_dependencies()[from_id as usize]; + if pm.to_update { + return Self::keep_while_refreshing(pm, from_id, from, to.dependencies); + } for to_i in to.dependencies.begin() as usize..to.dependencies.end() as usize { let lockfile = &*pm.lockfile; let buf = lockfile.buffers.string_bytes.as_slice(); let to_dep = &lockfile.buffers.dependencies[to_i]; - let kept = (from.begin() as usize..from.end() as usize).find_map(|from_i| { - let from_dep = &lockfile.buffers.dependencies[from_i]; - let locked = lockfile.buffers.resolutions[from_i]; - (from_dep.name_hash == to_dep.name_hash - && from_dep.behavior == to_dep.behavior - && (lockfile.packages.items_resolution().get(locked as usize)) - .is_some_and(|resolution| resolution.tag != ResolutionTag::Folder) + let kept = Self::locked_for(lockfile, from, to_dep, |from_dep, locked| { + (lockfile.packages.items_resolution().get(locked as usize)) + .is_some_and(|resolution| resolution.tag != ResolutionTag::Folder) && ((Dependency::eql(to_dep, from_dep, buf, buf) && !Self::locked_version_is_out_of_range( lockfile, @@ -1723,8 +1724,7 @@ impl Diff { to.dependencies.get(&lockfile.buffers.dependencies), to_i - to.dependencies.begin() as usize, )) - || Self::locked_version_satisfies(lockfile, locked, to_dep, buf))) - .then_some(locked) + || Self::locked_version_satisfies(lockfile, locked, to_dep, buf)) }); if let Some(locked) = kept { pm.lockfile.buffers.resolutions[to_i] = locked; @@ -1732,6 +1732,51 @@ impl Diff { } } + /// The package of the dependency in `from` with the name and group of `to_dep`, if `keeps` it. + fn locked_for( + lockfile: &Lockfile, + from: DependencySlice, + to_dep: &Dependency, + keeps: impl Fn(&Dependency, PackageID) -> bool, + ) -> Option { + (from.begin() as usize..from.end() as usize).find_map(|from_i| { + let from_dep = &lockfile.buffers.dependencies[from_i]; + let locked = lockfile.buffers.resolutions[from_i]; + (from_dep.name_hash == to_dep.name_hash + && from_dep.behavior == to_dep.behavior + && keeps(from_dep, locked)) + .then_some(locked) + }) + } + + /// A dependency in `to` that `refreshes_in_place` keeps the package of the same one in `from`. + pub(crate) fn keep_while_refreshing( + pm: &mut PackageManager, + declarer: PackageID, + from: DependencySlice, + to: DependencySlice, + ) { + use crate::package_manager_real::enqueue; + for to_i in to.begin()..to.end() { + let lockfile = &*pm.lockfile; + let buf = lockfile.buffers.string_bytes.as_slice(); + let to_dep = &lockfile.buffers.dependencies[to_i as usize]; + if lockfile.buffers.resolutions[to_i as usize] != invalid_package_id + || !enqueue::may_refresh_in_place(to_dep) + { + continue; + } + let kept = Self::locked_for(lockfile, from, to_dep, |from_dep, locked| { + Dependency::eql(to_dep, from_dep, buf, buf) + && enqueue::refreshes_in_place(pm, declarer, to_dep, locked) + }); + if let Some(locked) = kept { + pm.lockfile.buffers.resolutions[to_i as usize] = locked; + pm.lockfile.mark_refreshing(to_i); + } + } + } + /// `None` is the default list, which the lockfile never records. fn trusted_dependencies( summary: &mut DiffSummary, diff --git a/src/install/update_transitive.rs b/src/install/update_transitive.rs index df07c926c2db..e72d51c0e7cd 100644 --- a/src/install/update_transitive.rs +++ b/src/install/update_transitive.rs @@ -712,6 +712,40 @@ pub(crate) fn print_kept_patched(manager: &mut PackageManager) { manager.kept_patched_text = render_kept_rows(&rows); } +/// `bun update`, once everything is resolved: a dependency whose source could not be read kept the package of bun.lock (`Scratch::refreshing`), which the warning about the failed read does not say. Each such package gets the line that `warn_unchecked` prints for a package whose manifest could not be fetched. A command that has failed by now saves nothing, and gets no such line. +pub(crate) fn warn_unchecked_sources(manager: &PackageManager) { + if manager.subcommand != crate::Subcommand::Update + || manager.log_mut().has_errors() + || manager.update_requests.iter().any(|request| request.failed) + { + return; + } + let lockfile = &*manager.lockfile; + let buf = lockfile.buffers.string_bytes.as_slice(); + let mut kept: Vec<(&[u8], Box<[u8]>)> = (lockfile.kept_without_refresh().iter()) + .map(|&id| { + let resolution = &lockfile.packages.items_resolution()[id as usize]; + ( + lockfile.packages.items_name()[id as usize].slice(buf), + text(resolution.fmt(buf, bun_core::fmt::PathSep::Posix)), + ) + }) + .collect(); + index_sort::sort_vec_unstable_by(&mut kept, |a, b| a.cmp(b)); + let log = manager.log_mut(); + for (name, resolution) in kept { + log.add_warning_fmt( + None, + bun_ast::Loc::EMPTY, + format_args!( + "{}@{} was not checked for updates", + bun_core::fmt::for_terminal(BStr::new(name)), + bun_core::fmt::for_terminal(BStr::new(&resolution)), + ), + ); + } +} + /// Returns the source of the root package.json and its `patchedDependencies` object. fn root_patched_dependencies<'a>( cache: &'a mut WorkspacePackageJSONCache, diff --git a/test/cli/install/bun-install-retry.test.ts b/test/cli/install/bun-install-retry.test.ts index c501e48fd643..71726fa766dd 100644 --- a/test/cli/install/bun-install-retry.test.ts +++ b/test/cli/install/bun-install-retry.test.ts @@ -1,6 +1,6 @@ import { file, spawn } from "bun"; import { afterAll, afterEach, beforeAll, beforeEach, describe, expect, it, setDefaultTimeout } from "bun:test"; -import { lstatSync, readdirSync, rmSync } from "fs"; +import { copyFileSync, existsSync, lstatSync, mkdirSync, readdirSync, renameSync, rmSync, writeFileSync } from "fs"; import { access, writeFile } from "fs/promises"; import { bunExe, @@ -14,6 +14,7 @@ import { toHaveBins, } from "harness"; import { join } from "path"; +import { pathToFileURL } from "url"; import { dummyAfterAll, dummyAfterEach, @@ -1503,3 +1504,744 @@ describe.concurrent("a failed download that several dependencies share", () => { }); }); }); + +// A dependency on a git repository, a GitHub repository or a tarball has no manifest. `bun update` reads its source +// again, and so does `bun install --force` for a tarball. An optional one keeps the package that bun.lock has for it +// until the source gives another, so a source that cannot be read leaves that package in bun.lock. +describe.concurrent("a locked optional dependency whose source cannot be read again", () => { + const kinds = ["tarball URL", "GitHub", "git", "file: tarball"] as const; + type Kind = (typeof kinds)[number]; + // A package that is not in the project cannot name a path in it. + const fetched = ["tarball URL", "GitHub", "git"] as const; + const tarballs = ["tarball URL", "file: tarball"] as const; + const names: Record = { + "tarball URL": "url-dep", + "GitHub": "github-dep", + "git": "git-dep", + "file: tarball": "file-dep", + }; + const gitEnv = { + GIT_CONFIG_NOSYSTEM: "1", + GIT_AUTHOR_NAME: "Test", + GIT_AUTHOR_EMAIL: "test@example.com", + GIT_COMMITTER_NAME: "Test", + GIT_COMMITTER_EMAIL: "test@example.com", + }; + const tarball = (root: string, manifest: object) => + new Bun.Archive( + { [`${root}/`]: "", [`${root}/package.json`]: JSON.stringify(manifest) }, + { compress: "gzip" }, + ).bytes(); + async function git(cwd: string, ...args: string[]) { + await using proc = spawn({ + cmd: ["git", ...args], + cwd, + env: { ...env, ...gitEnv }, + stdout: "pipe", + stderr: "inherit", + }); + const [out, exitCode] = await Promise.all([proc.stdout.text(), proc.exited]); + expect(exitCode).toBe(0); + return out.trim(); + } + + type Options = { + /** Who declares the dependency: the root, the workspace `packages/member`, or the GitHub package `parent`. */ + in?: "root" | "member" | "package"; + group?: "optionalDependencies" | "dependencies"; + linker?: "hoisted" | "isolated"; + /** The key of the entry, if it is not the name of the package. */ + key?: string; + /** The root has the entry too. */ + alsoInRoot?: boolean; + /** More fields for the package.json of the dependency. */ + package?: object; + /** More fields for the package.json of the root and of the workspace. */ + root?: Record; + member?: Record; + }; + + async function project(kind: Kind, options: Options = {}) { + const { in: declaredIn = "root", group = "optionalDependencies", linker = "hoisted" } = options; + const name = names[kind]; + const key = options.key ?? name; + const manifest = (version: string, as = name) => ({ name: as, version, ...options.package }); + // The registry: each version of each package, in the order of publication. + const registry: Record> = {}; + // The packages of the registry whose tarball cannot be downloaded. + const withoutTarball = new Set(); + async function register(manifest: { name: string; version: string; dependencies?: object }) { + (registry[manifest.name] ??= {})[manifest.version] = { manifest, bytes: await tarball("package", manifest) }; + } + await register({ name: "good", version: "1.0.0" }); + let packed = await tarball("package", manifest("1.0.0")); + // GitHub names the directory in its archive after the commit. + let archive = await tarball("owner-repo-abc1234", manifest("1.0.0")); + let parent: Uint8Array | undefined; + // The answer to a request for the dependency whose path ends with `suffix`. + let lost: { status: number; suffix: string } | undefined; + let asked: string[] = []; + // Set while the manifest of `good` waits for a request for the dependency. + let held: PromiseWithResolvers | undefined; + const server = Bun.serve({ + port: 0, + async fetch(request) { + const { origin, pathname } = new URL(request.url); + const published = registry[pathname.slice(1)]; + if (published) { + if (pathname === "/good") await held?.promise; + const versions = Object.entries(published).map(([version, { manifest }]) => [ + version, + { ...manifest, dist: { tarball: `${origin}${pathname}-${version}.tgz` } }, + ]); + return Response.json({ + name: pathname.slice(1), + "dist-tags": { latest: versions.at(-1)![0] }, + versions: Object.fromEntries(versions), + }); + } + const [, tarballOf, version] = /^\/(.+)-(\d+\.\d+\.\d+)\.tgz$/.exec(pathname) ?? []; + if (withoutTarball.has(tarballOf)) return new Response("refused", { status: 503 }); + if (registry[tarballOf]?.[version]) return new Response(registry[tarballOf][version].bytes); + if (parent && pathname.startsWith("/repos/owner/parent/tarball/")) return new Response(parent); + asked.push(pathname); + held?.resolve(); + if (lost && pathname.endsWith(lost.suffix)) return new Response("refused", { status: lost.status }); + if (pathname.startsWith("/t/")) return new Response(packed); + if (pathname.startsWith("/repos/owner/repo/tarball/")) return new Response(archive); + return new Response("unexpected", { status: 404 }); + }, + }); + const origin = server.url.origin; + + const dir = tempDir("optional-unread", { + "bunfig.toml": Bun.TOML.stringify({ + install: { cache: "./.bun-cache", registry: `${origin}/`, saveTextLockfile: true, linker }, + }), + "packages/member/package.json": "{}", + }); + const path = (...parts: string[]) => join(String(dir), ...parts); + const owner = declaredIn === "member" ? "packages/member" : "."; + + // What the entry asks for, another source of the same kind, and the file or directory that `lose` takes away. + let spec: string; + let other: string; + let source: string | undefined; + switch (kind) { + case "tarball URL": + spec = `${origin}/t/url-dep-1.0.0.tgz`; + other = `${origin}/t/url-dep-2.0.0.tgz`; + break; + case "GitHub": + spec = "owner/repo#v1"; + other = "owner/repo#v2"; + break; + case "git": + source = path("repo"); + mkdirSync(source); + writeFileSync(join(source, "package.json"), JSON.stringify(manifest("1.0.0"))); + await git(source, "init", "-q"); + await git(source, "add", "."); + // The machine's config can ask for signed commits. + await git(source, "-c", "commit.gpgsign=false", "commit", "-q", "-m", "first"); + spec = `git+${pathToFileURL(source)}`; + other = `${spec}#other`; + break; + case "file: tarball": + source = path(owner, "vendor", "file-dep-1.0.0.tgz"); + mkdirSync(path(owner, "vendor"), { recursive: true }); + writeFileSync(source, packed); + spec = "file:./vendor/file-dep-1.0.0.tgz"; + other = "file:./vendor/file-dep-2.0.0.tgz"; + break; + } + + async function declare(entry: string) { + const root: Record = { + name: "foo", + workspaces: ["packages/*"], + ...options.root, + dependencies: { good: "^1.0.0", ...options.root?.dependencies }, + }; + const member: Record = { name: "member", version: "1.0.0", ...options.member }; + if (declaredIn === "package") { + parent = await tarball("owner-parent-def5678", { name: "parent", version: "1.0.0", [group]: { [key]: entry } }); + root.dependencies.parent = "owner/parent#v1"; + } else { + const declarer = declaredIn === "member" ? member : root; + declarer[group] = { ...declarer[group], [key]: entry }; + } + if (options.alsoInRoot) root[group] = { ...root[group], [key]: entry }; + await writeFile(path("package.json"), JSON.stringify(root, null, 2)); + await writeFile(path("packages/member/package.json"), JSON.stringify(member, null, 2)); + } + await declare(spec); + + async function runIn(cwd: string, ...args: string[]) { + await using proc = spawn({ + cmd: [bunExe(), ...args], + cwd: path(cwd), + env: { + ...env, + ...gitEnv, + BUN_INSTALL_CACHE_DIR: undefined, + BUN_CONFIG_HTTP_RETRY_COUNT: "0", + GITHUB_API_URL: origin, + }, + stdout: "pipe", + stderr: "pipe", + }); + const [out, err, exitCode] = await Promise.all([proc.stdout.text(), proc.stderr.text(), proc.exited]); + // Each line once: bun clones the failing git repository of a workspace once or twice. + const lines = (prefix: string) => [...new Set(err.split(/\r?\n/).filter(line => line.startsWith(prefix)))]; + return { + warnings: lines("warn:"), + errors: lines("error:"), + /** How often bun said that it kept a package. */ + unchecked: err.split("was not checked for updates").length - 1, + saved: err.includes("Saved lockfile"), + out, + err, + exitCode, + }; + } + const run = (...args: string[]) => runIn(".", ...args); + + /** The packages that bun.lock has under the name of the dependency, as `name@resolution`. */ + async function locked() { + const { packages } = Bun.JSONC.parse(await file(path("bun.lock")).text()) as { + packages: Record; + }; + return [...new Set(Object.values(packages).map(([id]) => id))].filter(id => id.startsWith(`${name}@`)); + } + /** The version of `of` that is installed, if any. */ + async function installed(of = key) { + for (const modules of ["node_modules", "packages/member/node_modules", "node_modules/parent/node_modules"]) { + if (existsSync(path(modules, of, "package.json"))) { + return (await file(path(modules, of, "package.json")).json()).version as string; + } + } + } + /** bun.lock and every package.json. */ + const written = () => + Promise.all(["bun.lock", "package.json", "packages/member/package.json"].map(f => file(path(f)).text())); + /** What a command prints about a source that it could not read. */ + function unread(answer = 503, ref = "v1") { + const lines: Record = { + "tarball URL": [`warn: GET ${spec} - ${answer}`], + "GitHub": [`warn: GET ${origin}/repos/owner/repo/tarball/${ref} - ${answer}`], + "git": [ + "warn: git failed with exit code 128", + `warn: "git fetch" for "${key}" failed`, + `warn: InstallFailed cloning repository for ${key}`, + ], + "file: tarball": [`warn: ENOENT extracting tarball from ${key}`], + }; + return lines[kind]; + } + let id = ""; + + return { + name, + key, + origin, + path, + /** Publishes a version of a package to the registry. */ + register, + runIn, + run, + declare, + locked, + installed, + written, + unread, + /** What the entry asks for. */ + spec, + /** Another URL, ref or path for the entry. Nothing is there. */ + other, + /** `name@resolution` of the package that the first install wrote to bun.lock. */ + get id() { + return id; + }, + /** Installs, and returns what was written. */ + async install() { + expect(await run("install")).toMatchObject({ warnings: [], errors: [], exitCode: 0 }); + const ids = await locked(); + expect([ids.length, await installed()]).toEqual([1, "1.0.0"]); + id = ids[0]; + return written(); + }, + /** + * From now on the source cannot be read: the server answers `answer` to each request whose path ends with + * `suffix`, and the repository or the file is gone. + */ + lose(answer = 503, suffix = "") { + lost = { status: answer, suffix }; + if (source) renameSync(source, `${source}.gone`); + }, + restore() { + lost = undefined; + if (source) renameSync(`${source}.gone`, source); + }, + /** + * The source has version `version` from now on, under the name `as`. A GitHub ref and a git branch point to + * another commit. + */ + async publish(version: string, as = name) { + packed = await tarball("package", manifest(version, as)); + archive = await tarball("owner-repo-def5678", manifest(version, as)); + if (kind === "file: tarball") writeFileSync(source!, packed); + if (kind === "git") { + writeFileSync(join(source!, "package.json"), JSON.stringify(manifest(version, as))); + await git(source!, "-c", "commit.gpgsign=false", "commit", "-q", "-a", "-m", version); + } + }, + /** From now on the registry answers 503 for the tarballs of `of`. */ + loseTarballOf(of: string) { + withoutTarball.add(of); + }, + /** The manifest of `good` is not sent before the dependency is asked for. */ + holdGood() { + held = Promise.withResolvers(); + }, + /** What `bun update` prints when it could not read the source, and what it leaves: bun.lock as it is. */ + kept(answer = 503, ref = "v1") { + const warnings = [...unread(answer, ref), `warn: ${id} was not checked for updates`]; + return { warnings, unchecked: 1, errors: [], saved: false, exitCode: 0 }; + }, + /** What `bun update` prints and leaves when the command line selects the entry: it fails, and writes nothing. */ + failed() { + return { warnings: unread(), unchecked: 0, errors: [], saved: false, exitCode: 1 }; + }, + /** The requests for the dependency since the last call. */ + asked() { + const paths = asked; + asked = []; + return paths; + }, + removeInstalled() { + rmSync(path("node_modules"), { recursive: true, force: true }); + rmSync(path("packages/member/node_modules"), { recursive: true, force: true }); + }, + [Symbol.dispose]() { + server.stop(true); + dir[Symbol.dispose](); + }, + }; + } + + /** The source is back: what bun.lock has is what a frozen install puts in node_modules. */ + async function expectInstallable(p: Awaited>) { + p.restore(); + p.removeInstalled(); + expect(await p.run("install", "--frozen-lockfile")).toMatchObject({ errors: [], exitCode: 0 }); + expect(await p.installed()).toBe("1.0.0"); + } + + describe.each(["root", "member", "package"] as const)("declared by the %s", declaredIn => { + const declarable: readonly Kind[] = declaredIn === "package" ? fetched : kinds; + + it.each(declarable)("%s: `bun update` keeps the package", async kind => { + using p = await project(kind, { in: declaredIn }); + const before = await p.install(); + // With a source that is as it was, there is nothing to write. + expect(await p.run("update")).toMatchObject({ warnings: [], errors: [], saved: false, exitCode: 0 }); + expect(await p.written()).toEqual(before); + + p.lose(); + expect(await p.run("update")).toMatchObject(p.kept()); + expect(await p.written()).toEqual(before); + if (declaredIn === "root") { + for (const flag of ["--latest", "--lockfile-only"]) { + expect(await p.run("update", flag)).toMatchObject(p.kept()); + expect(await p.written()).toEqual(before); + } + // A command that does not save bun.lock does not read a tarball again. + const unread = kind === "tarball URL" || kind === "file: tarball"; + const frozen = unread ? { ...p.kept(), warnings: [], unchecked: 0 } : p.kept(); + expect(await p.run("update", "--frozen-lockfile")).toMatchObject(frozen); + expect(await p.written()).toEqual(before); + } + if (declaredIn === "member") { + expect(await p.runIn("packages/member", "update")).toMatchObject(p.kept()); + expect(await p.run("update", "--filter", "member")).toMatchObject(p.kept()); + expect(await p.written()).toEqual(before); + } + + // Another dependency moves in the same run: bun.lock is written, and the package is still in it. + await p.register({ name: "good", version: "1.1.0" }); + expect(await p.run("update")).toMatchObject({ ...p.kept(), saved: true }); + expect([await p.locked(), await p.installed("good")]).toEqual([[p.id], "1.1.0"]); + await expectInstallable(p); + }); + + it.each(declarable)("%s: `bun update` takes what the source has now", async kind => { + using p = await project(kind, { in: declaredIn }); + const before = await p.install(); + await p.publish("1.1.0"); + // A tarball that has other bytes than bun.lock recorded is a warning. + expect(await p.run("update")).toMatchObject({ errors: [], saved: true, exitCode: 0 }); + expect(await p.installed()).toBe("1.1.0"); + expect((await p.written())[0]).not.toBe(before[0]); + expect(await p.locked()).toHaveLength(1); + }); + }); + + it.each(tarballs)("%s: `bun install --force` keeps the package", async kind => { + using p = await project(kind); + const before = await p.install(); + p.lose(); + expect(await p.run("install", "--force")).toMatchObject({ ...p.kept(), warnings: p.unread(), unchecked: 0 }); + expect(await p.written()).toEqual(before); + await expectInstallable(p); + }); + + // The linker asks for the package too, and nothing has it. It must not wait for the read that failed. + describe.each(["root", "member"] as const)("declared by the %s", declaredIn => { + it.each(tarballs)("%s: `bun update` with the isolated linker and an empty cache keeps the package", async kind => { + using p = await project(kind, { in: declaredIn, linker: "isolated" }); + const before = await p.install(); + p.lose(); + p.removeInstalled(); + rmSync(p.path(".bun-cache"), { recursive: true, force: true }); + expect(await p.run("update")).toMatchObject(p.kept()); + expect(await p.written()).toEqual(before); + expect(await p.installed()).toBeUndefined(); + await expectInstallable(p); + }); + }); + + // The workspace gets its turn after the read of the file has failed for the root. + it("file: tarball: `bun update` ends when a workspace gets the same entry and the cache is empty", async () => { + using p = await project("file: tarball", { linker: "isolated" }); + const root = { + name: "foo", + workspaces: ["packages/*"], + devDependencies: { good: "^1.0.0" }, + optionalDependencies: { "file-dep": "file:./vendor/file-dep-1.0.0.tgz" }, + }; + await writeFile(p.path("package.json"), JSON.stringify(root, null, 2)); + await p.install(); + const member = { name: "member", version: "1.0.0", optionalDependencies: root.optionalDependencies }; + await writeFile(p.path("packages/member/package.json"), JSON.stringify(member, null, 2)); + p.lose(); + p.removeInstalled(); + rmSync(p.path(".bun-cache"), { recursive: true, force: true }); + expect(await p.run("update")).toMatchObject({ ...p.kept(), saved: true }); + expect(await p.locked()).toEqual([p.id]); + }); + + // Both entries name `./vendor/file-dep-1.0.0.tgz`, so one read serves both. The workspace gets its turn after the + // manifest of its new dependency, when that read has failed. + it("file: tarball: `bun install --force` ends when the root and a workspace have the entry and the cache is empty", async () => { + const entry = { "file-dep": "file:./vendor/file-dep-1.0.0.tgz" }; + using p = await project("file: tarball", { linker: "isolated", member: { optionalDependencies: entry } }); + mkdirSync(p.path("packages/member/vendor")); + copyFileSync(p.path("vendor/file-dep-1.0.0.tgz"), p.path("packages/member/vendor/file-dep-1.0.0.tgz")); + await p.install(); + await p.register({ name: "aaa-new", version: "1.0.0" }); + const member = { + name: "member", + version: "1.0.0", + devDependencies: { "aaa-new": "^1.0.0" }, + optionalDependencies: entry, + }; + await writeFile(p.path("packages/member/package.json"), JSON.stringify(member, null, 2)); + p.lose(); + rmSync(p.path("packages/member/vendor"), { recursive: true }); + p.removeInstalled(); + rmSync(p.path(".bun-cache"), { recursive: true, force: true }); + expect(await p.run("install", "--force")).toMatchObject({ warnings: p.unread(), errors: [], exitCode: 0 }); + expect(await p.locked()).toEqual([p.id]); + }); + + // The second entry waits for the download of the first one, or finds it done. + it.each(["member", "package"] as const)( + "GitHub: `bun update` takes what the source has now for the root and the %s", + async declaredIn => { + using p = await project("GitHub", { in: declaredIn, alsoInRoot: true }); + await p.install(); + await p.publish("1.1.0"); + expect(await p.run("update")).toMatchObject({ warnings: [], errors: [], saved: true, exitCode: 0 }); + expect([await p.locked(), await p.installed()]).toEqual([["github-dep@github:owner/repo#def5678"], "1.1.0"]); + }, + ); + + it("GitHub: `bun update` says once that the package of two entries was not checked", async () => { + using p = await project("GitHub", { in: "member", alsoInRoot: true }); + const before = await p.install(); + p.lose(); + expect(await p.run("update")).toMatchObject(p.kept()); + expect(await p.written()).toEqual(before); + }); + + it.each([ + ["tarball URL", 404], + ["tarball URL", 500], + ["GitHub", 404], + ["GitHub", 500], + ] as const)("%s: `bun update` keeps the package after a %d", async (kind, status) => { + using p = await project(kind); + const before = await p.install(); + p.lose(status); + expect(await p.run("update")).toMatchObject(p.kept(status)); + expect(await p.written()).toEqual(before); + }); + + // The parent is read again, and so are its dependencies. Only its own name is asked for. + it.each(fetched)("%s of a package: `bun update ` keeps the package", async kind => { + using p = await project(kind, { in: "package" }); + const before = await p.install(); + p.lose(); + // bun.lock has the tarball of this URL. A GitHub ref and a git branch are read again. + const kept = kind === "tarball URL" ? { ...p.kept(), warnings: [], unchecked: 0 } : p.kept(); + expect(await p.run("update", "parent")).toMatchObject(kept); + expect(await p.written()).toEqual(before); + }); + + // While the dependency has no package, a peer dependency on its name asks the registry for one. + it("tarball URL: a peer dependency of a workspace on its name asks the registry nothing", async () => { + using p = await project("tarball URL", { member: { peerDependencies: { "url-dep": "*" } } }); + const before = await p.install(); + p.lose(); + p.asked(); + expect(await p.run("update")).toMatchObject(p.kept()); + expect(p.asked()).toEqual(["/t/url-dep-1.0.0.tgz"]); + expect(await p.written()).toEqual(before); + }); + + // Its turn comes after the one of `good`, and its download does not wait for that turn. + it.each(["tarball URL", "GitHub"] as const)("%s: `bun update` asks for it before its turn", async kind => { + using p = await project(kind, { in: "member", member: { dependencies: { good: "^1.0.0" } } }); + const before = await p.install(); + p.holdGood(); + expect(await p.run("update")).toMatchObject({ warnings: [], errors: [], saved: false, exitCode: 0 }); + expect(await p.written()).toEqual(before); + }); + + // A tarball is not read again by a command that does not save. + it.each(["GitHub", "git"] as const)("%s: `bun update --no-save` leaves it installed", async kind => { + using p = await project(kind); + const before = await p.install(); + p.lose(); + expect(await p.run("update", "--no-save")).toMatchObject(p.kept()); + expect([await p.written(), await p.installed()]).toEqual([before, "1.0.0"]); + }); + + // The cache has the archive of a commit, and only GitHub knows the commit of a ref. + it("GitHub: `bun update --offline` keeps the package", async () => { + using p = await project("GitHub"); + const before = await p.install(); + expect(await p.run("update", "--offline")).toMatchObject({ ...p.kept(), warnings: p.kept().warnings.slice(-1) }); + expect(await p.written()).toEqual(before); + }); + + it("GitHub: `bun update --silent` prints nothing", async () => { + using p = await project("GitHub"); + const before = await p.install(); + p.lose(); + expect(await p.run("update", "--silent")).toMatchObject({ err: "", out: "", exitCode: 0 }); + expect(await p.written()).toEqual(before); + }); + + describe("a command that selects the entry fails, and leaves bun.lock as it is", () => { + it.each(kinds)("%s: `bun update `", async kind => { + using p = await project(kind); + const before = await p.install(); + p.lose(); + expect(await p.run("update", p.name)).toMatchObject(p.failed()); + expect(await p.written()).toEqual(before); + }); + + it.each(["--production", "--depth 0", "*-dep"])("GitHub: `bun update %s`", async selector => { + using p = await project("GitHub"); + const before = await p.install(); + p.lose(); + expect(await p.run("update", ...selector.split(" "))).toMatchObject(p.failed()); + expect(await p.written()).toEqual(before); + }); + }); + + // The entry is `alias-dep`, and the package that bun.lock has for it is `github-dep`. That name makes `bun update` + // resolve the entry again, and does not make the command fail for it. + it("GitHub: `bun update ` keeps the package of an entry with another key", async () => { + using p = await project("GitHub", { key: "alias-dep" }); + const before = await p.install(); + p.lose(); + expect(await p.run("update", "github-dep")).toMatchObject(p.kept()); + expect(await p.written()).toEqual(before); + }); + + // `github-dep` and `alias-dep` are two refs of one commit, so bun.lock has one package for both. The name selects + // both entries, and only the ref of `alias-dep` cannot be read. + it("GitHub: `bun update ` does not fail for another entry on the same package", async () => { + using p = await project("GitHub", { + key: "alias-dep", + root: { optionalDependencies: { "github-dep": "owner/repo#v1" } }, + }); + await p.declare("owner/repo#stable"); + const before = await p.install(); + p.lose(503, "/stable"); + expect(await p.run("update", "github-dep")).toMatchObject(p.kept(503, "stable")); + expect(await p.written()).toEqual(before); + }); + + it.each(kinds)("%s: a required one has no package while it is read again, and fails `bun update`", async kind => { + using p = await project(kind, { group: "dependencies" }); + const before = await p.install(); + p.lose(); + const errors = [ + ...p.unread().map(line => line.replace("warn:", "error:")), + `error: ${p.key}@${p.spec} failed to resolve`, + ]; + expect(await p.run("update")).toMatchObject({ errors, unchecked: 0, saved: false, exitCode: 1 }); + expect(await p.written()).toEqual(before); + }); + + it.each(kinds)("%s: an entry that asks for another source does not keep the package", async kind => { + using p = await project(kind); + await p.install(); + await p.declare(p.other); + p.lose(); + expect(await p.run("update", "--lockfile-only")).toMatchObject({ errors: [], saved: true, exitCode: 0 }); + expect(await p.locked()).toEqual([]); + }); + + // A rule can give the entry another source, so an entry with a rule has no package while it is read again. + it.each([ + ["its key", "github-dep", "github-dep"], + ["the name of its package", "alias-dep", "github-dep"], + ])("GitHub: an entry with an `overrides` rule for %s does not keep the package", async (_, key, rule) => { + using p = await project("GitHub", { key, root: { overrides: { [rule]: "owner/repo#v1" } } }); + await p.install(); + p.lose(); + expect(await p.run("update")).toMatchObject({ unchecked: 0, errors: [], saved: true, exitCode: 0 }); + expect(await p.locked()).toEqual([]); + }); + + it("GitHub: an entry whose `overrides` rule is only in bun.lock does not keep the package", async () => { + using p = await project("GitHub", { root: { overrides: { "github-dep": "owner/repo#v1" } } }); + await p.install(); + const { overrides, ...root } = await file(p.path("package.json")).json(); + await writeFile(p.path("package.json"), JSON.stringify(root, null, 2)); + p.lose(); + expect(await p.run("update")).toMatchObject({ unchecked: 0, errors: [], saved: true, exitCode: 0 }); + expect(await p.locked()).toEqual([]); + }); + + // The name in the package.json of a repository is only known when it is read. + it("GitHub: an `overrides` rule for the name that the source has now replaces the package", async () => { + using p = await project("GitHub", { root: { overrides: { "renamed-dep": "1.0.0" } } }); + await p.register({ name: "renamed-dep", version: "1.0.0" }); + await p.install(); + await p.publish("1.1.0", "renamed-dep"); + expect(await p.run("update")).toMatchObject({ warnings: [], errors: [], saved: true, exitCode: 0 }); + const { packages } = Bun.JSONC.parse(await file(p.path("bun.lock")).text()) as { + packages: Record; + }; + expect(packages["github-dep"][0]).toBe("renamed-dep@1.0.0"); + }); + + // What bun.lock has is what gets installed. A required dependency of the kept package is an error there. + it("GitHub: with an empty cache, `bun update` fails like `bun install` for a dependency of the kept package", async () => { + using p = await project("GitHub", { package: { dependencies: { child: "^1.0.0" } } }); + await p.register({ name: "child", version: "1.0.0" }); + const before = await p.install(); + p.lose(); + p.loseTarballOf("child"); + p.removeInstalled(); + rmSync(p.path(".bun-cache"), { recursive: true, force: true }); + for (const command of ["update", "install"]) { + const errors = [`error: GET ${p.origin}/child-1.0.0.tgz - 503`]; + expect(await p.run(command)).toMatchObject({ errors, saved: false, exitCode: 1 }); + expect(await p.written()).toEqual(before); + } + }); + + // Up to Bun 1.4.3, bun.lock does not say which entries have no package. Reading one binds such an entry to any + // package of its name. + it("GitHub: an entry that an older bun.lock binds to a registry package of its name does not keep that package", async () => { + using p = await project("GitHub", { root: { dependencies: { parent: "^1.0.0" } } }); + await p.register({ name: "parent", version: "1.0.0", dependencies: { "github-dep": "^1.0.0" } }); + await p.register({ name: "github-dep", version: "1.0.0" }); + const tarballOf = (name: string) => `${p.origin}/${name}-1.0.0.tgz`; + await writeFile( + p.path("bun.lock"), + JSON.stringify({ + lockfileVersion: 1, + configVersion: 1, + workspaces: { + "": { + name: "foo", + dependencies: { good: "^1.0.0", parent: "^1.0.0" }, + optionalDependencies: { "github-dep": "owner/repo#v1" }, + }, + "packages/member": { name: "member", version: "1.0.0" }, + }, + packages: { + "github-dep": ["github-dep@1.0.0", tarballOf("github-dep"), {}, ""], + good: ["good@1.0.0", tarballOf("good"), {}, ""], + member: ["member@workspace:packages/member"], + parent: ["parent@1.0.0", tarballOf("parent"), { dependencies: { "github-dep": "^1.0.0" } }, ""], + }, + }), + ); + const [, ...manifests] = await p.written(); + p.lose(); + expect(await p.run("update", "--latest")).toMatchObject({ + warnings: p.unread(), + errors: [], + saved: true, + exitCode: 0, + }); + // The entry has no package now, and its specifier is not the range of that package. + const [lock, ...after] = await p.written(); + expect(after).toEqual(manifests); + expect(Bun.JSONC.parse(lock)).toMatchObject({ workspaces: { "": { unresolved: ["github-dep"] } } }); + }); + + // Only a package.json of the project may name a tarball in the project. Up to Bun 1.4.3 any package could, and + // its bun.lock has the package. + it("file: tarball of a GitHub package, which an older bun.lock has, does not keep the package", async () => { + using p = await project("file: tarball", { in: "package" }); + const entry = { "file-dep": "file:./vendor/file-dep-1.0.0.tgz" }; + await writeFile( + p.path("bun.lock"), + JSON.stringify({ + lockfileVersion: 1, + configVersion: 1, + workspaces: { + "": { name: "foo", dependencies: { good: "^1.0.0", parent: "owner/parent#v1" } }, + "packages/member": { name: "member", version: "1.0.0" }, + }, + packages: { + "file-dep": ["file-dep@./vendor/file-dep-1.0.0.tgz", {}], + good: ["good@1.0.0", `${p.origin}/good-1.0.0.tgz`, {}, ""], + member: ["member@workspace:packages/member"], + parent: ["parent@github:owner/parent#def5678", { optionalDependencies: entry }, "owner-parent-def5678"], + }, + }), + ); + expect(await p.run("update", "parent")).toMatchObject({ warnings: [], errors: [], saved: true, exitCode: 0 }); + expect([await p.locked(), await p.installed()]).toEqual([[], undefined]); + }); + + // The repository was read, and it answered that the branch is gone. + it("git: a ref that the repository no longer has does not keep the package", async () => { + using p = await project("git"); + await git(p.path("repo"), "branch", "next"); + await p.declare(`git+${pathToFileURL(p.path("repo"))}#next`); + await p.install(); + await git(p.path("repo"), "branch", "-D", "next"); + expect(await p.run("update")).toMatchObject({ + warnings: [ + "warn: git failed with exit code 128", + 'warn: no commit matching "next" found for "git-dep" (but repository exists)', + ], + errors: [], + saved: true, + exitCode: 0, + }); + expect([await p.locked(), await p.installed()]).toEqual([[], undefined]); + }); +});