fix: javascript workspace bugs #1309
Conversation
And add errors for cases where a metadata.dist exists but is being ignored.
|
currently missing: a checked in test that stresses this (did some local edits to axolotlsay-hybrid to smoke test, but we need a real test before landing) |
| /// Path to a dist.toml overriding this package's contents | ||
| /// | ||
| /// NOTE: this WILL be the same path as the manifest_path for a pure dist.toml | ||
| /// package. Don't assume these are disjoint! |
There was a problem hiding this comment.
is there a reason people might assume they are disjoint beyond that they are 2 diff vars? and if so what's the consequence? besides... being wrong
There was a problem hiding this comment.
Code like init which loads up some state, edits it, and then writes it back could go sideways if it's too clever about batching edits / caching state, but forgets that these "two" files are the same one.
This is not a problem with the current impl.
| @@ -465,6 +465,18 @@ pub enum DistError { | |||
| /// Error messsage details | |||
| message: String, | |||
| }, | |||
|
|
|||
| /// Something has metadata.dist but shouldn't | |||
| #[error("The metadata.dist entry in this Cargo.toml isn't being used:\n{manifest_path}")] | |||
| @@ -846,6 +846,22 @@ impl<'pkg_graph> DistGraphBuilder<'pkg_graph> { | |||
| ) -> DistResult<Self> { | |||
| let root_workspace_idx = workspaces.root_workspace_idx(); | |||
| let root_workspace = workspaces.workspace(root_workspace_idx); | |||
|
|
|||
| // Complain if someone still has [workspace.metadata.dist] in a dist-workspace.toml scenario | |||
| if let Some(dist_manifest_path) = root_workspace.dist_manifest_path.as_deref() { | |||
There was a problem hiding this comment.
are we turning this on now?
There was a problem hiding this comment.
This code only runs if someone has migrated to dist-workspace.toml, so this is basically just erroring on "it looks like you started a migration but I see some gunk you missed".
Gankra
changed the title
axoproject/src/generic.rs
Outdated
| fn merge_package_with_raw_generic( | ||
| package: &mut PackageInfo, | ||
| generic: Package, | ||
| generic_path: Utf8PathBuf, |
There was a problem hiding this comment.
Clarifying what generic_path here is in a comment might be nice
| /// Path to the dist.toml or dist-workspace.toml for the workspace | ||
| /// | ||
| /// If this is ever set, then this will be the same as manifest_path. | ||
| pub dist_manifest_path: Option<Utf8PathBuf>, |
There was a problem hiding this comment.
Is it true that this is the same as manifest_path for a package.json/Cargo.toml workspace?
There was a problem hiding this comment.
The "if this is ever set" clause is doing heavy lifting here. Basically this option is morally a boolean saying manifest_path might be a dist(-workspace).toml, but I decided to make it an option to make code that deals with the pacakge version of dist_manifest_path uniform.
There was a problem hiding this comment.
Or i guess to put it another way -- if this is ever set, it's shared/generic workspace.
Use the [dist] section package dist.tomls, and add errors for cases where a metadata.dist exists but is being ignored.
Also handles npm package scopes better.
Also handles url equality checks better.
fixes #1308
fixes #1307