From 22a91ebb7e40cf89d9aa77f9c645df705c9db582 Mon Sep 17 00:00:00 2001 From: nichmor Date: Wed, 16 Oct 2024 18:02:46 +0300 Subject: [PATCH 01/11] feat: global update should add new executables --- src/cli/global/install.rs | 71 ++++------------------------------ src/cli/global/list.rs | 4 +- src/cli/global/update.rs | 41 +++++++++++++++++++- src/global/install.rs | 22 +++++++++++ src/global/project/manifest.rs | 25 ++++++++++++ src/global/project/mod.rs | 69 +++++++++++++++++++++++++++------ 6 files changed, 154 insertions(+), 78 deletions(-) diff --git a/src/cli/global/install.rs b/src/cli/global/install.rs index fe3c2ed16..452a38df2 100644 --- a/src/cli/global/install.rs +++ b/src/cli/global/install.rs @@ -1,5 +1,3 @@ -use std::str::FromStr; - use clap::Parser; use fancy_display::FancyDisplay; use indexmap::IndexMap; @@ -9,8 +7,9 @@ use rattler_conda_types::{MatchSpec, NamedChannelOrUrl, PackageName, Platform}; use crate::{ cli::{global::revert_environment_after_error, has_specs::HasSpecs}, - global::{self, EnvironmentName, ExposedName, Mapping, Project, StateChange, StateChanges}, - prefix::Prefix, + global::{ + self, project::ExposedType, EnvironmentName, Mapping, Project, StateChange, StateChanges, + }, }; use pixi_config::{self, Config, ConfigCli}; @@ -169,41 +168,10 @@ async fn setup_environment( // Installing the environment to be able to find the bin paths later project.install_environment(env_name).await?; - // Cleanup removed executables - state_changes |= project.remove_broken_expose_names(env_name).await?; - - if args.expose.is_empty() { - // Add the expose binaries for all the packages that were requested to the manifest - for (package_name, _spec) in &specs { - let prefix = project.environment_prefix(env_name).await?; - let prefix_package = prefix.find_designated_package(package_name).await?; - let package_executables = prefix.find_executables(&[prefix_package]); - for (executable_name, _) in &package_executables { - let mapping = Mapping::new( - ExposedName::from_str(executable_name)?, - executable_name.clone(), - ); - project.manifest.add_exposed_mapping(env_name, &mapping)?; - } - // If no executables were found, automatically expose the package name itself from the other packages. - // This is useful for packages like `ansible` and `jupyter` which don't ship executables their own executables. - if !package_executables - .iter() - .any(|(name, _)| name.as_str() == package_name.as_normalized()) - { - if let Some((mapping, source_package_name)) = - find_binary_by_name(&prefix, package_name).await? - { - project.manifest.add_exposed_mapping(env_name, &mapping)?; - tracing::warn!( - "Automatically exposed `{}` from `{}`", - console::style(mapping.exposed_name()).yellow(), - console::style(source_package_name.as_normalized()).green() - ); - } - } - } - } + // Sync exposed binaries + let expose_type = ExposedType::new(args.expose.is_empty()); + + project.sync_exposed_names(env_name, expose_type).await?; // Figure out added packages and their corresponding versions let specs = specs.values().cloned().collect_vec(); @@ -217,28 +185,3 @@ async fn setup_environment( project.manifest.save().await?; Ok(state_changes) } - -/// Finds the package name in the prefix and automatically exposes it if an executable is found. -/// This is useful for packages like `ansible` and `jupyter` which don't ship executables their own executables. -/// This function will return the mapping and the package name of the package in which the binary was found. -async fn find_binary_by_name( - prefix: &Prefix, - package_name: &PackageName, -) -> miette::Result> { - let installed_packages = prefix.find_installed_packages(None).await?; - for package in &installed_packages { - let executables = prefix.find_executables(&[package.clone()]); - - // Check if any of the executables match the package name - if let Some(executable) = executables - .iter() - .find(|(name, _)| name.as_str() == package_name.as_normalized()) - { - return Ok(Some(( - Mapping::new(ExposedName::from_str(&executable.0)?, executable.0.clone()), - package.repodata_record.package_record.name.clone(), - ))); - } - } - Ok(None) -} diff --git a/src/cli/global/list.rs b/src/cli/global/list.rs index 5fa1e141d..52b3df0fe 100644 --- a/src/cli/global/list.rs +++ b/src/cli/global/list.rs @@ -382,7 +382,7 @@ fn format_exposed(env_name: &str, exposed: &IndexSet, last: bool) -> Op .iter() .any(|mapping| mapping.exposed_name().to_string() != env_name) { - let formatted_exposed = exposed.iter().map(format_mapping).join(", "); + let formatted_exposed = exposed.iter().map(format_mapping).sorted().join(", "); Some(format_asciiart_section( "exposes", formatted_exposed, @@ -394,6 +394,8 @@ fn format_exposed(env_name: &str, exposed: &IndexSet, last: bool) -> Op } } +/// If the exposed name is the same as the executable name, only return the executable name +/// otherwise return what is the exposed name and what is the executable name. fn format_mapping(mapping: &Mapping) -> String { let exp = mapping.exposed_name().to_string(); if exp == mapping.executable_name() { diff --git a/src/cli/global/update.rs b/src/cli/global/update.rs index 20be901d7..694e2caa3 100644 --- a/src/cli/global/update.rs +++ b/src/cli/global/update.rs @@ -1,7 +1,10 @@ use crate::cli::global::revert_environment_after_error; +use crate::global::project::ExposedType; use crate::global::{self, StateChanges}; use crate::global::{EnvironmentName, Project}; use clap::Parser; +use fancy_display::FancyDisplay; +use itertools::Itertools; use pixi_config::{Config, ConfigCli}; /// Updates environments in the global environment. @@ -25,11 +28,45 @@ pub async fn execute(args: Args) -> miette::Result<()> { project: &mut Project, ) -> miette::Result { let mut state_changes = StateChanges::default(); + + // See what executables were installed prior to update + let env_binaries = project.executables(env_name).await?; + + let parsed_env = project + .environment(env_name) + .ok_or_else(|| miette::miette!("Environment {} not found", env_name.fancy_display()))?; + + // Get the exposed binaries from mapping + let exposed_mapping_binaries = parsed_env.exposed(); + + // Check if they were all auto-exposed, or if the user manually exposed a subset of them + let env_binaries_names = env_binaries + .values() + .flatten() + .map(|(name, _)| name) + .collect_vec(); + + let exposed_binaries_names = exposed_mapping_binaries + .iter() + .map(|mapping| mapping.executable_name()) + .collect_vec(); + + let auto_exposed = env_binaries_names + .iter() + .all(|name| exposed_binaries_names.contains(&name.as_str())); + + let expose_type = ExposedType::new(auto_exposed); + // Reinstall the environment project.install_environment(env_name).await?; - // Remove broken executables - state_changes |= project.remove_broken_expose_names(env_name).await?; + // Sync executables exposed names with the manifest + project.sync_exposed_names(env_name, expose_type).await?; + + // Expose or prune executables of the new environment + state_changes |= project + .expose_executables_from_environment(env_name) + .await?; state_changes.insert_change(env_name, global::StateChange::UpdatedEnvironment); diff --git a/src/global/install.rs b/src/global/install.rs index e9156f697..f209a7c23 100644 --- a/src/global/install.rs +++ b/src/global/install.rs @@ -346,6 +346,28 @@ pub(crate) fn local_environment_matches_spec( } } +/// Finds the package name in the prefix and automatically exposes it if an executable is found. +/// This is useful for packages like `ansible` and `jupyter` which don't ship executables their own executables. +/// This function will return the mapping and the package name of the package in which the binary was found. +pub async fn find_binary_by_name( + prefix: &Prefix, + package_name: &PackageName, +) -> miette::Result> { + let installed_packages = prefix.find_installed_packages(None).await?; + for package in &installed_packages { + let executables = prefix.find_executables(&[package.clone()]); + + // Check if any of the executables match the package name + if let Some(executable) = executables + .iter() + .find(|(name, _)| name.as_str() == package_name.as_normalized()) + { + return Ok(Some(executable.clone())); + } + } + Ok(None) +} + #[cfg(test)] mod tests { use fs_err as fs; diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs index 396e5be6d..b7868df9c 100644 --- a/src/global/project/manifest.rs +++ b/src/global/project/manifest.rs @@ -468,6 +468,31 @@ impl FromStr for Mapping { } } +#[derive(Default)] +pub enum ExposedType { + #[default] + All, + Subset, +} + +impl ExposedType { + pub fn new(input: bool) -> Self { + match input { + true => Self::All, + false => Self::Subset, + } + } + + pub fn is_all(&self) -> bool { + matches!(self, Self::All) + } +} + +// pub struct PackageBinariesExposed { +// exposed: IndexMap, + +// } + #[cfg(test)] mod tests { use std::str::FromStr; diff --git a/src/global/project/mod.rs b/src/global/project/mod.rs index b198364a0..333dbb422 100644 --- a/src/global/project/mod.rs +++ b/src/global/project/mod.rs @@ -1,3 +1,4 @@ +use super::install::find_binary_by_name; use super::{extract_executable_from_script, BinDir, EnvRoot, StateChange, StateChanges}; use crate::global::common::{ channel_url_to_prioritized_channel, find_package_records, get_expose_scripts_sync_status, @@ -20,7 +21,8 @@ use fs_err as fs; use futures::stream::StreamExt; use indexmap::{IndexMap, IndexSet}; use itertools::Itertools; -pub(crate) use manifest::{Manifest, Mapping}; +// use manifest::PackageBinariesExposed; +pub(crate) use manifest::{ExposedType, Manifest, Mapping}; use miette::{miette, Context, IntoDiagnostic}; pub(crate) use parsed_manifest::ExposedName; pub(crate) use parsed_manifest::ParsedEnvironment; @@ -605,18 +607,51 @@ impl Project { Ok(state_changes) } + /// Get all installed executables for specific environment. + pub async fn executables( + &self, + env_name: &EnvironmentName, + ) -> miette::Result>> { + let parsed_env = self + .environment(env_name) + .ok_or_else(|| miette::miette!("Environment {} not found", env_name.fancy_display()))?; + + let package_names: Vec<_> = parsed_env.dependencies().keys().cloned().collect(); + + let mut executables_for_package = IndexMap::new(); + + for package_name in &package_names { + let prefix = self.environment_prefix(env_name).await?; + let prefix_package = prefix.find_designated_package(package_name).await?; + let mut package_executables = prefix.find_executables(&[prefix_package]); + + // Sometimes the package don't ship executables on their own. + // We need to search for it in different packages. + if !package_executables + .iter() + .any(|(exec_name, _)| exec_name.as_str() == package_name.as_normalized()) + { + if let Some(exec) = find_binary_by_name(&prefix, package_name).await? { + package_executables.push(exec); + } + } + + executables_for_package.insert(package_name.clone(), package_executables); + } + Ok(executables_for_package) + } + /// Find the exposed names that are no longer installed in the environment /// and remove them. /// This needs to happen after a different version of a package was installed /// which doesn't have the same executables anymore. - pub async fn remove_broken_expose_names( + pub async fn sync_exposed_names( &mut self, env_name: &EnvironmentName, - ) -> miette::Result { - // Figure out which package the exposed binaries belong to - let prefix = self.environment_prefix(env_name).await?; - let prefix_records = &prefix.find_installed_packages(None).await?; - let all_executables = &prefix.find_executables(prefix_records.as_slice()); + expose_type: ExposedType, + ) -> miette::Result<()> { + // Get env executables + let env_executables = self.executables(env_name).await?; // Get the parsed environment let environment = self @@ -629,8 +664,9 @@ impl Project { .iter() .filter_map(|mapping| { // If the executable is still requested, do not remove the mapping - if all_executables - .iter() + if env_executables + .values() + .flatten() .any(|(_, path)| executable_from_path(path) == mapping.executable_name()) { tracing::debug!("Not removing mapping to: {}", mapping.executable_name()); @@ -646,8 +682,19 @@ impl Project { self.manifest.remove_exposed_name(env_name, exposed_name)?; } - // Remove outdated binaries - self.prune_exposed(env_name).await + // auto-expose the executables if necessary + if expose_type.is_all() { + // Add new binaries that are not exposed + for (executable_name, _) in env_executables.values().flatten() { + let mapping = Mapping::new( + ExposedName::from_str(executable_name)?, + executable_name.to_string(), + ); + self.manifest.add_exposed_mapping(env_name, &mapping)?; + } + } + + Ok(()) } /// Check if the environment is in sync with the manifest From 86967525f9b7960ae525c4ba76fedd540c3e5063 Mon Sep 17 00:00:00 2001 From: nichmor Date: Wed, 16 Oct 2024 18:02:55 +0300 Subject: [PATCH 02/11] feat: global update should add new executables --- .pre-commit-config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 743d5cf67..59c6799ed 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,13 +12,13 @@ repos: entry: pixi run -e lint end-of-file-fixer language: system types: [text] - stages: [commit, push, manual] + stages: [pre-commit, pre-push, manual] - id: trailing-whitespace name: trailing-whitespace entry: pixi run -e lint trailing-whitespace-fixer language: system types: [text] - stages: [commit, push, manual] + stages: [pre-commit, pre-push, manual] # Use ruff for python examples - id: ruff name: ruff From 2f13c2b67c415b94c17d6af732ff0f255eadaecb Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 12:10:49 +0300 Subject: [PATCH 03/11] fix: failing tests --- src/cli/global/install.rs | 2 +- src/cli/global/update.rs | 2 +- src/global/project/manifest.rs | 20 ++++++------ src/global/project/mod.rs | 24 +++++++++----- tests/integration/test_global.py | 55 ++++++++++++++++++++++++++++---- 5 files changed, 76 insertions(+), 27 deletions(-) diff --git a/src/cli/global/install.rs b/src/cli/global/install.rs index 452a38df2..0762b1c13 100644 --- a/src/cli/global/install.rs +++ b/src/cli/global/install.rs @@ -169,7 +169,7 @@ async fn setup_environment( project.install_environment(env_name).await?; // Sync exposed binaries - let expose_type = ExposedType::new(args.expose.is_empty()); + let expose_type = ExposedType::from_mapping(args.expose.clone()); project.sync_exposed_names(env_name, expose_type).await?; diff --git a/src/cli/global/update.rs b/src/cli/global/update.rs index 694e2caa3..135da6696 100644 --- a/src/cli/global/update.rs +++ b/src/cli/global/update.rs @@ -55,7 +55,7 @@ pub async fn execute(args: Args) -> miette::Result<()> { .iter() .all(|name| exposed_binaries_names.contains(&name.as_str())); - let expose_type = ExposedType::new(auto_exposed); + let expose_type = ExposedType::from_bool(auto_exposed); // Reinstall the environment project.install_environment(env_name).await?; diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs index b7868df9c..d9a907a70 100644 --- a/src/global/project/manifest.rs +++ b/src/global/project/manifest.rs @@ -472,27 +472,25 @@ impl FromStr for Mapping { pub enum ExposedType { #[default] All, - Subset, + Subset(Vec), } impl ExposedType { - pub fn new(input: bool) -> Self { - match input { + pub fn from_mapping(mapping: Vec) -> Self { + match mapping.is_empty() { true => Self::All, - false => Self::Subset, + false => Self::Subset(mapping), } } - pub fn is_all(&self) -> bool { - matches!(self, Self::All) + pub fn from_bool(auto_exposed: bool) -> Self { + match auto_exposed { + true => Self::All, + false => Self::Subset(Default::default()), + } } } -// pub struct PackageBinariesExposed { -// exposed: IndexMap, - -// } - #[cfg(test)] mod tests { use std::str::FromStr; diff --git a/src/global/project/mod.rs b/src/global/project/mod.rs index 333dbb422..25b05a514 100644 --- a/src/global/project/mod.rs +++ b/src/global/project/mod.rs @@ -683,14 +683,22 @@ impl Project { } // auto-expose the executables if necessary - if expose_type.is_all() { - // Add new binaries that are not exposed - for (executable_name, _) in env_executables.values().flatten() { - let mapping = Mapping::new( - ExposedName::from_str(executable_name)?, - executable_name.to_string(), - ); - self.manifest.add_exposed_mapping(env_name, &mapping)?; + match expose_type { + ExposedType::All => { + // Add new binaries that are not exposed + for (executable_name, _) in env_executables.values().flatten() { + let mapping = Mapping::new( + ExposedName::from_str(executable_name)?, + executable_name.to_string(), + ); + self.manifest.add_exposed_mapping(env_name, &mapping)?; + } + } + ExposedType::Subset(mapping) => { + // Expose only the requested binaries + for mapping in mapping { + self.manifest.add_exposed_mapping(env_name, &mapping)?; + } } } diff --git a/tests/integration/test_global.py b/tests/integration/test_global.py index 4e6b08884..152f9dd42 100644 --- a/tests/integration/test_global.py +++ b/tests/integration/test_global.py @@ -1164,8 +1164,8 @@ def test_global_update_single_package( # After update be left with only the binary that was in both versions. assert package.is_file() assert not package0_1_0.is_file() - # pixi global update doesn't add new exposed mappings - assert not package0_2_0.is_file() + # pixi global update should add new exposed mappings, as they initially were auto-exposed + assert package0_2_0.is_file() def test_global_update_all_packages( @@ -1206,13 +1206,13 @@ def test_global_update_all_packages( assert package2.is_file() assert package.is_file() assert not package0_1_0.is_file() - # After update be left with only the binary that was in both versions. - assert not package0_2_0.is_file() + # After update be left we auto expose new binary, as they initially were auto-exposed + assert package0_2_0.is_file() # Check the manifest for removed binaries manifest_content = manifest.read_text() assert "package0.1.0" not in manifest_content - assert "package0.2.0" not in manifest_content + assert "package0.2.0" in manifest_content assert "package2" in manifest_content assert "package" in manifest_content @@ -1248,7 +1248,45 @@ def test_pixi_update_cleanup(pixi: Path, tmp_path: Path, global_update_channel_1 # Update the environment # The package should now have the version `0.2.0` and expose a different executable # The old executable should be removed - # The new executable will also not be there, since `pixi global update` doesn't add new exposed mappings to the manifest. + # The new executable should be there, since user initially auto-exposed all binaries and `pixi global update` should add new binary to the manifest. + verify_cli_command( + [pixi, "global", "update", "package"], + env=env, + ) + assert not package0_1_0.is_file() + assert package0_2_0.is_file() + + +def test_pixi_update_subset_expose( + pixi: Path, tmp_path: Path, global_update_channel_1: str +) -> None: + env = {"PIXI_HOME": str(tmp_path)} + + package0_1_0 = tmp_path / "bin" / exec_extension("package0.1.0") + package0_2_0 = tmp_path / "bin" / exec_extension("package0.2.0") + + verify_cli_command( + [pixi, "global", "install", "--channel", global_update_channel_1, "package==0.1.0"], + env=env, + ) + assert package0_1_0.is_file() + assert not package0_2_0.is_file() + + manifest = tmp_path.joinpath("manifests", "pixi-global.toml") + + # We change the matchspec to '*' + # So we expect to new binary to not be exposed, + # since we exposed only a small subset + parsed_toml = tomllib.loads(manifest.read_text()) + parsed_toml["envs"]["package"]["dependencies"]["package"] = "*" + parsed_toml["envs"]["package"]["exposed"] = {"package": "package0.1.0"} + + manifest.write_text(tomli_w.dumps(parsed_toml)) + + # Update the environment + # The package should now have the version `0.2.0` and expose a different executable + # The old executable should be removed + # The new executable should be there, since user initially auto-exposed all binaries and `pixi global update` should add new binary to the manifest. verify_cli_command( [pixi, "global", "update", "package"], env=env, @@ -1256,6 +1294,11 @@ def test_pixi_update_cleanup(pixi: Path, tmp_path: Path, global_update_channel_1 assert not package0_1_0.is_file() assert not package0_2_0.is_file() + # parse the manifest again + # and check that we don't have any new binary exposed + parsed_toml = tomllib.loads(manifest.read_text()) + assert "exposed" not in parsed_toml["envs"]["package"] + def test_auto_self_expose(pixi: Path, tmp_path: Path, non_self_expose_channel: str) -> None: env = {"PIXI_HOME": str(tmp_path)} From b0ab34d730bf47d3c5343f73456b2ae242132483 Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 12:18:52 +0300 Subject: [PATCH 04/11] misc: refactor a little --- src/cli/global/update.rs | 29 +++++++---------------------- src/global/common.rs | 29 +++++++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 24 deletions(-) diff --git a/src/cli/global/update.rs b/src/cli/global/update.rs index 135da6696..e9f7bd84f 100644 --- a/src/cli/global/update.rs +++ b/src/cli/global/update.rs @@ -1,10 +1,10 @@ use crate::cli::global::revert_environment_after_error; +use crate::global::common::check_auto_exposed; use crate::global::project::ExposedType; use crate::global::{self, StateChanges}; use crate::global::{EnvironmentName, Project}; use clap::Parser; use fancy_display::FancyDisplay; -use itertools::Itertools; use pixi_config::{Config, ConfigCli}; /// Updates environments in the global environment. @@ -32,30 +32,15 @@ pub async fn execute(args: Args) -> miette::Result<()> { // See what executables were installed prior to update let env_binaries = project.executables(env_name).await?; - let parsed_env = project - .environment(env_name) - .ok_or_else(|| miette::miette!("Environment {} not found", env_name.fancy_display()))?; - // Get the exposed binaries from mapping - let exposed_mapping_binaries = parsed_env.exposed(); + let exposed_mapping_binaries = project + .environment(env_name) + .ok_or_else(|| miette::miette!("Environment {} not found", env_name.fancy_display()))? + .exposed(); // Check if they were all auto-exposed, or if the user manually exposed a subset of them - let env_binaries_names = env_binaries - .values() - .flatten() - .map(|(name, _)| name) - .collect_vec(); - - let exposed_binaries_names = exposed_mapping_binaries - .iter() - .map(|mapping| mapping.executable_name()) - .collect_vec(); - - let auto_exposed = env_binaries_names - .iter() - .all(|name| exposed_binaries_names.contains(&name.as_str())); - - let expose_type = ExposedType::from_bool(auto_exposed); + let expose_type = + ExposedType::from_bool(check_auto_exposed(&env_binaries, exposed_mapping_binaries)); // Reinstall the environment project.install_environment(env_name).await?; diff --git a/src/global/common.rs b/src/global/common.rs index 77f8edd30..87dafe442 100644 --- a/src/global/common.rs +++ b/src/global/common.rs @@ -2,14 +2,16 @@ use super::{extract_executable_from_script, EnvironmentName, ExposedName, Mappin use fancy_display::FancyDisplay; use fs_err as fs; use fs_err::tokio as tokio_fs; -use indexmap::IndexSet; +use indexmap::{IndexMap, IndexSet}; use is_executable::IsExecutable; use itertools::Itertools; use miette::{Context, IntoDiagnostic}; use pixi_config::home_path; use pixi_manifest::PrioritizedChannel; use pixi_utils::executable_from_path; -use rattler_conda_types::{Channel, ChannelConfig, NamedChannelOrUrl, PackageRecord, PrefixRecord}; +use rattler_conda_types::{ + Channel, ChannelConfig, NamedChannelOrUrl, PackageName, PackageRecord, PrefixRecord, +}; use std::collections::HashMap; use std::ffi::OsStr; use std::str::FromStr; @@ -478,6 +480,29 @@ pub(crate) async fn get_expose_scripts_sync_status( Ok((to_remove, to_add)) } +/// Check if they were all auto-exposed, or if the user manually exposed a subset of them +pub fn check_auto_exposed( + env_binaries: &IndexMap>, + exposed_mapping_binaries: &IndexSet, +) -> bool { + let env_binaries_names = env_binaries + .values() + .flatten() + .map(|(name, _)| name) + .collect_vec(); + + let exposed_binaries_names = exposed_mapping_binaries + .iter() + .map(|mapping| mapping.executable_name()) + .collect_vec(); + + let auto_exposed = env_binaries_names + .iter() + .all(|name| exposed_binaries_names.contains(&name.as_str())); + + auto_exposed +} + #[cfg(test)] mod tests { use super::*; From f9a0f3b19abe36a4ffb79bee76a726f9a45bb77e Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 14:38:39 +0300 Subject: [PATCH 05/11] misc: rever pre-commit config --- .pre-commit-config.yaml | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 59c6799ed..743d5cf67 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -12,13 +12,13 @@ repos: entry: pixi run -e lint end-of-file-fixer language: system types: [text] - stages: [pre-commit, pre-push, manual] + stages: [commit, push, manual] - id: trailing-whitespace name: trailing-whitespace entry: pixi run -e lint trailing-whitespace-fixer language: system types: [text] - stages: [pre-commit, pre-push, manual] + stages: [commit, push, manual] # Use ruff for python examples - id: ruff name: ruff From 041b665d757ee97d0a5f95a1215ae6ff3dcd71dd Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 14:39:39 +0300 Subject: [PATCH 06/11] Update src/cli/global/list.rs Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com> --- src/cli/global/list.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cli/global/list.rs b/src/cli/global/list.rs index 52b3df0fe..4b5b04656 100644 --- a/src/cli/global/list.rs +++ b/src/cli/global/list.rs @@ -395,7 +395,7 @@ fn format_exposed(env_name: &str, exposed: &IndexSet, last: bool) -> Op } /// If the exposed name is the same as the executable name, only return the executable name -/// otherwise return what is the exposed name and what is the executable name. +/// otherwise return "{exposed name} -> {executable name}". fn format_mapping(mapping: &Mapping) -> String { let exp = mapping.exposed_name().to_string(); if exp == mapping.executable_name() { From 89239ecaef57b38aadb03a1be3ab694d4a66a736 Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 14:57:22 +0300 Subject: [PATCH 07/11] Update src/global/project/mod.rs Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com> --- src/global/project/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/global/project/mod.rs b/src/global/project/mod.rs index 25b05a514..6530b7406 100644 --- a/src/global/project/mod.rs +++ b/src/global/project/mod.rs @@ -21,7 +21,6 @@ use fs_err as fs; use futures::stream::StreamExt; use indexmap::{IndexMap, IndexSet}; use itertools::Itertools; -// use manifest::PackageBinariesExposed; pub(crate) use manifest::{ExposedType, Manifest, Mapping}; use miette::{miette, Context, IntoDiagnostic}; pub(crate) use parsed_manifest::ExposedName; From 8143cafad78523160fa66571ab1f36dd985a02cd Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 14:57:31 +0300 Subject: [PATCH 08/11] Update tests/integration/test_global.py Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com> --- tests/integration/test_global.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_global.py b/tests/integration/test_global.py index 152f9dd42..a89cdb981 100644 --- a/tests/integration/test_global.py +++ b/tests/integration/test_global.py @@ -1164,7 +1164,7 @@ def test_global_update_single_package( # After update be left with only the binary that was in both versions. assert package.is_file() assert not package0_1_0.is_file() - # pixi global update should add new exposed mappings, as they initially were auto-exposed + # pixi global update should add new exposed mappings, as all of them were exposed before assert package0_2_0.is_file() From 8eb8b1c5d0c72c0696b0f59f8ee0e775819c7712 Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 14:57:38 +0300 Subject: [PATCH 09/11] Update tests/integration/test_global.py Co-authored-by: Hofer-Julian <30049909+Hofer-Julian@users.noreply.github.com> --- tests/integration/test_global.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/integration/test_global.py b/tests/integration/test_global.py index a89cdb981..fe47c0c02 100644 --- a/tests/integration/test_global.py +++ b/tests/integration/test_global.py @@ -1206,7 +1206,7 @@ def test_global_update_all_packages( assert package2.is_file() assert package.is_file() assert not package0_1_0.is_file() - # After update be left we auto expose new binary, as they initially were auto-exposed + # After update be left we auto expose new binary, as all of them were exposed before assert package0_2_0.is_file() # Check the manifest for removed binaries From 6234700d641cf451c73cde782700d278e6dae541 Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 15:05:14 +0300 Subject: [PATCH 10/11] misc: apply refactor changes --- src/cli/global/install.rs | 2 +- src/cli/global/update.rs | 9 ++++++--- src/global/common.rs | 20 ++++++++------------ src/global/project/manifest.rs | 13 +++++-------- src/global/project/mod.rs | 12 ++++++++---- 5 files changed, 28 insertions(+), 28 deletions(-) diff --git a/src/cli/global/install.rs b/src/cli/global/install.rs index 0762b1c13..db03ca994 100644 --- a/src/cli/global/install.rs +++ b/src/cli/global/install.rs @@ -169,7 +169,7 @@ async fn setup_environment( project.install_environment(env_name).await?; // Sync exposed binaries - let expose_type = ExposedType::from_mapping(args.expose.clone()); + let expose_type = ExposedType::from_mappings(args.expose.clone()); project.sync_exposed_names(env_name, expose_type).await?; diff --git a/src/cli/global/update.rs b/src/cli/global/update.rs index e9f7bd84f..12eecafd4 100644 --- a/src/cli/global/update.rs +++ b/src/cli/global/update.rs @@ -1,5 +1,5 @@ use crate::cli::global::revert_environment_after_error; -use crate::global::common::check_auto_exposed; +use crate::global::common::check_all_exposed; use crate::global::project::ExposedType; use crate::global::{self, StateChanges}; use crate::global::{EnvironmentName, Project}; @@ -39,8 +39,11 @@ pub async fn execute(args: Args) -> miette::Result<()> { .exposed(); // Check if they were all auto-exposed, or if the user manually exposed a subset of them - let expose_type = - ExposedType::from_bool(check_auto_exposed(&env_binaries, exposed_mapping_binaries)); + let expose_type = if check_all_exposed(&env_binaries, exposed_mapping_binaries) { + ExposedType::default() + } else { + ExposedType::subset() + }; // Reinstall the environment project.install_environment(env_name).await?; diff --git a/src/global/common.rs b/src/global/common.rs index 87dafe442..7a6d4c13f 100644 --- a/src/global/common.rs +++ b/src/global/common.rs @@ -1,4 +1,5 @@ use super::{extract_executable_from_script, EnvironmentName, ExposedName, Mapping}; +use ahash::HashSet; use fancy_display::FancyDisplay; use fs_err as fs; use fs_err::tokio as tokio_fs; @@ -480,25 +481,20 @@ pub(crate) async fn get_expose_scripts_sync_status( Ok((to_remove, to_add)) } -/// Check if they were all auto-exposed, or if the user manually exposed a subset of them -pub fn check_auto_exposed( +/// Check if all binaries were exposed, or if the user selected a subset of them. +pub fn check_all_exposed( env_binaries: &IndexMap>, exposed_mapping_binaries: &IndexSet, ) -> bool { - let env_binaries_names = env_binaries - .values() - .flatten() - .map(|(name, _)| name) - .collect_vec(); + let mut env_binaries_names_iter = env_binaries.values().flatten().map(|(name, _)| name); - let exposed_binaries_names = exposed_mapping_binaries + let exposed_binaries_names: HashSet<&str> = exposed_mapping_binaries .iter() .map(|mapping| mapping.executable_name()) - .collect_vec(); + .collect(); - let auto_exposed = env_binaries_names - .iter() - .all(|name| exposed_binaries_names.contains(&name.as_str())); + let auto_exposed = + env_binaries_names_iter.all(|name| exposed_binaries_names.contains(&name.as_str())); auto_exposed } diff --git a/src/global/project/manifest.rs b/src/global/project/manifest.rs index d9a907a70..15964723e 100644 --- a/src/global/project/manifest.rs +++ b/src/global/project/manifest.rs @@ -476,18 +476,15 @@ pub enum ExposedType { } impl ExposedType { - pub fn from_mapping(mapping: Vec) -> Self { - match mapping.is_empty() { + pub fn from_mappings(mappings: Vec) -> Self { + match mappings.is_empty() { true => Self::All, - false => Self::Subset(mapping), + false => Self::Subset(mappings), } } - pub fn from_bool(auto_exposed: bool) -> Self { - match auto_exposed { - true => Self::All, - false => Self::Subset(Default::default()), - } + pub fn subset() -> Self { + Self::Subset(Default::default()) } } diff --git a/src/global/project/mod.rs b/src/global/project/mod.rs index 25b05a514..5e8098d79 100644 --- a/src/global/project/mod.rs +++ b/src/global/project/mod.rs @@ -641,10 +641,14 @@ impl Project { Ok(executables_for_package) } - /// Find the exposed names that are no longer installed in the environment - /// and remove them. - /// This needs to happen after a different version of a package was installed - /// which doesn't have the same executables anymore. + /// Sync the `exposed` field in manifest based on the executables in the environment and the expose type. + /// Expose type can be either: + /// * If the user initially chooses to auto-exposed everything, + /// we will add new binaries that are not exposed in the `exposed` field. + /// + /// * If the use chose to expose only a subset of binaries, + /// we will remove the binaries that are not anymore present in the environment + /// and will not expose the new ones pub async fn sync_exposed_names( &mut self, env_name: &EnvironmentName, From c45995d7bdad42e1ed7250f3ac6db5caa775987f Mon Sep 17 00:00:00 2001 From: nichmor Date: Thu, 17 Oct 2024 15:06:25 +0300 Subject: [PATCH 11/11] misc: apply refactor changes --- src/global/project/mod.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/global/project/mod.rs b/src/global/project/mod.rs index 5cf01769b..585358a4f 100644 --- a/src/global/project/mod.rs +++ b/src/global/project/mod.rs @@ -643,11 +643,11 @@ impl Project { /// Sync the `exposed` field in manifest based on the executables in the environment and the expose type. /// Expose type can be either: /// * If the user initially chooses to auto-exposed everything, - /// we will add new binaries that are not exposed in the `exposed` field. + /// we will add new binaries that are not exposed in the `exposed` field. /// /// * If the use chose to expose only a subset of binaries, - /// we will remove the binaries that are not anymore present in the environment - /// and will not expose the new ones + /// we will remove the binaries that are not anymore present in the environment + /// and will not expose the new ones pub async fn sync_exposed_names( &mut self, env_name: &EnvironmentName,