From cf697275cbfcba9d63cb117ba32f1d9b2ae479c1 Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Mon, 9 Sep 2024 21:16:29 -0400 Subject: [PATCH] Use version in output --- crates/uv-build/src/error.rs | 163 ++++++++++++++++++++++++++++------ crates/uv-build/src/lib.rs | 43 +++++++-- crates/uv-dispatch/src/lib.rs | 10 ++- 3 files changed, 183 insertions(+), 33 deletions(-) diff --git a/crates/uv-build/src/error.rs b/crates/uv-build/src/error.rs index de2475c38fb4..7ff728c624b7 100644 --- a/crates/uv-build/src/error.rs +++ b/crates/uv-build/src/error.rs @@ -1,4 +1,7 @@ +use crate::PythonRunnerOutput; use itertools::Itertools; +use pep440_rs::Version; +use pep508_rs::PackageName; use regex::Regex; use std::env; use std::fmt::{Display, Formatter}; @@ -8,8 +11,6 @@ use std::process::ExitStatus; use std::sync::LazyLock; use thiserror::Error; use tracing::error; - -use crate::PythonRunnerOutput; use uv_configuration::BuildOutput; use uv_fs::Simplified; @@ -46,6 +47,10 @@ static WHEEL_NOT_FOUND_RE: LazyLock = static TORCH_NOT_FOUND_RE: LazyLock = LazyLock::new(|| Regex::new(r"ModuleNotFoundError: No module named 'torch'").unwrap()); +/// e.g. `ModuleNotFoundError: No module named 'distutils'` +static DISTUTILS_NOT_FOUND_RE: LazyLock = + LazyLock::new(|| Regex::new(r"ModuleNotFoundError: No module named 'distutils'").unwrap()); + #[derive(Error, Debug)] pub enum Error { #[error(transparent)] @@ -99,12 +104,15 @@ pub enum Error { enum MissingLibrary { Header(String), Linker(String), - PythonPackage(String), + BuildDependency(String), + DeprecatedModule(String, Version), } #[derive(Debug, Error)] pub struct MissingHeaderCause { missing_library: MissingLibrary, + package_name: Option, + package_version: Option, version_id: String, } @@ -112,38 +120,80 @@ impl Display for MissingHeaderCause { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { match &self.missing_library { MissingLibrary::Header(header) => { - write!( - f, - "This error likely indicates that you need to install a library that provides \"{}\" for {}", - header, self.version_id - ) + if let (Some(package_name), Some(package_version)) = + (&self.package_name, &self.package_version) + { + write!( + f, + "This error likely indicates that you need to install a library that provides \"{header}\" for {package_name}@{package_version}", + ) + } else { + write!( + f, + "This error likely indicates that you need to install a library that provides \"{header}\" for {}", + self.version_id + ) + } } MissingLibrary::Linker(library) => { - write!( - f, - "This error likely indicates that you need to install the library that provides a shared library \ - for {library} for {version_id} (e.g. lib{library}-dev)", - library = library, version_id = self.version_id - ) + if let (Some(package_name), Some(package_version)) = + (&self.package_name, &self.package_version) + { + write!( + f, + "This error likely indicates that you need to install the library that provides a shared library for {library} for {package_name}@{package_version} (e.g., lib{library}-dev)", + ) + } else { + write!( + f, + "This error likely indicates that you need to install the library that provides a shared library for {library} for {version_id} (e.g., lib{library}-dev)", + version_id = self.version_id + ) + } + } + MissingLibrary::BuildDependency(package) => { + if let (Some(package_name), Some(package_version)) = + (&self.package_name, &self.package_version) + { + write!( + f, + "This error likely indicates that {package_name}@{package_version} depends on {package}, but doesn't declare it as a build dependency. If {package_name} is a first-party package, consider adding {package} to its `build-system.requires`. Otherwise, `uv pip install {package}` into the environment and re-run with `--no-build-isolation`." + ) + } else { + write!( + f, + "This error likely indicates that {version_id} depends on {package}, but doesn't declare it as a build dependency. If {version_id} is a first-party package, consider adding {package} to its `build-system.requires`. Otherwise, `uv pip install {package}` into the environment and re-run with `--no-build-isolation`.", + version_id = self.version_id + ) + } } - MissingLibrary::PythonPackage(package) => { - write!( - f, - "This error likely indicates that {version_id} depends on {package}, but doesn't declare it as a build dependency. \ - If {version_id} is a first-party package, consider adding {package} to its `build-system.requires`. \ - Otherwise, `uv pip install {package}` into the environment and re-run with `--no-build-isolation`.", - package = package, version_id = self.version_id - ) + MissingLibrary::DeprecatedModule(package, version) => { + if let (Some(package_name), Some(package_version)) = + (&self.package_name, &self.package_version) + { + write!( + f, + "{package} was removed from the standard library in Python {version}. Consider adding a constraint (like `{package_name} >{package_version}`) to avoid building a version of {package_name} that depends on {package}.", + ) + } else { + write!( + f, + "{package} was removed from the standard library in Python {version}. Consider adding a constraint to avoid building a package-version that depends on {package}.", + ) + } } } } } impl Error { + /// Construct an [`Error`] from the output of a failed command. pub(crate) fn from_command_output( message: String, output: &PythonRunnerOutput, level: BuildOutput, + name: Option<&PackageName>, + version: Option<&Version>, version_id: impl Into, ) -> Self { // In the cases I've seen it was the 5th and 3rd last line (see test case), 10 seems like a reasonable cutoff. @@ -160,9 +210,14 @@ impl Error { { Some(MissingLibrary::Linker(library.to_string())) } else if WHEEL_NOT_FOUND_RE.is_match(line.trim()) { - Some(MissingLibrary::PythonPackage("wheel".to_string())) + Some(MissingLibrary::BuildDependency("wheel".to_string())) } else if TORCH_NOT_FOUND_RE.is_match(line.trim()) { - Some(MissingLibrary::PythonPackage("torch".to_string())) + Some(MissingLibrary::BuildDependency("torch".to_string())) + } else if DISTUTILS_NOT_FOUND_RE.is_match(line.trim()) { + Some(MissingLibrary::DeprecatedModule( + "distutils".to_string(), + Version::new([3, 12]), + )) } else { None } @@ -175,6 +230,8 @@ impl Error { exit_code: output.status, missing_header_cause: MissingHeaderCause { missing_library, + package_name: name.cloned(), + package_version: version.cloned(), version_id: version_id.into(), }, }, @@ -185,6 +242,8 @@ impl Error { stderr: output.stderr.iter().join("\n"), missing_header_cause: MissingHeaderCause { missing_library, + package_name: name.cloned(), + package_version: version.cloned(), version_id: version_id.into(), }, }, @@ -208,10 +267,12 @@ impl Error { #[cfg(test)] mod test { - use std::process::ExitStatus; - use crate::{Error, PythonRunnerOutput}; use indoc::indoc; + use pep440_rs::Version; + use pep508_rs::PackageName; + use std::process::ExitStatus; + use std::str::FromStr; use uv_configuration::BuildOutput; #[test] @@ -244,6 +305,8 @@ mod test { "Failed building wheel through setup.py".to_string(), &output, BuildOutput::Debug, + None, + None, "pygraphviz-1.11", ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); @@ -297,6 +360,8 @@ mod test { "Failed building wheel through setup.py".to_string(), &output, BuildOutput::Debug, + None, + None, "pygraphviz-1.11", ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); @@ -316,7 +381,7 @@ mod test { "###); insta::assert_snapshot!( std::error::Error::source(&err).unwrap(), - @"This error likely indicates that you need to install the library that provides a shared library for ncurses for pygraphviz-1.11 (e.g. libncurses-dev)" + @"This error likely indicates that you need to install the library that provides a shared library for ncurses for pygraphviz-1.11 (e.g., libncurses-dev)" ); } @@ -343,6 +408,8 @@ mod test { "Failed building wheel through setup.py".to_string(), &output, BuildOutput::Debug, + None, + None, "pygraphviz-1.11", ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); @@ -366,4 +433,46 @@ mod test { @"This error likely indicates that pygraphviz-1.11 depends on wheel, but doesn't declare it as a build dependency. If pygraphviz-1.11 is a first-party package, consider adding wheel to its `build-system.requires`. Otherwise, `uv pip install wheel` into the environment and re-run with `--no-build-isolation`." ); } + + #[test] + fn missing_distutils() { + let output = PythonRunnerOutput { + status: ExitStatus::default(), // This is wrong but `from_raw` is platform-gated. + stdout: Vec::new(), + stderr: indoc!( + r" + import distutils.core + ModuleNotFoundError: No module named 'distutils' + " + ) + .lines() + .map(ToString::to_string) + .collect(), + }; + + let err = Error::from_command_output( + "Failed building wheel through setup.py".to_string(), + &output, + BuildOutput::Debug, + Some(&PackageName::from_str("pygraphviz").unwrap()), + Some(&Version::new([1, 11])), + "pygraphviz-1.11", + ); + assert!(matches!(err, Error::MissingHeaderOutput { .. })); + // Unix uses exit status, Windows uses exit code. + let formatted = err.to_string().replace("exit status: ", "exit code: "); + insta::assert_snapshot!(formatted, @r###" + Failed building wheel through setup.py with exit code: 0 + --- stdout: + + --- stderr: + import distutils.core + ModuleNotFoundError: No module named 'distutils' + --- + "###); + insta::assert_snapshot!( + std::error::Error::source(&err).unwrap(), + @"distutils was removed from the standard library in Python 3.12. Consider adding a constraint (like `pygraphviz >1.11`) to avoid building a version of pygraphviz that depends on distutils." + ); + } } diff --git a/crates/uv-build/src/lib.rs b/crates/uv-build/src/lib.rs index c0cb48bed496..43a2c72b7b4a 100644 --- a/crates/uv-build/src/lib.rs +++ b/crates/uv-build/src/lib.rs @@ -214,7 +214,12 @@ pub struct SourceBuild { /// > directory created by `prepare_metadata_for_build_wheel`, including any unrecognized files /// > it created. metadata_directory: Option, - /// Package id such as `foo-1.2.3`, for error reporting + /// The name of the package, if known. + package_name: Option, + /// The version of the package, if known. + package_version: Option, + /// Distribution identifier, e.g., `foo-1.2.3`. Used for error reporting if the name and + /// version are unknown. version_id: String, /// Whether we do a regular PEP 517 build or an PEP 660 editable build build_kind: BuildKind, @@ -237,6 +242,7 @@ impl SourceBuild { source: &Path, subdirectory: Option<&Path>, fallback_package_name: Option<&PackageName>, + fallback_package_version: Option<&Version>, interpreter: &Interpreter, build_context: &impl BuildContext, source_build_context: SourceBuildContext, @@ -262,10 +268,19 @@ impl SourceBuild { let (pep517_backend, project) = Self::extract_pep517_backend(&source_tree, &default_backend).map_err(|err| *err)?; - let package_name = project.as_ref().map(|p| &p.name).or(fallback_package_name); + let package_name = project + .as_ref() + .map(|project| &project.name) + .or(fallback_package_name) + .cloned(); + let package_version = project + .as_ref() + .and_then(|project| project.version.as_ref()) + .or(fallback_package_version) + .cloned(); // Create a virtual environment, or install into the shared environment if requested. - let venv = if let Some(venv) = build_isolation.shared_environment(package_name) { + let venv = if let Some(venv) = build_isolation.shared_environment(package_name.as_ref()) { venv.clone() } else { uv_virtualenv::create_venv( @@ -280,7 +295,7 @@ impl SourceBuild { // Setup the build environment. If build isolation is disabled, we assume the build // environment is already setup. - if build_isolation.is_isolated(package_name) { + if build_isolation.is_isolated(package_name.as_ref()) { debug!("Resolving build requirements"); let resolved_requirements = Self::get_resolved_requirements( @@ -334,7 +349,7 @@ impl SourceBuild { // Create the PEP 517 build environment. If build isolation is disabled, we assume the build // environment is already setup. let runner = PythonRunner::new(concurrent_builds, level); - if build_isolation.is_isolated(package_name) { + if build_isolation.is_isolated(package_name.as_ref()) { debug!("Creating PEP 517 build environment"); create_pep517_build_environment( @@ -343,6 +358,8 @@ impl SourceBuild { &venv, &pep517_backend, build_context, + package_name.as_ref(), + package_version.as_ref(), &version_id, build_kind, level, @@ -364,6 +381,8 @@ impl SourceBuild { level, config_settings, metadata_directory: None, + package_name, + package_version, version_id, environment_variables, modified_path, @@ -548,6 +567,8 @@ impl SourceBuild { format!("Build backend failed to determine metadata through `prepare_metadata_for_build_{}`", self.build_kind), &output, self.level, + self.package_name.as_ref(), + self.package_version.as_ref(), &self.version_id, )); } @@ -678,6 +699,8 @@ impl SourceBuild { ), &output, self.level, + self.package_name.as_ref(), + self.package_version.as_ref(), &self.version_id, )); } @@ -691,6 +714,8 @@ impl SourceBuild { ), &output, self.level, + self.package_name.as_ref(), + self.package_version.as_ref(), &self.version_id, )); } @@ -721,6 +746,8 @@ async fn create_pep517_build_environment( venv: &PythonEnvironment, pep517_backend: &Pep517Backend, build_context: &impl BuildContext, + package_name: Option<&PackageName>, + package_version: Option<&Version>, version_id: &str, build_kind: BuildKind, level: BuildOutput, @@ -778,6 +805,8 @@ async fn create_pep517_build_environment( format!("Build backend failed to determine extra requires with `build_{build_kind}()`"), &output, level, + package_name, + package_version, version_id, )); } @@ -790,6 +819,8 @@ async fn create_pep517_build_environment( ), &output, level, + package_name, + package_version, version_id, ) })?; @@ -802,6 +833,8 @@ async fn create_pep517_build_environment( ), &output, level, + package_name, + package_version, version_id, ) })?; diff --git a/crates/uv-dispatch/src/lib.rs b/crates/uv-dispatch/src/lib.rs index 52da7fdff277..565ee078a897 100644 --- a/crates/uv-dispatch/src/lib.rs +++ b/crates/uv-dispatch/src/lib.rs @@ -12,7 +12,7 @@ use rustc_hash::FxHashMap; use tracing::{debug, instrument}; use distribution_types::{ - CachedDist, IndexCapabilities, IndexLocations, Name, Resolution, SourceDist, + CachedDist, IndexCapabilities, IndexLocations, Name, Resolution, SourceDist, VersionOrUrlRef, }; use pypi_types::Requirement; use uv_build::{SourceBuild, SourceBuildContext}; @@ -318,6 +318,13 @@ impl<'a> BuildContext for BuildDispatch<'a> { build_output: BuildOutput, ) -> Result { let dist_name = dist.map(distribution_types::Name::name); + let dist_version = dist + .map(distribution_types::DistributionMetadata::version_or_url) + .and_then(|version| match version { + VersionOrUrlRef::Version(version) => Some(version), + VersionOrUrlRef::Url(_) => None, + }); + // Note we can only prevent builds by name for packages with names // unless all builds are disabled. if self @@ -339,6 +346,7 @@ impl<'a> BuildContext for BuildDispatch<'a> { source, subdirectory, dist_name, + dist_version, self.interpreter, self, self.source_build_context.clone(),