From 4a5a79eed8ba1f1a7f98a2a4a30c2b84c2f99350 Mon Sep 17 00:00:00 2001 From: konsti Date: Wed, 30 Oct 2024 20:12:23 +0100 Subject: [PATCH] Support transitive dependencies in Git workspaces (#8665) When resolving workspace dependencies (from one workspace member to another) from a workspace that's in git, we need to emit these transitive dependencies as git dependencies, not path dependencies as all other workspace deps. This fixes a bug where we would treat them as path dependencies inside the checkout directory, leading either to clashes (between a local path and another direct git dependency) or invalid lockfiles (referencing the checkout dir in the lockfile when we should be referencing the git repo). Fixes #8087 Fixes #4920 Fixes #3936 since we needed that information anyway --------- Co-authored-by: Charlie Marsh --- crates/uv-build-frontend/src/lib.rs | 2 + .../uv-distribution/src/metadata/lowering.rs | 26 ++- crates/uv-distribution/src/metadata/mod.rs | 24 ++- .../src/metadata/requires_dist.rs | 25 ++- crates/uv-distribution/src/source/mod.rs | 15 +- crates/uv/tests/it/workspace.rs | 156 +++++++++++++++++- 6 files changed, 230 insertions(+), 18 deletions(-) diff --git a/crates/uv-build-frontend/src/lib.rs b/crates/uv-build-frontend/src/lib.rs index ca7eded39348..ee3bea5b89de 100644 --- a/crates/uv-build-frontend/src/lib.rs +++ b/crates/uv-build-frontend/src/lib.rs @@ -474,6 +474,7 @@ impl SourceBuild { let requires_dist = RequiresDist::from_project_maybe_workspace( requires_dist, install_path, + None, locations, source_strategy, LowerBound::Allow, @@ -927,6 +928,7 @@ async fn create_pep517_build_environment( let requires_dist = RequiresDist::from_project_maybe_workspace( requires_dist, install_path, + None, locations, source_strategy, LowerBound::Allow, diff --git a/crates/uv-distribution/src/metadata/lowering.rs b/crates/uv-distribution/src/metadata/lowering.rs index a081c37a562d..71a093f9f279 100644 --- a/crates/uv-distribution/src/metadata/lowering.rs +++ b/crates/uv-distribution/src/metadata/lowering.rs @@ -1,9 +1,11 @@ -use either::Either; use std::collections::BTreeMap; use std::io; use std::path::{Path, PathBuf}; + +use either::Either; use thiserror::Error; use url::Url; + use uv_configuration::LowerBound; use uv_distribution_filename::DistExtension; use uv_distribution_types::{Index, IndexLocations, IndexName, Origin}; @@ -16,6 +18,8 @@ use uv_warnings::warn_user_once; use uv_workspace::pyproject::{PyProjectToml, Source, Sources}; use uv_workspace::Workspace; +use crate::metadata::GitWorkspaceMember; + #[derive(Debug, Clone)] pub struct LoweredRequirement(Requirement); @@ -38,6 +42,7 @@ impl LoweredRequirement { locations: &'data IndexLocations, workspace: &'data Workspace, lower_bound: LowerBound, + git_member: Option<&'data GitWorkspaceMember<'data>>, ) -> impl Iterator> + 'data { let (source, origin) = if let Some(source) = project_sources.get(&requirement.name) { (Some(source), RequirementOrigin::Project) @@ -216,7 +221,24 @@ impl LoweredRequirement { )) })?; - let source = if member.pyproject_toml().is_package() { + let source = if let Some(git_member) = &git_member { + // If the workspace comes from a git dependency, all workspace + // members need to be git deps, too. + let subdirectory = + uv_fs::relative_to(member.root(), git_member.fetch_root) + .expect("Workspace member must be relative"); + RequirementSource::Git { + repository: git_member.git_source.git.repository().clone(), + reference: git_member.git_source.git.reference().clone(), + precise: git_member.git_source.git.precise(), + subdirectory: if subdirectory == PathBuf::new() { + None + } else { + Some(subdirectory) + }, + url, + } + } else if member.pyproject_toml().is_package() { RequirementSource::Directory { install_path, url, diff --git a/crates/uv-distribution/src/metadata/mod.rs b/crates/uv-distribution/src/metadata/mod.rs index 26b20d5f8acd..05ed1ec6aa6e 100644 --- a/crates/uv-distribution/src/metadata/mod.rs +++ b/crates/uv-distribution/src/metadata/mod.rs @@ -4,7 +4,7 @@ use std::path::Path; use thiserror::Error; use uv_configuration::{LowerBound, SourceStrategy}; -use uv_distribution_types::IndexLocations; +use uv_distribution_types::{GitSourceUrl, IndexLocations}; use uv_normalize::{ExtraName, GroupName, PackageName}; use uv_pep440::{Version, VersionSpecifiers}; use uv_pypi_types::{HashDigest, ResolutionMetadata}; @@ -65,23 +65,26 @@ impl Metadata { pub async fn from_workspace( metadata: ResolutionMetadata, install_path: &Path, + git_source: Option<&GitWorkspaceMember<'_>>, locations: &IndexLocations, sources: SourceStrategy, bounds: LowerBound, ) -> Result { // Lower the requirements. + let requires_dist = uv_pypi_types::RequiresDist { + name: metadata.name, + requires_dist: metadata.requires_dist, + provides_extras: metadata.provides_extras, + }; let RequiresDist { name, requires_dist, provides_extras, dependency_groups, } = RequiresDist::from_project_maybe_workspace( - uv_pypi_types::RequiresDist { - name: metadata.name, - requires_dist: metadata.requires_dist, - provides_extras: metadata.provides_extras, - }, + requires_dist, install_path, + git_source, locations, sources, bounds, @@ -133,3 +136,12 @@ impl From for ArchiveMetadata { } } } + +/// A workspace member from a checked-out Git repo. +#[derive(Debug, Clone)] +pub struct GitWorkspaceMember<'a> { + /// The root of the checkout, which may be the root of the workspace or may be above the + /// workspace root. + pub fetch_root: &'a Path, + pub git_source: &'a GitSourceUrl<'a>, +} diff --git a/crates/uv-distribution/src/metadata/requires_dist.rs b/crates/uv-distribution/src/metadata/requires_dist.rs index 0f8962e97898..8814d571950c 100644 --- a/crates/uv-distribution/src/metadata/requires_dist.rs +++ b/crates/uv-distribution/src/metadata/requires_dist.rs @@ -1,7 +1,7 @@ use std::collections::BTreeMap; use std::path::Path; -use crate::metadata::{LoweredRequirement, MetadataError}; +use crate::metadata::{GitWorkspaceMember, LoweredRequirement, MetadataError}; use crate::Metadata; use uv_configuration::{LowerBound, SourceStrategy}; use uv_distribution_types::IndexLocations; @@ -39,15 +39,27 @@ impl RequiresDist { pub async fn from_project_maybe_workspace( metadata: uv_pypi_types::RequiresDist, install_path: &Path, + git_member: Option<&GitWorkspaceMember<'_>>, locations: &IndexLocations, sources: SourceStrategy, lower_bound: LowerBound, ) -> Result { - // TODO(konsti): Limit discovery for Git checkouts to Git root. // TODO(konsti): Cache workspace discovery. + let discovery_options = if let Some(git_member) = &git_member { + DiscoveryOptions { + stop_discovery_at: Some( + git_member + .fetch_root + .parent() + .expect("git checkout has a parent"), + ), + ..Default::default() + } + } else { + DiscoveryOptions::default() + }; let Some(project_workspace) = - ProjectWorkspace::from_maybe_project_root(install_path, &DiscoveryOptions::default()) - .await? + ProjectWorkspace::from_maybe_project_root(install_path, &discovery_options).await? else { return Ok(Self::from_metadata23(metadata)); }; @@ -55,6 +67,7 @@ impl RequiresDist { Self::from_project_workspace( metadata, &project_workspace, + git_member, locations, sources, lower_bound, @@ -64,6 +77,7 @@ impl RequiresDist { fn from_project_workspace( metadata: uv_pypi_types::RequiresDist, project_workspace: &ProjectWorkspace, + git_member: Option<&GitWorkspaceMember<'_>>, locations: &IndexLocations, source_strategy: SourceStrategy, lower_bound: LowerBound, @@ -142,6 +156,7 @@ impl RequiresDist { locations, project_workspace.workspace(), lower_bound, + git_member, ) .map(move |requirement| { match requirement { @@ -196,6 +211,7 @@ impl RequiresDist { locations, project_workspace.workspace(), lower_bound, + git_member, ) .map(move |requirement| match requirement { Ok(requirement) => Ok(requirement.into_inner()), @@ -266,6 +282,7 @@ mod test { Ok(RequiresDist::from_project_workspace( requires_dist, &project_workspace, + None, &IndexLocations::default(), SourceStrategy::default(), LowerBound::default(), diff --git a/crates/uv-distribution/src/source/mod.rs b/crates/uv-distribution/src/source/mod.rs index 4e408350936c..b3bbbe94a843 100644 --- a/crates/uv-distribution/src/source/mod.rs +++ b/crates/uv-distribution/src/source/mod.rs @@ -33,7 +33,7 @@ use zip::ZipArchive; use crate::distribution_database::ManagedClient; use crate::error::Error; -use crate::metadata::{ArchiveMetadata, Metadata}; +use crate::metadata::{ArchiveMetadata, GitWorkspaceMember, Metadata}; use crate::reporter::Facade; use crate::source::built_wheel_metadata::BuiltWheelMetadata; use crate::source::revision::Revision; @@ -389,6 +389,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { let requires_dist = RequiresDist::from_project_maybe_workspace( requires_dist, project_root, + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1083,6 +1084,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, resource.install_path.as_ref(), + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1119,6 +1121,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, resource.install_path.as_ref(), + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1150,6 +1153,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, resource.install_path.as_ref(), + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1197,6 +1201,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, resource.install_path.as_ref(), + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1378,10 +1383,15 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { if let Some(metadata) = Self::read_static_metadata(source, fetch.path(), resource.subdirectory).await? { + let git_member = GitWorkspaceMember { + fetch_root: fetch.path(), + git_source: resource, + }; return Ok(ArchiveMetadata::from( Metadata::from_workspace( metadata, &path, + Some(&git_member), self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1410,6 +1420,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, &path, + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1442,6 +1453,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, &path, + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), @@ -1489,6 +1501,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { Metadata::from_workspace( metadata, fetch.path(), + None, self.build_context.locations(), self.build_context.sources(), self.build_context.bounds(), diff --git a/crates/uv/tests/it/workspace.rs b/crates/uv/tests/it/workspace.rs index 368a7c1420dc..2b70a5a1b208 100644 --- a/crates/uv/tests/it/workspace.rs +++ b/crates/uv/tests/it/workspace.rs @@ -708,21 +708,21 @@ fn workspace_lock_idempotence_virtual_workspace() -> Result<()> { } /// Extract just the sources from the lockfile, to test path resolution. -#[derive(Deserialize, Serialize)] +#[derive(Deserialize, Serialize, Debug, PartialEq)] struct SourceLock { package: Vec, } impl SourceLock { - fn sources(self) -> BTreeMap { + fn sources(&self) -> BTreeMap { self.package - .into_iter() - .map(|package| (package.name, package.source)) + .iter() + .map(|package| (package.name.clone(), package.source.clone())) .collect() } } -#[derive(Deserialize, Serialize)] +#[derive(Deserialize, Serialize, Debug, PartialEq)] struct Package { name: String, source: toml::Value, @@ -1761,3 +1761,149 @@ fn test_path_hopping() -> Result<()> { Ok(()) } + +/// `c` is a package in a git workspace, and it has a workspace dependency to `d`. Check that we +/// are correctly resolving `d` to a git dependency with a subdirectory and not relative to the +/// checkout directory. +#[test] +#[cfg(not(windows))] +fn transitive_dep_in_git_workspace_no_root() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#" + [project] + name = "a" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["c"] + + [tool.uv.sources] + c = { git = "https://github.com/astral-sh/workspace-virtual-root-test", subdirectory = "packages/c", rev = "fac39c8d4c5d0ef32744e2bb309bbe34a759fd46" } + "# + )?; + + context.lock().assert().success(); + + let lock1: SourceLock = + toml::from_str(&fs_err::read_to_string(context.temp_dir.child("uv.lock"))?)?; + + // TODO(charlie): Fails on Windows due to non-portable subdirectory path in URL. + assert_json_snapshot!(lock1.sources(), @r###" + { + "a": { + "virtual": "." + }, + "anyio": { + "registry": "https://pypi.org/simple" + }, + "c": { + "git": "https://github.com/astral-sh/workspace-virtual-root-test?subdirectory=packages%2Fc&rev=fac39c8d4c5d0ef32744e2bb309bbe34a759fd46#fac39c8d4c5d0ef32744e2bb309bbe34a759fd46" + }, + "d": { + "git": "https://github.com/astral-sh/workspace-virtual-root-test?subdirectory=packages%2Fd&rev=fac39c8d4c5d0ef32744e2bb309bbe34a759fd46#fac39c8d4c5d0ef32744e2bb309bbe34a759fd46" + }, + "idna": { + "registry": "https://pypi.org/simple" + }, + "sniffio": { + "registry": "https://pypi.org/simple" + } + } + "###); + + // Check that we don't report a conflict here either. + pyproject_toml.write_str( + r#" + [project] + name = "a" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["c", "d"] + + [tool.uv.sources] + c = { git = "https://github.com/astral-sh/workspace-virtual-root-test", subdirectory = "packages/c", rev = "fac39c8d4c5d0ef32744e2bb309bbe34a759fd46" } + d = { git = "https://github.com/astral-sh/workspace-virtual-root-test", subdirectory = "packages/d", rev = "fac39c8d4c5d0ef32744e2bb309bbe34a759fd46" } + "# + )?; + + context.lock().assert().success(); + + let lock2: SourceLock = + toml::from_str(&fs_err::read_to_string(context.temp_dir.child("uv.lock"))?)?; + + assert_eq!(lock1, lock2, "sources changed"); + + Ok(()) +} + +/// `workspace-member-in-subdir` is a package in a git workspace, and it has a workspace dependency +/// to `uv-git-workspace-in-root`. Check that we are correctly resolving `uv-git-workspace-in-root` +/// to a git dependency without a subdirectory and not relative to the checkout directory. +#[test] +fn transitive_dep_in_git_workspace_with_root() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#" + [project] + name = "git-with-root" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = [ + "workspace-member-in-subdir", + ] + + [tool.uv.sources] + workspace-member-in-subdir = { git = "https://github.com/astral-sh/workspace-in-root-test", subdirectory = "workspace-member-in-subdir", rev = "d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68" } + "# + )?; + + context.lock().assert().success(); + + let lock1: SourceLock = + toml::from_str(&fs_err::read_to_string(context.temp_dir.child("uv.lock"))?)?; + assert_json_snapshot!(lock1.sources(), @r###" + { + "git-with-root": { + "virtual": "." + }, + "uv-git-workspace-in-root": { + "git": "https://github.com/astral-sh/workspace-in-root-test?rev=d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68#d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68" + }, + "workspace-member-in-subdir": { + "git": "https://github.com/astral-sh/workspace-in-root-test?subdirectory=workspace-member-in-subdir&rev=d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68#d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68" + } + } + "###); + + // Check that we don't report a conflict here either + pyproject_toml.write_str( + r#" + [project] + name = "git-with-root" + version = "0.1.0" + description = "Add your description here" + readme = "README.md" + requires-python = ">=3.12" + dependencies = [ + "workspace-member-in-subdir", + "uv-git-workspace-in-root", + ] + + [tool.uv.sources] + workspace-member-in-subdir = { git = "https://github.com/astral-sh/workspace-in-root-test", subdirectory = "workspace-member-in-subdir", rev = "d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68" } + uv-git-workspace-in-root = { git = "https://github.com/astral-sh/workspace-in-root-test", rev = "d3ab48d2338296d47e28dbb2fb327c5e2ac4ac68" } + "# + )?; + + context.lock().assert().success(); + let lock2: SourceLock = + toml::from_str(&fs_err::read_to_string(context.temp_dir.child("uv.lock"))?)?; + + assert_eq!(lock1, lock2, "sources changed"); + + Ok(()) +}