-
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 all commits
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 |
|---|---|---|
|
|
@@ -2131,7 +2131,8 @@ | |
| // 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,8 @@ | |
| 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]), | ||
| /// `workspace:1.x` on a dependency that is a workspace, or a directory, `workspace:../core`. | ||
| WorkspaceRangeOrDirectory(&'a [u8]), | ||
| /// `catalog:` / `catalog:<name>`: that catalog's entry for the dependency. | ||
| Catalog { catalog_name: &'a [u8] }, | ||
| } | ||
|
|
@@ -3210,18 +3211,14 @@ | |
| 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 +3254,52 @@ | |
| .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 | ||
| } | ||
|
|
||
| /// Directory first: `"pkg1": "workspace:../pkg1"` reads both ways; only a directory has a version. | ||
| 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 { | ||
| // `workspace:<name>@<range>` installs workspace `<name>` under `dependency_name`. | ||
| 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), | ||
| )); | ||
| }; | ||
|
Check warning on line 3282 in src/runtime/cli/pack_command.rs
|
||
|
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 { | ||
| // What pnpm publishes too: the alias installs the workspace's package under this name. | ||
| 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. | ||
| 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 +3337,6 @@ | |
|
|
||
| // `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 +3350,17 @@ | |
| }; | ||
| 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.
🟡
has_workspacehas a false-positive hole:workspace_pathsis not just the glob-matched workspace names —Package::parse_dependency'sfeatures.is_mainbranch (Package.rs:2017) also inserts the root's ownworkspace:-tagged dependency keys into it. So when packing the root with"nope": "workspace:1.2.3"wherenopeis not a workspace,has_workspace("nope")returns true (self-inserted while parsing this very dependency) and pack copies1.2.3verbatim instead of failing. Not a regression (pre-PR copied verbatim too) and only bites when packing the root itself, but the doc comment is inaccurate and the new error path has a gap; iteratingroot_dependenciesfor aBehavior::WORKSPACEentry with the given name (mirroringworkspace_name_at_path) would be robust.Extended reasoning...
What the bug is
has_workspace(name)atworkspace_manifests.rs:121-124is documented as "Whethernameis one of the workspaces" and is implemented asself.lockfile.workspace_paths.contains(&name_hash). The assumption is thatworkspace_pathscontains exactly the names matched by the root'sworkspacesglob. That assumption is wrong:workspace_pathsalso contains the dependency keys of everyworkspace:-tagged entry in the root's owndependencies/devDependencies/peerDependencies/optionalDependencies, whether or not those keys name a real workspace.The code path that triggers it
ScratchManifests::parse_rootcallsPackage::parse_with_jsonwithFeatures::main()(resolver_hooks.rs:1252-1261:is_main=true,dependencies=true,workspaces=true).parse_with_json_impl(Package.rs:2271-2286) processes theWORKSPACESgroup first, thenDEPENDENCIESetc. When it reaches a root entry like"nope": "workspace:1.2.3":workspace_paths.get(&hash("nope"))isNone(the workspaces glob just ran andnopeis not one of them), soworkspace_pathisNone.else if features.is_main || features.is_workspacebranch (Package.rs:1959).workspace_paths.get_or_put(name_hash), which insertshash("nope")into the map.workspace_versionisNoneandfound_existingisfalse, so no error is logged; line 2069 sets the entry's value.fail_on_logged_errors()passes, andWorkspaceManifestsis built with the pollutedworkspace_paths.Now
publish_spec_for_workspace_range_or_directory(pack_command.rs:3268-3280) processes the same"nope": "workspace:1.2.3"while packing the root:workspace_name_at_path(root_dir, "1.2.3")returnsNone— thenopeentry inroot_dependencieshas theDEPENDENCIES-group behavior, notBehavior::WORKSPACE, so the.is_workspace()filter excludes it, and no real workspace lives at<root>/1.2.3.is_aliasisfalse(no@after index 0).manifests.has_workspace("nope")returns true (the self-inserted entry), so the function returnsOk("1.2.3")verbatim instead of the intendedErr("…has no workspace named \"nope\"…").Why existing code doesn't prevent it
The installer's own check at Package.rs:1834 runs before the self-insertion at line 2017, so
bun installcorrectly seesnopeas not-a-workspace and later fails resolution. But pack'shas_workspaceruns after the whole root parse, when the self-insertion has already happened — a temporal mismatch.workspace_name_at_pathis unaffected because it filters onBehavior::WORKSPACE, which the self-inserted entry lacks; only theworkspace_paths.containscheck is fooled. ThenotAWorkspaceSpecstests don't catch this because they packpkgs/ui(a member), andparse_rootnever processes a member's dependencies withis_main=true.Step-by-step repro
parse_root: WORKSPACES pass insertshash("pkg1"); DEPENDENCIES pass hits Package.rs:2017 and insertshash("nope"). No error logged.edit_root_package_json→WorkspaceRangeOrDirectory("1.2.3")→publish_spec_for_workspace_range_or_directory:workspace_name_at_path(root, "1.2.3")→Noneis_alias→falsehas_workspace("nope")→true(false positive)Ok(b"1.2.3")"nope": "1.2.3"and pack exits 0.bun installon this same root would have failed, so the PR's stated invariant ("pack fails whenbun installwould fail on the same spec") is violated for this case.Impact
Low, hence nit: (a) not a regression — pre-PR
WorkspaceRangealso copied1.2.3verbatim, so the published output is byte-identical to before; (b) only reachable when packing the root package (roots are usuallyprivateand rarely packed); (c) the input configuration already failsbun install, so nobody with a green install hits it. The concrete defects are that the new error detection has a hole for exactly the case thenotAWorkspaceSpecstests target (just at the root instead of a member), and the doc comment onhas_workspaceis inaccurate.How to fix
Mirror
workspace_name_at_path: iterateself.root_dependenciesfor an entry whosebehavior.is_workspace()and whose name matches, instead of consultingworkspace_paths:That reads the same
Behavior::WORKSPACEentriesworkspace_name_at_pathalready trusts, so both lookups agree on what counts as a workspace. Alternatively,workspace_versions(populated only from the glob walk) would work as the membership set.