-
Notifications
You must be signed in to change notification settings - Fork 5k
pack: publish workspace: directory dependencies as the workspace's version #38835
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.
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
base: farm/672d542e/pack-workspace-catalog-from-manifests
Are you sure you want to change the base?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,9 +3,11 @@ use core::fmt; | |
| use bstr::BStr; | ||
| use bun_collections::HashMap; | ||
| use bun_core::{Global, Output}; | ||
| use bun_paths::{platform, resolve_path}; | ||
| use bun_resolver::fs::FileSystem; | ||
|
|
||
| use crate::dependency::{Behavior, Tag as DependencyTag}; | ||
| use crate::lockfile::{Lockfile, Package}; | ||
| use crate::lockfile::{DependencySlice, Lockfile, Package}; | ||
| use crate::{Features, PackageNameHash}; | ||
|
|
||
| use super::PackageManager; | ||
|
|
@@ -92,6 +94,10 @@ impl ScratchManifests { | |
| /// and releases bump versions between that install and the publish. | ||
| pub struct WorkspaceManifests { | ||
| lockfile: Lockfile, | ||
| /// The root package's dependencies. The root parse turns each entry of its `workspaces` into a | ||
| /// `Behavior::WORKSPACE` dependency named after the workspace whose `workspace` value is its | ||
| /// directory relative to the root. | ||
| root_dependencies: DependencySlice, | ||
| root_package_json_path: Box<[u8]>, | ||
| } | ||
|
|
||
|
|
@@ -108,10 +114,60 @@ impl WorkspaceManifests { | |
| } | ||
| WorkspaceManifests { | ||
| lockfile: scratch.lockfile, | ||
| root_dependencies: scratch.root.dependencies, | ||
| root_package_json_path: root_package_json_path(), | ||
| } | ||
| } | ||
|
|
||
| /// Whether one of the workspaces (the root package is not one) is named `name`. This is what | ||
| /// decides, in `Package::parse_dependency`, whether the text after `workspace:` in a dependency | ||
| /// of that name is a version range or a directory. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub fn has_workspace(&self, name: &[u8]) -> bool { | ||
| let name_hash: PackageNameHash = bun_semver::string::Builder::string_hash(name); | ||
| self.lockfile.workspace_paths.contains(&name_hash) | ||
| } | ||
|
Comment on lines
+121
to
+124
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Extended reasoning...What the bug is
The code path that triggers it
Now
Why existing code doesn't prevent itThe installer's own check at Package.rs:1834 runs before the self-insertion at line 2017, so Step-by-step repro
ImpactLow, hence nit: (a) not a regression — pre-PR How to fixMirror pub fn has_workspace(&self, name: &[u8]) -> bool {
let string_buf = self.lockfile.buffers.string_bytes.as_slice();
self.root_dependencies
.get(self.lockfile.buffers.dependencies.as_slice())
.iter()
.any(|d| d.behavior.is_workspace() && d.name.slice(string_buf) == name)
}That reads the same |
||
|
|
||
| /// The name of the workspace whose directory is `path` taken relative to `package_dir`, the | ||
| /// directory of the package.json declaring `workspace:<path>`. `Package::parse_dependency` | ||
| /// links such a dependency by computing the same root-relative directory and matching it | ||
| /// against the workspaces, so this resolves exactly what `bun install` linked. `None` when no | ||
| /// workspace is in that directory. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| pub fn workspace_name_at_path(&self, package_dir: &[u8], path: &[u8]) -> Option<&[u8]> { | ||
| // `workspace:` with nothing after it is an empty range, not the declaring package's own | ||
| // directory. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| if path.is_empty() { | ||
| return None; | ||
| } | ||
| let top_level_dir = FileSystem::get().top_level_dir(); | ||
| let mut directory_buf = bun_paths::path_buffer_pool::get(); | ||
| let directory = resolve_path::join_abs_string_buf_checked::<platform::Auto>( | ||
| top_level_dir, | ||
| &mut directory_buf[..], | ||
| &[package_dir, path], | ||
| )?; | ||
| let relative_directory: &[u8] = resolve_path::relative(top_level_dir, directory); | ||
| // The workspaces' directories are stored with `/` separators on every platform. | ||
| #[cfg(windows)] | ||
| let mut posix_buf = bun_paths::path_buffer_pool::get(); | ||
| #[cfg(windows)] | ||
| let relative_directory: &[u8] = { | ||
| let len = relative_directory.len(); | ||
| posix_buf[..len].copy_from_slice(relative_directory); | ||
| bun_paths::dangerously_convert_path_to_posix_in_place::<u8>(&mut posix_buf[..len]); | ||
| &posix_buf[..len] | ||
| }; | ||
|
|
||
| let string_buf = self.lockfile.buffers.string_bytes.as_slice(); | ||
| self.root_dependencies | ||
| .get(self.lockfile.buffers.dependencies.as_slice()) | ||
| .iter() | ||
| .find(|dependency| { | ||
| dependency.behavior.is_workspace() | ||
| && dependency.version.workspace().slice(string_buf) == relative_directory | ||
| }) | ||
| .map(|dependency| dependency.name.slice(string_buf)) | ||
| } | ||
|
|
||
| /// The package.json whose `workspaces` and catalogs these are: the workspace root's when the | ||
| /// package being packed is one of its workspaces, otherwise the package's own. | ||
| pub fn root_package_json_path(&self) -> &[u8] { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2131,7 +2131,8 @@ pub(crate) fn pack<const FOR_PUBLISH: bool>( | |
| // Loading added the other workspaces' package.json files to the cache `json` points into. | ||
| json = read_package_json(manager_ptr, abs_package_json_path); | ||
| } | ||
| let edited_package_json = edit_root_package_json(workspace_manifests.as_ref(), json)?; | ||
| let edited_package_json = | ||
| edit_root_package_json(workspace_manifests.as_ref(), abs_workspace_path, json)?; | ||
|
|
||
| let root_dir: Dir = 'root_dir: { | ||
| let mut path_buf = PathBuffer::uninit(); | ||
|
|
@@ -3197,8 +3198,11 @@ fn add_archive_entry( | |
| enum Substitution<'a> { | ||
| /// `workspace:^`, `workspace:~`, `workspace:*`: the workspace's current version behind that prefix. | ||
| WorkspaceVersion { prefix: &'static str }, | ||
| /// `workspace:1.2.3`, `workspace:1.x`, ...: the range as written. | ||
| WorkspaceRange(&'a [u8]), | ||
| /// Anything else after `workspace:`. `bun install` reads it as a version range when the | ||
| /// dependency is itself one of the workspaces (`"pkg1": "workspace:1.x"`) and otherwise as the | ||
| /// directory of the workspace to link (`"c7": "workspace:../core"`), so which one it is takes | ||
| /// the manifests; see `publish_spec_for_workspace_range_or_directory`. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| WorkspaceRangeOrDirectory(&'a [u8]), | ||
| /// `catalog:` / `catalog:<name>`: that catalog's entry for the dependency. | ||
| Catalog { catalog_name: &'a [u8] }, | ||
| } | ||
|
|
@@ -3210,18 +3214,14 @@ impl<'a> Substitution<'a> { | |
| b"^" => Substitution::WorkspaceVersion { prefix: "^" }, | ||
| b"~" => Substitution::WorkspaceVersion { prefix: "~" }, | ||
| b"*" => Substitution::WorkspaceVersion { prefix: "" }, | ||
| _ => Substitution::WorkspaceRange(range), | ||
| _ => Substitution::WorkspaceRangeOrDirectory(range), | ||
| }); | ||
| } | ||
| let catalog_name = strings::without_prefix_if_possible_comptime(spec, b"catalog:")?; | ||
| Some(Substitution::Catalog { | ||
| catalog_name: strings::trim(catalog_name, &strings::WHITESPACE_CHARS), | ||
| }) | ||
| } | ||
|
|
||
| fn needs_workspace_manifests(&self) -> bool { | ||
| !matches!(self, Substitution::WorkspaceRange(_)) | ||
| } | ||
| } | ||
|
|
||
| /// Section order is the order errors get reported in. | ||
|
|
@@ -3257,15 +3257,57 @@ fn needs_workspace_manifests(package_json: Expr) -> bool { | |
| .as_ref() | ||
| .and_then(Expr::as_utf8_string_literal) | ||
| .and_then(Substitution::for_spec) | ||
| .is_some_and(|substitution| substitution.needs_workspace_manifests()); | ||
| .is_some(); | ||
| }); | ||
| needed | ||
| } | ||
|
|
||
| /// What the tarball gets for `Substitution::WorkspaceRangeOrDirectory`, or why there is nothing to | ||
| /// publish for it. The directory reading is tried first because `"pkg1": "workspace:../pkg1"` | ||
| /// satisfies both readings and only the directory has a version to publish: that workspace's own, | ||
| /// as an `npm:` alias when the dependency is declared under another name (the registry's way of | ||
| /// installing a package under a different name, and what pnpm publishes for this spec). A range is | ||
| /// copied as written; so is `workspace:<name>@<range>`, which installs the workspace `<name>` under | ||
| /// the dependency's name (the installer, too, only reads it that way when `<name>` is a workspace). | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| fn publish_spec_for_workspace_range_or_directory( | ||
| manifests: &WorkspaceManifests, | ||
| package_dir: &[u8], | ||
| dependency_name: &[u8], | ||
| spec: &[u8], | ||
| ) -> Result<Vec<u8>, String> { | ||
| let Some(workspace_name) = manifests.workspace_name_at_path(package_dir, spec) else { | ||
| let is_alias = strings::last_index_of_char(spec, b'@') | ||
| .is_some_and(|at| at > 0 && manifests.has_workspace(&spec[..at])); | ||
| if manifests.has_workspace(dependency_name) || is_alias { | ||
| return Ok(spec.to_vec()); | ||
| } | ||
| return Err(format!( | ||
| "\"{}\" has no workspace named \"{}\" and no workspace in the directory \"{}\"", | ||
| bstr::BStr::new(manifests.root_package_json_path()), | ||
| bstr::BStr::new(dependency_name), | ||
| bstr::BStr::new(spec), | ||
| )); | ||
| }; | ||
|
Comment on lines
+3262
to
+3282
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 The directory-first ordering here diverges from the installer's precedence: Extended reasoning...What the bug is
Concrete triggerRoot { "workspaces": ["a", "b"], "dependencies": { "pkg-a": "workspace:b" } }with Installer path (Package.rs:1822 → 1834 → 1938-1941): Pack path (this function): Why the existing code doesn't prevent itThe directory-first ordering was chosen so that ImpactLow. The trigger requires (1) the dependency key to be a workspace's package name, and (2) the spec after How to fixThe naive fix — check |
||
| let Some(version) = manifests.workspace_version(workspace_name) else { | ||
| return Err(format!( | ||
| "the package.json of workspace \"{}\" in the directory \"{}\" has no version", | ||
| bstr::BStr::new(workspace_name), | ||
| bstr::BStr::new(spec), | ||
| )); | ||
| }; | ||
| Ok(if workspace_name == dependency_name { | ||
| format!("{version}").into_bytes() | ||
| } else { | ||
| format!("npm:{}@{version}", bstr::BStr::new(workspace_name)).into_bytes() | ||
| }) | ||
| } | ||
|
|
||
| /// Edits `json.root` in place (`bun publish` sends that tree to the registry) and returns it printed. | ||
| /// `workspace_manifests` is `Some` whenever `needs_workspace_manifests(json.root)` is. | ||
| /// `workspace_manifests` is `Some` whenever `needs_workspace_manifests(json.root)` is. `package_dir` | ||
| /// is the directory of `json`, which `workspace:<directory>` specs are relative to. | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code |
||
| fn edit_root_package_json( | ||
| workspace_manifests: Option<&WorkspaceManifests>, | ||
| package_dir: &[u8], | ||
| json: &mut WorkspacePackageJSONCache::MapEntry, | ||
| ) -> Result<Box<[u8]>, AllocError> { | ||
| let bump = pack_bump(); | ||
|
|
@@ -3303,7 +3345,6 @@ fn edit_root_package_json( | |
|
|
||
| // `E::EString::init` keeps a pointer to the bytes, so they go into the pack arena. | ||
| let replacement: &[u8] = match substitution { | ||
| Substitution::WorkspaceRange(range) => bump.alloc_slice_copy(range), | ||
| Substitution::WorkspaceVersion { prefix } => { | ||
| let Some(version) = manifests().workspace_version(dependency_name) else { | ||
| fail( | ||
|
|
@@ -3317,6 +3358,17 @@ fn edit_root_package_json( | |
| }; | ||
| bump.alloc_slice_copy(format!("{prefix}{version}").as_bytes()) | ||
| } | ||
| Substitution::WorkspaceRangeOrDirectory(spec) => { | ||
| match publish_spec_for_workspace_range_or_directory( | ||
| manifests(), | ||
| package_dir, | ||
| dependency_name, | ||
| spec, | ||
| ) { | ||
| Ok(published) => bump.alloc_slice_copy(&published), | ||
| Err(why) => fail("workspace", format_args!("{why}")), | ||
| } | ||
| } | ||
| Substitution::Catalog { catalog_name } => { | ||
| match manifests().catalog_version(catalog_name, dependency_name) { | ||
| Some(version) => bump.alloc_slice_copy(version), | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you need a paragraph-long comment to justify why the workaround is OK, the code is wrong — fix the code