Detect cyclic dependencies during builds (#10258)

## Summary

Closes
https://github.com/astral-sh/uv/issues/10255#issuecomment-2566782671.
This commit is contained in:
Charlie Marsh 2024-12-31 22:22:42 -05:00 committed by GitHub
parent 9ebd94cb93
commit 9e0b35ad82
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
11 changed files with 165 additions and 30 deletions

View File

@ -84,10 +84,12 @@ pub enum Error {
#[error("Failed to build PATH for build script")]
BuildScriptPath(#[source] env::JoinPathsError),
// For the convenience of typing `setup_build` properly.
#[error("Building source distributions for {0} is disabled")]
#[error("Building source distributions for `{0}` is disabled")]
NoSourceDistBuild(PackageName),
#[error("Building source distributions is disabled")]
NoSourceDistBuilds,
#[error("Cyclic build dependency detected for `{0}`")]
CyclicBuildDependency(PackageName),
}
impl IsBuildBackendError for Error {
@ -103,7 +105,8 @@ impl IsBuildBackendError for Error {
| Self::RequirementsInstall(_, _)
| Self::Virtualenv(_)
| Self::NoSourceDistBuild(_)
| Self::NoSourceDistBuilds => false,
| Self::NoSourceDistBuilds
| Self::CyclicBuildDependency(_) => false,
Self::CommandFailed(_, _)
| Self::BuildBackend(_)
| Self::MissingHeader(_)

View File

@ -35,7 +35,7 @@ use uv_pep508::PackageName;
use uv_pypi_types::{Requirement, VerbatimParsedUrl};
use uv_python::{Interpreter, PythonEnvironment};
use uv_static::EnvVars;
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, SourceBuildTrait};
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, BuildStack, SourceBuildTrait};
pub use crate::error::{Error, MissingHeaderCause};
@ -255,6 +255,7 @@ impl SourceBuild {
source_strategy: SourceStrategy,
config_settings: ConfigSettings,
build_isolation: BuildIsolation<'_>,
build_stack: &BuildStack,
build_kind: BuildKind,
mut environment_variables: FxHashMap<OsString, OsString>,
level: BuildOutput,
@ -318,11 +319,12 @@ impl SourceBuild {
source_build_context,
&default_backend,
&pep517_backend,
build_stack,
)
.await?;
build_context
.install(&resolved_requirements, &venv)
.install(&resolved_requirements, &venv, build_stack)
.await
.map_err(|err| Error::RequirementsInstall("`build-system.requires`", err.into()))?;
} else {
@ -378,6 +380,7 @@ impl SourceBuild {
version_id,
locations,
source_strategy,
build_stack,
build_kind,
level,
&config_settings,
@ -412,6 +415,7 @@ impl SourceBuild {
source_build_context: SourceBuildContext,
default_backend: &Pep517Backend,
pep517_backend: &Pep517Backend,
build_stack: &BuildStack,
) -> Result<Resolution, Error> {
Ok(
if pep517_backend.requirements == default_backend.requirements {
@ -420,7 +424,7 @@ impl SourceBuild {
resolved_requirements.clone()
} else {
let resolved_requirements = build_context
.resolve(&default_backend.requirements)
.resolve(&default_backend.requirements, build_stack)
.await
.map_err(|err| {
Error::RequirementsResolve("`setup.py` build", err.into())
@ -430,7 +434,7 @@ impl SourceBuild {
}
} else {
build_context
.resolve(&pep517_backend.requirements)
.resolve(&pep517_backend.requirements, build_stack)
.await
.map_err(|err| {
Error::RequirementsResolve("`build-system.requires`", err.into())
@ -806,6 +810,7 @@ async fn create_pep517_build_environment(
version_id: Option<&str>,
locations: &IndexLocations,
source_strategy: SourceStrategy,
build_stack: &BuildStack,
build_kind: BuildKind,
level: BuildOutput,
config_settings: &ConfigSettings,
@ -929,12 +934,15 @@ async fn create_pep517_build_environment(
.cloned()
.chain(extra_requires)
.collect();
let resolution = build_context.resolve(&requirements).await.map_err(|err| {
Error::RequirementsResolve("`build-system.requires`", AnyErrorBuild::from(err))
})?;
let resolution = build_context
.resolve(&requirements, build_stack)
.await
.map_err(|err| {
Error::RequirementsResolve("`build-system.requires`", AnyErrorBuild::from(err))
})?;
build_context
.install(&resolution, venv)
.install(&resolution, venv, build_stack)
.await
.map_err(|err| {
Error::RequirementsInstall("`build-system.requires`", AnyErrorBuild::from(err))

View File

@ -23,8 +23,8 @@ use uv_configuration::{BuildOutput, Concurrency};
use uv_distribution::DistributionDatabase;
use uv_distribution_filename::DistFilename;
use uv_distribution_types::{
CachedDist, DependencyMetadata, IndexCapabilities, IndexLocations, IsBuildBackendError, Name,
Resolution, SourceDist, VersionOrUrlRef,
CachedDist, DependencyMetadata, Identifier, IndexCapabilities, IndexLocations,
IsBuildBackendError, Name, Resolution, SourceDist, VersionOrUrlRef,
};
use uv_git::GitResolver;
use uv_installer::{Installer, Plan, Planner, Preparer, SitePackages};
@ -35,7 +35,8 @@ use uv_resolver::{
PythonRequirement, Resolver, ResolverEnvironment,
};
use uv_types::{
AnyErrorBuild, BuildContext, BuildIsolation, EmptyInstalledPackages, HashStrategy, InFlight,
AnyErrorBuild, BuildContext, BuildIsolation, BuildStack, EmptyInstalledPackages, HashStrategy,
InFlight,
};
#[derive(Debug, Error)]
@ -208,6 +209,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
async fn resolve<'data>(
&'data self,
requirements: &'data [Requirement],
build_stack: &'data BuildStack,
) -> Result<Resolution, BuildDispatchError> {
let python_requirement = PythonRequirement::from_interpreter(self.interpreter);
let marker_env = self.interpreter.resolver_marker_environment();
@ -223,8 +225,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
.build(),
&python_requirement,
ResolverEnvironment::specific(marker_env),
// Conflicting groups only make sense when doing
// universal resolution.
// Conflicting groups only make sense when doing universal resolution.
Conflicts::empty(),
Some(tags),
self.flat_index,
@ -232,7 +233,8 @@ impl<'a> BuildContext for BuildDispatch<'a> {
self.hasher,
self,
EmptyInstalledPackages,
DistributionDatabase::new(self.client, self, self.concurrency.downloads),
DistributionDatabase::new(self.client, self, self.concurrency.downloads)
.with_build_stack(build_stack),
)?;
let resolution = Resolution::from(resolver.resolve().await.with_context(|| {
format!(
@ -257,6 +259,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
&'data self,
resolution: &'data Resolution,
venv: &'data PythonEnvironment,
build_stack: &'data BuildStack,
) -> Result<Vec<CachedDist>, BuildDispatchError> {
debug!(
"Installing in {} in {}",
@ -296,17 +299,27 @@ impl<'a> BuildContext for BuildDispatch<'a> {
return Ok(vec![]);
}
// Verify that none of the missing distributions are already in the build stack.
for dist in &remote {
let id = dist.distribution_id();
if build_stack.contains(&id) {
return Err(BuildDispatchError::BuildFrontend(
uv_build_frontend::Error::CyclicBuildDependency(dist.name().clone()).into(),
));
}
}
// Download any missing distributions.
let wheels = if remote.is_empty() {
vec![]
} else {
// TODO(konstin): Check that there is no endless recursion.
let preparer = Preparer::new(
self.cache,
tags,
self.hasher,
self.build_options,
DistributionDatabase::new(self.client, self, self.concurrency.downloads),
DistributionDatabase::new(self.client, self, self.concurrency.downloads)
.with_build_stack(build_stack),
);
debug!(
@ -367,6 +380,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
sources: SourceStrategy,
build_kind: BuildKind,
build_output: BuildOutput,
mut build_stack: BuildStack,
) -> Result<SourceBuild, uv_build_frontend::Error> {
let dist_name = dist.map(uv_distribution_types::Name::name);
let dist_version = dist
@ -392,6 +406,11 @@ impl<'a> BuildContext for BuildDispatch<'a> {
return Err(err);
}
// Push the current distribution onto the build stack, to prevent cyclic dependencies.
if let Some(dist) = dist {
build_stack.insert(dist.distribution_id());
}
let builder = SourceBuild::setup(
source,
subdirectory,
@ -406,6 +425,7 @@ impl<'a> BuildContext for BuildDispatch<'a> {
sources,
self.config_settings.clone(),
self.build_isolation,
&build_stack,
build_kind,
self.build_extra_env_vars.clone(),
build_output,

View File

@ -28,7 +28,7 @@ use uv_extract::hash::Hasher;
use uv_fs::write_atomic;
use uv_platform_tags::Tags;
use uv_pypi_types::HashDigest;
use uv_types::BuildContext;
use uv_types::{BuildContext, BuildStack};
use crate::archive::Archive;
use crate::locks::Locks;
@ -71,7 +71,16 @@ impl<'a, Context: BuildContext> DistributionDatabase<'a, Context> {
}
}
/// Set the [`Reporter`] to use for this source distribution fetcher.
/// Set the build stack to use for the [`DistributionDatabase`].
#[must_use]
pub fn with_build_stack(self, build_stack: &'a BuildStack) -> Self {
Self {
builder: self.builder.with_build_stack(build_stack),
..self
}
}
/// Set the [`Reporter`] to use for the [`DistributionDatabase`].
#[must_use]
pub fn with_reporter(self, reporter: impl Reporter + 'static) -> Self {
let reporter = Arc::new(reporter);

View File

@ -38,7 +38,7 @@ use uv_normalize::PackageName;
use uv_pep440::{release_specifiers_to_ranges, Version};
use uv_platform_tags::Tags;
use uv_pypi_types::{HashAlgorithm, HashDigest, Metadata12, RequiresTxt, ResolutionMetadata};
use uv_types::{BuildContext, SourceBuildTrait};
use uv_types::{BuildContext, BuildStack, SourceBuildTrait};
use zip::ZipArchive;
mod built_wheel_metadata;
@ -47,6 +47,7 @@ mod revision;
/// Fetch and build a source distribution from a remote source, or from a local cache.
pub(crate) struct SourceDistributionBuilder<'a, T: BuildContext> {
build_context: &'a T,
build_stack: Option<&'a BuildStack>,
reporter: Option<Arc<dyn Reporter>>,
}
@ -67,11 +68,21 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
pub(crate) fn new(build_context: &'a T) -> Self {
Self {
build_context,
build_stack: None,
reporter: None,
}
}
/// Set the [`Reporter`] to use for this source distribution fetcher.
/// Set the [`BuildStack`] to use for the [`SourceDistributionBuilder`].
#[must_use]
pub(crate) fn with_build_stack(self, build_stack: &'a BuildStack) -> Self {
Self {
build_stack: Some(build_stack),
..self
}
}
/// Set the [`Reporter`] to use for the [`SourceDistributionBuilder`].
#[must_use]
pub(crate) fn with_reporter(self, reporter: Arc<dyn Reporter>) -> Self {
Self {
@ -1898,6 +1909,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
BuildKind::Wheel
},
BuildOutput::Debug,
self.build_stack.cloned().unwrap_or_default(),
)
.await
.map_err(|err| Error::Build(err.into()))?
@ -1974,6 +1986,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
BuildKind::Wheel
},
BuildOutput::Debug,
self.build_stack.cloned().unwrap_or_default(),
)
.await
.map_err(|err| Error::Build(err.into()))?;

View File

@ -220,6 +220,8 @@ pub enum Error {
DerivationChain,
#[source] uv_distribution::Error,
),
#[error("Cyclic build dependency detected for `{0}`")]
CyclicBuildDependency(PackageName),
#[error("Unzip failed in another thread: {0}")]
Thread(String),
}

View File

@ -2,17 +2,18 @@ use std::fmt::{Debug, Display, Formatter};
use std::future::Future;
use std::ops::Deref;
use std::path::{Path, PathBuf};
use uv_distribution_filename::DistFilename;
use anyhow::Result;
use rustc_hash::FxHashSet;
use uv_cache::Cache;
use uv_configuration::{
BuildKind, BuildOptions, BuildOutput, ConfigSettings, LowerBound, SourceStrategy,
};
use uv_distribution_filename::DistFilename;
use uv_distribution_types::{
CachedDist, DependencyMetadata, IndexCapabilities, IndexLocations, InstalledDist,
IsBuildBackendError, Resolution, SourceDist,
CachedDist, DependencyMetadata, DistributionId, IndexCapabilities, IndexLocations,
InstalledDist, IsBuildBackendError, Resolution, SourceDist,
};
use uv_git::GitResolver;
use uv_pep508::PackageName;
@ -96,6 +97,7 @@ pub trait BuildContext {
fn resolve<'a>(
&'a self,
requirements: &'a [Requirement],
build_stack: &'a BuildStack,
) -> impl Future<Output = Result<Resolution, impl IsBuildBackendError>> + 'a;
/// Install the given set of package versions into the virtual environment. The environment must
@ -104,6 +106,7 @@ pub trait BuildContext {
&'a self,
resolution: &'a Resolution,
venv: &'a PythonEnvironment,
build_stack: &'a BuildStack,
) -> impl Future<Output = Result<Vec<CachedDist>, impl IsBuildBackendError>> + 'a;
/// Set up a source distribution build by installing the required dependencies. A wrapper for
@ -123,6 +126,7 @@ pub trait BuildContext {
sources: SourceStrategy,
build_kind: BuildKind,
build_output: BuildOutput,
build_stack: BuildStack,
) -> impl Future<Output = Result<Self::SourceDistBuilder, impl IsBuildBackendError>> + 'a;
/// Build by calling directly into the uv build backend without PEP 517, if possible.
@ -244,3 +248,23 @@ impl Deref for AnyErrorBuild {
&*self.0
}
}
/// The stack of packages being built.
#[derive(Debug, Clone, Default)]
pub struct BuildStack(FxHashSet<DistributionId>);
impl BuildStack {
/// Return an empty stack.
pub fn empty() -> Self {
Self(FxHashSet::default())
}
pub fn contains(&self, id: &DistributionId) -> bool {
self.0.contains(id)
}
/// Push a package onto the stack.
pub fn insert(&mut self, id: DistributionId) -> bool {
self.0.insert(id)
}
}

View File

@ -42,7 +42,7 @@ use uv_python::{
use uv_requirements::RequirementsSource;
use uv_resolver::{ExcludeNewer, FlatIndex, RequiresPython};
use uv_settings::PythonInstallMirrors;
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, HashStrategy};
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, BuildStack, HashStrategy};
use uv_workspace::{DiscoveryOptions, Workspace, WorkspaceError};
#[derive(Debug, Error)]
@ -921,6 +921,7 @@ async fn build_sdist(
sources,
BuildKind::Sdist,
build_output,
BuildStack::default(),
)
.await
.map_err(|err| Error::BuildDispatch(err.into()))?;
@ -1018,6 +1019,7 @@ async fn build_wheel(
sources,
BuildKind::Wheel,
build_output,
BuildStack::default(),
)
.await
.map_err(|err| Error::BuildDispatch(err.into()))?;

View File

@ -26,7 +26,7 @@ use uv_python::{
use uv_resolver::{ExcludeNewer, FlatIndex};
use uv_settings::PythonInstallMirrors;
use uv_shell::{shlex_posix, shlex_windows, Shell};
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, HashStrategy};
use uv_types::{AnyErrorBuild, BuildContext, BuildIsolation, BuildStack, HashStrategy};
use uv_warnings::{warn_user, warn_user_once};
use uv_workspace::{DiscoveryOptions, VirtualProject, WorkspaceError};
@ -353,16 +353,18 @@ async fn venv_impl(
)]
};
let build_stack = BuildStack::default();
// Resolve and install the requirements.
//
// Since the virtual environment is empty, and the set of requirements is trivial (no
// constraints, no editables, etc.), we can use the build dispatch APIs directly.
let resolution = build_dispatch
.resolve(&requirements)
.resolve(&requirements, &build_stack)
.await
.map_err(|err| VenvError::Seed(err.into()))?;
let installed = build_dispatch
.install(&resolution, &venv)
.install(&resolution, &venv, &build_stack)
.await
.map_err(|err| VenvError::Seed(err.into()))?;

View File

@ -20592,7 +20592,7 @@ fn lock_no_build_dynamic_metadata() -> Result<()> {
----- stderr -----
× Failed to build `dummy @ file://[TEMP_DIR]/`
Building source distributions for dummy is disabled
Building source distributions for `dummy` is disabled
"###);
Ok(())

View File

@ -7858,3 +7858,55 @@ fn static_metadata_already_installed() -> Result<()> {
Ok(())
}
/// `circular-one` depends on `circular-two` as a build dependency, but `circular-two` depends on
/// `circular-one` was a runtime dependency.
#[test]
fn cyclic_build_dependency() {
static EXCLUDE_NEWER: &str = "2025-01-02T00:00:00Z";
let context = TestContext::new("3.13");
// Installing with `--no-binary circular-one` should fail, since we'll end up in a recursive
// build.
uv_snapshot!(context.filters(), context.pip_install()
.env(EnvVars::UV_EXCLUDE_NEWER, EXCLUDE_NEWER)
.arg("circular-one")
.arg("--extra-index-url")
.arg("https://test.pypi.org/simple")
.arg("--index-strategy")
.arg("unsafe-best-match")
.arg("--no-binary")
.arg("circular-one"), @r###"
success: false
exit_code: 1
----- stdout -----
----- stderr -----
Resolved 1 package in [TIME]
× Failed to download and build `circular-one==0.2.0`
Failed to install requirements from `build-system.requires`
Cyclic build dependency detected for `circular-one`
"###
);
// Installing without `--no-binary circular-one` should succeed, since we can use the wheel.
uv_snapshot!(context.filters(), context.pip_install()
.env(EnvVars::UV_EXCLUDE_NEWER, EXCLUDE_NEWER)
.arg("circular-one")
.arg("--extra-index-url")
.arg("https://test.pypi.org/simple")
.arg("--index-strategy")
.arg("unsafe-best-match"), @r###"
success: true
exit_code: 0
----- stdout -----
----- stderr -----
Resolved 1 package in [TIME]
Prepared 1 package in [TIME]
Installed 1 package in [TIME]
+ circular-one==0.2.0
"###
);
}