From 76a41066ace64478eebeb8c55a16b325a06d3911 Mon Sep 17 00:00:00 2001 From: konsti Date: Mon, 13 Nov 2023 17:14:07 +0100 Subject: [PATCH] Filter out incompatible dists (#398) Filter out source dists and wheels whose `requires-python` from the simple api is incompatible with the current python version. This change showed an important problem: When we use a fake python version for resolving, building source distributions breaks down because we can only build with versions we actually have. This change became surprisingly big. The tests now require python 3.7 to be installed, but changing that would mean an even bigger change. Fixes #388 --- .github/workflows/ci.yaml | 5 +- .gitignore | 3 + crates/pep440-rs/src/version_specifier.rs | 10 ++ crates/puffin-cli/src/commands/pip_compile.rs | 7 +- crates/puffin-cli/src/commands/pip_sync.rs | 5 +- crates/puffin-cli/tests/pip_compile.rs | 49 +++++++ crates/puffin-cli/tests/pip_sync.rs | 49 ++++++- ..._compile__compile_constraints_markers.snap | 4 +- .../pip_compile__compile_constraints_txt.snap | 4 +- .../pip_compile__compile_pyproject_toml.snap | 4 +- ...le__compile_pyproject_toml_all_extras.snap | 4 +- ...compile__compile_pyproject_toml_extra.snap | 4 +- ...project_toml_extra_name_normalization.snap | 4 +- .../pip_compile__compile_python_37.snap | 36 +---- .../pip_compile__compile_requirements_in.snap | 4 +- .../pip_sync__install_numpy_py38.snap | 23 +++ crates/puffin-dispatch/src/lib.rs | 2 +- crates/puffin-distribution/src/traits.rs | 4 +- .../src/interpreter_info.rs | 21 +++ crates/puffin-resolver/Cargo.toml | 1 + crates/puffin-resolver/src/finder.rs | 23 ++- crates/puffin-resolver/src/resolver.rs | 15 ++ crates/puffin-resolver/tests/resolver.rs | 131 ++++++------------ crates/pypi-types/src/lenient_requirement.rs | 64 +++++++++ crates/pypi-types/src/lib.rs | 2 + crates/pypi-types/src/metadata.rs | 49 +------ crates/pypi-types/src/simple_json.rs | 25 +++- 27 files changed, 365 insertions(+), 187 deletions(-) create mode 100644 crates/puffin-cli/tests/snapshots/pip_sync__install_numpy_py38.snap create mode 100644 crates/pypi-types/src/lenient_requirement.rs diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 2c5ee44387c1..43e2f80050db 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -48,10 +48,13 @@ jobs: name: "cargo test | ${{ matrix.os }}" steps: - uses: actions/checkout@v4 + # TODO(konstin): Mock the venv in the installer test so we don't need 3.8 anymore - name: "Install Python" uses: actions/setup-python@v4 with: - python-version: ${{ env.PYTHON_VERSION }} + python-version: | + 3.8 + ${{ env.PYTHON_VERSION }} - name: "Install Rust toolchain" run: rustup show - name: "Install cargo insta" diff --git a/.gitignore b/.gitignore index ea73bbd3e4db..da5e94a6c326 100644 --- a/.gitignore +++ b/.gitignore @@ -13,3 +13,6 @@ target/ # Use e.g. `--cache-dir cache-docker` to keep a cache across container invocations cache-* + +# python tmp files +__pycache__ diff --git a/crates/pep440-rs/src/version_specifier.rs b/crates/pep440-rs/src/version_specifier.rs index e4f96b298c94..142bd487f6ec 100644 --- a/crates/pep440-rs/src/version_specifier.rs +++ b/crates/pep440-rs/src/version_specifier.rs @@ -540,6 +540,9 @@ impl Display for VersionSpecifier { /// ``` pub fn parse_version_specifiers(spec: &str) -> Result, Pep440Error> { let mut version_ranges = Vec::new(); + if spec.is_empty() { + return Ok(version_ranges); + } let mut start: usize = 0; let separator = ","; for version_range_spec in spec.split(separator) { @@ -1301,4 +1304,11 @@ mod test { ">=3.7, <4.0, !=3.9.0" ); } + + /// These occur in the simple api, e.g. + /// + #[test] + fn test_version_specifiers_empty() { + assert_eq!(VersionSpecifiers::from_str("").unwrap().to_string(), ""); + } } diff --git a/crates/puffin-cli/src/commands/pip_compile.rs b/crates/puffin-cli/src/commands/pip_compile.rs index 04e9c41bea47..b943db706026 100644 --- a/crates/puffin-cli/src/commands/pip_compile.rs +++ b/crates/puffin-cli/src/commands/pip_compile.rs @@ -125,6 +125,11 @@ pub(crate) async fn pip_compile( || Cow::Borrowed(venv.interpreter_info().markers()), |python_version| Cow::Owned(python_version.markers(venv.interpreter_info().markers())), ); + // Inject the fake python version if necessary + let interpreter_info = venv + .interpreter_info() + .clone() + .patch_markers(markers.clone().into_owned()); // Instantiate a client. let client = { @@ -143,7 +148,7 @@ pub(crate) async fn pip_compile( let build_dispatch = BuildDispatch::new( client.clone(), cache.to_path_buf(), - venv.interpreter_info().clone(), + interpreter_info, fs::canonicalize(venv.python_executable())?, no_build, ); diff --git a/crates/puffin-cli/src/commands/pip_sync.rs b/crates/puffin-cli/src/commands/pip_sync.rs index 00c189fc0d99..7a4ccaa8dc80 100644 --- a/crates/puffin-cli/src/commands/pip_sync.rs +++ b/crates/puffin-cli/src/commands/pip_sync.rs @@ -128,8 +128,9 @@ pub(crate) async fn sync_requirements( } else { let start = std::time::Instant::now(); - let wheel_finder = puffin_resolver::DistFinder::new(&tags, &client) - .with_reporter(FinderReporter::from(printer).with_length(remote.len() as u64)); + let wheel_finder = + puffin_resolver::DistFinder::new(&tags, &client, venv.interpreter_info()) + .with_reporter(FinderReporter::from(printer).with_length(remote.len() as u64)); let resolution = wheel_finder.resolve(&remote).await?; let s = if resolution.len() == 1 { "" } else { "s" }; diff --git a/crates/puffin-cli/tests/pip_compile.rs b/crates/puffin-cli/tests/pip_compile.rs index 170e20fe780a..a3716e7fe923 100644 --- a/crates/puffin-cli/tests/pip_compile.rs +++ b/crates/puffin-cli/tests/pip_compile.rs @@ -630,6 +630,55 @@ fn compile_python_37() -> Result<()> { Ok(()) } +/// Test that we select the last 3.8 compatible numpy version instead of trying to compile an +/// incompatible sdist +#[test] +fn compile_numpy_py38() -> 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 requirements_in = temp_dir.child("requirements.in"); + requirements_in.touch()?; + requirements_in.write_str("numpy")?; + + 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("--python-version") + .arg("py38") + .arg("--cache-dir") + .arg(cache_dir.path()) + // Check that we select the wheel and not the sdist + .arg("--no-build") + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir), @r###" + success: false + exit_code: 2 + ----- stdout ----- + + ----- stderr ----- + error: Failed to build distribution: numpy-1.24.4.tar.gz + Caused by: Building source distributions is disabled + "###); + }); + + Ok(()) +} + /// Resolve a specific Flask wheel via a URL dependency. #[test] fn compile_wheel_url_dependency() -> Result<()> { diff --git a/crates/puffin-cli/tests/pip_sync.rs b/crates/puffin-cli/tests/pip_sync.rs index 6d66c72f58f5..001fd730e79b 100644 --- a/crates/puffin-cli/tests/pip_sync.rs +++ b/crates/puffin-cli/tests/pip_sync.rs @@ -2,7 +2,7 @@ use std::process::Command; -use anyhow::Result; +use anyhow::{Context, Result}; use assert_cmd::prelude::*; use assert_fs::prelude::*; use insta_cmd::_macro_support::insta; @@ -928,3 +928,50 @@ fn install_version_then_install_url() -> Result<()> { Ok(()) } + +/// Test that we select the last 3.8 compatible numpy version instead of trying to compile an +/// incompatible sdist +#[test] +fn install_numpy_py38() -> 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("--python") + // TODO(konstin): Mock the venv in the installer test so we don't need this anymore + .arg(which::which("python3.8").context("python3.8 must be installed")?) + .arg("--cache-dir") + .arg(cache_dir.path()) + .current_dir(&temp_dir) + .assert() + .success(); + venv.assert(predicates::path::is_dir()); + + let requirements_txt = temp_dir.child("requirements.txt"); + requirements_txt.touch()?; + requirements_txt.write_str("numpy")?; + + insta::with_settings!({ + filters => INSTA_FILTERS.to_vec() + }, { + assert_cmd_snapshot!(Command::new(get_cargo_bin(BIN_NAME)) + .arg("pip-sync") + .arg("requirements.txt") + .arg("--cache-dir") + .arg(cache_dir.path()) + .env("VIRTUAL_ENV", venv.as_os_str()) + .current_dir(&temp_dir)); + }); + + Command::new(venv.join("bin").join("python")) + .arg("-c") + .arg("import numpy") + .current_dir(&temp_dir) + .assert() + .success(); + + Ok(()) +} diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_markers.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_markers.snap index 070d5bf3201b..042ad37b078c 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_markers.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_markers.snap @@ -8,9 +8,9 @@ info: - "--constraint" - constraints.txt - "--cache-dir" - - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp1Ts53o + - /tmp/.tmp81zzAa env: - VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp1vzBQa/.venv + VIRTUAL_ENV: /tmp/.tmpFXMFLd/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_txt.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_txt.snap index e2b3adf725e5..49cd0a9e4d9b 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_txt.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_constraints_txt.snap @@ -8,9 +8,9 @@ info: - "--constraint" - constraints.txt - "--cache-dir" - - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpvjdHOb + - /tmp/.tmpuS4Jn4 env: - VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpSAaWi3/.venv + VIRTUAL_ENV: /tmp/.tmpnFovuD/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml.snap index 1ace0bbb35f3..42b7e1d50e90 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml.snap @@ -6,9 +6,9 @@ info: - pip-compile - pyproject.toml - "--cache-dir" - - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpOOTFwj + - /tmp/.tmpWzFzu7 env: - VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpU0VXyY/.venv + VIRTUAL_ENV: /tmp/.tmpSiRk6d/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_all_extras.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_all_extras.snap index 2e65eb521d31..d37d801d099c 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_all_extras.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_all_extras.snap @@ -7,9 +7,9 @@ info: - pyproject.toml - "--all-extras" - "--cache-dir" - - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpFzJKRe + - /tmp/.tmpQHVFqr env: - VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpdadcmu/.venv + VIRTUAL_ENV: /tmp/.tmpmKXqaF/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra.snap index 78e32bbacef9..66f648165ac4 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra.snap @@ -8,9 +8,9 @@ info: - "--extra" - foo - "--cache-dir" - - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpAYEAdM + - /tmp/.tmpLjFVr0 env: - VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmp1xuOcV/.venv + VIRTUAL_ENV: /tmp/.tmpDT0oRC/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra_name_normalization.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra_name_normalization.snap index 5eac4f49f1de..36f83c9a5158 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra_name_normalization.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_pyproject_toml_extra_name_normalization.snap @@ -8,9 +8,9 @@ info: - "--extra" - FRiENDlY-...-_-BARd - "--cache-dir" - - /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpFyLXQn + - /tmp/.tmpga6JO1 env: - VIRTUAL_ENV: /var/folders/bc/qlsk3t6x7c9fhhbvvcg68k9c0000gp/T/.tmpxfZatv/.venv + VIRTUAL_ENV: /tmp/.tmpwmJIym/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_python_37.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_python_37.snap index ec4e94eb422b..288ba44826d0 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_python_37.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_python_37.snap @@ -8,38 +8,16 @@ info: - "--python-version" - py37 - "--cache-dir" - - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpzwzUVe + - /tmp/.tmpHz7Dc2 env: - VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpqFv4YL/.venv + VIRTUAL_ENV: /tmp/.tmpoMjDJT/.venv --- -success: true -exit_code: 0 +success: false +exit_code: 1 ----- stdout ----- -# This file was autogenerated by Puffin v0.0.1 via the following command: -# [BIN_PATH] pip-compile requirements.in --python-version py37 --cache-dir [CACHE_DIR] -black==23.10.1 -click==8.1.7 - # via black -importlib-metadata==6.8.0 - # via click -mypy-extensions==1.0.0 - # via black -packaging==23.2 - # via black -pathspec==0.11.2 - # via black -platformdirs==4.0.0 - # via black -tomli==2.0.1 - # via black -typing-extensions==4.8.0 - # via - # black - # importlib-metadata - # platformdirs -zipp==3.17.0 - # via importlib-metadata ----- stderr ----- -Resolved 10 packages in [TIME] + × 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. diff --git a/crates/puffin-cli/tests/snapshots/pip_compile__compile_requirements_in.snap b/crates/puffin-cli/tests/snapshots/pip_compile__compile_requirements_in.snap index 04474bfc932c..da69b17702cd 100644 --- a/crates/puffin-cli/tests/snapshots/pip_compile__compile_requirements_in.snap +++ b/crates/puffin-cli/tests/snapshots/pip_compile__compile_requirements_in.snap @@ -6,9 +6,9 @@ info: - pip-compile - requirements.in - "--cache-dir" - - /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmpxF1zFY + - /tmp/.tmpQB4Ze4 env: - VIRTUAL_ENV: /var/folders/nt/6gf2v7_s3k13zq_t3944rwz40000gn/T/.tmp33QZdv/.venv + VIRTUAL_ENV: /tmp/.tmpSYcMgG/.venv --- success: true exit_code: 0 diff --git a/crates/puffin-cli/tests/snapshots/pip_sync__install_numpy_py38.snap b/crates/puffin-cli/tests/snapshots/pip_sync__install_numpy_py38.snap new file mode 100644 index 000000000000..a7d0072a3f63 --- /dev/null +++ b/crates/puffin-cli/tests/snapshots/pip_sync__install_numpy_py38.snap @@ -0,0 +1,23 @@ +--- +source: crates/puffin-cli/tests/pip_sync.rs +info: + program: puffin + args: + - pip-sync + - requirements.txt + - "--cache-dir" + - /tmp/.tmpL9E9fl + env: + VIRTUAL_ENV: /tmp/.tmp0yDHj5/.venv +--- +success: true +exit_code: 0 +----- stdout ----- + +----- stderr ----- +Resolved 1 package in [TIME] +Downloaded 1 package in [TIME] +Unzipped 1 package in [TIME] +Installed 1 package in [TIME] + + numpy==1.24.4 + diff --git a/crates/puffin-dispatch/src/lib.rs b/crates/puffin-dispatch/src/lib.rs index ad6577a26b50..f275a266b2c9 100644 --- a/crates/puffin-dispatch/src/lib.rs +++ b/crates/puffin-dispatch/src/lib.rs @@ -135,7 +135,7 @@ impl BuildContext for BuildDispatch { if remote.len() == 1 { "" } else { "s" }, remote.iter().map(ToString::to_string).join(", ") ); - let resolution = DistFinder::new(&tags, &self.client) + let resolution = DistFinder::new(&tags, &self.client, self.interpreter_info()) .resolve(&remote) .await .context("Failed to resolve build dependencies")?; diff --git a/crates/puffin-distribution/src/traits.rs b/crates/puffin-distribution/src/traits.rs index 858f228b1506..a40bdda6a3ee 100644 --- a/crates/puffin-distribution/src/traits.rs +++ b/crates/puffin-distribution/src/traits.rs @@ -12,8 +12,8 @@ pub trait Metadata { /// Return the normalized [`PackageName`] of the distribution. fn name(&self) -> &PackageName; - /// Return a [`Version`], for registry-based distributions, or a [`Url`], for URL-based - /// distributions. + /// Return a [`pep440_rs::Version`], for registry-based distributions, or a [`url::Url`], + /// for URL-based distributions. fn version_or_url(&self) -> VersionOrUrl; /// Returns a unique identifier for the package. diff --git a/crates/puffin-interpreter/src/interpreter_info.rs b/crates/puffin-interpreter/src/interpreter_info.rs index 7f06a1ba6e1f..9fdbe47bc360 100644 --- a/crates/puffin-interpreter/src/interpreter_info.rs +++ b/crates/puffin-interpreter/src/interpreter_info.rs @@ -44,6 +44,21 @@ impl InterpreterInfo { base_prefix: info.base_prefix, }) } + + // TODO(konstin): Find a better way mocking the fields + pub fn artificial( + platform: Platform, + markers: MarkerEnvironment, + base_exec_prefix: PathBuf, + base_prefix: PathBuf, + ) -> Self { + Self { + platform: PythonPlatform(platform), + markers, + base_exec_prefix, + base_prefix, + } + } } impl InterpreterInfo { @@ -76,6 +91,12 @@ impl InterpreterInfo { pub fn base_prefix(&self) -> &Path { &self.base_prefix } + + /// Inject markers of a fake python version + #[must_use] + pub fn patch_markers(self, markers: MarkerEnvironment) -> Self { + Self { markers, ..self } + } } #[derive(Debug, Error)] diff --git a/crates/puffin-resolver/Cargo.toml b/crates/puffin-resolver/Cargo.toml index c3af190bcfa5..7e07fcbdfbdc 100644 --- a/crates/puffin-resolver/Cargo.toml +++ b/crates/puffin-resolver/Cargo.toml @@ -21,6 +21,7 @@ puffin-cache = { path = "../puffin-cache" } puffin-client = { path = "../puffin-client" } puffin-distribution = { path = "../puffin-distribution" } puffin-git = { path = "../puffin-git" } +puffin-interpreter = { path = "../puffin-interpreter" } puffin-normalize = { path = "../puffin-normalize" } puffin-traits = { path = "../puffin-traits" } pypi-types = { path = "../pypi-types" } diff --git a/crates/puffin-resolver/src/finder.rs b/crates/puffin-resolver/src/finder.rs index 98ac10b327f8..7f26d1f6f2e4 100644 --- a/crates/puffin-resolver/src/finder.rs +++ b/crates/puffin-resolver/src/finder.rs @@ -14,6 +14,7 @@ use pep508_rs::{Requirement, VersionOrUrl}; use platform_tags::Tags; use puffin_client::RegistryClient; use puffin_distribution::Dist; +use puffin_interpreter::InterpreterInfo; use puffin_normalize::PackageName; use pypi_types::{File, SimpleJson}; @@ -24,15 +25,21 @@ pub struct DistFinder<'a> { tags: &'a Tags, client: &'a RegistryClient, reporter: Option>, + interpreter_info: &'a InterpreterInfo, } impl<'a> DistFinder<'a> { /// Initialize a new distribution finder. - pub fn new(tags: &'a Tags, client: &'a RegistryClient) -> Self { + pub fn new( + tags: &'a Tags, + client: &'a RegistryClient, + interpreter_info: &'a InterpreterInfo, + ) -> Self { Self { tags, client, reporter: None, + interpreter_info, } } @@ -129,6 +136,20 @@ impl<'a> DistFinder<'a> { fn select(&self, requirement: &Requirement, files: Vec) -> Option { let mut fallback = None; for file in files.into_iter().rev() { + // Only add dists compatible with the python version. + // 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() + .map_or(true, |requires_python| { + requires_python.contains(self.interpreter_info.version()) + }) + { + continue; + } if let Ok(wheel) = WheelFilename::from_str(file.filename.as_str()) { if !wheel.is_compatible(self.tags) { continue; diff --git a/crates/puffin-resolver/src/resolver.rs b/crates/puffin-resolver/src/resolver.rs index 18bd03533594..17aceb395d97 100644 --- a/crates/puffin-resolver/src/resolver.rs +++ b/crates/puffin-resolver/src/resolver.rs @@ -536,6 +536,21 @@ impl<'a, Context: BuildContext + Sync> Resolver<'a, Context> { // distributions. let mut version_map: VersionMap = BTreeMap::new(); for file in metadata.files { + // Only add dists compatible with the python version. + // 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() + .map_or(true, |requires_python| { + requires_python + .contains(self.build_context.interpreter_info().version()) + }) + { + continue; + } if let Ok(filename) = WheelFilename::from_str(file.filename.as_str()) { if filename.is_compatible(self.tags) { let version = PubGrubVersion::from(filename.version.clone()); diff --git a/crates/puffin-resolver/tests/resolver.rs b/crates/puffin-resolver/tests/resolver.rs index 87451691699d..f8550a5eba0a 100644 --- a/crates/puffin-resolver/tests/resolver.rs +++ b/crates/puffin-resolver/tests/resolver.rs @@ -4,7 +4,7 @@ //! `PyPI` directly. use std::future::Future; -use std::path::Path; +use std::path::{Path, PathBuf}; use std::pin::Pin; use std::str::FromStr; @@ -17,10 +17,12 @@ use platform_host::{Arch, Os, Platform}; use platform_tags::Tags; use puffin_client::RegistryClientBuilder; use puffin_interpreter::{InterpreterInfo, Virtualenv}; -use puffin_resolver::{Manifest, PreReleaseMode, ResolutionMode, Resolver}; +use puffin_resolver::{Graph, Manifest, PreReleaseMode, ResolutionMode, Resolver}; use puffin_traits::BuildContext; -struct DummyContext; +struct DummyContext { + interpreter_info: InterpreterInfo, +} impl BuildContext for DummyContext { fn cache(&self) -> &Path { @@ -28,7 +30,7 @@ impl BuildContext for DummyContext { } fn interpreter_info(&self) -> &InterpreterInfo { - panic!("The test should not need to build source distributions") + &self.interpreter_info } fn base_python(&self) -> &Path { @@ -61,13 +63,29 @@ impl BuildContext for DummyContext { } } +async fn resolve( + manifest: Manifest, + markers: &'static MarkerEnvironment, + tags: &Tags, +) -> Result { + let tempdir = tempdir()?; + let client = RegistryClientBuilder::new(tempdir.path()).build(); + let build_context = DummyContext { + interpreter_info: InterpreterInfo::artificial( + Platform::current()?, + markers.clone(), + PathBuf::from("/dev/null"), + PathBuf::from("/dev/null"), + ), + }; + let resolver = Resolver::new(manifest, markers, tags, &client, &build_context); + Ok(resolver.resolve().await?) +} + #[tokio::test] async fn black() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![], @@ -77,8 +95,7 @@ async fn black() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -89,9 +106,6 @@ async fn black() -> Result<()> { async fn black_colorama() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black[colorama]<=23.9.1").unwrap()], vec![], @@ -101,8 +115,7 @@ async fn black_colorama() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -113,9 +126,6 @@ async fn black_colorama() -> Result<()> { async fn black_python_310() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![], @@ -125,8 +135,7 @@ async fn black_python_310() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_310, &TAGS_310, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_310, &TAGS_310).await?; insta::assert_display_snapshot!(resolution); @@ -139,9 +148,6 @@ async fn black_python_310() -> Result<()> { async fn black_mypy_extensions() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![Requirement::from_str("mypy-extensions<0.4.4").unwrap()], @@ -151,8 +157,7 @@ async fn black_mypy_extensions() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -165,9 +170,6 @@ async fn black_mypy_extensions() -> Result<()> { async fn black_mypy_extensions_extra() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![Requirement::from_str("mypy-extensions[extra]<0.4.4").unwrap()], @@ -177,8 +179,7 @@ async fn black_mypy_extensions_extra() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -191,9 +192,6 @@ async fn black_mypy_extensions_extra() -> Result<()> { async fn black_flake8() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![Requirement::from_str("flake8<1").unwrap()], @@ -203,8 +201,7 @@ async fn black_flake8() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -215,9 +212,6 @@ async fn black_flake8() -> Result<()> { async fn black_lowest() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black>21").unwrap()], vec![], @@ -227,8 +221,7 @@ async fn black_lowest() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -239,9 +232,6 @@ async fn black_lowest() -> Result<()> { async fn black_lowest_direct() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black>21").unwrap()], vec![], @@ -251,8 +241,7 @@ async fn black_lowest_direct() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -263,9 +252,6 @@ async fn black_lowest_direct() -> Result<()> { async fn black_respect_preference() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![], @@ -275,8 +261,7 @@ async fn black_respect_preference() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -287,9 +272,6 @@ async fn black_respect_preference() -> Result<()> { async fn black_ignore_preference() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=23.9.1").unwrap()], vec![], @@ -299,8 +281,7 @@ async fn black_ignore_preference() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -311,9 +292,6 @@ async fn black_ignore_preference() -> Result<()> { async fn black_disallow_prerelease() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=20.0").unwrap()], vec![], @@ -323,8 +301,9 @@ async fn black_disallow_prerelease() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let err = resolver.resolve().await.unwrap_err(); + let err = resolve(manifest, &MARKERS_311, &TAGS_311) + .await + .unwrap_err(); insta::assert_display_snapshot!(err); @@ -335,9 +314,6 @@ async fn black_disallow_prerelease() -> Result<()> { async fn black_allow_prerelease_if_necessary() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("black<=20.0").unwrap()], vec![], @@ -347,10 +323,11 @@ async fn black_allow_prerelease_if_necessary() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await.unwrap_err(); + let err = resolve(manifest, &MARKERS_311, &TAGS_311) + .await + .unwrap_err(); - insta::assert_display_snapshot!(resolution); + insta::assert_display_snapshot!(err); Ok(()) } @@ -359,9 +336,6 @@ async fn black_allow_prerelease_if_necessary() -> Result<()> { async fn pylint_disallow_prerelease() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("pylint==2.3.0").unwrap()], vec![], @@ -371,8 +345,7 @@ async fn pylint_disallow_prerelease() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -383,9 +356,6 @@ async fn pylint_disallow_prerelease() -> Result<()> { async fn pylint_allow_prerelease() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![Requirement::from_str("pylint==2.3.0").unwrap()], vec![], @@ -395,8 +365,7 @@ async fn pylint_allow_prerelease() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -407,9 +376,6 @@ async fn pylint_allow_prerelease() -> Result<()> { async fn pylint_allow_explicit_prerelease_without_marker() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![ Requirement::from_str("pylint==2.3.0").unwrap(), @@ -422,8 +388,7 @@ async fn pylint_allow_explicit_prerelease_without_marker() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); @@ -434,9 +399,6 @@ async fn pylint_allow_explicit_prerelease_without_marker() -> Result<()> { async fn pylint_allow_explicit_prerelease_with_marker() -> Result<()> { colored::control::set_override(false); - let tempdir = tempdir()?; - let client = RegistryClientBuilder::new(tempdir.path()).build(); - let manifest = Manifest::new( vec![ Requirement::from_str("pylint==2.3.0").unwrap(), @@ -449,8 +411,7 @@ async fn pylint_allow_explicit_prerelease_with_marker() -> Result<()> { None, ); - let resolver = Resolver::new(manifest, &MARKERS_311, &TAGS_311, &client, &DummyContext); - let resolution = resolver.resolve().await?; + let resolution = resolve(manifest, &MARKERS_311, &TAGS_311).await?; insta::assert_display_snapshot!(resolution); diff --git a/crates/pypi-types/src/lenient_requirement.rs b/crates/pypi-types/src/lenient_requirement.rs new file mode 100644 index 000000000000..fc597682e0f3 --- /dev/null +++ b/crates/pypi-types/src/lenient_requirement.rs @@ -0,0 +1,64 @@ +use pep440_rs::{Pep440Error, VersionSpecifiers}; +use serde::{de, Deserialize, Deserializer, Serialize}; +use std::str::FromStr; +use tracing::warn; + +/// Like [`VersionSpecifiers`], but attempts to correct some common errors in user-provided requirements. +/// +/// We turn `>=3.x.*` into `>=3.x` +#[derive(Debug, Clone, Serialize, Eq, PartialEq)] +pub struct LenientVersionSpecifiers(VersionSpecifiers); + +impl FromStr for LenientVersionSpecifiers { + type Err = Pep440Error; + + fn from_str(s: &str) -> Result { + match VersionSpecifiers::from_str(s) { + Ok(specifiers) => Ok(Self(specifiers)), + Err(err) => { + // Given `>=3.5.*`, rewrite to `>=3.5`. + let patched = match s { + ">=3.12.*" => Some(">=3.12"), + ">=3.11.*" => Some(">=3.11"), + ">=3.10.*" => Some(">=3.10"), + ">=3.9.*" => Some(">=3.9"), + ">=3.8.*" => Some(">=3.8"), + ">=3.7.*" => Some(">=3.7"), + ">=3.6.*" => Some(">=3.6"), + ">=3.5.*" => Some(">=3.5"), + ">=3.4.*" => Some(">=3.4"), + ">=3.3.*" => Some(">=3.3"), + ">=3.2.*" => Some(">=3.2"), + ">=3.1.*" => Some(">=3.1"), + ">=3.0.*" => Some(">=3.0"), + _ => None, + }; + if let Some(patched) = patched { + if let Ok(specifier) = VersionSpecifiers::from_str(patched) { + warn!( + "Correcting invalid wildcard bound on version specifier (before: `{s}`; after: `{patched}`)", + ); + return Ok(Self(specifier)); + } + } + Err(err) + } + } + } +} + +impl From for VersionSpecifiers { + fn from(specifiers: LenientVersionSpecifiers) -> Self { + specifiers.0 + } +} + +impl<'de> Deserialize<'de> for LenientVersionSpecifiers { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let s = String::deserialize(deserializer)?; + Self::from_str(&s).map_err(de::Error::custom) + } +} diff --git a/crates/pypi-types/src/lib.rs b/crates/pypi-types/src/lib.rs index 04393f84889e..6fccd14755ec 100644 --- a/crates/pypi-types/src/lib.rs +++ b/crates/pypi-types/src/lib.rs @@ -1,7 +1,9 @@ pub use direct_url::{ArchiveInfo, DirectUrl, VcsInfo, VcsKind}; +pub use lenient_requirement::LenientVersionSpecifiers; pub use metadata::{Error, Metadata21}; pub use simple_json::{File, SimpleJson}; mod direct_url; +mod lenient_requirement; mod metadata; mod simple_json; diff --git a/crates/pypi-types/src/metadata.rs b/crates/pypi-types/src/metadata.rs index e4d7258c4d85..f63140be99f9 100644 --- a/crates/pypi-types/src/metadata.rs +++ b/crates/pypi-types/src/metadata.rs @@ -11,6 +11,7 @@ use serde::{Deserialize, Serialize}; use thiserror::Error; use tracing::warn; +use crate::lenient_requirement::LenientVersionSpecifiers; use pep440_rs::{Pep440Error, Version, VersionSpecifiers}; use pep508_rs::{Pep508Error, Requirement}; use puffin_normalize::{ExtraName, InvalidNameError, PackageName}; @@ -271,54 +272,6 @@ impl From for Requirement { } } -/// Like [`VersionSpecifiers`], but attempts to correct some common errors in user-provided requirements. -#[derive(Debug, Clone, Serialize, Deserialize, Eq, PartialEq)] -struct LenientVersionSpecifiers(VersionSpecifiers); - -impl FromStr for LenientVersionSpecifiers { - type Err = Pep440Error; - - fn from_str(s: &str) -> Result { - match VersionSpecifiers::from_str(s) { - Ok(specifiers) => Ok(Self(specifiers)), - Err(err) => { - // Given `>=3.5.*`, rewrite to `>=3.5`. - let patched = match s { - ">=3.12.*" => Some(">=3.12"), - ">=3.11.*" => Some(">=3.11"), - ">=3.10.*" => Some(">=3.10"), - ">=3.9.*" => Some(">=3.9"), - ">=3.8.*" => Some(">=3.8"), - ">=3.7.*" => Some(">=3.7"), - ">=3.6.*" => Some(">=3.6"), - ">=3.5.*" => Some(">=3.5"), - ">=3.4.*" => Some(">=3.4"), - ">=3.3.*" => Some(">=3.3"), - ">=3.2.*" => Some(">=3.2"), - ">=3.1.*" => Some(">=3.1"), - ">=3.0.*" => Some(">=3.0"), - _ => None, - }; - if let Some(patched) = patched { - if let Ok(specifier) = VersionSpecifiers::from_str(patched) { - warn!( - "Correcting invalid wildcard bound on version specifier (before: `{s}`; after: `{patched}`)", - ); - return Ok(Self(specifier)); - } - } - Err(err) - } - } - } -} - -impl From for VersionSpecifiers { - fn from(specifiers: LenientVersionSpecifiers) -> Self { - specifiers.0 - } -} - #[cfg(test)] mod tests { use std::str::FromStr; diff --git a/crates/pypi-types/src/simple_json.rs b/crates/pypi-types/src/simple_json.rs index 4f424b28011f..56138b8f3b0d 100644 --- a/crates/pypi-types/src/simple_json.rs +++ b/crates/pypi-types/src/simple_json.rs @@ -1,4 +1,8 @@ -use serde::{Deserialize, Serialize}; +use pep440_rs::VersionSpecifiers; +use serde::{de, Deserialize, Deserializer, Serialize}; +use std::str::FromStr; + +use crate::lenient_requirement::LenientVersionSpecifiers; #[derive(Debug, Clone, Serialize, Deserialize)] pub struct SimpleJson { @@ -15,13 +19,30 @@ pub struct File { pub data_dist_info_metadata: Metadata, pub filename: String, pub hashes: Hashes, - pub requires_python: Option, + /// Note: Deserialized with [`LenientVersionSpecifiers`] since there are a number of invalid + /// versions on pypi + #[serde(deserialize_with = "deserialize_version_specifiers_lenient")] + pub requires_python: Option, pub size: usize, pub upload_time: String, pub url: String, pub yanked: Yanked, } +fn deserialize_version_specifiers_lenient<'de, D>( + deserializer: D, +) -> Result, D::Error> +where + D: Deserializer<'de>, +{ + let maybe_string: Option = Option::deserialize(deserializer)?; + let Some(string) = maybe_string else { + return Ok(None); + }; + let lenient = LenientVersionSpecifiers::from_str(&string).map_err(de::Error::custom)?; + Ok(Some(lenient.into())) +} + #[derive(Debug, Clone, Serialize, Deserialize)] #[serde(untagged)] pub enum Metadata {