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 <charlie.r.marsh@gmail.com>
This commit is contained in:
konsti 2024-10-30 20:12:23 +01:00 committed by GitHub
parent 4dd36b799f
commit 4a5a79eed8
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
6 changed files with 230 additions and 18 deletions

View file

@ -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,

View file

@ -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,

View file

@ -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>,
}

View file

@ -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<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));
};
@ -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(),

View file

@ -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(),

View file

@ -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<Package>,
}
impl SourceLock {
fn sources(self) -> BTreeMap<String, toml::Value> {
fn sources(&self) -> BTreeMap<String, toml::Value> {
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(())
}