From 08b3c4e8f7ce99f7d76220df7ed9df4dfb4c4d4a Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Fri, 22 Mar 2024 15:12:11 +0100 Subject: [PATCH 01/10] feat: dont remove non pixi pypi installs --- src/install_pypi.rs | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/install_pypi.rs b/src/install_pypi.rs index 00fcaec1a..450eece49 100644 --- a/src/install_pypi.rs +++ b/src/install_pypi.rs @@ -39,6 +39,8 @@ use uv_traits::{ConfigSettings, SetupPyStrategy}; type CombinedPypiPackageData = (PypiPackageData, PypiPackageEnvironmentData); +const PIXI_UV_INSTALLER: &str = "pixi-uv"; + fn elapsed(duration: Duration) -> String { let secs = duration.as_secs(); @@ -400,7 +402,7 @@ fn whats_the_plan<'a>( let installed = installed.iter().filter(|dist| { dist.installer() .unwrap_or_default() - .is_some_and(|installer| installer == "uv") + .is_some_and(|installer| installer == PIXI_UV_INSTALLER) }); let mut extraneous = vec![]; @@ -492,7 +494,27 @@ async fn uninstall_outdated_site_packages(site_packages: &Path) -> miette::Resul }; if let Some(installed_dist) = installed_dist { - installed.push(installed_dist); + // Get the installer + let installer = installed_dist.installer(); + + // If we can't get the installer, we can't be certain that we have installed it + let installer = match installer { + Ok(installer) => installer, + Err(e) => { + tracing::warn!( + "could not get installer for {}: {}, will not remove distribution", + installed_dist.name(), + e + ); + continue; + } + }; + + // Only remove if have actually installed it + // by checking the installer + if installer.unwrap_or_default() == PIXI_UV_INSTALLER { + installed.push(installed_dist); + } } } } @@ -732,6 +754,7 @@ pub async fn update_python_distributions( let start = std::time::Instant::now(); uv_installer::Installer::new(&venv) .with_link_mode(LinkMode::default()) + .with_installer_name(Some(PIXI_UV_INSTALLER.to_string())) .with_reporter(UvReporter::new(options)) .install(&wheels) .unwrap(); From f39a596ffbcf81ea52a57952fc0786ea87c5252d Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Fri, 22 Mar 2024 15:36:45 +0100 Subject: [PATCH 02/10] fix: remove single line --- src/install_pypi.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/src/install_pypi.rs b/src/install_pypi.rs index 450eece49..1ade571c0 100644 --- a/src/install_pypi.rs +++ b/src/install_pypi.rs @@ -494,11 +494,8 @@ async fn uninstall_outdated_site_packages(site_packages: &Path) -> miette::Resul }; if let Some(installed_dist) = installed_dist { - // Get the installer - let installer = installed_dist.installer(); - // If we can't get the installer, we can't be certain that we have installed it - let installer = match installer { + let installer = match installed_dist.installer() { Ok(installer) => installer, Err(e) => { tracing::warn!( From d2d22dd4b9279235a9ed38dd298c0c22c7cab4e8 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Tue, 30 Apr 2024 14:06:47 +0200 Subject: [PATCH 03/10] feat: changed the logic so we also re-install if the package is managed by us Also, added some logging when this happens because this might be unexpected. --- src/install_pypi.rs | 78 ++++++++++++++++++++++++++++++++------------- 1 file changed, 56 insertions(+), 22 deletions(-) diff --git a/src/install_pypi.rs b/src/install_pypi.rs index 20dc2dbb4..391eb805c 100644 --- a/src/install_pypi.rs +++ b/src/install_pypi.rs @@ -5,6 +5,7 @@ use std::borrow::Cow; use distribution_filename::DistFilename; +use itertools::Itertools; use miette::{miette, IntoDiagnostic, WrapErr}; use pep440_rs::Version; use pep508_rs::VerbatimUrl; @@ -47,7 +48,7 @@ use uv_resolver::FlatIndex; type CombinedPypiPackageData = (PypiPackageData, PypiPackageEnvironmentData); -const PIXI_UV_INSTALLER: &str = "pixi-uv"; +const PIXI_UV_INSTALLER: &str = "uv-pixi"; fn elapsed(duration: Duration) -> String { let secs = duration.as_secs(); @@ -80,6 +81,10 @@ struct PixiInstallPlan { /// Any distributions that are already installed in the current environment, and are /// _not_ necessary to satisfy the requirements. pub extraneous: Vec, + + /// Keep track of any packages that have been re-installed because of installer mismatch + /// we can warn the user later that this has happened + pub installer_mismatch: Vec, } /// Converts our locked data to a file @@ -405,6 +410,8 @@ fn whats_the_plan<'a>( // i.e. need to be removed before being installed let mut reinstalls = vec![]; + let mut installer_mismatch = vec![]; + // First decide what we need to do with any editables for resolved_editable in editables { match resolved_editable { @@ -438,35 +445,46 @@ fn whats_the_plan<'a>( } } - // Filter out packages not installed by uv - let installed = site_packages.iter().filter(|dist| { - dist.installer() - .unwrap_or_default() - .is_some_and(|installer| installer == PIXI_UV_INSTALLER) - }); - // Walk over all installed packages and check if they are required - for dist in installed { - if let Some(pkg) = required_map.remove(&dist.name()) { - // Check if we need to reinstall - match need_reinstall(dist, pkg, python_version)? { - ValidateInstall::Keep => { - // Continue with the loop - continue; - } - ValidateInstall::Reinstall => { - reinstalls.push(dist.clone()); + for dist in site_packages.iter() { + // Check if we require the package to be installed + let pkg = required_map.remove(&dist.name()); + // Get the installer name + let installer = dist + .installer() + // Empty string if no installer or any other error + .map_or(String::new(), |f| f.unwrap_or_default()); + + if let Some(pkg) = pkg { + if installer != PIXI_UV_INSTALLER { + // We are managing the package but something else has installed a version + // let's re-install to make sure that we have the **correct** version + reinstalls.push(dist.clone()); + installer_mismatch.push(dist.name().clone()); + } else { + // Check if we need to reinstall + match need_reinstall(dist, pkg, python_version)? { + ValidateInstall::Keep => { + // We are done here + continue; + } + ValidateInstall::Reinstall => { + reinstalls.push(dist.clone()); + } } } - // Check if we need to revalidate - // In that case + // Okay so we need to re-install the package + // let's see if we need the remote or local version + + // Check if we need to revalidate the package + // then we should get it from the remote if uv_cache.must_revalidate(&pkg.name) { remote.push(convert_to_dist(pkg, lock_file_dir)); continue; } - // Do we have in the cache? + // Have we cached the wheel? let wheel = registry_index .get(&pkg.name) .find(|(version, _)| **version == pkg.version); @@ -475,8 +493,12 @@ fn whats_the_plan<'a>( } else { remote.push(convert_to_dist(pkg, lock_file_dir)); } + } else if installer != PIXI_UV_INSTALLER { + // Ignore packages that we are not managed by us + continue; } else { - // We can uninstall + // Add to the extraneous list + // as we do manage it but have no need for it extraneous.push(dist.clone()); } } @@ -508,6 +530,7 @@ fn whats_the_plan<'a>( remote, reinstalls, extraneous, + installer_mismatch, }) } @@ -817,6 +840,7 @@ pub async fn update_python_distributions( remote, reinstalls, extraneous, + installer_mismatch, } = whats_the_plan( &python_packages, &editables_with_temp.resolved_editables, @@ -913,6 +937,16 @@ pub async fn update_python_distributions( wheels }; + // Notify the user if there are any packages that were re-installed because they were installed + // by a different installer. + if !installer_mismatch.is_empty() { + let packages = installer_mismatch + .iter() + .map(|name| name.to_string()) + .join(", "); + tracing::warn!("The following packages have been re-installed because they were installed by a different installer: {packages}") + } + // Remove any unnecessary packages. if !extraneous.is_empty() || !reinstalls.is_empty() { let start = std::time::Instant::now(); From e7f8348de11885d97d247d7fc5bedc05388ef44a Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 6 May 2024 15:51:48 +0200 Subject: [PATCH 04/10] feat: create a test for installer name --- src/consts.rs | 1 + src/install_pypi.rs | 4 +-- tests/common/mod.rs | 8 +++++ tests/install_tests.rs | 66 +++++++++++++++++++++++++++++++++++++++--- 4 files changed, 72 insertions(+), 7 deletions(-) diff --git a/src/consts.rs b/src/consts.rs index 045517a18..fc7154f9f 100644 --- a/src/consts.rs +++ b/src/consts.rs @@ -12,6 +12,7 @@ pub const ENVIRONMENTS_DIR: &str = "envs"; pub const SOLVE_GROUP_ENVIRONMENTS_DIR: &str = "solve-group-envs"; pub const PYPI_DEPENDENCIES: &str = "pypi-dependencies"; pub const TASK_CACHE_DIR: &str = "task-cache-v0"; +pub const PIXI_UV_INSTALLER: &str = "uv-pixi"; pub const ONE_TIME_MESSAGES_DIR: &str = "one-time-messages"; diff --git a/src/install_pypi.rs b/src/install_pypi.rs index 32414d650..6ea028cc9 100644 --- a/src/install_pypi.rs +++ b/src/install_pypi.rs @@ -22,7 +22,7 @@ use uv_configuration::{ConfigSettings, SetupPyStrategy}; use uv_resolver::InMemoryIndex; use uv_types::HashStrategy; -use crate::consts::PROJECT_MANIFEST; +use crate::consts::{PIXI_UV_INSTALLER, PROJECT_MANIFEST}; use crate::lock_file::UvResolutionContext; use crate::project::manifest::SystemRequirements; @@ -50,8 +50,6 @@ use uv_resolver::FlatIndex; type CombinedPypiPackageData = (PypiPackageData, PypiPackageEnvironmentData); -const PIXI_UV_INSTALLER: &str = "uv-pixi"; - fn elapsed(duration: Duration) -> String { let secs = duration.as_secs(); diff --git a/tests/common/mod.rs b/tests/common/mod.rs index 4aa0b4184..cf35a98c4 100644 --- a/tests/common/mod.rs +++ b/tests/common/mod.rs @@ -184,6 +184,14 @@ impl PixiControl { self.tmpdir.path() } + /// Get path to default environment + pub fn default_env_path(&self) -> miette::Result { + let project = self.project()?; + let env = project.environment("default"); + let env = env.ok_or_else(|| miette::miette!("default environment not found"))?; + Ok(self.tmpdir.path().join(env.dir())) + } + pub fn manifest_path(&self) -> PathBuf { self.project_path().join(consts::PROJECT_MANIFEST) } diff --git a/tests/install_tests.rs b/tests/install_tests.rs index b19cbb11f..11bd1490e 100644 --- a/tests/install_tests.rs +++ b/tests/install_tests.rs @@ -7,7 +7,7 @@ use crate::common::builders::string_from_iter; use crate::common::package_database::{Package, PackageDatabase}; use common::{LockFileExt, PixiControl}; use pixi::cli::{run, LockFileUsageArgs}; -use pixi::consts::DEFAULT_ENVIRONMENT_NAME; +use pixi::consts::{DEFAULT_ENVIRONMENT_NAME, PIXI_UV_INSTALLER}; use rattler_conda_types::Platform; use serial_test::serial; use tempfile::TempDir; @@ -226,7 +226,7 @@ async fn pypi_reinstall_python() { .await .unwrap(); - let prefix = pixi.project().unwrap().root().join(".pixi/envs/default"); + let prefix = pixi.default_env_path().unwrap(); let cache = uv_cache::Cache::temp().unwrap(); @@ -272,7 +272,7 @@ async fn pypi_add_remove() { .await .unwrap(); - let prefix = pixi.project().unwrap().root().join(".pixi/envs/default"); + let prefix = pixi.default_env_path().unwrap(); let cache = uv_cache::Cache::temp().unwrap(); @@ -352,7 +352,7 @@ async fn install_conda_meta_history() { // Add and update lockfile with this version of python pixi.add("python==3.11").with_install(true).await.unwrap(); - let prefix = pixi.project().unwrap().root().join(".pixi/envs/default"); + let prefix = pixi.default_env_path().unwrap(); let conda_meta_history_file = prefix.join("conda-meta/history"); assert!(conda_meta_history_file.exists()); @@ -399,3 +399,61 @@ async fn minimal_lockfile_update_pypi() { pep508_rs::Requirement::from_str("click==7.1.2").unwrap() )); } + +/// Create a test that installs a package with pixi +/// change the installer and see if it does not touch the package +/// then change the installer back and see if it reinstalls the package +/// with a new version +#[tokio::test(flavor = "multi_thread", worker_threads = 1)] +#[serial] +#[cfg_attr(not(feature = "slow_integration_tests"), ignore)] +async fn test_installer_name() { + let pixi = PixiControl::new().unwrap(); + pixi.init().await.unwrap(); + + // Add and update lockfile with this version of python + pixi.add("python==3.11").with_install(true).await.unwrap(); + pixi.add("click==8.0.0") + .set_type(pixi::DependencyType::PypiDependency) + .with_install(true) + .await + .unwrap(); + dbg!(pixi.default_env_path().unwrap()); + + // Check that installer name is uv-pixi + let dist_info = pixi + .default_env_path() + .unwrap() + .join("lib/python3.11/site-packages/click-8.0.0.dist-info"); + assert!(dist_info.exists()); + let installer = dist_info.join("INSTALLER"); + let installer = std::fs::read_to_string(installer).unwrap(); + assert_eq!(installer, PIXI_UV_INSTALLER); + + // Write a new installer name to the INSTALLER file + // so that we fake that it is not installed by pixi + std::fs::write(dist_info.join("INSTALLER"), "not-pixi").unwrap(); + pixi.remove("click==8.0.0") + .with_install(true) + .set_type(pixi::DependencyType::PypiDependency) + .await + .unwrap(); + + // dist info folder should still exists + // and should have the old installer name + // we know that pixi did not touch the package + assert!(dist_info.exists()); + let installer = dist_info.join("INSTALLER"); + let installer = std::fs::read_to_string(installer).unwrap(); + assert_eq!(installer, "not-pixi"); + + // re-manage the package by adding it, this should cause a reinstall + pixi.add("click==8.0.0") + .set_type(pixi::DependencyType::PypiDependency) + .with_install(true) + .await + .unwrap(); + let installer = dist_info.join("INSTALLER"); + let installer = std::fs::read_to_string(installer).unwrap(); + assert_eq!(installer, PIXI_UV_INSTALLER); +} From d6bdfc61f394b6c72bb3fe6fbf65cc090b3c30a8 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 6 May 2024 15:58:04 +0200 Subject: [PATCH 05/10] feat: change warning a bit --- src/install_pypi.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/install_pypi.rs b/src/install_pypi.rs index 6ea028cc9..43d4fa8ff 100644 --- a/src/install_pypi.rs +++ b/src/install_pypi.rs @@ -954,7 +954,7 @@ pub async fn update_python_distributions( .iter() .map(|name| name.to_string()) .join(", "); - tracing::warn!("The following packages have been re-installed because they were installed by a different installer: {packages}") + tracing::warn!("These pypi-packages were re-installed because they were previously installed by a different installer but are currently managed by pixi: \n\t{packages}") } // Remove any unnecessary packages. From 75a020d38a60e94c75a552944218d6ae157c4792 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 6 May 2024 16:12:35 +0200 Subject: [PATCH 06/10] feat: update test for windows --- tests/install_tests.rs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/install_tests.rs b/tests/install_tests.rs index 11bd1490e..91ad46c99 100644 --- a/tests/install_tests.rs +++ b/tests/install_tests.rs @@ -420,11 +420,17 @@ async fn test_installer_name() { .unwrap(); dbg!(pixi.default_env_path().unwrap()); + // Get the correct dist-info folder + let dist_info = if cfg!(not(target_os = "windows")) { + pixi.default_env_path() + .unwrap() + .join("lib/python3.11/site-packages/click-8.0.0.dist-info") + } else { + pixi.default_env_path() + .unwrap() + .join("Lib/python/site-packages/click-8.0.0.dist-info") + }; // Check that installer name is uv-pixi - let dist_info = pixi - .default_env_path() - .unwrap() - .join("lib/python3.11/site-packages/click-8.0.0.dist-info"); assert!(dist_info.exists()); let installer = dist_info.join("INSTALLER"); let installer = std::fs::read_to_string(installer).unwrap(); From ff162e9d3a635a305b60171f7cab1e6a37412b4b Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 6 May 2024 16:23:47 +0200 Subject: [PATCH 07/10] feat: change warning to info --- src/install_pypi.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/install_pypi.rs b/src/install_pypi.rs index 43d4fa8ff..c261707d0 100644 --- a/src/install_pypi.rs +++ b/src/install_pypi.rs @@ -954,7 +954,8 @@ pub async fn update_python_distributions( .iter() .map(|name| name.to_string()) .join(", "); - tracing::warn!("These pypi-packages were re-installed because they were previously installed by a different installer but are currently managed by pixi: \n\t{packages}") + // BREAK(0.20.1): change this into a warning in a future release + tracing::info!("These pypi-packages were re-installed because they were previously installed by a different installer but are currently managed by pixi: \n\t{packages}") } // Remove any unnecessary packages. From 3706e3bc804d3c9b754c6b956f8fb1e676c9dca7 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 6 May 2024 20:19:31 +0200 Subject: [PATCH 08/10] fix: try to fix windows ci --- tests/install_tests.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tests/install_tests.rs b/tests/install_tests.rs index 91ad46c99..e0bf907f3 100644 --- a/tests/install_tests.rs +++ b/tests/install_tests.rs @@ -426,12 +426,17 @@ async fn test_installer_name() { .unwrap() .join("lib/python3.11/site-packages/click-8.0.0.dist-info") } else { - pixi.default_env_path() - .unwrap() - .join("Lib/python/site-packages/click-8.0.0.dist-info") + let default_env_path = pixi.default_env_path().unwrap(); + assert!(default_env_path.exists(), "default_env_path does not exist"); + assert!(default_env_path.join("Lib").exists(), "lib does not exist"); + assert!( + default_env_path.join("Lib/python").exists(), + "python does not exist" + ); + default_env_path.join("Lib/python/site-packages/click-8.0.0.dist-info") }; // Check that installer name is uv-pixi - assert!(dist_info.exists()); + assert!(dist_info.exists(), "{dist_info:?} does not exist"); let installer = dist_info.join("INSTALLER"); let installer = std::fs::read_to_string(installer).unwrap(); assert_eq!(installer, PIXI_UV_INSTALLER); From 3b58848fc93367f7a70ca15f29e845990edbbf09 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Mon, 6 May 2024 21:05:09 +0200 Subject: [PATCH 09/10] fix: try uppercase --- tests/install_tests.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/tests/install_tests.rs b/tests/install_tests.rs index e0bf907f3..11a956ddb 100644 --- a/tests/install_tests.rs +++ b/tests/install_tests.rs @@ -433,7 +433,11 @@ async fn test_installer_name() { default_env_path.join("Lib/python").exists(), "python does not exist" ); - default_env_path.join("Lib/python/site-packages/click-8.0.0.dist-info") + assert!( + default_env_path.join("Lib/Python/site-packages").exists(), + "site-packages does not exist" + ); + default_env_path.join("Lib/Python/site-packages/click-8.0.0.dist-info") }; // Check that installer name is uv-pixi assert!(dist_info.exists(), "{dist_info:?} does not exist"); From 7e6f52c0194ae688a305517b9d383ba3b2f4b469 Mon Sep 17 00:00:00 2001 From: Tim de Jager Date: Tue, 7 May 2024 09:11:18 +0200 Subject: [PATCH 10/10] fix: windows path --- tests/install_tests.rs | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/tests/install_tests.rs b/tests/install_tests.rs index 11a956ddb..1b639a322 100644 --- a/tests/install_tests.rs +++ b/tests/install_tests.rs @@ -427,17 +427,7 @@ async fn test_installer_name() { .join("lib/python3.11/site-packages/click-8.0.0.dist-info") } else { let default_env_path = pixi.default_env_path().unwrap(); - assert!(default_env_path.exists(), "default_env_path does not exist"); - assert!(default_env_path.join("Lib").exists(), "lib does not exist"); - assert!( - default_env_path.join("Lib/python").exists(), - "python does not exist" - ); - assert!( - default_env_path.join("Lib/Python/site-packages").exists(), - "site-packages does not exist" - ); - default_env_path.join("Lib/Python/site-packages/click-8.0.0.dist-info") + default_env_path.join("Lib/site-packages/click-8.0.0.dist-info") }; // Check that installer name is uv-pixi assert!(dist_info.exists(), "{dist_info:?} does not exist");