Skip to content

Commit

Permalink
Model Python version as a PubGrub package
Browse files Browse the repository at this point in the history
  • Loading branch information
charliermarsh committed Jan 2, 2024
1 parent 26f597a commit ad0a416
Show file tree
Hide file tree
Showing 14 changed files with 195 additions and 73 deletions.
12 changes: 10 additions & 2 deletions crates/puffin-cli/src/commands/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -207,8 +207,16 @@ pub(crate) async fn pip_compile(
);

// Resolve the dependencies.
let resolver = Resolver::new(manifest, options, &markers, tags, &client, &build_dispatch)
.with_reporter(ResolverReporter::from(printer));
let resolver = Resolver::new(
manifest,
options,
&markers,
&interpreter,
tags,
&client,
&build_dispatch,
)
.with_reporter(ResolverReporter::from(printer));
let resolution = match resolver.resolve().await {
Err(puffin_resolver::ResolveError::NoSolution(err)) => {
#[allow(clippy::print_stderr)]
Expand Down
16 changes: 13 additions & 3 deletions crates/puffin-cli/src/commands/pip_install.rs
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ use puffin_dispatch::BuildDispatch;
use puffin_installer::{
BuiltEditable, Downloader, InstallPlan, Reinstall, ResolvedEditable, SitePackages,
};
use puffin_interpreter::Virtualenv;
use puffin_interpreter::{Interpreter, Virtualenv};
use puffin_normalize::PackageName;
use puffin_resolver::{
Manifest, PreReleaseMode, ResolutionGraph, ResolutionMode, ResolutionOptions, Resolver,
Expand Down Expand Up @@ -173,6 +173,7 @@ pub(crate) async fn pip_install(
&editables,
&site_packages,
reinstall,
&interpreter,
tags,
markers,
&client,
Expand Down Expand Up @@ -320,6 +321,7 @@ async fn resolve(
editables: &[BuiltEditable],
site_packages: &SitePackages<'_>,
reinstall: &Reinstall,
interpreter: &Interpreter,
tags: &Tags,
markers: &MarkerEnvironment,
client: &RegistryClient,
Expand Down Expand Up @@ -361,8 +363,16 @@ async fn resolve(
);

// Resolve the dependencies.
let resolver = Resolver::new(manifest, options, markers, tags, client, build_dispatch)
.with_reporter(ResolverReporter::from(printer));
let resolver = Resolver::new(
manifest,
options,
markers,
interpreter,
tags,
client,
build_dispatch,
)
.with_reporter(ResolverReporter::from(printer));
let resolution = resolver.resolve().await?;

let s = if resolution.len() == 1 { "" } else { "s" };
Expand Down
19 changes: 9 additions & 10 deletions crates/puffin-cli/tests/pip_compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -669,8 +669,9 @@ fn compile_python_37() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because there is no version of black available matching ==23.10.1 and
root depends on black==23.10.1, version solving failed.
╰─▶ Because there is no version of Python available matching >=3.8 and
black==23.10.1 depends on Python*, black==23.10.1 is forbidden.
And because root depends on black==23.10.1, version solving failed.
"###);
});

Expand Down Expand Up @@ -776,15 +777,13 @@ fn compile_numpy_py38() -> Result<()> {
.arg("--no-build")
.env("VIRTUAL_ENV", venv.as_os_str())
.current_dir(&temp_dir), @r###"
success: true
exit_code: 0
success: false
exit_code: 2
----- stdout -----
# This file was autogenerated by Puffin v0.0.1 via the following command:
# puffin pip-compile requirements.in --cache-dir [CACHE_DIR]
numpy==1.24.4
----- stderr -----
Resolved 1 package in [TIME]
error: Failed to download and build: numpy==1.26.2
Caused by: Building source distributions is disabled
"###);
});

Expand Down Expand Up @@ -1552,8 +1551,8 @@ fn conflicting_transitive_url_dependency() -> Result<()> {
----- stderr -----
× No solution found when resolving dependencies:
╰─▶ Because there is no version of werkzeug available matching >=3.0.0 and
flask==3.0.0 depends on werkzeug>=3.0.0, flask==3.0.0 is forbidden.
╰─▶ Because flask==3.0.0 depends on werkzeug>=3.0.0 and there is no version
of werkzeug available matching >=3.0.0, flask==3.0.0 is forbidden.
And because root depends on flask==3.0.0, version solving failed.
"###);
});
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-dev/src/resolve_cli.rs
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@ pub(crate) async fn resolve_cli(args: ResolveCliArgs) -> Result<()> {
Manifest::simple(args.requirements.clone()),
ResolutionOptions::default(),
venv.interpreter().markers(),
venv.interpreter(),
tags,
&client,
&build_dispatch,
Expand Down
2 changes: 1 addition & 1 deletion crates/puffin-dispatch/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,14 +81,14 @@ impl<'a> BuildContext for BuildDispatch<'a> {
self.no_build
}

//#[instrument(skip(self, requirements), fields(requirements = requirements.iter().map(ToString::to_string).join(", ")))]
async fn resolve<'data>(&'data self, requirements: &'data [Requirement]) -> Result<Resolution> {
let markers = self.interpreter.markers();
let tags = self.interpreter.tags()?;
let resolver = Resolver::new(
Manifest::simple(requirements.to_vec()),
self.options,
markers,
self.interpreter,
tags,
self.client,
self,
Expand Down
2 changes: 0 additions & 2 deletions crates/puffin-resolver/src/finder.rs
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,6 @@ impl<'a> DistFinder<'a> {
// This is relevant for source dists which give no other indication of their
// compatibility and wheels which may be tagged `py3-none-any` but
// have `requires-python: ">=3.9"`
// TODO(konstin): https://github.com/astral-sh/puffin/issues/406
if !file
.requires_python
.as_ref()
Expand Down Expand Up @@ -170,7 +169,6 @@ impl<'a> DistFinder<'a> {
// This is relevant for source dists which give no other indication of their
// compatibility and wheels which may be tagged `py3-none-any` but
// have `requires-python: ">=3.9"`
// TODO(konstin): https://github.com/astral-sh/puffin/issues/406
if !file
.requires_python
.as_ref()
Expand Down
6 changes: 6 additions & 0 deletions crates/puffin-resolver/src/pubgrub/dependencies.rs
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,12 @@ fn merge_package(
// Either package is `root`.
(PubGrubPackage::Root(_), _) | (_, PubGrubPackage::Root(_)) => Ok(None),

// Either package is the installed Python.
(PubGrubPackage::InstalledPython, _) | (_, PubGrubPackage::InstalledPython) => Ok(None),

// Either package is the target Python.
(PubGrubPackage::TargetPython, _) | (_, PubGrubPackage::TargetPython) => Ok(None),

// Left package has a URL. Propagate the URL.
(PubGrubPackage::Package(name, extra, Some(url)), PubGrubPackage::Package(.., None)) => {
Ok(Some(PubGrubPackage::Package(
Expand Down
1 change: 1 addition & 0 deletions crates/puffin-resolver/src/pubgrub/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ pub(crate) use crate::pubgrub::distribution::PubGrubDistribution;
pub(crate) use crate::pubgrub::package::PubGrubPackage;
pub(crate) use crate::pubgrub::priority::{PubGrubPriorities, PubGrubPriority};
pub(crate) use crate::pubgrub::report::{PubGrubHints, PubGrubReportFormatter};
pub(crate) use crate::pubgrub::specifier::PubGrubSpecifier;
pub(crate) use crate::pubgrub::version::{PubGrubVersion, MIN_VERSION};

mod dependencies;
Expand Down
8 changes: 8 additions & 0 deletions crates/puffin-resolver/src/pubgrub/package.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,13 @@ use puffin_normalize::{ExtraName, PackageName};
#[derive(Debug, Clone, Eq, Derivative)]
#[derivative(PartialEq, Hash)]
pub enum PubGrubPackage {
/// The root package, which is used to start the resolution process.
Root(Option<PackageName>),
/// The installed Python version.
InstalledPython,
/// The target Python version.
TargetPython,
/// A Python package.
Package(
PackageName,
Option<ExtraName>,
Expand Down Expand Up @@ -80,6 +86,8 @@ impl std::fmt::Display for PubGrubPackage {
write!(f, "root")
}
}
PubGrubPackage::InstalledPython => write!(f, "Python"),
PubGrubPackage::TargetPython => write!(f, "Python"),
PubGrubPackage::Package(name, None, ..) => write!(f, "{name}"),
PubGrubPackage::Package(name, Some(extra), ..) => {
write!(f, "{name}[{extra}]")
Expand Down
2 changes: 2 additions & 0 deletions crates/puffin-resolver/src/pubgrub/priority.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,8 @@ impl PubGrubPriorities {
pub(crate) fn get(&self, package: &PubGrubPackage) -> Option<PubGrubPriority> {
match package {
PubGrubPackage::Root(_) => Some(Reverse(0)),
PubGrubPackage::InstalledPython => Some(Reverse(0)),
PubGrubPackage::TargetPython => Some(Reverse(0)),
PubGrubPackage::Package(name, _, _) => self
.0
.get(name)
Expand Down
4 changes: 2 additions & 2 deletions crates/puffin-resolver/src/resolution_options.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
use crate::{PreReleaseMode, ResolutionMode};
use chrono::{DateTime, Utc};

use crate::{PreReleaseMode, ResolutionMode};

/// Options for resolving a manifest.
#[derive(Debug, Default, Copy, Clone)]
pub struct ResolutionOptions {
// TODO(konstin): These should be pub(crate) again
pub resolution_mode: ResolutionMode,
pub prerelease_mode: PreReleaseMode,
pub exclude_newer: Option<DateTime<Utc>>,
Expand Down
59 changes: 56 additions & 3 deletions crates/puffin-resolver/src/resolver.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ use anyhow::Result;
use chrono::{DateTime, Utc};
use futures::channel::mpsc::UnboundedReceiver;
use futures::{pin_mut, FutureExt, StreamExt, TryFutureExt};
use itertools::Itertools;
use pubgrub::error::PubGrubError;
use pubgrub::range::Range;
use pubgrub::solver::{Incompatibility, State};
Expand All @@ -25,6 +26,7 @@ use pep508_rs::{MarkerEnvironment, Requirement};
use platform_tags::Tags;
use puffin_client::RegistryClient;
use puffin_distribution::{DistributionDatabase, DistributionDatabaseError};
use puffin_interpreter::Interpreter;
use puffin_normalize::PackageName;
use puffin_traits::{BuildContext, OnceMap};
use pypi_types::{BaseUrl, Metadata21};
Expand All @@ -35,8 +37,8 @@ use crate::manifest::Manifest;
use crate::overrides::Overrides;
use crate::pins::FilePins;
use crate::pubgrub::{
PubGrubDependencies, PubGrubDistribution, PubGrubPackage, PubGrubPriorities, PubGrubVersion,
MIN_VERSION,
PubGrubDependencies, PubGrubDistribution, PubGrubPackage, PubGrubPriorities, PubGrubSpecifier,
PubGrubVersion, MIN_VERSION,
};
use crate::resolution::ResolutionGraph;
use crate::version_map::VersionMap;
Expand Down Expand Up @@ -152,6 +154,7 @@ pub struct Resolver<'a, Provider: ResolverProvider> {
overrides: Overrides,
allowed_urls: AllowedUrls,
markers: &'a MarkerEnvironment,
interpreter: &'a Interpreter,
selector: CandidateSelector,
index: Arc<Index>,
editables: FxHashMap<PackageName, (LocalEditable, Metadata21)>,
Expand All @@ -165,6 +168,7 @@ impl<'a, Context: BuildContext + Send + Sync> Resolver<'a, DefaultResolverProvid
manifest: Manifest,
options: ResolutionOptions,
markers: &'a MarkerEnvironment,
interpreter: &'a Interpreter,
tags: &'a Tags,
client: &'a RegistryClient,
build_context: &'a Context,
Expand All @@ -182,7 +186,7 @@ impl<'a, Context: BuildContext + Send + Sync> Resolver<'a, DefaultResolverProvid
.chain(manifest.constraints.iter())
.collect(),
);
Self::new_custom_io(manifest, options, markers, provider)
Self::new_custom_io(manifest, options, markers, interpreter, provider)
}
}

Expand All @@ -192,6 +196,7 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
manifest: Manifest,
options: ResolutionOptions,
markers: &'a MarkerEnvironment,
interpreter: &'a Interpreter,
provider: Provider,
) -> Self {
let selector = CandidateSelector::for_resolution(&manifest, options);
Expand Down Expand Up @@ -246,6 +251,7 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
constraints: manifest.constraints,
overrides: Overrides::from_requirements(manifest.overrides),
markers,
interpreter,
editables,
reporter: None,
provider,
Expand Down Expand Up @@ -441,6 +447,8 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
) -> Result<(), ResolveError> {
match package {
PubGrubPackage::Root(_) => {}
PubGrubPackage::InstalledPython => {}
PubGrubPackage::TargetPython => {}
PubGrubPackage::Package(package_name, _extra, None) => {
// Emit a request to fetch the metadata for this package.
if index.packages.register(package_name) {
Expand Down Expand Up @@ -508,6 +516,26 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
return match package {
PubGrubPackage::Root(_) => Ok(Some(MIN_VERSION.clone())),

PubGrubPackage::InstalledPython => {
let interpreter_version = self.interpreter.version();
let version = PubGrubVersion::from(interpreter_version.clone());
if range.contains(&version) {
Ok(Some(version))
} else {
Ok(None)
}
}

PubGrubPackage::TargetPython => {
let marker_version = &self.markers.python_version.version;
let version = PubGrubVersion::from(marker_version.clone());
if range.contains(&version) {
Ok(Some(version))
} else {
Ok(None)
}
}

PubGrubPackage::Package(package_name, extra, Some(url)) => {
if let Some(extra) = extra {
debug!(
Expand Down Expand Up @@ -652,6 +680,14 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
Ok(Dependencies::Known(constraints.into()))
}

PubGrubPackage::InstalledPython => {
Ok(Dependencies::Known(DependencyConstraints::default()))
}

PubGrubPackage::TargetPython => {
Ok(Dependencies::Known(DependencyConstraints::default()))
}

PubGrubPackage::Package(package_name, extra, url) => {
// Wait for the metadata to be available.
let dist = match url {
Expand All @@ -677,6 +713,21 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
Self::visit_package(package, priorities, index, request_sink)?;
}

// If a package has a `requires_python` field, add a constraint on the target
// Python version.
if let Some(requires_python) = metadata.requires_python.as_ref() {
let version = requires_python
.iter()
.map(PubGrubSpecifier::try_from)
.fold_ok(Range::full(), |range, specifier| {
range.intersection(&specifier.into())
})?;

constraints.insert(PubGrubPackage::InstalledPython, version.clone());
constraints.insert(PubGrubPackage::TargetPython, version.clone());
}

// If a package has an extra, insert a constraint on the base package.
if extra.is_some() {
constraints.insert(
PubGrubPackage::Package(package_name.clone(), None, None),
Expand Down Expand Up @@ -773,6 +824,8 @@ impl<'a, Provider: ResolverProvider> Resolver<'a, Provider> {
if let Some(reporter) = self.reporter.as_ref() {
match package {
PubGrubPackage::Root(_) => {}
PubGrubPackage::InstalledPython => {}
PubGrubPackage::TargetPython => {}
PubGrubPackage::Package(package_name, _extra, Some(url)) => {
reporter.on_progress(package_name, VersionOrUrl::Url(url));
}
Expand Down
Loading

0 comments on commit ad0a416

Please sign in to comment.