From 2d1e19e474cbbbcd0008e150988807f893aacada Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 5 Dec 2023 03:44:06 -0500 Subject: [PATCH] Allow yanked versions when specified via `==` (#561) ## Summary This enables users to rely on yanked versions via explicit `==` markers, which is necessary in some projects (and, in my opinion, reasonable). Closes #551. --- crates/puffin-cli/tests/pip_compile.rs | 54 +++++++++++++++++++ ...ompile__compile_yanked_version_direct.snap | 24 +++++++++ ...pile__compile_yanked_version_indirect.snap | 23 ++++++++ .../puffin-resolver/src/candidate_selector.rs | 2 +- crates/puffin-resolver/src/lib.rs | 1 + crates/puffin-resolver/src/resolver.rs | 8 +++ crates/puffin-resolver/src/version_map.rs | 27 +++++++--- crates/puffin-resolver/src/yanks.rs | 47 ++++++++++++++++ 8 files changed, 178 insertions(+), 8 deletions(-) create mode 100644 crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_direct.snap create mode 100644 crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_indirect.snap create mode 100644 crates/puffin-resolver/src/yanks.rs diff --git a/crates/puffin-cli/tests/pip_compile.rs b/crates/puffin-cli/tests/pip_compile.rs index 1ca043b0a..9d36dd4dd 100644 --- a/crates/puffin-cli/tests/pip_compile.rs +++ b/crates/puffin-cli/tests/pip_compile.rs @@ -1552,3 +1552,57 @@ fn compile_wheel_path_dependency_missing() -> Result<()> { Ok(()) } + +/// Resolve a yanked version of `attrs` by specifying the version directly. +#[test] +fn compile_yanked_version_direct() -> Result<()> { + let temp_dir = TempDir::new()?; + let cache_dir = TempDir::new()?; + let venv = create_venv_py312(&temp_dir, &cache_dir); + + let requirements_in = temp_dir.child("requirements.in"); + requirements_in.write_str("attrs==21.1.0")?; + + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-compile") + .arg("requirements.in") + .arg("--cache-dir") + .arg(cache_dir.path()) + .arg("--exclude-newer") + .arg(EXCLUDE_NEWER) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir)); + }); + + Ok(()) +} + +/// Fail to resolve `attrs` due to the indirect use of a yanked version (`21.1.0`). +#[test] +fn compile_yanked_version_indirect() -> Result<()> { + let temp_dir = TempDir::new()?; + let cache_dir = TempDir::new()?; + let venv = create_venv_py312(&temp_dir, &cache_dir); + + let requirements_in = temp_dir.child("requirements.in"); + requirements_in.write_str("attrs>20.3.0,<21.2.0")?; + + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-compile") + .arg("requirements.in") + .arg("--cache-dir") + .arg(cache_dir.path()) + .arg("--exclude-newer") + .arg(EXCLUDE_NEWER) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir)); + }); + + Ok(()) +} diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_direct.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_direct.snap new file mode 100644 index 000000000..0348c5754 --- /dev/null +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_direct.snap @@ -0,0 +1,24 @@ +--- +source: crates/puffin-cli/tests/pip_compile.rs +info: + program: puffin + args: + - pip-compile + - requirements.in + - "--cache-dir" + - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmplcbD5Q + - "--exclude-newer" + - "2023-11-18T12:00:00Z" + env: + VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpRGHD2t/.venv +--- +success: true +exit_code: 0 +----- stdout ----- +# This file was autogenerated by Puffin v0.0.1 via the following command: +# puffin pip-compile requirements.in --cache-dir [CACHE_DIR] +attrs==21.1.0 + +----- stderr ----- +Resolved 1 package in [TIME] + diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_indirect.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_indirect.snap new file mode 100644 index 000000000..ced310b25 --- /dev/null +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_yanked_version_indirect.snap @@ -0,0 +1,23 @@ +--- +source: crates/puffin-cli/tests/pip_compile.rs +info: + program: puffin + args: + - pip-compile + - requirements.in + - "--cache-dir" + - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpKpwYoD + - "--exclude-newer" + - "2023-11-18T12:00:00Z" + env: + VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmptdJhvi/.venv +--- +success: false +exit_code: 1 +----- stdout ----- + +----- stderr ----- + × No solution found when resolving dependencies: + ╰─▶ Because there is no version of attrs available matching >20.3.0, <21.2.0 + and root depends on attrs>20.3.0, <21.2.0, version solving failed. + diff --git a/crates/puffin-resolver/src/candidate_selector.rs b/crates/puffin-resolver/src/candidate_selector.rs index 2363c6b74..bbad4441f 100644 --- a/crates/puffin-resolver/src/candidate_selector.rs +++ b/crates/puffin-resolver/src/candidate_selector.rs @@ -56,7 +56,7 @@ impl From<&[Requirement]> for Preferences { else { return None; }; - let [version_specifier] = &**version_specifiers else { + let [version_specifier] = version_specifiers.as_ref() else { return None; }; let version = PubGrubVersion::from(version_specifier.version().clone()); diff --git a/crates/puffin-resolver/src/lib.rs b/crates/puffin-resolver/src/lib.rs index 8713d3bc0..3f6e4b461 100644 --- a/crates/puffin-resolver/src/lib.rs +++ b/crates/puffin-resolver/src/lib.rs @@ -20,3 +20,4 @@ mod resolution_mode; mod resolution_options; mod resolver; mod version_map; +mod yanks; diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index 7337927d3..9b7190439 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -36,6 +36,7 @@ use crate::pubgrub::{ }; use crate::resolution::Graph; use crate::version_map::VersionMap; +use crate::yanks::AllowedYanks; use crate::ResolutionOptions; pub struct Resolver<'a, Context: BuildContext + Send + Sync> { @@ -43,6 +44,7 @@ pub struct Resolver<'a, Context: BuildContext + Send + Sync> { requirements: Vec, constraints: Vec, allowed_urls: AllowedUrls, + allowed_yanks: AllowedYanks, markers: &'a MarkerEnvironment, tags: &'a Tags, client: &'a RegistryClient, @@ -79,6 +81,11 @@ impl<'a, Context: BuildContext + Send + Sync> Resolver<'a, Context> { } }) .collect(), + allowed_yanks: manifest + .requirements + .iter() + .chain(manifest.constraints.iter()) + .collect(), project: manifest.project, requirements: manifest.requirements, constraints: manifest.constraints, @@ -553,6 +560,7 @@ impl<'a, Context: BuildContext + Send + Sync> Resolver<'a, Context> { self.tags, self.markers, self.build_context.interpreter(), + &self.allowed_yanks, self.exclude_newer.as_ref(), ); self.index diff --git a/crates/puffin-resolver/src/version_map.rs b/crates/puffin-resolver/src/version_map.rs index 772f075e7..e960bbb4f 100644 --- a/crates/puffin-resolver/src/version_map.rs +++ b/crates/puffin-resolver/src/version_map.rs @@ -15,6 +15,7 @@ use pypi_types::{SimpleJson, Yanked}; use crate::file::{DistFile, SdistFile, WheelFile}; use crate::pubgrub::PubGrubVersion; +use crate::yanks::AllowedYanks; /// A map from versions to distributions. #[derive(Debug, Default)] @@ -28,6 +29,7 @@ impl VersionMap { tags: &Tags, markers: &MarkerEnvironment, interpreter: &Interpreter, + allowed_yanks: &AllowedYanks, exclude_newer: Option<&DateTime>, ) -> Self { let mut version_map: BTreeMap = @@ -72,14 +74,16 @@ impl VersionMap { } } - // When resolving, exclude yanked files. - // TODO(konstin): When we fail resolving due to a dependency locked to yanked version, - // we should tell the user. - if file.yanked.as_ref().is_some_and(Yanked::is_yanked) { - continue; - } - if let Ok(filename) = WheelFilename::from_str(file.filename.as_str()) { + // When resolving, exclude yanked files. + if file.yanked.as_ref().is_some_and(Yanked::is_yanked) { + if allowed_yanks.allowed(package_name, &filename.version) { + warn!("Allowing yanked version: {}", file.filename); + } else { + continue; + } + } + let priority = filename.compatibility(tags); match version_map.entry(filename.version.into()) { @@ -96,6 +100,15 @@ impl VersionMap { } else if let Ok(filename) = SourceDistFilename::parse(file.filename.as_str(), package_name) { + // When resolving, exclude yanked files. + if file.yanked.as_ref().is_some_and(Yanked::is_yanked) { + if allowed_yanks.allowed(package_name, &filename.version) { + warn!("Allowing yanked version: {}", file.filename); + } else { + continue; + } + } + match version_map.entry(filename.version.into()) { Entry::Occupied(mut entry) => { entry.get_mut().insert_source(SdistFile(file)); diff --git a/crates/puffin-resolver/src/yanks.rs b/crates/puffin-resolver/src/yanks.rs new file mode 100644 index 000000000..44492d8be --- /dev/null +++ b/crates/puffin-resolver/src/yanks.rs @@ -0,0 +1,47 @@ +use fxhash::{FxHashMap, FxHashSet}; + +use pep440_rs::Version; +use pep508_rs::Requirement; +use puffin_normalize::PackageName; + +/// A set of package versions that are permitted, even if they're marked as yanked by the +/// relevant index. +#[derive(Debug, Default)] +pub(crate) struct AllowedYanks(FxHashMap>); + +impl AllowedYanks { + /// Returns `true` if the given package version is allowed, even if it's marked as yanked by + /// the relevant index. + pub(crate) fn allowed(&self, package_name: &PackageName, version: &Version) -> bool { + self.0 + .get(package_name) + .map(|allowed_yanks| allowed_yanks.contains(version)) + .unwrap_or_default() + } +} + +impl<'a> FromIterator<&'a Requirement> for AllowedYanks { + fn from_iter>(iter: T) -> Self { + let mut allowed_yanks = FxHashMap::>::default(); + for requirement in iter { + let Some(pep508_rs::VersionOrUrl::VersionSpecifier(specifiers)) = + &requirement.version_or_url + else { + continue; + }; + let [specifier] = specifiers.as_ref() else { + continue; + }; + if matches!( + specifier.operator(), + pep440_rs::Operator::Equal | pep440_rs::Operator::ExactEqual + ) { + allowed_yanks + .entry(requirement.name.clone()) + .or_default() + .insert(specifier.version().clone()); + } + } + Self(allowed_yanks) + } +}