From e1382cc74706433b05fb39078102394ef9c1bd64 Mon Sep 17 00:00:00 2001 From: Zanie Blue Date: Fri, 3 Nov 2023 10:22:10 -0500 Subject: [PATCH] Report project name instead of `root` when using `pyproject.toml` files (#295) Part of https://github.com/astral-sh/puffin/issues/214 Adds a `project: Option` to the `Manifest`, `Resolver`, and `RequirementsSpecification`. To populate an optional `name` for `PubGubPackage::Root`. I'll work on removing the version number next. Should we consider using the parent directory name when a `pyproject.toml` file is not present? --- crates/puffin-cli/src/commands/pip_compile.rs | 2 + crates/puffin-cli/src/commands/pip_sync.rs | 1 + .../puffin-cli/src/commands/pip_uninstall.rs | 1 + crates/puffin-cli/src/requirements.rs | 15 +++++- crates/puffin-cli/tests/pip_compile.rs | 48 +++++++++++++++++++ ...pile__compile_unsolvable_requirements.snap | 20 ++++++++ crates/puffin-dispatch/src/lib.rs | 1 + crates/puffin-resolver/src/manifest.rs | 4 ++ crates/puffin-resolver/src/pubgrub/package.rs | 10 +++- .../puffin-resolver/src/pubgrub/priority.rs | 2 +- crates/puffin-resolver/src/resolver.rs | 12 +++-- crates/puffin-resolver/tests/resolver.rs | 16 +++++++ 12 files changed, 123 insertions(+), 9 deletions(-) create mode 100644 crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap diff --git a/crates/puffin-cli/src/commands/pip_compile.rs b/crates/puffin-cli/src/commands/pip_compile.rs index 0108ba628..bb4f7403b 100644 --- a/crates/puffin-cli/src/commands/pip_compile.rs +++ b/crates/puffin-cli/src/commands/pip_compile.rs @@ -44,6 +44,7 @@ pub(crate) async fn pip_compile( // Read all requirements from the provided sources. let RequirementsSpecification { + project, requirements, constraints, extras: used_extras, @@ -84,6 +85,7 @@ pub(crate) async fn pip_compile( preferences, resolution_mode, prerelease_mode, + project, ); // Detect the current Python interpreter. diff --git a/crates/puffin-cli/src/commands/pip_sync.rs b/crates/puffin-cli/src/commands/pip_sync.rs index 79bfaef6f..0b170b288 100644 --- a/crates/puffin-cli/src/commands/pip_sync.rs +++ b/crates/puffin-cli/src/commands/pip_sync.rs @@ -33,6 +33,7 @@ pub(crate) async fn pip_sync( ) -> Result { // Read all requirements from the provided sources. let RequirementsSpecification { + project: _, requirements, constraints: _, extras: _, diff --git a/crates/puffin-cli/src/commands/pip_uninstall.rs b/crates/puffin-cli/src/commands/pip_uninstall.rs index 35b992d82..82da955ec 100644 --- a/crates/puffin-cli/src/commands/pip_uninstall.rs +++ b/crates/puffin-cli/src/commands/pip_uninstall.rs @@ -22,6 +22,7 @@ pub(crate) async fn pip_uninstall( // Read all requirements from the provided sources. let RequirementsSpecification { + project: _, requirements, constraints: _, extras: _, diff --git a/crates/puffin-cli/src/requirements.rs b/crates/puffin-cli/src/requirements.rs index 5d152f25e..b950d5444 100644 --- a/crates/puffin-cli/src/requirements.rs +++ b/crates/puffin-cli/src/requirements.rs @@ -8,7 +8,7 @@ use anyhow::{Context, Result}; use fs_err as fs; use pep508_rs::Requirement; -use puffin_normalize::ExtraName; +use puffin_normalize::{ExtraName, PackageName}; use puffin_package::requirements_txt::RequirementsTxt; #[derive(Debug)] @@ -58,6 +58,8 @@ impl ExtrasSpecification<'_> { #[derive(Debug, Default)] pub(crate) struct RequirementsSpecification { + /// The name of the project specifying requirements. + pub(crate) project: Option, /// The requirements for the project. pub(crate) requirements: Vec, /// The constraints for the project. @@ -77,6 +79,7 @@ impl RequirementsSpecification { let requirement = Requirement::from_str(name) .with_context(|| format!("Failed to parse `{name}`"))?; Self { + project: None, requirements: vec![requirement], constraints: vec![], extras: HashSet::new(), @@ -85,6 +88,7 @@ impl RequirementsSpecification { RequirementsSource::RequirementsTxt(path) => { let requirements_txt = RequirementsTxt::parse(path, std::env::current_dir()?)?; Self { + project: None, requirements: requirements_txt .requirements .into_iter() @@ -100,6 +104,7 @@ impl RequirementsSpecification { .with_context(|| format!("Failed to read `{}`", path.display()))?; let mut used_extras = HashSet::new(); let mut requirements = Vec::new(); + let mut project_name = None; if let Some(project) = pyproject_toml.project { requirements.extend(project.dependencies.unwrap_or_default()); // Include any optional dependencies specified in `extras` @@ -114,9 +119,12 @@ impl RequirementsSpecification { } } } + // Parse the project name + project_name = Some(PackageName::new(project.name)); } Self { + project: project_name, requirements, constraints: vec![], extras: used_extras, @@ -141,6 +149,11 @@ impl RequirementsSpecification { spec.requirements.extend(source.requirements); spec.constraints.extend(source.constraints); spec.extras.extend(source.extras); + + // Use the first project name discovered + if spec.project.is_none() { + spec.project = source.project; + } } // Read all constraints, treating both requirements _and_ constraints as constraints. diff --git a/crates/puffin-cli/tests/pip_compile.rs b/crates/puffin-cli/tests/pip_compile.rs index d4d9bf90a..5db3a61cf 100644 --- a/crates/puffin-cli/tests/pip_compile.rs +++ b/crates/puffin-cli/tests/pip_compile.rs @@ -1047,3 +1047,51 @@ optional-dependencies.bar = [ Ok(()) } + +/// Compile requirements that cannot be solved due to conflict in a `pyproject.toml` fil;e. +#[test] +fn compile_unsolvable_requirements() -> Result<()> { + let temp_dir = assert_fs::TempDir::new()?; + let cache_dir = assert_fs::TempDir::new()?; + let venv = temp_dir.child(".venv"); + + Command::new(get_cargo_bin(BIN_NAME)) + .arg("venv") + .arg(venv.as_os_str()) + .arg("--cache-dir") + .arg(cache_dir.path()) + .current_dir(&temp_dir) + .assert() + .success(); + venv.assert(predicates::path::is_dir()); + + let pyproject_toml = temp_dir.child("pyproject.toml"); + pyproject_toml.touch()?; + pyproject_toml.write_str( + r#"[build-system] +requires = ["setuptools", "wheel"] + +[project] +name = "my-project" +dependencies = ["django==5.0b1", "django==5.0a1"] +"#, + )?; + + insta::with_settings!({ + filters => vec![ + (r"\d(ms|s)", "[TIME]"), + (r"# .* pip-compile", "# [BIN_PATH] pip-compile"), + (r"--cache-dir .*", "--cache-dir [CACHE_DIR]"), + ] + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-compile") + .arg("pyproject.toml") + .arg("--cache-dir") + .arg(cache_dir.path()) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir)); + }); + + Ok(()) +} diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap new file mode 100644 index 000000000..8d21139fe --- /dev/null +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_unsolvable_requirements.snap @@ -0,0 +1,20 @@ +--- +source: crates/puffin-cli/tests/pip_compile.rs +info: + program: puffin + args: + - pip-compile + - pyproject.toml + - "--cache-dir" + - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpfuaPhl + env: + VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpLxUB5I/.venv +--- +success: false +exit_code: 1 +----- stdout ----- + +----- stderr ----- + × No solution found when resolving dependencies: + ╰─▶ my-project 0a0.dev0 depends on django ∅ + diff --git a/crates/puffin-dispatch/src/lib.rs b/crates/puffin-dispatch/src/lib.rs index 2a2fc97f7..b4dd21f3d 100644 --- a/crates/puffin-dispatch/src/lib.rs +++ b/crates/puffin-dispatch/src/lib.rs @@ -75,6 +75,7 @@ impl BuildContext for BuildDispatch { Vec::default(), ResolutionMode::default(), PreReleaseMode::default(), + None, // TODO(zanieb): We may want to provide a project name here ), self.interpreter_info.markers(), &tags, diff --git a/crates/puffin-resolver/src/manifest.rs b/crates/puffin-resolver/src/manifest.rs index 8b710d180..a4861e0fe 100644 --- a/crates/puffin-resolver/src/manifest.rs +++ b/crates/puffin-resolver/src/manifest.rs @@ -1,4 +1,5 @@ use pep508_rs::Requirement; +use puffin_normalize::PackageName; use crate::prerelease_mode::PreReleaseMode; use crate::resolution_mode::ResolutionMode; @@ -11,6 +12,7 @@ pub struct Manifest { pub(crate) preferences: Vec, pub(crate) resolution_mode: ResolutionMode, pub(crate) prerelease_mode: PreReleaseMode, + pub(crate) project: Option, } impl Manifest { @@ -20,6 +22,7 @@ impl Manifest { preferences: Vec, resolution_mode: ResolutionMode, prerelease_mode: PreReleaseMode, + project: Option, ) -> Self { Self { requirements, @@ -27,6 +30,7 @@ impl Manifest { preferences, resolution_mode, prerelease_mode, + project, } } } diff --git a/crates/puffin-resolver/src/pubgrub/package.rs b/crates/puffin-resolver/src/pubgrub/package.rs index 14f07c62a..28885d90d 100644 --- a/crates/puffin-resolver/src/pubgrub/package.rs +++ b/crates/puffin-resolver/src/pubgrub/package.rs @@ -13,7 +13,7 @@ use puffin_normalize::{ExtraName, PackageName}; #[derive(Debug, Clone, Eq, Derivative)] #[derivative(PartialEq, Hash)] pub enum PubGrubPackage { - Root, + Root(Option), Package( PackageName, Option, @@ -27,7 +27,13 @@ pub enum PubGrubPackage { impl std::fmt::Display for PubGrubPackage { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - PubGrubPackage::Root => write!(f, "root"), + PubGrubPackage::Root(name) => { + if let Some(name) = name { + write!(f, "{}", name.as_ref()) + } else { + write!(f, "root") + } + } PubGrubPackage::Package(name, None, ..) => write!(f, "{name}"), PubGrubPackage::Package(name, Some(extra), ..) => { write!(f, "{name}[{extra}]") diff --git a/crates/puffin-resolver/src/pubgrub/priority.rs b/crates/puffin-resolver/src/pubgrub/priority.rs index 2bad201c1..dc8213bcc 100644 --- a/crates/puffin-resolver/src/pubgrub/priority.rs +++ b/crates/puffin-resolver/src/pubgrub/priority.rs @@ -19,7 +19,7 @@ impl PubGrubPriorities { /// Return the priority of the given package, if it exists. pub(crate) fn get(&self, package: &PubGrubPackage) -> Option { match package { - PubGrubPackage::Root => Some(Reverse(0)), + PubGrubPackage::Root(_) => Some(Reverse(0)), PubGrubPackage::Package(name, _, _) => self .0 .get(name) diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index 2ebd03f51..eada89a65 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -37,6 +37,7 @@ use crate::pubgrub::{PubGrubPackage, PubGrubPriorities, PubGrubVersion, MIN_VERS use crate::resolution::Graph; pub struct Resolver<'a, Context: BuildContext + Sync> { + project: Option, requirements: Vec, constraints: Vec, markers: &'a MarkerEnvironment, @@ -60,6 +61,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { Self { selector: CandidateSelector::from(&manifest), index: Arc::new(Index::default()), + project: manifest.project, requirements: manifest.requirements, constraints: manifest.constraints, markers, @@ -115,7 +117,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { &self, request_sink: &futures::channel::mpsc::UnboundedSender, ) -> Result { - let root = PubGrubPackage::Root; + let root = PubGrubPackage::Root(self.project.clone()); // Keep track of the packages for which we've requested metadata. let mut in_flight = InFlight::default(); @@ -284,7 +286,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { request_sink: &futures::channel::mpsc::UnboundedSender, ) -> Result<(), ResolveError> { match package { - PubGrubPackage::Root => {} + PubGrubPackage::Root(_) => {} PubGrubPackage::Package(package_name, _extra, None) => { // Emit a request to fetch the metadata for this package. if in_flight.insert_package(package_name) { @@ -371,7 +373,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { request_sink: &futures::channel::mpsc::UnboundedSender, ) -> Result, ResolveError> { return match package { - PubGrubPackage::Root => Ok(Some(MIN_VERSION.clone())), + PubGrubPackage::Root(_) => Ok(Some(MIN_VERSION.clone())), PubGrubPackage::Package(package_name, _extra, Some(url)) => { debug!("Searching for a compatible version of {package_name} @ {url} ({range})",); @@ -461,7 +463,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { request_sink: &futures::channel::mpsc::UnboundedSender, ) -> Result { match package { - PubGrubPackage::Root => { + PubGrubPackage::Root(_) => { let mut constraints = DependencyConstraints::>::default(); @@ -754,7 +756,7 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { fn on_progress(&self, package: &PubGrubPackage, version: &PubGrubVersion) { if let Some(reporter) = self.reporter.as_ref() { match package { - PubGrubPackage::Root => {} + PubGrubPackage::Root(_) => {} PubGrubPackage::Package(package_name, extra, Some(url)) => { reporter.on_progress(package_name, extra.as_ref(), VersionOrUrl::Url(url)); } diff --git a/crates/puffin-resolver/tests/resolver.rs b/crates/puffin-resolver/tests/resolver.rs index 962c33722..282868e75 100644 --- a/crates/puffin-resolver/tests/resolver.rs +++ b/crates/puffin-resolver/tests/resolver.rs @@ -70,6 +70,7 @@ async fn black() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -92,6 +93,7 @@ async fn black_colorama() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -114,6 +116,7 @@ async fn black_python_310() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_310, &TAGS_310, &client, &DummyContext); @@ -138,6 +141,7 @@ async fn black_mypy_extensions() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -162,6 +166,7 @@ async fn black_mypy_extensions_extra() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -186,6 +191,7 @@ async fn black_flake8() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -208,6 +214,7 @@ async fn black_lowest() -> Result<()> { vec![], ResolutionMode::Lowest, PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -230,6 +237,7 @@ async fn black_lowest_direct() -> Result<()> { vec![], ResolutionMode::LowestDirect, PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -252,6 +260,7 @@ async fn black_respect_preference() -> Result<()> { vec![Requirement::from_str("black==23.9.0").unwrap()], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -274,6 +283,7 @@ async fn black_ignore_preference() -> Result<()> { vec![Requirement::from_str("black==23.9.2").unwrap()], ResolutionMode::default(), PreReleaseMode::default(), + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -296,6 +306,7 @@ async fn black_disallow_prerelease() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::Disallow, + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -318,6 +329,7 @@ async fn black_allow_prerelease_if_necessary() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::IfNecessary, + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -340,6 +352,7 @@ async fn pylint_disallow_prerelease() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::Disallow, + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -362,6 +375,7 @@ async fn pylint_allow_prerelease() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::Allow, + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -387,6 +401,7 @@ async fn pylint_allow_explicit_prerelease_without_marker() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::Explicit, + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); @@ -412,6 +427,7 @@ async fn pylint_allow_explicit_prerelease_with_marker() -> Result<()> { vec![], ResolutionMode::default(), PreReleaseMode::Explicit, + None, ); let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext);