From 65096ef50e8d978b0128670a6877078fe323ee8d Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Fri, 13 Sep 2024 10:35:35 -0400 Subject: [PATCH] Make version ID optional for source builds --- crates/uv-build/src/error.rs | 40 +++++++++++++++--------- crates/uv-build/src/lib.rs | 23 +++++++------- crates/uv-dispatch/src/lib.rs | 4 +-- crates/uv-distribution/src/source/mod.rs | 6 ++-- crates/uv-types/src/traits.rs | 2 +- crates/uv/src/commands/build.rs | 16 +++++----- 6 files changed, 52 insertions(+), 39 deletions(-) diff --git a/crates/uv-build/src/error.rs b/crates/uv-build/src/error.rs index 7ff728c624b7..643eb0af9ec4 100644 --- a/crates/uv-build/src/error.rs +++ b/crates/uv-build/src/error.rs @@ -113,7 +113,7 @@ pub struct MissingHeaderCause { missing_library: MissingLibrary, package_name: Option, package_version: Option, - version_id: String, + version_id: Option, } impl Display for MissingHeaderCause { @@ -127,11 +127,15 @@ impl Display for MissingHeaderCause { f, "This error likely indicates that you need to install a library that provides \"{header}\" for {package_name}@{package_version}", ) + } else if let Some(version_id) = &self.version_id { + write!( + f, + "This error likely indicates that you need to install a library that provides \"{header}\" for {version_id}", + ) } else { write!( f, - "This error likely indicates that you need to install a library that provides \"{header}\" for {}", - self.version_id + "This error likely indicates that you need to install a library that provides \"{header}\"", ) } } @@ -143,11 +147,15 @@ impl Display for MissingHeaderCause { 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 { + } else if let Some(version_id) = &self.version_id { 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 + ) + } else { + write!( + f, + "This error likely indicates that you need to install the library that provides a shared library for {library} (e.g., lib{library}-dev)", ) } } @@ -159,11 +167,15 @@ impl Display for MissingHeaderCause { 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 { + } else if let Some(version_id) = &self.version_id { 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 + ) + } else { + write!( + f, + "This error likely indicates that a package depends on {package}, but doesn't declare it as a build dependency. If the package 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`.", ) } } @@ -194,7 +206,7 @@ impl Error { level: BuildOutput, name: Option<&PackageName>, version: Option<&Version>, - version_id: impl Into, + version_id: Option<&str>, ) -> Self { // In the cases I've seen it was the 5th and 3rd last line (see test case), 10 seems like a reasonable cutoff. let missing_library = output.stderr.iter().rev().take(10).find_map(|line| { @@ -232,7 +244,7 @@ impl Error { missing_library, package_name: name.cloned(), package_version: version.cloned(), - version_id: version_id.into(), + version_id: version_id.map(ToString::to_string), }, }, BuildOutput::Debug => Self::MissingHeaderOutput { @@ -244,7 +256,7 @@ impl Error { missing_library, package_name: name.cloned(), package_version: version.cloned(), - version_id: version_id.into(), + version_id: version_id.map(ToString::to_string), }, }, }; @@ -307,7 +319,7 @@ mod test { BuildOutput::Debug, None, None, - "pygraphviz-1.11", + Some("pygraphviz-1.11"), ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); // Unix uses exit status, Windows uses exit code. @@ -362,7 +374,7 @@ mod test { BuildOutput::Debug, None, None, - "pygraphviz-1.11", + Some("pygraphviz-1.11"), ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); // Unix uses exit status, Windows uses exit code. @@ -410,7 +422,7 @@ mod test { BuildOutput::Debug, None, None, - "pygraphviz-1.11", + Some("pygraphviz-1.11"), ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); // Unix uses exit status, Windows uses exit code. @@ -456,7 +468,7 @@ mod test { BuildOutput::Debug, Some(&PackageName::from_str("pygraphviz").unwrap()), Some(&Version::new([1, 11])), - "pygraphviz-1.11", + Some("pygraphviz-1.11"), ); assert!(matches!(err, Error::MissingHeaderOutput { .. })); // Unix uses exit status, Windows uses exit code. diff --git a/crates/uv-build/src/lib.rs b/crates/uv-build/src/lib.rs index 43a2c72b7b4a..cbe01c1922a9 100644 --- a/crates/uv-build/src/lib.rs +++ b/crates/uv-build/src/lib.rs @@ -220,7 +220,7 @@ pub struct SourceBuild { 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, + version_id: Option, /// Whether we do a regular PEP 517 build or an PEP 660 editable build build_kind: BuildKind, /// Whether to send build output to `stderr` or `tracing`, etc. @@ -246,7 +246,7 @@ impl SourceBuild { interpreter: &Interpreter, build_context: &impl BuildContext, source_build_context: SourceBuildContext, - version_id: String, + version_id: Option, config_settings: ConfigSettings, build_isolation: BuildIsolation<'_>, build_kind: BuildKind, @@ -293,7 +293,7 @@ impl SourceBuild { )? }; - // Setup the build environment. If build isolation is disabled, we assume the build + // Set up the build environment. If build isolation is disabled, we assume the build // environment is already setup. if build_isolation.is_isolated(package_name.as_ref()) { debug!("Resolving build requirements"); @@ -316,10 +316,11 @@ impl SourceBuild { debug!("Proceeding without build isolation"); } - // Figure out what the modified path should be - // Remove the PATH variable from the environment variables if it's there + // Figure out what the modified path should be, and remove the PATH variable from the + // environment variables if it's there. let user_path = environment_variables.remove(&OsString::from("PATH")); - // See if there is an OS PATH variable + + // See if there is an OS PATH variable. let os_path = env::var_os("PATH"); // Prepend the user supplied PATH to the existing OS PATH @@ -360,7 +361,7 @@ impl SourceBuild { build_context, package_name.as_ref(), package_version.as_ref(), - &version_id, + version_id.as_deref(), build_kind, level, &config_settings, @@ -569,7 +570,7 @@ impl SourceBuild { self.level, self.package_name.as_ref(), self.package_version.as_ref(), - &self.version_id, + self.version_id.as_deref(), )); } @@ -701,7 +702,7 @@ impl SourceBuild { self.level, self.package_name.as_ref(), self.package_version.as_ref(), - &self.version_id, + self.version_id.as_deref(), )); } @@ -716,7 +717,7 @@ impl SourceBuild { self.level, self.package_name.as_ref(), self.package_version.as_ref(), - &self.version_id, + self.version_id.as_deref(), )); } Ok(distribution_filename) @@ -748,7 +749,7 @@ async fn create_pep517_build_environment( build_context: &impl BuildContext, package_name: Option<&PackageName>, package_version: Option<&Version>, - version_id: &str, + version_id: Option<&str>, build_kind: BuildKind, level: BuildOutput, config_settings: &ConfigSettings, diff --git a/crates/uv-dispatch/src/lib.rs b/crates/uv-dispatch/src/lib.rs index 773a33d648e9..00fc00be6316 100644 --- a/crates/uv-dispatch/src/lib.rs +++ b/crates/uv-dispatch/src/lib.rs @@ -313,7 +313,7 @@ impl<'a> BuildContext for BuildDispatch<'a> { &'data self, source: &'data Path, subdirectory: Option<&'data Path>, - version_id: &'data str, + version_id: Option, dist: Option<&'data SourceDist>, build_kind: BuildKind, build_output: BuildOutput, @@ -351,7 +351,7 @@ impl<'a> BuildContext for BuildDispatch<'a> { self.interpreter, self, self.source_build_context.clone(), - version_id.to_string(), + version_id, self.config_settings.clone(), self.build_isolation, build_kind, diff --git a/crates/uv-distribution/src/source/mod.rs b/crates/uv-distribution/src/source/mod.rs index acf53285432a..bb49fb2e99b2 100644 --- a/crates/uv-distribution/src/source/mod.rs +++ b/crates/uv-distribution/src/source/mod.rs @@ -1490,7 +1490,7 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { .setup_build( source_root, subdirectory, - &source.to_string(), + Some(source.to_string()), source.as_dist(), if source.is_editable() { BuildKind::Editable @@ -1526,13 +1526,13 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> { ) -> Result, Error> { debug!("Preparing metadata for: {source}"); - // Setup the builder. + // Set up the builder. let mut builder = self .build_context .setup_build( source_root, subdirectory, - &source.to_string(), + Some(source.to_string()), source.as_dist(), if source.is_editable() { BuildKind::Editable diff --git a/crates/uv-types/src/traits.rs b/crates/uv-types/src/traits.rs index f8e18f737746..798b81a3c06f 100644 --- a/crates/uv-types/src/traits.rs +++ b/crates/uv-types/src/traits.rs @@ -102,7 +102,7 @@ pub trait BuildContext { &'a self, source: &'a Path, subdirectory: Option<&'a Path>, - version_id: &'a str, + version_id: Option, dist: Option<&'a SourceDist>, build_kind: BuildKind, build_output: BuildOutput, diff --git a/crates/uv/src/commands/build.rs b/crates/uv/src/commands/build.rs index 9fee4fac130b..96ebf1e68b54 100644 --- a/crates/uv/src/commands/build.rs +++ b/crates/uv/src/commands/build.rs @@ -348,7 +348,7 @@ async fn build_impl( // Prepare some common arguments for the build. let subdirectory = None; - let version_id = src.path().file_name().unwrap().to_string_lossy(); + let version_id = src.path().file_name().and_then(|name| name.to_str()); let dist = None; let assets = match plan { @@ -360,7 +360,7 @@ async fn build_impl( .setup_build( src.path(), subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Sdist, BuildOutput::Stderr, @@ -391,7 +391,7 @@ async fn build_impl( .setup_build( &extracted, subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Wheel, BuildOutput::Stderr, @@ -408,7 +408,7 @@ async fn build_impl( .setup_build( src.path(), subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Sdist, BuildOutput::Stderr, @@ -425,7 +425,7 @@ async fn build_impl( .setup_build( src.path(), subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Wheel, BuildOutput::Stderr, @@ -441,7 +441,7 @@ async fn build_impl( .setup_build( src.path(), subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Sdist, BuildOutput::Stderr, @@ -454,7 +454,7 @@ async fn build_impl( .setup_build( src.path(), subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Wheel, BuildOutput::Stderr, @@ -487,7 +487,7 @@ async fn build_impl( .setup_build( &extracted, subdirectory, - &version_id, + version_id.map(ToString::to_string), dist, BuildKind::Wheel, BuildOutput::Stderr,