From bb6696aed1268b361cd0dac223debef7b513efff Mon Sep 17 00:00:00 2001 From: Zoltan Kochan Date: Thu, 17 Sep 2026 18:33:44 +0200 Subject: [PATCH] fix(python): compare workspace paths canonically and read the workspace only under a declaration (#15031) Follow-up to pnpm/pnpm#15019 for the review items that had no inline thread when it merged. pnpm run and pnpm exec compare the configured workspace path and the command's directory with links resolved, so a workspace reached through a link still finds the environment its members share. An add without --filter reads the workspace around the edited project only when a [tool.uv.workspace] is declared at or above it, which is where a project can take siblings from the repository or share their environment. A project outside any declared workspace is read alone, as it was before, so the common add does not walk the whole workspace. Shared membership reuses the workspace each project belongs to from the scope reading, rather than walking ancestors again per member. The disagreement search compares distinct ranges rather than every member's requirement, so many members asking for one range add no pairs. Related to pnpm/pnpm#15015. --- pnpm/crates/cli/src/ecosystem_add.rs | 15 ++-- pnpm/crates/cli/tests/suite/python/shared.rs | 72 +++++++++++++++++++ pnpm/crates/python-installer/src/lib.rs | 1 + .../src/projects/disagreement.rs | 64 ++++++++++++----- pnpm/crates/python-installer/src/tests.rs | 22 +++++- .../python-installer/src/workspace/members.rs | 46 +++++++++--- 6 files changed, 184 insertions(+), 36 deletions(-) diff --git a/pnpm/crates/cli/src/ecosystem_add.rs b/pnpm/crates/cli/src/ecosystem_add.rs index 8993e19728..3f9102a58f 100644 --- a/pnpm/crates/cli/src/ecosystem_add.rs +++ b/pnpm/crates/cli/src/ecosystem_add.rs @@ -84,9 +84,10 @@ async fn cargo_add_task( /// The Python half of the add, and the projects it was resolved against. /// /// Without a `--filter` selection the add acts on the project the command -/// was run in, the way the npm add does. In a workspace it still reads the -/// other projects: what that project may take from the repository, and -/// whether it shares an environment, is declared around it. +/// was run in, the way the npm add does. Under a declared uv workspace it +/// still reads the other projects: what that project may take from the +/// repository, and whether it shares an environment, is declared around +/// it. A project outside any is read alone. async fn python_add_task( context: InstallContext, root: &Path, @@ -108,12 +109,12 @@ async fn python_add_task( } else { let project = pnpm_python_installer::writable_project(root)?; let discovery = match config.workspace_dir.clone() { - Some(workspace_root) => { + Some(workspace_root) + if pnpm_python_installer::in_declared_workspace(&workspace_root, &project) => + { python::discover_around(config, &inventory(workspace_root), &project).await? } - None => { - pnpm_python_installer::discover(config, &[project.join("pyproject.toml")]).await? - } + _ => pnpm_python_installer::discover(config, &[project.join("pyproject.toml")]).await?, }; (discovery, BTreeSet::from([project])) }; diff --git a/pnpm/crates/cli/tests/suite/python/shared.rs b/pnpm/crates/cli/tests/suite/python/shared.rs index a3835c2043..1cbfa04082 100644 --- a/pnpm/crates/cli/tests/suite/python/shared.rs +++ b/pnpm/crates/cli/tests/suite/python/shared.rs @@ -117,6 +117,23 @@ async fn members_that_cannot_be_installed_together_are_refused() { ), ); assert!(!root.path().join("pylock.toml").exists()); + + // A member asking both ranges does not hide the other member asking + // one of them. + python_project( + &root.path().join("packages/a"), + "a", + "dependencies = ['alpha==1.0', 'alpha==2.0']", + ); + python_project(&root.path().join("packages/b"), "b", "dependencies = ['alpha==1.0']"); + assert_failure_contains( + pacquet_in(root.path()).arg("install"), + &format!( + "{} requires `alpha==1.0` and {} requires `alpha==2.0`", + packages.join("b").display(), + packages.join("a").display(), + ), + ); } #[tokio::test] @@ -284,3 +301,58 @@ async fn a_shared_members_metadata_is_prepared_with_the_interpreter_the_root_ask .success() .stdout(line("0.0.1")); } + +#[tokio::test] +async fn an_add_outside_the_declared_workspace_reads_the_project_alone() { + let root = tempfile::tempdir().unwrap(); + let mut server = mockito::Server::new_async().await; + let _alpha = serve(&mut server, "alpha", &[("1.0", wheel("alpha", "1.0", "", &[]))]).await; + project(root.path(), &server.url(), &[]); + fs::remove_file(root.path().join("pyproject.toml")).unwrap(); + let plain = |name: &str| { + format!( + "[project]\nname = '{name}'\nversion = '1.0'\nrequires-python = '>=3.10'\n\ + dependencies = []\n", + ) + }; + for (directory, manifest) in [ + ( + "api", + "[tool.uv.workspace]\nmembers = ['providers/*']\n\n[tool.pnpm.python]\n\ + shared-environment = true\n" + .to_string(), + ), + ("api/providers/good", plain("good")), + ("api/providers/broken", "[project\n".to_string()), + ("tools/x", plain("x")), + ] { + fs::create_dir_all(root.path().join(directory)).unwrap(); + fs::write( + root.path() + .join(directory) + .join("pyproject.toml"), + manifest, + ) + .unwrap(); + } + let outside = root.path().join("tools/x"); + + pacquet_in(&outside) + .args(["add", "pypi:alpha"]) + .assert() + .success(); + + assert!(fs::read_to_string(outside.join("pyproject.toml")).unwrap().contains("alpha>=1.0")); + assert!(outside.join("pylock.toml").is_file(), "an environment of its own"); + assert!( + !root + .path() + .join("api/pylock.toml") + .exists(), + "the workspace was not installed", + ); + pacquet_in(&root.path().join("api/providers/good")) + .args(["add", "pypi:alpha"]) + .assert() + .failure(); +} diff --git a/pnpm/crates/python-installer/src/lib.rs b/pnpm/crates/python-installer/src/lib.rs index a61a6605a6..0ac2de9fbc 100644 --- a/pnpm/crates/python-installer/src/lib.rs +++ b/pnpm/crates/python-installer/src/lib.rs @@ -1,6 +1,7 @@ pub use add::{AddOptions, plan_add, writable_project}; pub use discovery::{Discovery, PythonProject, discover}; pub use manifest::DependencySelection; +pub use workspace::members::in_declared_workspace; mod add; mod build; diff --git a/pnpm/crates/python-installer/src/projects/disagreement.rs b/pnpm/crates/python-installer/src/projects/disagreement.rs index d993ceb896..58ed5977cb 100644 --- a/pnpm/crates/python-installer/src/projects/disagreement.rs +++ b/pnpm/crates/python-installer/src/projects/disagreement.rs @@ -4,7 +4,9 @@ //! //! pubgrub reports the conflict as a chain of terms, which names the //! project as one root. The members are what the reader has to change, so -//! the two that disagree are named ahead of that report. +//! the two that disagree are named ahead of that report. The search is +//! bounded by the distinct ranges the members ask for, not by how many +//! members ask them. use super::Member; use crate::registry::Resolution; @@ -25,28 +27,53 @@ pub(crate) fn disagreement( } } -/// A version requirement one member declares on this environment. +/// One version range members declare on this environment, and what +/// each member asking it wrote, by member. struct Asked<'a> { - root: &'a Path, - requirement: &'a Requirement, specifiers: &'a VersionSpecifiers, + by_root: BTreeMap<&'a Path, &'a Requirement>, +} + +impl<'a> Asked<'a> { + /// A member asking this range and a different member asking `other`, + /// each with the requirement it wrote, or `None` when one member + /// alone asks both. + fn distinct_roots(&self, other: &Self) -> Option<[(&'a Path, &'a Requirement); 2]> { + self.by_root + .iter() + .find_map(|(root, requirement)| { + other.by_root + .iter() + .find(|(second, _)| *second != root) + .map(|(second, theirs)| [(*root, *requirement), (*second, *theirs)]) + }) + } } fn find(resolution: &Resolution, members: &[Member]) -> Option { let environment = &resolution.target.environment; - let mut by_name = BTreeMap::<&PackageName, Vec>>::new(); + // One entry per distinct range, with every member asking it: two + // members asking the same range cannot disagree with each other, so + // the pairs compared are of distinct ranges, each reported for a + // member of its own. + let mut by_name = BTreeMap::<&PackageName, BTreeMap>>::new(); for member in members { for requirement in &member.requirements.all { let Some(VersionOrUrl::VersionSpecifier(specifiers)) = &requirement.version_or_url else { continue; }; - if requirement.marker.evaluate(environment, &[]) { - by_name - .entry(&requirement.name) - .or_default() - .push(Asked { root: &member.root, requirement, specifiers }); + if !requirement.marker.evaluate(environment, &[]) { + continue; } + by_name + .entry(&requirement.name) + .or_default() + .entry(specifiers.to_string()) + .or_insert_with(|| Asked { specifiers, by_root: BTreeMap::new() }) + .by_root + .entry(&member.root) + .or_insert(requirement); } } by_name @@ -55,6 +82,7 @@ fn find(resolution: &Resolution, members: &[Member]) -> Option { let offered = resolution.packages.candidates .get(name) .filter(|offered| !offered.is_empty())?; + let asked = asked.into_values().collect::>(); conflicting_pair(name, &asked, &offered.keys().collect::>()) }) } @@ -72,8 +100,8 @@ fn conflicting_pair( .find_map(|(position, first)| { asked[position + 1..] .iter() - .filter(|second| second.root != first.root) - .find(|second| { + .filter_map(|second| Some((second, first.distinct_roots(second)?))) + .find(|(second, _)| { !offered .iter() .any(|version| { @@ -81,15 +109,13 @@ fn conflicting_pair( && second.specifiers.contains(version) }) }) - .map(|second| { + .map(|(_, [(root, requirement), (other, theirs)])| { format!( "the Python projects sharing one environment cannot be installed together: \ - {} requires `{}` and {} requires `{}`, and no version of {name} the index \ - offers satisfies both", - first.root.display(), - first.requirement, - second.root.display(), - second.requirement, + {} requires `{requirement}` and {} requires `{theirs}`, and no version of \ + {name} the index offers satisfies both", + root.display(), + other.display(), ) }) }) diff --git a/pnpm/crates/python-installer/src/tests.rs b/pnpm/crates/python-installer/src/tests.rs index 2dd61f74a2..f5631e5dd6 100644 --- a/pnpm/crates/python-installer/src/tests.rs +++ b/pnpm/crates/python-installer/src/tests.rs @@ -334,7 +334,10 @@ const UNSHARED_ROOT: &str = "[tool.uv.workspace]\nmembers = ['libs/*']\n"; #[test] fn a_command_uses_the_environment_its_project_shares_or_its_own() { let workspace = tempfile::tempdir().expect("workspace directory"); - let root = workspace.path(); + // The lookup answers with links resolved, and a temporary directory + // may be reached through one. + let root = dunce::canonicalize(workspace.path()).expect("canonical workspace"); + let root = root.as_path(); let environment_of = |dir: &std::path::Path| super::environment_dir(Some(root), dir); for project in ["packages/app", "packages/tool", "packages/nested/libs/x", "packages/inner"] { std::fs::create_dir_all(root.join(project).join("src")).expect("project directory"); @@ -366,3 +369,20 @@ fn a_command_uses_the_environment_its_project_shares_or_its_own() { let inner = root.join("packages/inner"); assert_eq!(environment_of(&inner), inner.join(".venv"), "its own workspace root"); } + +#[test] +fn a_workspace_reached_through_a_link_still_shares_its_environment() { + let outside = tempfile::tempdir().expect("outside directory"); + let real = outside.path().join("real"); + let member = real.join("packages/app"); + std::fs::create_dir_all(&member).expect("member directory"); + std::fs::write(real.join("pyproject.toml"), SHARED_ROOT).expect("shared workspace"); + std::fs::write(member.join("pyproject.toml"), MEMBER).expect("member"); + let link = outside.path().join("link"); + pnpm_fs::force_symlink_dir(&real, &link).expect("workspace link"); + let canonical = dunce::canonicalize(&real).expect("canonical workspace"); + + assert_eq!(super::environment_dir(Some(&link), &member), canonical.join(".venv")); + assert!(super::in_declared_workspace(&link, &member)); + assert!(!super::in_declared_workspace(&link, outside.path()), "outside the workspace"); +} diff --git a/pnpm/crates/python-installer/src/workspace/members.rs b/pnpm/crates/python-installer/src/workspace/members.rs index 7427cde136..419bf05bcd 100644 --- a/pnpm/crates/python-installer/src/workspace/members.rs +++ b/pnpm/crates/python-installer/src/workspace/members.rs @@ -61,7 +61,7 @@ impl Workspace { ) -> Result<()> { let mut declared = BTreeMap::<&PackageName, &Path>::new(); for (member, manifest) in member_projects(root, declaration, projects) { - if manifest.project.is_none() || !Self::declared_in(member, projects, root) { + if manifest.project.is_none() || self.declared_by(member) != Some(root) { continue; } if let Some(name) = manifest.distribution() @@ -80,11 +80,16 @@ impl Workspace { Ok(()) } - /// Whether `root` is the workspace the project at `member` belongs - /// to: the nearest one declared at or above it. - fn declared_in(member: &Path, projects: &[(PathBuf, Arc)], root: &Path) -> bool { - Self::declaring_root(member, projects) - .is_some_and(|(declared_in, _, _)| declared_in == root) + /// The workspace the project at `member` belongs to: the nearest one + /// declared at or above it, which reading the scopes already found. + fn declared_by<'a>(&'a self, member: &'a Path) -> Option<&'a Path> { + if let Some((declared_in, _)) = self.inherited.get(member) { + return Some(declared_in); + } + self.manifests + .get(member) + .filter(|manifest| manifest.tool.uv.workspace.is_some()) + .map(|_| member) } /// The directory whose lockfile and environment the project at `root` @@ -189,14 +194,18 @@ impl Patterns { /// parse counts for nothing here and is reported by the next install. pub(crate) fn environment_root_of(workspace: Option<&Path>, dir: &Path) -> PathBuf { let Some(stop) = workspace else { return dir.to_path_buf() }; + // The workspace may be configured by a path that reaches it through + // a link, while the command's directory is canonical. + let stop = canonical(stop); + let dir = canonical(dir); let project = dir .ancestors() - .take_while(|ancestor| ancestor.starts_with(stop)) + .take_while(|ancestor| ancestor.starts_with(&stop)) .find(|ancestor| ancestor.join("pyproject.toml").is_file()) - .unwrap_or(dir); + .unwrap_or(&dir); let declared = project .ancestors() - .take_while(|ancestor| ancestor.starts_with(stop)) + .take_while(|ancestor| ancestor.starts_with(&stop)) .find_map(|ancestor| Some((ancestor, workspace_declaration(ancestor)?))); match declared { Some((root, (true, patterns))) if patterns.contain(root, project) => root.to_path_buf(), @@ -204,6 +213,25 @@ pub(crate) fn environment_root_of(workspace: Option<&Path>, dir: &Path) -> PathB } } +/// Whether a `[tool.uv.workspace]` is declared at or above `project`, up +/// to `workspace`. A project in one may take its siblings from the +/// repository and may share their environment, so an add there reads +/// the workspace around it; a project outside any reads itself alone. +#[must_use] +pub fn in_declared_workspace(workspace: &Path, project: &Path) -> bool { + let stop = canonical(workspace); + canonical(project) + .ancestors() + .take_while(|ancestor| ancestor.starts_with(&stop)) + .any(|ancestor| workspace_declaration(ancestor).is_some()) +} + +/// The path with links resolved, or as given where that fails: a path +/// that cannot be resolved is still the one the caller means. +fn canonical(path: &Path) -> PathBuf { + dunce::canonicalize(path).unwrap_or_else(|_| path.to_path_buf()) +} + /// Whether the manifest at `root` shares its environment, and which /// projects it contains, for one that declares a workspace. fn workspace_declaration(root: &Path) -> Option<(bool, Patterns)> {