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.

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

Enforce Requires-Python when syncing #4068

Merged
merged 2 commits into from
Jun 5, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
27 changes: 24 additions & 3 deletions crates/uv-resolver/src/lock.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ use distribution_types::{
GitSourceDist, IndexUrl, PathBuiltDist, PathSourceDist, RegistryBuiltDist, RegistryBuiltWheel,
RegistrySourceDist, RemoteSource, Resolution, ResolvedDist, ToUrlError,
};
use pep440_rs::Version;
use pep440_rs::{Version, VersionSpecifiers};
use pep508_rs::{MarkerEnvironment, MarkerTree, VerbatimUrl};
use platform_tags::{TagCompatibility, TagPriority, Tags};
use pypi_types::{HashDigest, ParsedArchiveUrl, ParsedGitUrl};
Expand All @@ -36,6 +36,8 @@ use crate::{lock, ResolutionGraph};
pub struct Lock {
version: u32,
distributions: Vec<Distribution>,
/// The range of supported Python versions.
requires_python: Option<VersionSpecifiers>,
/// A map from distribution ID to index in `distributions`.
///
/// This can be used to quickly lookup the full distribution for any ID
Expand Down Expand Up @@ -87,15 +89,21 @@ impl Lock {
}
}

let lock = Self::new(locked_dists.into_values().collect())?;
let distributions = locked_dists.into_values().collect();
let requires_python = graph.requires_python.clone();
let lock = Self::new(distributions, requires_python)?;
Ok(lock)
}

