From 9e0b35ad8280924fc9e347f668d6153d925673de Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 31 Dec 2024 22:22:42 -0500 Subject: [PATCH] Detect cyclic dependencies during builds (#10258) ## Summary Closes https://github.com/astral-sh/uv/issues/10255#issuecomment-2566782671. --- crates/uv-build-frontend/src/error.rs | 7 ++- crates/uv-build-frontend/src/lib.rs | 24 ++++++--- crates/uv-dispatch/src/lib.rs | 36 ++++++++++--- .../src/distribution_database.rs | 13 ++++- crates/uv-distribution/src/source/mod.rs | 17 +++++- crates/uv-installer/src/preparer.rs | 2 + crates/uv-types/src/traits.rs | 30 +++++++++-- crates/uv/src/commands/build_frontend.rs | 4 +- crates/uv/src/commands/venv.rs | 8 +-- crates/uv/tests/it/lock.rs | 2 +- crates/uv/tests/it/pip_install.rs | 52 +++++++++++++++++++ 11 files changed, 165 insertions(+), 30 deletions(-) diff --git a/crates/uv-build-frontend/src/error.rs b/crates/uv-build-frontend/src/error.rs index 81fda9d87..e8d10d65f 100644 --- a/crates/uv-build-frontend/src/error.rs +++ b/crates/uv-build-frontend/src/error.rs @@ -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(_) diff --git a/crates/uv-build-frontend/src/lib.rs b/crates/uv-build-frontend/src/lib.rs index 75ea2f638..d8952b553 100644 --- a/crates/uv-build-frontend/src/lib.rs +++ b/crates/uv-build-frontend/src/lib.rs @@ -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, 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 { 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)) diff --git a/crates/uv-dispatch/src/lib.rs b/crates/uv-dispatch/src/lib.rs index 77e8c237d..c84e81271 100644 --- a/crates/uv-dispatch/src/lib.rs +++ b/crates/uv-dispatch/src/lib.rs @@ -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 { 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, 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 { 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, diff --git a/crates/uv-distribution/src/distribution_database.rs b/crates/uv-distribution/src/distribution_database.rs index d1ab5a5d0..090426919 100644 --- a/crates/uv-distribution/src/distribution_database.rs +++ b/crates/uv-distribution/src/distribution_database.rs @@ -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); diff --git a/crates/uv-distribution/src/source/mod.rs b/crates/uv-distribution/src/source/mod.rs index 2beae3ca4..62164aabb 100644 --- a/crates/uv-distribution/src/source/mod.rs +++ b/crates/uv-distribution/src/source/mod.rs @@ -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>, } @@ -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) -> 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()))?; diff --git a/crates/uv-installer/src/preparer.rs b/crates/uv-installer/src/preparer.rs index 541383de1..4f3e3e46e 100644 --- a/crates/uv-installer/src/preparer.rs +++ b/crates/uv-installer/src/preparer.rs @@ -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), } diff --git a/crates/uv-types/src/traits.rs b/crates/uv-types/src/traits.rs index fc88cab09..bfc2bf8a0 100644 --- a/crates/uv-types/src/traits.rs +++ b/crates/uv-types/src/traits.rs @@ -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> + '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, 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> + '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); + +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) + } +} diff --git a/crates/uv/src/commands/build_frontend.rs b/crates/uv/src/commands/build_frontend.rs index 0ec665e38..0569b72f1 100644 --- a/crates/uv/src/commands/build_frontend.rs +++ b/crates/uv/src/commands/build_frontend.rs @@ -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()))?; diff --git a/crates/uv/src/commands/venv.rs b/crates/uv/src/commands/venv.rs index 71ff32b62..6b7c10f95 100644 --- a/crates/uv/src/commands/venv.rs +++ b/crates/uv/src/commands/venv.rs @@ -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()))?; diff --git a/crates/uv/tests/it/lock.rs b/crates/uv/tests/it/lock.rs index 962eca2de..b8533a09f 100644 --- a/crates/uv/tests/it/lock.rs +++ b/crates/uv/tests/it/lock.rs @@ -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(()) diff --git a/crates/uv/tests/it/pip_install.rs b/crates/uv/tests/it/pip_install.rs index 4742a85bf..ba7134271 100644 --- a/crates/uv/tests/it/pip_install.rs +++ b/crates/uv/tests/it/pip_install.rs @@ -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 + "### + ); +}