From cccb6820dbafae1175ea7bd1806c8b87ffd3c25f Mon Sep 17 00:00:00 2001 From: Charlie Marsh Date: Tue, 6 Aug 2024 20:54:51 -0400 Subject: [PATCH] Avoid reusing incompatible distributions across lock and sync (#5845) ## Summary We need to avoid using incompatible versions for build dependencies that are also part of the resolved environment. This is a very subtle issue, but: when locking, we don't enforce platform compatibility. So, if we reuse the resolver state to install, and the install itself has to preform a resolution (e.g., for the build dependencies of a source distribution), that resolution may choose incompatible versions. The key property here is that there's a shared package between the build dependencies and the project dependencies. Closes https://github.com/astral-sh/uv/issues/5836. --- crates/uv/src/commands/project/add.rs | 7 ++- crates/uv/src/commands/project/lock.rs | 17 ++++-- crates/uv/src/commands/project/remove.rs | 7 ++- crates/uv/src/commands/project/run.rs | 1 - crates/uv/src/commands/project/sync.rs | 7 ++- crates/uv/src/commands/project/tree.rs | 3 -- crates/uv/tests/sync.rs | 67 ++++++++++++++++++++++++ 7 files changed, 89 insertions(+), 20 deletions(-) diff --git a/crates/uv/src/commands/project/add.rs b/crates/uv/src/commands/project/add.rs index 432bb8cb1..b0e1c97b7 100644 --- a/crates/uv/src/commands/project/add.rs +++ b/crates/uv/src/commands/project/add.rs @@ -259,9 +259,6 @@ pub(crate) async fn add( .with_pyproject_toml(pyproject.to_toml()?) .context("Failed to update `pyproject.toml`")?; - // Initialize any shared state. - let state = SharedState::default(); - // Lock and sync the environment, if necessary. let lock = match project::lock::do_safe_lock( locked, @@ -269,7 +266,6 @@ pub(crate) async fn add( project.workspace(), venv.interpreter(), settings.as_ref().into(), - &state, preview, connectivity, concurrency, @@ -387,6 +383,9 @@ pub(crate) async fn add( } }; + // Initialize any shared state. + let state = SharedState::default(); + project::sync::do_sync( &project, &venv, diff --git a/crates/uv/src/commands/project/lock.rs b/crates/uv/src/commands/project/lock.rs index a5d7fbc93..567168cc7 100644 --- a/crates/uv/src/commands/project/lock.rs +++ b/crates/uv/src/commands/project/lock.rs @@ -84,7 +84,6 @@ pub(crate) async fn lock( &workspace, &interpreter, settings.as_ref(), - &SharedState::default(), preview, connectivity, concurrency, @@ -118,7 +117,6 @@ pub(super) async fn do_safe_lock( workspace: &Workspace, interpreter: &Interpreter, settings: ResolverSettingsRef<'_>, - state: &SharedState, preview: PreviewMode, connectivity: Connectivity, concurrency: Concurrency, @@ -126,6 +124,17 @@ pub(super) async fn do_safe_lock( cache: &Cache, printer: Printer, ) -> Result { + // Use isolate state for universal resolution. When resolving, we don't enforce that the + // prioritized distributions match the current platform. So if we lock here, then try to + // install from the same state, and we end up performing a resolution during the sync (i.e., + // for the build dependencies of a source distribution), we may try to use incompatible + // distributions. + // TODO(charlie): In universal resolution, we should still track version compatibility! We + // just need to accept versions that are platform-incompatible. That would also make us more + // likely to (e.g.) download a wheel that we'll end up using when installing. This would + // make it safe to share the state. + let state = SharedState::default(); + if frozen { // Read the existing lockfile, but don't attempt to lock the project. let existing = read(workspace) @@ -147,7 +156,7 @@ pub(super) async fn do_safe_lock( interpreter, Some(&existing), settings, - state, + &state, preview, connectivity, concurrency, @@ -176,7 +185,7 @@ pub(super) async fn do_safe_lock( interpreter, existing.as_ref(), settings, - state, + &state, preview, connectivity, concurrency, diff --git a/crates/uv/src/commands/project/remove.rs b/crates/uv/src/commands/project/remove.rs index c687632e2..60d634f0e 100644 --- a/crates/uv/src/commands/project/remove.rs +++ b/crates/uv/src/commands/project/remove.rs @@ -104,9 +104,6 @@ pub(crate) async fn remove( ) .await?; - // Initialize any shared state. - let state = SharedState::default(); - // Lock and sync the environment, if necessary. let lock = project::lock::do_safe_lock( locked, @@ -114,7 +111,6 @@ pub(crate) async fn remove( project.workspace(), venv.interpreter(), settings.as_ref().into(), - &state, preview, connectivity, concurrency, @@ -129,6 +125,9 @@ pub(crate) async fn remove( let extras = ExtrasSpecification::All; let dev = true; + // Initialize any shared state. + let state = SharedState::default(); + project::sync::do_sync( &VirtualProject::Project(project), &venv, diff --git a/crates/uv/src/commands/project/run.rs b/crates/uv/src/commands/project/run.rs index a35201dca..a257a4dea 100644 --- a/crates/uv/src/commands/project/run.rs +++ b/crates/uv/src/commands/project/run.rs @@ -259,7 +259,6 @@ pub(crate) async fn run( project.workspace(), venv.interpreter(), settings.as_ref().into(), - &state, preview, connectivity, concurrency, diff --git a/crates/uv/src/commands/project/sync.rs b/crates/uv/src/commands/project/sync.rs index 015d9dd1a..4722a877c 100644 --- a/crates/uv/src/commands/project/sync.rs +++ b/crates/uv/src/commands/project/sync.rs @@ -72,16 +72,12 @@ pub(crate) async fn sync( ) .await?; - // Initialize any shared state. - let state = SharedState::default(); - let lock = match do_safe_lock( locked, frozen, project.workspace(), venv.interpreter(), settings.as_ref().into(), - &state, preview, connectivity, concurrency, @@ -102,6 +98,9 @@ pub(crate) async fn sync( Err(err) => return Err(err.into()), }; + // Initialize any shared state. + let state = SharedState::default(); + // Perform the sync operation. do_sync( &project, diff --git a/crates/uv/src/commands/project/tree.rs b/crates/uv/src/commands/project/tree.rs index d0ad56555..91fbfffc7 100644 --- a/crates/uv/src/commands/project/tree.rs +++ b/crates/uv/src/commands/project/tree.rs @@ -18,8 +18,6 @@ use crate::commands::{project, ExitStatus}; use crate::printer::Printer; use crate::settings::ResolverSettings; -use super::SharedState; - /// Run a command. #[allow(clippy::fn_params_excessive_bools)] pub(crate) async fn tree( @@ -72,7 +70,6 @@ pub(crate) async fn tree( &workspace, &interpreter, settings.as_ref(), - &SharedState::default(), preview, connectivity, concurrency, diff --git a/crates/uv/tests/sync.rs b/crates/uv/tests/sync.rs index cf42ca151..daf5f59d4 100644 --- a/crates/uv/tests/sync.rs +++ b/crates/uv/tests/sync.rs @@ -516,3 +516,70 @@ fn sync_build_isolation() -> Result<()> { Ok(()) } + +/// Avoid using incompatible versions for build dependencies that are also part of the resolved +/// environment. This is a very subtle issue, but: when locking, we don't enforce platform +/// compatibility. So, if we reuse the resolver state to install, and the install itself has to +/// preform a resolution (e.g., for the build dependencies of a source distribution), that +/// resolution may choose incompatible versions. +/// +/// The key property here is that there's a shared package between the build dependencies and the +/// project dependencies. +#[test] +fn sync_reset_state() -> Result<()> { + let context = TestContext::new("3.12"); + + let pyproject_toml = context.temp_dir.child("pyproject.toml"); + pyproject_toml.write_str( + r#" + [project] + name = "project" + version = "0.1.0" + requires-python = ">=3.12" + dependencies = ["pydantic-core"] + + [build-system] + requires = ["setuptools", "pydantic-core"] + build-backend = "setuptools.build_meta:__legacy__" + "#, + )?; + + let setup_py = context.temp_dir.child("setup.py"); + setup_py.write_str(indoc::indoc! { r#" + from setuptools import setup + import pydantic_core + + setup( + name="project", + version="0.1.0", + packages=["project"], + install_requires=["pydantic-core"], + ) + "# })?; + + let src = context.temp_dir.child("project"); + src.create_dir_all()?; + + let init = src.child("__init__.py"); + init.touch()?; + + // Running `uv sync` should succeed. + uv_snapshot!(context.filters(), context.sync(), @r###" + success: true + exit_code: 0 + ----- stdout ----- + + ----- stderr ----- + warning: `uv sync` is experimental and may change without warning + Resolved 3 packages in [TIME] + Prepared 3 packages in [TIME] + Installed 3 packages in [TIME] + + project==0.1.0 (from file://[TEMP_DIR]/) + + pydantic-core==2.17.0 + + typing-extensions==4.10.0 + "###); + + assert!(context.temp_dir.child("uv.lock").exists()); + + Ok(()) +}