Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a dedicated error for packages that fail due to distutils deprecation #7239

Merged
merged 1 commit into from
Sep 10, 2024
+183 −33
Merged

Add a dedicated error for packages that fail due to distutils deprecation #7239

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
163 changes: 136 additions & 27 deletions crates/uv-build/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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};
Expand All @@ -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;

Expand Down Expand Up @@ -46,6 +47,10 @@ static WHEEL_NOT_FOUND_RE: LazyLock<Regex> =
static TORCH_NOT_FOUND_RE: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"ModuleNotFoundError: No module named 'torch'").unwrap());

/// e.g. `ModuleNotFoundError: No module named 'distutils'`
static DISTUTILS_NOT_FOUND_RE: LazyLock<Regex> =
LazyLock::new(|| Regex::new(r"ModuleNotFoundError: No module named 'distutils'").unwrap());

#[derive(Error, Debug)]
pub enum Error {
#[error(transparent)]
Expand Down Expand Up @@ -99,51 +104,96 @@ 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<PackageName>,
package_version: Option<Version>,
version_id: String,
}

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}",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I love this type of specific error handling btw (I really miss the Cargo style great error messages in Python in general).

For context, over half of Python app build failures on our platform fail during the pip install step (not surprising given it's the biggest point of user-provided input), so improving UX there is something I'm very interested in.

We've tried a small amount of log parsing based suggestions in the past, but (a) hadn't had a chance to put more effort into them, (b) the package manager really has more context than something wrapping it to try and make more intelligent suggestions - so having uv handle these instead of the buildpack would be ideal.

I presume you would be interested to know more about any real world failure modes that are confusing for users, for cases where uv doesn't already handle them well? (For now all such examples will be based on pip, but I'm sure most will translate.)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, absolutely. Any data or info you can share on common errors would be much appreciated.

)
} 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<String>,
) -> Self {
// In the cases I've seen it was the 5th and 3rd last line (see test case), 10 seems like a reasonable cutoff.
Expand All @@ -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
}
Expand All @@ -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(),
},
},
Expand All @@ -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(),
},
},
Expand All @@ -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]
Expand Down Expand Up @@ -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 { .. }));
Expand Down Expand Up @@ -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 { .. }));
Expand All @@ -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)"
);
}

Expand All @@ -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 { .. }));
Expand All @@ -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."
);
}
}
Loading
Loading