From b61b9d842b67e59eea179d4490b6b5b618ff33e0 Mon Sep 17 00:00:00 2001 From: joaogdemacedo Date: Sat, 11 Nov 2023 01:39:26 +0000 Subject: [PATCH 1/6] Offer a multiselect plugin upgrade experience Signed-off-by: joaogdemacedo --- src/commands/plugins.rs | 167 ++++++++++++++++++++++++++++++---------- 1 file changed, 125 insertions(+), 42 deletions(-) diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index cca9c9c9d1..a1a15b1eea 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -63,7 +63,6 @@ pub struct Install { name = PLUGIN_NAME_OPT, conflicts_with = PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, conflicts_with = PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT, - required_unless_present_any = [PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT], )] pub name: Option, @@ -230,6 +229,8 @@ impl Upgrade { /// Upgrades one or all plugins by reinstalling the latest or a specified /// version of a plugin. If downgrade is specified, first uninstalls the /// plugin. + /// Also, by default, Spin displays the list of installed plugins that are in + /// the catalogue and prompts user to choose which ones to upgrade. pub async fn run(self) -> Result<()> { let manager = PluginManager::try_default()?; let manifests_dir = manager.store().installed_manifests_directory(); @@ -242,11 +243,71 @@ impl Upgrade { if self.all { self.upgrade_all(manifests_dir).await + } else if self.name.is_none() + && self.local_manifest_src.is_none() + && self.remote_manifest_src.is_none() + { + // Default behavior + self.upgrade_default().await } else { self.upgrade_one().await } } + async fn upgrade_default(self) -> Result<()> { + let catalogue_plugins = list_catalogue_plugins().await?; + let installed_plugins = list_installed_plugins()?; + + let installed_in_catalogue: Vec<_> = installed_plugins + .into_iter() + .filter(|installed| { + catalogue_plugins.iter().any(|catalogue| { + installed.manifest == catalogue.manifest + && matches!(catalogue.compatibility, PluginCompatibility::Compatible) + }) + }) + .collect(); + + eprintln!( + "Select repos to upgrade. Use Space to select/deselect and Enter to confirm selection." + ); + let selected_indexes = match dialoguer::MultiSelect::new() + .items(&installed_in_catalogue) + .interact_opt()? + { + Some(indexes) => indexes, + None => return Ok(()), + }; + + let plugins_selected = elements_at(installed_in_catalogue, selected_indexes); + + if plugins_selected.is_empty() { + eprintln!("No plugins selected"); + return Ok(()); + } + + // Upgrade plugins selected + for plugin in plugins_selected { + let manager = PluginManager::try_default()?; + let manifest_location = ManifestLocation::PluginsRepository(PluginLookup::new( + &plugin.name, + Some(plugin.version.parse::()?), + )); + + try_install( + &plugin.manifest, + &manager, + true, // not sure + true, // not sure + false, // not sure + &manifest_location, + ) + .await?; + } + + Ok(()) + } + // Install the latest of all currently installed plugins async fn upgrade_all(&self, manifests_dir: impl AsRef) -> Result<()> { let manager = PluginManager::try_default()?; @@ -320,6 +381,46 @@ impl Upgrade { } } +// Make list_installed_plugins and list_catalogue_plugins into 'free' module-level functions +// in order to call them in Upgrade::upgrade_default +fn list_installed_plugins() -> Result> { + let manager = PluginManager::try_default()?; + let store = manager.store(); + let manifests = store.installed_manifests()?; + let descriptors = manifests + .into_iter() + .map(|m| PluginDescriptor { + name: m.name(), + version: m.version().to_owned(), + installed: true, + compatibility: PluginCompatibility::for_current(&m), + manifest: m, + }) + .collect(); + Ok(descriptors) +} + +async fn list_catalogue_plugins() -> Result> { + if update_silent().await.is_err() { + terminal::warn!("Couldn't update plugins registry cache - using most recent"); + } + + let manager = PluginManager::try_default()?; + let store = manager.store(); + let manifests = store.catalogue_manifests(); + let descriptors = manifests? + .into_iter() + .map(|m| PluginDescriptor { + name: m.name(), + version: m.version().to_owned(), + installed: m.is_installed_in(store), + compatibility: PluginCompatibility::for_current(&m), + manifest: m, + }) + .collect(); + Ok(descriptors) +} + /// List available or installed plugins. #[derive(Parser, Debug)] pub struct List { @@ -335,7 +436,7 @@ pub struct List { impl List { pub async fn run(self) -> Result<()> { let mut plugins = if self.installed { - Self::list_installed_plugins() + list_installed_plugins() } else { Self::list_catalogue_and_installed_plugins().await }?; @@ -350,47 +451,9 @@ impl List { Ok(()) } - fn list_installed_plugins() -> Result> { - let manager = PluginManager::try_default()?; - let store = manager.store(); - let manifests = store.installed_manifests()?; - let descriptors = manifests - .into_iter() - .map(|m| PluginDescriptor { - name: m.name(), - version: m.version().to_owned(), - installed: true, - compatibility: PluginCompatibility::for_current(&m), - manifest: m, - }) - .collect(); - Ok(descriptors) - } - - async fn list_catalogue_plugins() -> Result> { - if update_silent().await.is_err() { - terminal::warn!("Couldn't update plugins registry cache - using most recent"); - } - - let manager = PluginManager::try_default()?; - let store = manager.store(); - let manifests = store.catalogue_manifests(); - let descriptors = manifests? - .into_iter() - .map(|m| PluginDescriptor { - name: m.name(), - version: m.version().to_owned(), - installed: m.is_installed_in(store), - compatibility: PluginCompatibility::for_current(&m), - manifest: m, - }) - .collect(); - Ok(descriptors) - } - async fn list_catalogue_and_installed_plugins() -> Result> { - let catalogue = Self::list_catalogue_plugins().await?; - let installed = Self::list_installed_plugins()?; + let catalogue = list_catalogue_plugins().await?; + let installed = list_installed_plugins()?; Ok(merge_plugin_lists(catalogue, installed)) } @@ -474,6 +537,26 @@ impl PluginDescriptor { } } +impl std::fmt::Display for PluginDescriptor { + fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { + write!(f, "{} v{}", self.name, self.version).map_err(std::fmt::Error::from) + } +} + +fn elements_at(source: Vec, indexes: Vec) -> Vec { + source + .into_iter() + .enumerate() + .filter_map(|(index, s)| { + if indexes.contains(&index) { + Some(s) + } else { + None + } + }) + .collect() +} + fn merge_plugin_lists(a: Vec, b: Vec) -> Vec { let mut result = a; From 3d919e193c81e6b11f7960abe9b3adeba2cff660 Mon Sep 17 00:00:00 2001 From: joaogdemacedo Date: Mon, 13 Nov 2023 12:05:18 +0000 Subject: [PATCH 2/6] Offer a multiselect plugin upgrade experience Signed-off-by: joaogdemacedo --- src/commands/plugins.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index a1a15b1eea..37c8a5c535 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -247,14 +247,15 @@ impl Upgrade { && self.local_manifest_src.is_none() && self.remote_manifest_src.is_none() { - // Default behavior - self.upgrade_default().await + // Default behavior (multiselect) + self.upgrade_multiselect().await } else { self.upgrade_one().await } } - async fn upgrade_default(self) -> Result<()> { + // Multiselect plugin upgrade experience + async fn upgrade_multiselect(self) -> Result<()> { let catalogue_plugins = list_catalogue_plugins().await?; let installed_plugins = list_installed_plugins()?; @@ -269,7 +270,7 @@ impl Upgrade { .collect(); eprintln!( - "Select repos to upgrade. Use Space to select/deselect and Enter to confirm selection." + "Select plugins to upgrade. Use Space to select/deselect and Enter to confirm selection." ); let selected_indexes = match dialoguer::MultiSelect::new() .items(&installed_in_catalogue) @@ -297,9 +298,9 @@ impl Upgrade { try_install( &plugin.manifest, &manager, - true, // not sure - true, // not sure - false, // not sure + true, + false, + false, &manifest_location, ) .await?; @@ -382,7 +383,7 @@ impl Upgrade { } // Make list_installed_plugins and list_catalogue_plugins into 'free' module-level functions -// in order to call them in Upgrade::upgrade_default +// in order to call them in Upgrade::upgrade_multiselect fn list_installed_plugins() -> Result> { let manager = PluginManager::try_default()?; let store = manager.store(); @@ -542,7 +543,7 @@ impl std::fmt::Display for PluginDescriptor { write!(f, "{} v{}", self.name, self.version).map_err(std::fmt::Error::from) } } - +// Auxiliar function for Upgrade::upgrade_multiselect fn elements_at(source: Vec, indexes: Vec) -> Vec { source .into_iter() From 33c151d056073d98d2971d08d865928781f49330 Mon Sep 17 00:00:00 2001 From: joaogdemacedo Date: Sun, 19 Nov 2023 23:46:58 +0000 Subject: [PATCH 3/6] Address changes to the code review of multiselect plugin upgrade Signed-off-by: joaogdemacedo --- src/commands/plugins.rs | 82 ++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 30 deletions(-) diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index 37c8a5c535..8349c582dc 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -63,6 +63,7 @@ pub struct Install { name = PLUGIN_NAME_OPT, conflicts_with = PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, conflicts_with = PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT, + required_unless_present_any = [PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT], )] pub name: Option, @@ -166,7 +167,6 @@ pub struct Upgrade { #[clap( name = PLUGIN_NAME_OPT, conflicts_with = PLUGIN_ALL_OPT, - required_unless_present_any = [PLUGIN_ALL_OPT, PLUGIN_REMOTE_PLUGIN_MANIFEST_OPT, PLUGIN_LOCAL_PLUGIN_MANIFEST_OPT], )] pub name: Option, @@ -269,18 +269,53 @@ impl Upgrade { }) .collect(); + let mut eligible_plugins = Vec::new(); + + // Getting only eligible plugins to upgrade + for installed_plugin in installed_in_catalogue { + let manager = PluginManager::try_default()?; + let manifest_location = ManifestLocation::PluginsRepository(PluginLookup::new( + &installed_plugin.name, + None, + )); + + // Attempt to get the manifest to check eligibility to upgrade + if let Ok(manifest) = manager + .get_manifest(&manifest_location, false, SPIN_VERSION) + .await + { + if installed_plugin.version != manifest.version() { + eligible_plugins.push((installed_plugin, manifest)); + } + } + } + + if eligible_plugins.is_empty() { + eprintln!("No plugins found to upgrade"); + return Ok(()); + } + + let names: Vec<_> = eligible_plugins + .iter() + .map(|(descriptor, manifest)| { + format!( + "{} from version {} to {}", + descriptor.name.clone(), + descriptor.version.clone(), + manifest.version() + ) + }) + .collect(); + eprintln!( "Select plugins to upgrade. Use Space to select/deselect and Enter to confirm selection." ); - let selected_indexes = match dialoguer::MultiSelect::new() - .items(&installed_in_catalogue) - .interact_opt()? - { + let selected_indexes = match dialoguer::MultiSelect::new().items(&names).interact_opt()? { Some(indexes) => indexes, None => return Ok(()), }; - let plugins_selected = elements_at(installed_in_catalogue, selected_indexes); + let plugins_selected = elements_at(eligible_plugins, selected_indexes); if plugins_selected.is_empty() { eprintln!("No plugins selected"); @@ -288,22 +323,14 @@ impl Upgrade { } // Upgrade plugins selected - for plugin in plugins_selected { + for (installed_plugin, manifest) in plugins_selected { let manager = PluginManager::try_default()?; let manifest_location = ManifestLocation::PluginsRepository(PluginLookup::new( - &plugin.name, - Some(plugin.version.parse::()?), + &installed_plugin.name, + None, )); - try_install( - &plugin.manifest, - &manager, - true, - false, - false, - &manifest_location, - ) - .await?; + try_install(&manifest, &manager, true, false, false, &manifest_location).await?; } Ok(()) @@ -422,6 +449,12 @@ async fn list_catalogue_plugins() -> Result> { Ok(descriptors) } +async fn list_catalogue_and_installed_plugins() -> Result> { + let catalogue = list_catalogue_plugins().await?; + let installed = list_installed_plugins()?; + Ok(merge_plugin_lists(catalogue, installed)) +} + /// List available or installed plugins. #[derive(Parser, Debug)] pub struct List { @@ -439,7 +472,7 @@ impl List { let mut plugins = if self.installed { list_installed_plugins() } else { - Self::list_catalogue_and_installed_plugins().await + list_catalogue_and_installed_plugins().await }?; plugins.sort_by(|p, q| p.cmp(q)); @@ -452,12 +485,6 @@ impl List { Ok(()) } - async fn list_catalogue_and_installed_plugins() -> Result> { - let catalogue = list_catalogue_plugins().await?; - let installed = list_installed_plugins()?; - Ok(merge_plugin_lists(catalogue, installed)) - } - fn print(plugins: &[PluginDescriptor]) { if plugins.is_empty() { println!("No plugins found"); @@ -538,11 +565,6 @@ impl PluginDescriptor { } } -impl std::fmt::Display for PluginDescriptor { - fn fmt(&self, f: &mut std::fmt::Formatter) -> std::fmt::Result { - write!(f, "{} v{}", self.name, self.version).map_err(std::fmt::Error::from) - } -} // Auxiliar function for Upgrade::upgrade_multiselect fn elements_at(source: Vec, indexes: Vec) -> Vec { source From d2fb4cc837f9e5775b5909cf48557c202d9d9598 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jo=C3=A3o=20De=20Macedo?= Date: Tue, 21 Nov 2023 15:19:29 +0000 Subject: [PATCH 4/6] Update error message of "All plugins up to date" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Change error message: - from "No plugins found to upgrade"; - to "All plugins are up to date"; Co-authored-by: itowlson Signed-off-by: João De Macedo --- src/commands/plugins.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index 8349c582dc..768ed5e5e1 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -291,7 +291,7 @@ impl Upgrade { } if eligible_plugins.is_empty() { - eprintln!("No plugins found to upgrade"); + eprintln!("All plugins are up to date"); return Ok(()); } From 45edb2bfbdf99e1d371b351a34faa2135807921c Mon Sep 17 00:00:00 2001 From: joaogdemacedo Date: Tue, 21 Nov 2023 22:50:49 +0000 Subject: [PATCH 5/6] Fix compatibility check and version comparison Signed-off-by: joaogdemacedo --- src/commands/plugins.rs | 25 +++++++++++++++++++++---- 1 file changed, 21 insertions(+), 4 deletions(-) diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index 768ed5e5e1..485e4589bd 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -269,6 +269,11 @@ impl Upgrade { }) .collect(); + if installed_in_catalogue.is_empty() { + eprintln!("No plugins found to upgrade"); + return Ok(()); + } + let mut eligible_plugins = Vec::new(); // Getting only eligible plugins to upgrade @@ -284,8 +289,13 @@ impl Upgrade { .get_manifest(&manifest_location, false, SPIN_VERSION) .await { - if installed_plugin.version != manifest.version() { - eligible_plugins.push((installed_plugin, manifest)); + // Check if upgraded candidates have a newer version and if are compatible + if is_potential_upgrade(&installed_plugin.manifest, &manifest) { + if let PluginCompatibility::Compatible = + PluginCompatibility::for_current(&manifest) + { + eligible_plugins.push((installed_plugin, manifest)); + } } } } @@ -300,8 +310,8 @@ impl Upgrade { .map(|(descriptor, manifest)| { format!( "{} from version {} to {}", - descriptor.name.clone(), - descriptor.version.clone(), + descriptor.name, + descriptor.version, manifest.version() ) }) @@ -409,6 +419,13 @@ impl Upgrade { } } +fn is_potential_upgrade(current: &PluginManifest, candidate: &PluginManifest) -> bool { + match (current.try_version(), candidate.try_version()) { + (Ok(cur_ver), Ok(cand_ver)) => cand_ver > cur_ver, + _ => current.version() != candidate.version(), + } +} + // Make list_installed_plugins and list_catalogue_plugins into 'free' module-level functions // in order to call them in Upgrade::upgrade_multiselect fn list_installed_plugins() -> Result> { From a362755215263836109b47dddcef0083ea860624 Mon Sep 17 00:00:00 2001 From: joaogdemacedo Date: Tue, 21 Nov 2023 23:50:35 +0000 Subject: [PATCH 6/6] improve compatibility check with PartialEq Signed-off-by: joaogdemacedo --- src/commands/plugins.rs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/src/commands/plugins.rs b/src/commands/plugins.rs index 485e4589bd..fa60433434 100644 --- a/src/commands/plugins.rs +++ b/src/commands/plugins.rs @@ -262,10 +262,9 @@ impl Upgrade { let installed_in_catalogue: Vec<_> = installed_plugins .into_iter() .filter(|installed| { - catalogue_plugins.iter().any(|catalogue| { - installed.manifest == catalogue.manifest - && matches!(catalogue.compatibility, PluginCompatibility::Compatible) - }) + catalogue_plugins + .iter() + .any(|catalogue| installed.manifest == catalogue.manifest) }) .collect(); @@ -290,12 +289,11 @@ impl Upgrade { .await { // Check if upgraded candidates have a newer version and if are compatible - if is_potential_upgrade(&installed_plugin.manifest, &manifest) { - if let PluginCompatibility::Compatible = - PluginCompatibility::for_current(&manifest) - { - eligible_plugins.push((installed_plugin, manifest)); - } + if is_potential_upgrade(&installed_plugin.manifest, &manifest) + && PluginCompatibility::Compatible + == PluginCompatibility::for_current(&manifest) + { + eligible_plugins.push((installed_plugin, manifest)); } } } @@ -537,7 +535,7 @@ impl Search { } } -#[derive(Debug)] +#[derive(Debug, PartialEq)] pub(crate) enum PluginCompatibility { Compatible, IncompatibleSpin(String),