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)> {