From 7b4197bc0e8801781df707e99fc3a0cc54ce0d0c Mon Sep 17 00:00:00 2001 From: Micha Reiser Date: Fri, 15 Nov 2024 05:03:00 +0100 Subject: [PATCH] Detect nested workspace inside the current workspace and members with identical names (#9094) ## Summary Align uv's workspace discovery with red knots (see https://github.com/astral-sh/ruff/pull/14308#issuecomment-2474296308) * Detect nested workspace inside the current workspace rather than testing if the current workspace is a member of any outer workspace. * Detect packages with identical names. ## Test Plan I added two integration tests. I also back ported the tests to main to verify that both these invalid workspaces aren't catched by uv today. That makes this, technically, a breaking change but I would consider the change a bug fix. --------- Co-authored-by: Charlie Marsh --- crates/uv-workspace/src/lib.rs | 4 +- crates/uv-workspace/src/workspace.rs | 141 +++++++-------------- crates/uv-workspace/src/workspace/tests.rs | 133 +++++++++++++++++-- 3 files changed, 169 insertions(+), 109 deletions(-) diff --git a/crates/uv-workspace/src/lib.rs b/crates/uv-workspace/src/lib.rs index 1fde70b4a..bd435c1f1 100644 --- a/crates/uv-workspace/src/lib.rs +++ b/crates/uv-workspace/src/lib.rs @@ -1,6 +1,6 @@ pub use workspace::{ - check_nested_workspaces, DiscoveryOptions, MemberDiscovery, ProjectWorkspace, VirtualProject, - Workspace, WorkspaceError, WorkspaceMember, + DiscoveryOptions, MemberDiscovery, ProjectWorkspace, VirtualProject, Workspace, WorkspaceError, + WorkspaceMember, }; pub mod dependency_groups; diff --git a/crates/uv-workspace/src/workspace.rs b/crates/uv-workspace/src/workspace.rs index 668c42042..b6260e6c4 100644 --- a/crates/uv-workspace/src/workspace.rs +++ b/crates/uv-workspace/src/workspace.rs @@ -13,7 +13,7 @@ use uv_normalize::{GroupName, PackageName, DEV_DEPENDENCIES}; use uv_pep508::{MarkerTree, RequirementOrigin, VerbatimUrl}; use uv_pypi_types::{Conflicts, Requirement, RequirementSource, SupportedEnvironments}; use uv_static::EnvVars; -use uv_warnings::{warn_user, warn_user_once}; +use uv_warnings::warn_user_once; use crate::dependency_groups::{DependencyGroupError, FlatDependencyGroups}; use crate::pyproject::{ @@ -34,6 +34,14 @@ pub enum WorkspaceError { MissingWorkspace(PathBuf), #[error("The project is marked as unmanaged: `{}`", _0.simplified_display())] NonWorkspace(PathBuf), + #[error("Nested workspaces are not supported, but workspace member (`{}`) has a `uv.workspace` table", _0.simplified_display())] + NestedWorkspace(PathBuf), + #[error("Two workspace members are both named: `{name}`: `{}` and `{}`", first.simplified_display(), second.simplified_display())] + DuplicatePackage { + name: PackageName, + first: PathBuf, + second: PathBuf, + }, #[error("pyproject.toml section is declared as dynamic, but must be static: `{0}`")] DynamicNotAllowed(&'static str), #[error("Failed to find directories for glob: `{0}`")] @@ -185,8 +193,6 @@ impl Workspace { workspace_root.simplified_display() ); - check_nested_workspaces(&workspace_root, options); - // Unlike in `ProjectWorkspace` discovery, we might be in a legacy non-project root without // being in any specific project. let current_project = pyproject_toml @@ -635,14 +641,20 @@ impl Workspace { ); seen.insert(workspace_root.clone()); - workspace_members.insert( + if let Some(existing) = workspace_members.insert( project.name.clone(), WorkspaceMember { root: workspace_root.clone(), project: project.clone(), pyproject_toml, }, - ); + ) { + return Err(WorkspaceError::DuplicatePackage { + name: project.name.clone(), + first: existing.root.clone(), + second: workspace_root, + }); + } }; } @@ -766,16 +778,39 @@ impl Workspace { "Adding discovered workspace member: `{}`", member_root.simplified_display() ); - workspace_members.insert( + + if let Some(existing) = workspace_members.insert( project.name.clone(), WorkspaceMember { root: member_root.clone(), project, pyproject_toml, }, - ); + ) { + return Err(WorkspaceError::DuplicatePackage { + name: existing.project.name, + first: existing.root.clone(), + second: member_root, + }); + } } } + + // Test for nested workspaces. + for member in workspace_members.values() { + if member.root() != &workspace_root + && member + .pyproject_toml + .tool + .as_ref() + .and_then(|tool| tool.uv.as_ref()) + .and_then(|uv| uv.workspace.as_ref()) + .is_some() + { + return Err(WorkspaceError::NestedWorkspace(member.root.clone())); + } + } + let workspace_sources = workspace_pyproject_toml .tool .clone() @@ -783,6 +818,7 @@ impl Workspace { .and_then(|uv| uv.sources) .map(ToolUvSources::into_inner) .unwrap_or_default(); + let workspace_indexes = workspace_pyproject_toml .tool .clone() @@ -1222,95 +1258,6 @@ async fn find_workspace( Ok(None) } -/// Warn when the valid workspace is included in another workspace. -pub fn check_nested_workspaces(inner_workspace_root: &Path, options: &DiscoveryOptions) { - for outer_workspace_root in inner_workspace_root - .ancestors() - .take_while(|path| { - // Only walk up the given directory, if any. - options - .stop_discovery_at - .and_then(Path::parent) - .map(|stop_discovery_at| stop_discovery_at != *path) - .unwrap_or(true) - }) - .skip(1) - { - let pyproject_toml_path = outer_workspace_root.join("pyproject.toml"); - if !pyproject_toml_path.is_file() { - continue; - } - let contents = match fs_err::read_to_string(&pyproject_toml_path) { - Ok(contents) => contents, - Err(err) => { - warn!( - "Unreadable pyproject.toml `{}`: {err}", - pyproject_toml_path.simplified_display() - ); - return; - } - }; - let pyproject_toml: PyProjectToml = match toml::from_str(&contents) { - Ok(contents) => contents, - Err(err) => { - warn!( - "Invalid pyproject.toml `{}`: {err}", - pyproject_toml_path.simplified_display() - ); - return; - } - }; - - if let Some(workspace) = pyproject_toml - .tool - .as_ref() - .and_then(|tool| tool.uv.as_ref()) - .and_then(|uv| uv.workspace.as_ref()) - { - let is_included = match is_included_in_workspace( - inner_workspace_root, - outer_workspace_root, - workspace, - ) { - Ok(contents) => contents, - Err(err) => { - warn!( - "Invalid pyproject.toml `{}`: {err}", - pyproject_toml_path.simplified_display() - ); - return; - } - }; - - let is_excluded = match is_excluded_from_workspace( - inner_workspace_root, - outer_workspace_root, - workspace, - ) { - Ok(contents) => contents, - Err(err) => { - warn!( - "Invalid pyproject.toml `{}`: {err}", - pyproject_toml_path.simplified_display() - ); - return; - } - }; - - if is_included && !is_excluded { - warn_user!( - "Nested workspaces are not supported, but outer workspace (`{}`) includes `{}`", - outer_workspace_root.simplified_display().cyan(), - inner_workspace_root.simplified_display().cyan() - ); - } - } - - // We're in the examples or tests of another project (not a workspace), this is fine. - return; - } -} - /// Check if we're in the `tool.uv.workspace.excluded` of a workspace. fn is_excluded_from_workspace( project_path: &Path, @@ -1423,8 +1370,6 @@ impl VirtualProject { .map_err(WorkspaceError::Normalize)? .clone(); - check_nested_workspaces(&project_path, options); - let workspace = Workspace::collect_members( project_path, workspace.clone(), diff --git a/crates/uv-workspace/src/workspace/tests.rs b/crates/uv-workspace/src/workspace/tests.rs index a2a0b66d0..dab98ba7b 100644 --- a/crates/uv-workspace/src/workspace/tests.rs +++ b/crates/uv-workspace/src/workspace/tests.rs @@ -5,12 +5,13 @@ use std::str::FromStr; use anyhow::Result; use assert_fs::fixture::ChildPath; use assert_fs::prelude::*; -use insta::assert_json_snapshot; +use insta::{assert_json_snapshot, assert_snapshot}; use uv_normalize::GroupName; use crate::pyproject::{DependencyGroupSpecifier, PyProjectToml}; use crate::workspace::{DiscoveryOptions, ProjectWorkspace}; +use crate::WorkspaceError; async fn workspace_test(folder: &str) -> (ProjectWorkspace, String) { let root_dir = env::current_dir() @@ -28,12 +29,15 @@ async fn workspace_test(folder: &str) -> (ProjectWorkspace, String) { (project, root_escaped) } -async fn temporary_test(folder: &Path) -> (ProjectWorkspace, String) { +async fn temporary_test( + folder: &Path, +) -> Result<(ProjectWorkspace, String), (WorkspaceError, String)> { + let root_escaped = regex::escape(folder.to_string_lossy().as_ref()); let project = ProjectWorkspace::discover(folder, &DiscoveryOptions::default()) .await - .unwrap(); - let root_escaped = regex::escape(folder.to_string_lossy().as_ref()); - (project, root_escaped) + .map_err(|error| (error, root_escaped.clone()))?; + + Ok((project, root_escaped)) } #[tokio::test] @@ -467,7 +471,7 @@ async fn exclude_package() -> Result<()> { .child("__init__.py") .touch()?; - let (project, root_escaped) = temporary_test(root.as_ref()).await; + let (project, root_escaped) = temporary_test(root.as_ref()).await.unwrap(); let filters = vec![(root_escaped.as_str(), "[ROOT]")]; insta::with_settings!({filters => filters}, { assert_json_snapshot!( @@ -570,7 +574,7 @@ async fn exclude_package() -> Result<()> { )?; // `bird-feeder` should still be excluded. - let (project, root_escaped) = temporary_test(root.as_ref()).await; + let (project, root_escaped) = temporary_test(root.as_ref()).await.unwrap(); let filters = vec![(root_escaped.as_str(), "[ROOT]")]; insta::with_settings!({filters => filters}, { assert_json_snapshot!( @@ -674,7 +678,7 @@ async fn exclude_package() -> Result<()> { )?; // `bird-feeder` should now be included. - let (project, root_escaped) = temporary_test(root.as_ref()).await; + let (project, root_escaped) = temporary_test(root.as_ref()).await.unwrap(); let filters = vec![(root_escaped.as_str(), "[ROOT]")]; insta::with_settings!({filters => filters}, { assert_json_snapshot!( @@ -791,7 +795,7 @@ async fn exclude_package() -> Result<()> { )?; // `bird-feeder` and `seeds` should now be excluded. - let (project, root_escaped) = temporary_test(root.as_ref()).await; + let (project, root_escaped) = temporary_test(root.as_ref()).await.unwrap(); let filters = vec![(root_escaped.as_str(), "[ROOT]")]; insta::with_settings!({filters => filters}, { assert_json_snapshot!( @@ -900,3 +904,114 @@ bar = ["b"] &[DependencyGroupSpecifier::Requirement("b".to_string())] ); } + +#[tokio::test] +async fn nested_workspace() -> Result<()> { + let root = tempfile::TempDir::new()?; + let root = ChildPath::new(root.path()); + + // Create the root. + root.child("pyproject.toml").write_str( + r#" + [project] + name = "albatross" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["tqdm>=4,<5"] + + [tool.uv.workspace] + members = ["packages/*"] + "#, + )?; + + // Create an included package (`seeds`). + root.child("packages") + .child("seeds") + .child("pyproject.toml") + .write_str( + r#" + [project] + name = "seeds" + version = "1.0.0" + requires-python = ">=3.12" + dependencies = ["idna==3.6"] + + [tool.uv.workspace] + members = ["nested_packages/*"] + "#, + )?; + + let (error, root_escaped) = temporary_test(root.as_ref()).await.unwrap_err(); + let filters = vec![(root_escaped.as_str(), "[ROOT]")]; + insta::with_settings!({filters => filters}, { + assert_snapshot!( + error, + @"Nested workspaces are not supported, but workspace member (`[ROOT]/packages/seeds`) has a `uv.workspace` table"); + }); + + Ok(()) +} + +#[tokio::test] +async fn duplicate_names() -> Result<()> { + let root = tempfile::TempDir::new()?; + let root = ChildPath::new(root.path()); + + // Create the root. + root.child("pyproject.toml").write_str( + r#" + [project] + name = "albatross" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["tqdm>=4,<5"] + + [tool.uv.workspace] + members = ["packages/*"] + "#, + )?; + + // Create an included package (`seeds`). + root.child("packages") + .child("seeds") + .child("pyproject.toml") + .write_str( + r#" + [project] + name = "seeds" + version = "1.0.0" + requires-python = ">=3.12" + dependencies = ["idna==3.6"] + + [tool.uv.workspace] + members = ["nested_packages/*"] + "#, + )?; + + // Create an included package (`seeds2`). + root.child("packages") + .child("seeds2") + .child("pyproject.toml") + .write_str( + r#" + [project] + name = "seeds" + version = "1.0.0" + requires-python = ">=3.12" + dependencies = ["idna==3.6"] + + [tool.uv.workspace] + members = ["nested_packages/*"] + "#, + )?; + + let (error, root_escaped) = temporary_test(root.as_ref()).await.unwrap_err(); + let filters = vec![(root_escaped.as_str(), "[ROOT]")]; + insta::with_settings!({filters => filters}, { + assert_snapshot!( + error, + @"Two workspace members are both named: `seeds`: `[ROOT]/packages/seeds` and `[ROOT]/packages/seeds2`"); + }); + + Ok(()) +}