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

Jump to bottom

Make version ID optional for source builds #7362

Merged
merged 1 commit into from
Sep 13, 2024
Merged

Make version ID optional for source builds #7362

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
40 changes: 26 additions & 14 deletions crates/uv-build/src/error.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ pub struct MissingHeaderCause {
missing_library: MissingLibrary,
package_name: Option<PackageName>,
package_version: Option<Version>,
version_id: String,
version_id: Option<String>,
}

impl Display for MissingHeaderCause {
Expand All @@ -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}\"",
)
}
}
Expand All @@ -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)",
)
}
}
Expand All @@ -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`.",
)
}
}
Expand Down Expand Up @@ -194,7 +206,7 @@ impl Error {
level: BuildOutput,
name: Option<&PackageName>,
version: Option<&Version>,
version_id: impl Into<String>,
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| {
Expand Down Expand Up @@ -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 {
Expand All @@ -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),
},
},
};
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down Expand Up @@ -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.
Expand Down
23 changes: 12 additions & 11 deletions crates/uv-build/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,7 @@ pub struct SourceBuild {
package_version: Option<Version>,
/// 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<String>,
/// 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.
Expand All @@ -246,7 +246,7 @@ impl SourceBuild {
interpreter: &Interpreter,
build_context: &impl BuildContext,
source_build_context: SourceBuildContext,
version_id: String,
version_id: Option<String>,
config_settings: ConfigSettings,
build_isolation: BuildIsolation<'_>,
build_kind: BuildKind,
Expand Down Expand Up @@ -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");
Expand All @@ -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
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -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(),
));
}

Expand Down Expand Up @@ -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(),
));
}

Expand All @@ -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)
Expand Down Expand Up @@ -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,
Expand Down
4 changes: 2 additions & 2 deletions crates/uv-dispatch/src/lib.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<String>,
dist: Option<&'data SourceDist>,
build_kind: BuildKind,
build_output: BuildOutput,
Expand Down Expand Up @@ -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,
Expand Down
6 changes: 3 additions & 3 deletions crates/uv-distribution/src/source/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -1526,13 +1526,13 @@ impl<'a, T: BuildContext> SourceDistributionBuilder<'a, T> {
) -> Result<Option<Metadata23>, 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
Expand Down
2 changes: 1 addition & 1 deletion crates/uv-types/src/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,7 +102,7 @@ pub trait BuildContext {
&'a self,
source: &'a Path,
subdirectory: Option<&'a Path>,
version_id: &'a str,
version_id: Option<String>,
dist: Option<&'a SourceDist>,
build_kind: BuildKind,
build_output: BuildOutput,
Expand Down
16 changes: 8 additions & 8 deletions crates/uv/src/commands/build.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down
Loading