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 <[email protected]>
astral-sh · Oct 30, 2024 · 4a5a79e · 4a5a79e
1 parent 4dd36b7
commit 4a5a79e
Show file tree

Hide file tree

Showing 6 changed files with 230 additions and 18 deletions.
diff --git 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
@@ -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<Item = Result<Self, LoweringError>> + '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
@@ -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<Self, MetadataError> {
         // 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<Metadata> 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
@@ -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,22 +39,35 @@ 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<Self, MetadataError> {
-        // 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));
         };
 
         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
@@ -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(),