refactor: improve candidate selection logic and streamline version handling (#853)

Author: halajohn

core/src/ten_manager/src/cmd/cmd_install.rs

@@ -35,7 +35,10 @@ use ten_rust::pkg_info::{
 
 use crate::{
     config::{is_verbose, metadata::TmanMetadata, TmanConfig},
-    constants::{APP_DIR_IN_DOT_TEN_DIR, DOT_TEN_DIR},
+    constants::{
+        APP_DIR_IN_DOT_TEN_DIR, DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL,
+        DOT_TEN_DIR,
+    },
     dep_and_candidate::get_all_candidates_from_deps,
     fs::{check_is_addon_folder, find_nearest_app_dir},
     install::{
@@ -87,6 +90,7 @@ pub struct InstallCommand {
     pub local_install_mode: LocalInstallMode,
     pub standalone: bool,
     pub cwd: String,
+    pub max_latest_versions: i32,
 
     /// When the user only inputs a single path parameter, if a `manifest.json`
     /// exists under that path, it indicates installation from a local path.
@@ -146,6 +150,15 @@ pub fn create_sub_cmd(args_cfg: &crate::cmd_line::ArgsCfg) -> Command {
                 .value_name("DIR")
                 .required(false),
         )
+        .arg(
+            Arg::new("MAX_LATEST_VERSIONS")
+                .long("max-latest-versions")
+                .help("Maximum number of latest versions to consider")
+                .value_name("NUMBER")
+                .value_parser(clap::value_parser!(i32))
+                .default_value(crate::constants::DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL_STR)
+                .required(false),
+        )
 }
 
 pub fn parse_sub_cmd(sub_cmd_args: &ArgMatches) -> Result<InstallCommand> {
@@ -163,6 +176,7 @@ pub fn parse_sub_cmd(sub_cmd_args: &ArgMatches) -> Result<InstallCommand> {
         local_install_mode: LocalInstallMode::Invalid,
         standalone: false,
         cwd: String::new(),
+        max_latest_versions: DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL,
         local_path: None,
     };
 
@@ -175,6 +189,13 @@ pub fn parse_sub_cmd(sub_cmd_args: &ArgMatches) -> Result<InstallCommand> {
         cmd.cwd = crate::fs::get_cwd()?.to_string_lossy().to_string();
     }
 
+    // Set max_latest_versions if provided.
+    if let Some(max_versions) =
+        sub_cmd_args.get_one::<i32>("MAX_LATEST_VERSIONS")
+    {
+        cmd.max_latest_versions = *max_versions;
+    }
+
     // Retrieve the first positional parameter (in the `PACKAGE_TYPE`
     // parameter).
     if let Some(first_arg) = sub_cmd_args.get_one::<String>("PACKAGE_TYPE") {
@@ -472,9 +493,7 @@ pub async fn execute_cmd(
         let local_path_str = command_data.local_path.clone().unwrap();
         let local_path = Path::new(&local_path_str);
         let local_path = local_path.canonicalize().with_context(|| {
-            format!(
-                "Failed to find the specified local path {local_path_str}"
-            )
+            format!("Failed to find the specified local path {local_path_str}")
         })?;
 
         let local_manifest_dir = if local_path.is_dir() {
@@ -632,6 +651,7 @@ pub async fn execute_cmd(
         &all_candidates,
         locked_pkgs.as_ref(),
         out.clone(),
+        command_data.max_latest_versions,
     )
     .await?;
 

core/src/ten_manager/src/constants/mod.rs

@@ -68,3 +68,6 @@ pub const CONFIG_JSON: &str = "config.json";
 pub const METADATA_FILE: &str = "metadata.json";
 
 pub const BUF_WRITER_BUF_SIZE: usize = 1024 * 1024;
+
+pub const DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL: i32 = 3;
+pub const DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL_STR: &str = "3";

core/src/ten_manager/src/designer/builtin_function/install.rs

@@ -35,6 +35,8 @@ impl WsBuiltinFunction {
             standalone: false,
             local_path: None,
             cwd: base_dir.clone(),
+            max_latest_versions:
+                crate::constants::DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL,
         };
 
         run_installation(

core/src/ten_manager/src/designer/builtin_function/install_all.rs

@@ -26,6 +26,8 @@ impl WsBuiltinFunction {
             standalone: false,
             local_path: None,
             cwd: base_dir.clone(),
+            max_latest_versions:
+                crate::constants::DEFAULT_MAX_LATEST_VERSIONS_WHEN_INSTALL,
         };
 
         run_installation(

core/src/ten_manager/src/solver/solve.rs

@@ -433,6 +433,7 @@ fn create_input_str_for_pkg_info_dependencies(
     pkg_info: &PkgInfo,
     dumped_pkgs_info: &mut HashSet<PkgBasicInfo>,
     all_candidates: &HashMap<PkgTypeAndName, HashMap<PkgBasicInfo, PkgInfo>>,
+    max_latest_versions: i32,
 ) -> Result<()> {
     // If this package has already been dumped, skip it.
     if dumped_pkgs_info.contains(&pkg_info.into()) {
@@ -470,13 +471,33 @@ fn create_input_str_for_pkg_info_dependencies(
             if let Some(candidates) = candidates {
                 let mut found_matched = false;
 
-                for candidate in candidates {
+                let mut candidates_vec: Vec<&PkgInfo> =
+                    candidates.values().collect();
+
+                // The sorting below places the larger versions at the front,
+                // thus having smaller indexes. This is correct because, in the
+                // Clingo solver, our optimization strategy is to minimize the
+                // overall weight, and we prefer larger version numbers.
+                // Therefore, larger version numbers have smaller weights, and
+                // the index here is equivalent to the concept of weight in the
+                // Clingo solver.
+                candidates_vec.sort_by(|a, b| {
+                    b.manifest.version.cmp(&a.manifest.version)
+                });
+
+                for (idx, candidate) in candidates_vec.into_iter().enumerate() {
+                    if max_latest_versions >= 0
+                        && idx >= max_latest_versions as usize
+                    {
+                        break;
+                    }
+
                     // Get version requirement from dependency.
                     let version_matches = match dependency {
                         ManifestDependency::RegistryDependency {
                             version_req,
                             ..
-                        } => version_req.matches(&candidate.1.manifest.version),
+                        } => version_req.matches(&candidate.manifest.version),
                         ManifestDependency::LocalDependency { .. } => {
                             // For local dependencies, just return true to
                             // match all versions.
@@ -491,31 +512,35 @@ fn create_input_str_for_pkg_info_dependencies(
                             pkg_info.manifest.type_and_name.pkg_type,
                             pkg_info.manifest.type_and_name.name,
                             pkg_info.manifest.version,
-                            candidate.1.manifest.type_and_name.pkg_type,
-                            candidate.1.manifest.type_and_name.name,
-                            candidate.1.manifest.version,
+                            candidate.manifest.type_and_name.pkg_type,
+                            candidate.manifest.type_and_name.name,
+                            candidate.manifest.version,
                         ));
 
                         create_input_str_for_pkg_info_dependencies(
                             input_str,
-                            candidate.1,
+                            candidate,
                             dumped_pkgs_info,
                             all_candidates,
+                            max_latest_versions,
                         )?;
 
                         found_matched = true;
                     }
                 }
 
-                if !found_matched {
+                if max_latest_versions < 0 && !found_matched {
                     return Err(anyhow!(
                         "Failed to find candidates for {}",
                         match dependency {
                             ManifestDependency::RegistryDependency {
                                 pkg_type,
                                 name,
                                 version_req,
-                            } => format!("[{pkg_type}]{name} ({version_req})"),
+                            } => format!(
+                                "[{pkg_type}]{name}
+                ({version_req})"
+                            ),
                             ManifestDependency::LocalDependency {
                                 path,
                                 ..
@@ -546,14 +571,14 @@ fn create_input_str_for_pkg_info_dependencies(
 
 fn create_input_str_for_pkg_info_without_dependencies(
     input_str: &mut String,
-    pkg_info: &PkgBasicInfo,
+    pkg_info: &PkgInfo,
     weight: &usize,
 ) -> Result<()> {
     input_str.push_str(&format!(
         "version_declared(\"{}\", \"{}\", \"{}\", {}).\n",
-        pkg_info.type_and_name.pkg_type,
-        pkg_info.type_and_name.name,
-        pkg_info.version,
+        pkg_info.manifest.type_and_name.pkg_type,
+        pkg_info.manifest.type_and_name.name,
+        pkg_info.manifest.version,
         weight
     ));
 
@@ -564,18 +589,19 @@ fn create_input_str_for_all_possible_pkgs_info(
     input_str: &mut String,
     all_candidates: &HashMap<PkgTypeAndName, HashMap<PkgBasicInfo, PkgInfo>>,
     locked_pkgs: Option<&HashMap<PkgTypeAndName, PkgInfo>>,
+    max_latest_versions: i32,
 ) -> Result<()> {
     for candidates in all_candidates {
-        let mut candidates_vec: Vec<PkgBasicInfo> =
-            candidates.1.values().map(|pkg_info| pkg_info.into()).collect();
+        let mut candidates_vec: Vec<&PkgInfo> = candidates.1.values().collect();
 
         // The sorting below places the larger versions at the front, thus
         // having smaller indexes. This is correct because, in the Clingo
         // solver, our optimization strategy is to minimize the overall weight,
         // and we prefer larger version numbers. Therefore, larger version
         // numbers have smaller weights, and the index here is equivalent to the
         // concept of weight in the Clingo solver.
-        candidates_vec.sort_by(|a, b| b.cmp(a));
+        candidates_vec
+            .sort_by(|a, b| b.manifest.version.cmp(&a.manifest.version));
 
         // Check if the locked package exists in the candidates. If it does,
         // move it to the front of the candidates_vec so that it has a smaller
@@ -588,26 +614,31 @@ fn create_input_str_for_all_possible_pkgs_info(
             // dependency, do not prioritize any candidate packages.
             if !locked_pkg.is_local_dependency {
                 let idx = candidates_vec.iter().position(|pkg_info| {
-                    locked_pkg.manifest.version == pkg_info.version
+                    locked_pkg.manifest.version == pkg_info.manifest.version
                 });
 
                 if let Some(idx) = idx {
                     candidates_vec.remove(idx);
-                    candidates_vec.insert(0, locked_pkg.into());
+                    candidates_vec.insert(0, locked_pkg);
                 }
             }
         }
 
         for (idx, candidate) in candidates_vec.into_iter().enumerate() {
+            if max_latest_versions >= 0 && idx >= max_latest_versions as usize {
+                break;
+            }
+
             create_input_str_for_pkg_info_without_dependencies(
-                input_str, &candidate, &idx,
+                input_str, candidate, &idx,
             )?;
         }
     }
 
     Ok(())
 }
 
+#[allow(clippy::too_many_arguments)]
 async fn create_input_str(
     tman_config: Arc<tokio::sync::RwLock<TmanConfig>>,
     pkg_type: &PkgType,
@@ -616,6 +647,7 @@ async fn create_input_str(
     all_candidates: &HashMap<PkgTypeAndName, HashMap<PkgBasicInfo, PkgInfo>>,
     locked_pkgs: Option<&HashMap<PkgTypeAndName, PkgInfo>>,
     out: Arc<Box<dyn TmanOutput>>,
+    max_latest_versions: i32,
 ) -> Result<String> {
     let mut input_str = String::new();
 
@@ -627,6 +659,7 @@ async fn create_input_str(
         &mut input_str,
         all_candidates,
         locked_pkgs,
+        max_latest_versions,
     )?;
 
     create_input_str_for_dependency_relationship(
@@ -644,6 +677,7 @@ async fn create_input_str(
                 candidate.1,
                 &mut dumped_pkgs_info,
                 all_candidates,
+                max_latest_versions,
             )?;
         }
     }
@@ -655,6 +689,7 @@ async fn create_input_str(
     Ok(input_str)
 }
 
+#[allow(clippy::too_many_arguments)]
 pub async fn solve_all(
     tman_config: Arc<tokio::sync::RwLock<TmanConfig>>,
     pkg_type: &PkgType,
@@ -663,6 +698,7 @@ pub async fn solve_all(
     all_candidates: &HashMap<PkgTypeAndName, HashMap<PkgBasicInfo, PkgInfo>>,
     locked_pkgs: Option<&HashMap<PkgTypeAndName, PkgInfo>>,
     out: Arc<Box<dyn TmanOutput>>,
+    max_latest_versions: i32,
 ) -> SolveResult {
     let input_str = create_input_str(
         tman_config.clone(),
@@ -672,6 +708,7 @@ pub async fn solve_all(
         all_candidates,
         locked_pkgs,
         out.clone(),
+        max_latest_versions,
     )
     .await?;
     solve(tman_config, &input_str, out).await

tools/rust/cargo_clippy.sh

@@ -1,11 +1,11 @@
 #!/bin/bash
 
 cd core/src/ten_rust || exit 1
-cargo clippy --all-features --tests -- -D warnings -W clippy::all
-cargo clippy --release --all-features --tests -- -D warnings -W clippy::all
+cargo clippy --tests -- -D warnings -W clippy::all
+cargo clippy --release --tests -- -D warnings -W clippy::all
 
 cd ../../..
 
 cd core/src/ten_manager || exit 1
-cargo clippy --all-features --tests -- -D warnings -W clippy::all
-cargo clippy --release --all-features --tests -- -D warnings -W clippy::all
+cargo clippy --tests -- -D warnings -W clippy::all
+cargo clippy --release --tests -- -D warnings -W clippy::all