/// Initialize a [`Lock`] from a list of [`Distribution`] entries.
fn new(distributions: Vec<Distribution>) -> Result<Self, LockError> {
fn new(
distributions: Vec<Distribution>,
requires_python: Option<VersionSpecifiers>,
) -> Result<Self, LockError> {
let wire = LockWire {
version: 1,
distributions,
requires_python,
};
Self::try_from(wire)
}
Expand All @@ -105,6 +113,11 @@ impl Lock {
&self.distributions
}

/// Returns the supported Python version range for the lockfile, if present.
pub fn requires_python(&self) -> Option<&VersionSpecifiers> {
self.requires_python.as_ref()
}

/// Convert the [`Lock`] to a [`Resolution`] using the given marker environment, tags, and root.
pub fn to_resolution(
&self,
Expand Down Expand Up @@ -196,13 +209,16 @@ struct LockWire {
version: u32,
#[serde(rename = "distribution")]
distributions: Vec<Distribution>,
#[serde(rename = "requires-python")]
requires_python: Option<VersionSpecifiers>,
}

impl From<Lock> for LockWire {
fn from(lock: Lock) -> LockWire {
LockWire {
version: lock.version,
distributions: lock.distributions,
requires_python: lock.requires_python,
}
}
}
Expand All @@ -215,6 +231,10 @@ impl Lock {
let mut doc = toml_edit::DocumentMut::new();
doc.insert("version", value(i64::from(self.version)));

if let Some(ref requires_python) = self.requires_python {
doc.insert("requires-python", value(requires_python.to_string()));
}

let mut distributions = ArrayOfTables::new();
for dist in &self.distributions {
let mut table = Table::new();
Expand Down Expand Up @@ -344,6 +364,7 @@ impl TryFrom<LockWire> for Lock {
Ok(Lock {
version: wire.version,
distributions: wire.distributions,
requires_python: wire.requires_python,
by_id,
})
}
Expand Down
13 changes: 11 additions & 2 deletions crates/uv-resolver/src/python_requirement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -60,13 +60,14 @@ impl PythonRequirement {

#[derive(Debug, Clone, Eq, PartialEq)]
pub enum RequiresPython {
/// The `RequiresPython` specifier is a single version specifier, as provided via
/// The [`RequiresPython`] specifier is a single version specifier, as provided via
/// `--python-version` on the command line.
///
/// The use of a separate enum variant allows us to use a verbatim representation when reporting
/// back to the user.
Specifier(StringVersion),
/// The `RequiresPython` specifier is a set of version specifiers.
/// The [`RequiresPython`] specifier is a set of version specifiers, as extracted from the
/// `Requires-Python` field in a `pyproject.toml` or `METADATA` file.
Specifiers(VersionSpecifiers),
}

Expand All @@ -93,6 +94,14 @@ impl RequiresPython {
}
}
}

/// Returns the [`VersionSpecifiers`] for the [`RequiresPython`] specifier.
pub fn as_specifiers(&self) -> Option<&VersionSpecifiers> {
match self {
RequiresPython::Specifier(_) => None,
RequiresPython::Specifiers(specifiers) => Some(specifiers),
}
}
}

impl std::fmt::Display for RequiresPython {
Expand Down
21 changes: 18 additions & 3 deletions crates/uv-resolver/src/resolution/graph.rs
Original file line number Diff line number Diff line change
Expand Up @@ -9,25 +9,30 @@ use rustc_hash::{FxHashMap, FxHashSet};
use distribution_types::{
Dist, DistributionMetadata, Name, ResolutionDiagnostic, VersionId, VersionOrUrlRef,
};
use pep440_rs::{Version, VersionSpecifier};
use pep440_rs::{Version, VersionSpecifier, VersionSpecifiers};
use pep508_rs::{MarkerEnvironment, MarkerTree};
use pypi_types::{ParsedUrlError, Yanked};
use uv_git::GitResolver;
use uv_normalize::{ExtraName, PackageName};

use crate::preferences::Preferences;
use crate::pubgrub::{PubGrubDistribution, PubGrubPackageInner};
use crate::python_requirement::RequiresPython;
use crate::redirect::url_to_precise;
use crate::resolution::AnnotatedDist;
use crate::resolver::Resolution;
use crate::{InMemoryIndex, Manifest, MetadataResponse, ResolveError, VersionsResponse};
use crate::{
InMemoryIndex, Manifest, MetadataResponse, PythonRequirement, ResolveError, VersionsResponse,
};

/// A complete resolution graph in which every node represents a pinned package and every edge
/// represents a dependency between two pinned packages.
#[derive(Debug)]
pub struct ResolutionGraph {
/// The underlying graph.
pub(crate) petgraph: Graph<AnnotatedDist, Version, Directed>,
/// The range of supported Python versions.
pub(crate) requires_python: Option<VersionSpecifiers>,
/// Any diagnostics that were encountered while building the graph.
pub(crate) diagnostics: Vec<ResolutionDiagnostic>,
}
Expand All @@ -39,9 +44,10 @@ impl ResolutionGraph {
index: &InMemoryIndex,
preferences: &Preferences,
git: &GitResolver,
python: &PythonRequirement,
resolution: Resolution,
) -> anyhow::Result<Self, ResolveError> {
// Collect all marker expressions from relevant pubgrub packages.
// Collect all marker expressions from relevant PubGrub packages.
let mut markers: FxHashMap<(&PackageName, &Version, &Option<ExtraName>), MarkerTree> =
FxHashMap::default();
for (package, versions) in &resolution.packages {
Expand Down Expand Up @@ -267,8 +273,17 @@ impl ResolutionGraph {
}
}

// Extract the `Requires-Python` range, if provided.
// TODO(charlie): Infer the supported Python range from the `Requires-Python` of the
// included packages.
let requires_python = python
.target()
.and_then(RequiresPython::as_specifiers)
.cloned();

Ok(Self {
petgraph,
requires_python,
diagnostics,
})
}
Expand Down
8 changes: 7 additions & 1 deletion crates/uv-resolver/src/resolver/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -557,7 +557,13 @@ impl<InstalledPackages: InstalledPackagesProvider> ResolverState<InstalledPackag
for resolution in resolutions {
combined.union(resolution);
}
ResolutionGraph::from_state(&self.index, &self.preferences, &self.git, combined)
ResolutionGraph::from_state(
&self.index,
&self.preferences,
&self.git,
&self.python_requirement,
combined,
)
}

/// Visit a [`PubGrubPackage`] prior to selection. This should be called on a [`PubGrubPackage`]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ Ok(
optional_dependencies: {},
},
],
requires_python: None,
by_id: {
DistributionId {
name: PackageName(
Expand Down
4 changes: 4 additions & 0 deletions crates/uv/src/commands/project/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use tracing::debug;

use distribution_types::{IndexLocations, Resolution};
use install_wheel_rs::linker::LinkMode;
use pep440_rs::{Version, VersionSpecifiers};
use uv_cache::Cache;
use uv_client::{BaseClientBuilder, Connectivity, RegistryClientBuilder};
use uv_configuration::{
Expand All @@ -32,6 +33,9 @@ pub(crate) mod sync;

#[derive(thiserror::Error, Debug)]
pub(crate) enum ProjectError {
#[error("The current Python version ({0}) is not compatible with the locked Python requirement ({1})")]
RequiresPython(Version, VersionSpecifiers),

#[error(transparent)]
Interpreter(#[from] uv_interpreter::Error),

Expand Down
10 changes: 10 additions & 0 deletions crates/uv/src/commands/project/sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,16 @@ pub(super) async fn do_sync(
cache: &Cache,
printer: Printer,
) -> Result<(), ProjectError> {
// Validate that the Python version is supported by the lockfile.
if let Some(requires_python) = lock.requires_python() {
if !requires_python.contains(venv.interpreter().python_version()) {
return Err(ProjectError::RequiresPython(
venv.interpreter().python_version().clone(),
requires_python.clone(),
));
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this be a warning?

Copy link
Member

Choose a reason for hiding this comment

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

Probably not, I feel like for a lockfile it makes sense to fail?

}
}

let markers = venv.interpreter().markers();
let tags = venv.interpreter().tags()?;

Expand Down
Loading
Loading