Skip to content

Commit

Permalink
Review feedback
Browse files Browse the repository at this point in the history
  • Loading branch information
@charliermarsh
charliermarsh committed Sep 30, 2024
1 parent 86dc744 commit dd89b1c
Show file tree
Hide file tree
Showing 5 changed files with 67 additions and 54 deletions.
18 changes: 2 additions & 16 deletions crates/pep508-rs/src/marker/tree.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1630,20 +1630,6 @@ impl Serialize for MarkerTreeContents {
}
}

impl<'de> serde::Deserialize<'de> for MarkerTreeContents {
fn deserialize<D>(deserializer: D) -> Result<MarkerTreeContents, D::Error>
where
D: Deserializer<'de>,
{
let s = String::deserialize(deserializer)?;
let marker = MarkerTree::from_str(&s).map_err(de::Error::custom)?;
let marker = marker
.contents()
.ok_or_else(|| de::Error::custom("expected at least one marker expression"))?;
Ok(marker)
}
}

impl Display for MarkerTreeContents {
fn fmt(&self, f: &mut Formatter<'_>) -> fmt::Result {
// Normalize all `false` expressions to the same trivially false expression.
Expand Down Expand Up @@ -1681,9 +1667,9 @@ impl Display for MarkerTreeContents {
}

#[cfg(feature = "schemars")]
impl schemars::JsonSchema for MarkerTreeContents {
impl schemars::JsonSchema for MarkerTree {
fn schema_name() -> String {
"MarkerTreeContents".to_string()
"MarkerTree".to_string()
}

fn json_schema(_gen: &mut schemars::gen::SchemaGenerator) -> schemars::schema::Schema {
Expand Down
26 changes: 10 additions & 16 deletions crates/uv-distribution/src/metadata/lowering.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,10 @@ impl LoweredRequirement {
project_dir: &'data Path,
project_sources: &'data BTreeMap<PackageName, Sources>,
workspace: &'data Workspace,
) -> impl Iterator<Item = Result<LoweredRequirement, LoweringError>> + 'data {
) -> Result<
impl Iterator<Item = Result<LoweredRequirement, LoweringError>> + 'data,
LoweringError,
> {
let (source, origin) = if let Some(source) = project_sources.get(&requirement.name) {
(Some(source), Origin::Project)
} else if let Some(source) = workspace.sources().get(&requirement.name) {
Expand All @@ -55,9 +58,7 @@ impl LoweredRequirement {
// `framework[machine_learning]` depends on `framework[cuda]`.
|| &requirement.name == project_name;
if !workspace_package_declared {
return Either::Left(std::iter::once(Err(
LoweringError::UndeclaredWorkspacePackage,
)));
return Err(LoweringError::UndeclaredWorkspacePackage);
}

let Some(source) = source else {
Expand All @@ -72,7 +73,9 @@ impl LoweredRequirement {
requirement.name
);
}
return Either::Left(std::iter::once(Ok(Self(Requirement::from(requirement)))));
return Ok(Either::Left(std::iter::once(Ok(Self(Requirement::from(
requirement,
))))));
};

// Determine whether the markers cover the full space for the requirement. If not, fill the
Expand All @@ -94,7 +97,7 @@ impl LoweredRequirement {
})
};

Either::Right(
Ok(Either::Right(
source
.into_iter()
.map(move |source| {
Expand All @@ -117,7 +120,6 @@ impl LoweredRequirement {
tag,
branch,
)?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Url {
Expand All @@ -129,7 +131,6 @@ impl LoweredRequirement {
return Err(LoweringError::ConflictingUrls);
}
let source = url_source(url, subdirectory.map(PathBuf::from))?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Path {
Expand All @@ -147,12 +148,10 @@ impl LoweredRequirement {
workspace.install_path(),
editable.unwrap_or(false),
)?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Registry { index, marker } => {
let source = registry_source(&requirement, index)?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Workspace {
Expand Down Expand Up @@ -211,7 +210,6 @@ impl LoweredRequirement {
r#virtual: true,
}
};
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::CatchAll { .. } => {
Expand All @@ -235,7 +233,7 @@ impl LoweredRequirement {
Ok(requirement) => !requirement.0.marker.is_false(),
Err(_) => true,
}),
)
))
}

/// Lower a [`pep508_rs::Requirement`] in a non-workspace setting (for example, in a PEP 723
Expand Down Expand Up @@ -293,7 +291,6 @@ impl LoweredRequirement {
tag,
branch,
)?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Url {
Expand All @@ -305,7 +302,6 @@ impl LoweredRequirement {
return Err(LoweringError::ConflictingUrls);
}
let source = url_source(url, subdirectory.map(PathBuf::from))?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Path {
Expand All @@ -323,12 +319,10 @@ impl LoweredRequirement {
dir,
editable.unwrap_or(false),
)?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Registry { index, marker } => {
let source = registry_source(&requirement, index)?;
let marker = marker.map(MarkerTree::from).unwrap_or_default();
(source, marker)
}
Source::Workspace { .. } => {
Expand Down
4 changes: 4 additions & 0 deletions crates/uv-distribution/src/metadata/requires_dist.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,8 @@ impl RequiresDist {
sources,
project_workspace.workspace(),
)
.into_iter()
.flatten()
.map(move |requirement| match requirement {
Ok(requirement) => Ok(requirement.into_inner()),
Err(err) => {
Expand Down Expand Up @@ -125,6 +127,8 @@ impl RequiresDist {
sources,
project_workspace.workspace(),
)
.into_iter()
.flatten()
.map(move |requirement| match requirement {
Ok(requirement) => Ok(requirement.into_inner()),
Err(err) => {
Expand Down
68 changes: 50 additions & 18 deletions crates/uv-workspace/src/pyproject.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ use thiserror::Error;
use url::Url;

use pep440_rs::{Version, VersionSpecifiers};
use pep508_rs::{MarkerTree, MarkerTreeContents};
use pep508_rs::MarkerTree;
use pypi_types::{RequirementSource, SupportedEnvironments, VerbatimParsedUrl};
use uv_fs::{relative_to, PortablePathBuf};
use uv_git::GitReference;
Expand Down Expand Up @@ -416,6 +416,8 @@ pub struct Sources(#[cfg_attr(feature = "schemars", schemars(with = "SourcesWire

impl Sources {
/// Return an [`Iterator`] over the sources.
///
/// If the iterator contains multiple entries, they will always use disjoint markers.
pub fn iter(&self) -> impl Iterator<Item = &Source> {
self.0.iter()
}
Expand Down Expand Up @@ -514,7 +516,12 @@ pub enum Source {
rev: Option<String>,
tag: Option<String>,
branch: Option<String>,
marker: Option<MarkerTreeContents>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
},
/// A remote `http://` or `https://` URL, either a wheel (`.whl`) or a source distribution
/// (`.zip`, `.tar.gz`).
Expand All @@ -528,7 +535,12 @@ pub enum Source {
/// For source distributions, the path to the directory with the `pyproject.toml`, if it's
/// not in the archive root.
subdirectory: Option<PortablePathBuf>,
marker: Option<MarkerTreeContents>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
},
/// The path to a dependency, either a wheel (a `.whl` file), source distribution (a `.zip` or
/// `.tar.gz` file), or source tree (i.e., a directory containing a `pyproject.toml` or
Expand All @@ -537,20 +549,35 @@ pub enum Source {
path: PortablePathBuf,
/// `false` by default.
editable: Option<bool>,
marker: Option<MarkerTreeContents>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
},
/// A dependency pinned to a specific index, e.g., `torch` after setting `torch` to `https://download.pytorch.org/whl/cu118`.
Registry {
// TODO(konstin): The string is more-or-less a placeholder
index: String,
marker: Option<MarkerTreeContents>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
},
/// A dependency on another package in the workspace.
Workspace {
/// When set to `false`, the package will be fetched from the remote index, rather than
/// included as a workspace package.
workspace: bool,
marker: Option<MarkerTreeContents>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
},
/// A catch-all variant used to emit precise error messages when deserializing.
CatchAll {
Expand All @@ -563,7 +590,12 @@ pub enum Source {
path: PortablePathBuf,
index: String,
workspace: bool,
marker: Option<MarkerTreeContents>,
#[serde(
skip_serializing_if = "pep508_rs::marker::ser::is_empty",
serialize_with = "pep508_rs::marker::ser::serialize",
default
)]
marker: MarkerTree,
},
}

Expand Down Expand Up @@ -625,7 +657,7 @@ impl Source {
RequirementSource::Registry { .. } | RequirementSource::Directory { .. } => {
Ok(Some(Source::Workspace {
workspace: true,
marker: None,
marker: MarkerTree::TRUE,
}))
}
RequirementSource::Url { .. } => {
Expand All @@ -650,14 +682,14 @@ impl Source {
.or_else(|_| std::path::absolute(&install_path))
.map_err(SourceError::Absolute)?,
),
marker: None,
marker: MarkerTree::TRUE,
},
RequirementSource::Url {
subdirectory, url, ..
} => Source::Url {
url: url.to_url(),
subdirectory: subdirectory.map(PortablePathBuf::from),
marker: None,
marker: MarkerTree::TRUE,
},
RequirementSource::Git {
repository,
Expand All @@ -682,7 +714,7 @@ impl Source {
branch,
git: repository,
subdirectory: subdirectory.map(PortablePathBuf::from),
marker: None,
marker: MarkerTree::TRUE,
}
} else {
Source::Git {
Expand All @@ -691,7 +723,7 @@ impl Source {
branch,
git: repository,
subdirectory: subdirectory.map(PortablePathBuf::from),
marker: None,
marker: MarkerTree::TRUE,
}
}
}
Expand All @@ -703,12 +735,12 @@ impl Source {
/// Return the [`MarkerTree`] for the source.
pub fn marker(&self) -> MarkerTree {
match self {
Source::Git { marker, .. } => MarkerTree::from(marker.clone()),
Source::Url { marker, .. } => MarkerTree::from(marker.clone()),
Source::Path { marker, .. } => MarkerTree::from(marker.clone()),
Source::Registry { marker, .. } => MarkerTree::from(marker.clone()),
Source::Workspace { marker, .. } => MarkerTree::from(marker.clone()),
Source::CatchAll { marker, .. } => MarkerTree::from(marker.clone()),
Source::Git { marker, .. } => marker.clone(),
Source::Url { marker, .. } => marker.clone(),
Source::Path { marker, .. } => marker.clone(),
Source::Registry { marker, .. } => marker.clone(),
Source::Workspace { marker, .. } => marker.clone(),
Source::CatchAll { marker, .. } => marker.clone(),
}
}
}
Expand Down
5 changes: 1 addition & 4 deletions crates/uv/src/commands/project/run.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -202,10 +202,7 @@ pub(crate) async fn run(
script_dir,
script_sources,
)
.map(move |requirement| match requirement {
Ok(requirement) => Ok(requirement.into_inner()),
Err(err) => Err(err),
})
.map_ok(uv_distribution::LoweredRequirement::into_inner)
})
.collect::<Result<_, _>>()?;
let spec = RequirementsSpecification::from_requirements(requirements);
Expand Down

0 comments on commit dd89b1c

Please sign in to comment.