Merge pull request #108 from cat-in-136/clap-degression

Fix degrades of `--auto-req` caused by clap migration (#83)

Cargo.toml

@@ -17,6 +17,7 @@ rpm = { version = "0.13.1", default-features = false }
 toml = "0.7"
 cargo_toml = "0.15"
 clap = { version = "4.3", features = ["derive"] }
+color-print = "0.3"
 thiserror = "1"
 elf = "0.7"
 

README.md

@@ -131,17 +131,17 @@ It is necessary to place a space between version and symbols such as `<`, `<=`,
 
 This command automatically determines what shared libraries a package requires.
 There may be times when the automatic dependency processing is not desired.
-In this case, the package author may set `package.metadata.generate-rpm.auto-req` to `"no"` or
-the user who executes this command may specify command line option `--auto-req no`.
+The packege author and users can configure the processing.
 
-* `--auto-req auto`: The following rules are used to determine the preferred automatic dependency process:
+* `--auto-req auto` or `--auto-req` not specified: Use the preferred automatic dependency process.
+  The following rules are used:
     * If `package.metadata.generate-rpm.auto-req` set to `"no"` or `"disabled"`, the process is disabled.
-    * If `/usr/lib/rpm/find-requires` exists, it is used (same behaviour as `--auto-req /usr/lib/rpm/find-requires`).
+    * If `/usr/lib/rpm/find-requires` exists, it is used (same behaviour as `--auto-req find-requires`).
     * Otherwise, builtin procedure is used (same behaviour as `--auto-req builtin`).
-* `--auto-req builtin`: the builtin procedure using `ldd` is used.
-* `--auto-req /path/to/find-requires`: the specified external program is used. This behavior is the same as the
-  original `rpmbuild`.
-* `--auto-req no`: the process is disabled.
+* `--auto-req disabled`, `--auto-req no`: Disable the discovery of dependencies.
+* `--auto-req builtin`: Use the builtin procedure based on `ldd`.
+* `--auto-req find-requires`: Use `/usr/lib/rpm/find-requires`. This behavior is the same as the original `rpmbuild`.
+* `--auto-req /path/to/find-requires`: Use the specified external program is used.
 
 `/bin/sh` is always added to the package requirements. To disable it, set `package.metadata.generate-rpm.require-sh`
 to `false`. You should not do this if you use scripts such as `pre_install_script` or if your assets contain shell

src/auto_req/mod.rs

@@ -4,6 +4,7 @@ use std::path::{Path, PathBuf};
 mod builtin;
 mod script;
 
+/// The path to the system default find-requires program
 const RPM_FIND_REQUIRES: &str = "/usr/lib/rpm/find-requires";
 
 /// The method to auto-req
@@ -19,17 +20,14 @@ pub enum AutoReqMode {
     BuiltIn,
 }
 
-impl From<&Option<cli::AutoReqMode>> for AutoReqMode {
-    fn from(value: &Option<cli::AutoReqMode>) -> Self {
-        use cli::AutoReqMode as M;
-        use AutoReqMode::*;
-
+impl From<cli::AutoReqMode> for AutoReqMode {
+    fn from(value: cli::AutoReqMode) -> Self {
         match value {
-            None => Auto,
-            Some(M::Disabled) => Disabled,
-            Some(M::Builtin) => BuiltIn,
-            Some(M::Script(path)) => Script(path.into()),
-            Some(M::FindRequires) => Script(PathBuf::from(RPM_FIND_REQUIRES)),
+            cli::AutoReqMode::Auto => AutoReqMode::Auto,
+            cli::AutoReqMode::Disabled => AutoReqMode::Disabled,
+            cli::AutoReqMode::Builtin => AutoReqMode::BuiltIn,
+            cli::AutoReqMode::FindRequires => AutoReqMode::Script(PathBuf::from(RPM_FIND_REQUIRES)),
+            cli::AutoReqMode::Script(path) => AutoReqMode::Script(path),
         }
     }
 }

src/cli.rs

@@ -1,4 +1,8 @@
-use clap::{builder::PossibleValue, Parser, ValueEnum};
+use clap::{
+    builder::{PathBufValueParser, PossibleValuesParser, TypedValueParser, ValueParserFactory},
+    Arg, Command, Parser, ValueEnum,
+};
+use std::ffi::OsStr;
 use std::path::PathBuf;
 
 /// Wrapper used when the application is executed as Cargo plugin
@@ -29,8 +33,17 @@ pub struct Cli {
     pub package: Option<String>,
 
     /// Automatic dependency processing mode.
-    #[arg(long)]
-    pub auto_req: Option<AutoReqMode>,
+    #[arg(long, default_value = "auto",
+        help = "Automatic dependency processing mode. \
+        [possible values: auto, disabled, builtin, find-requires, /path/to/find-requires]",
+        long_help = color_print::cstr!("Automatic dependency processing mode.\n\n\
+        Possible values:\n\
+        - <bold>auto</bold>:                   Use the preferred automatic dependency process.\n\
+        - <bold>disabled</bold>:               Disable the discovery of dependencies. [alias: no]\n\
+        - <bold>builtin</bold>:                Use the builtin procedure based on ldd.\n\
+        - <bold>find-requires</bold>:          Use /usr/lib/rpm/find-requires.\n\
+        - <bold>/path/to/find-requires</bold>: Use the specified external program."))]
+    pub auto_req: AutoReqMode,
 
     /// Sub-directory name for all generated artifacts. May be
     /// specified with CARGO_BUILD_TARGET environment
@@ -102,53 +115,115 @@ impl From<Compression> for rpm::CompressionWithLevel {
     }
 }
 
-#[derive(Clone, Debug)]
+#[derive(Clone, Debug, PartialEq, Eq)]
 pub enum AutoReqMode {
+    Auto,
     Disabled,
     Builtin,
     FindRequires,
-    Script(String),
+    Script(PathBuf),
 }
 
-static AUTO_REQ_VARIANTS: &[AutoReqMode] = &[
-    AutoReqMode::Disabled,
-    AutoReqMode::Builtin,
-    AutoReqMode::FindRequires,
-    AutoReqMode::Script(String::new()),
-];
+impl ValueParserFactory for AutoReqMode {
+    type Parser = AutoReqModeParser;
 
-impl ValueEnum for AutoReqMode {
-    fn value_variants<'a>() -> &'a [Self] {
-        AUTO_REQ_VARIANTS
+    fn value_parser() -> Self::Parser {
+        AutoReqModeParser
     }
+}
 
-    fn to_possible_value(&self) -> Option<PossibleValue> {
-        use AutoReqMode::*;
-
-        let val = match self {
-            Disabled => PossibleValue::new("disabled")
-                .help("Disable automatic discovery of dependencies")
-                .alias("no"),
-            Builtin => {
-                PossibleValue::new("builtin").help("Use the builtin procedure based on ldd.")
+#[derive(Clone, Debug)]
+pub struct AutoReqModeParser;
+
+impl TypedValueParser for AutoReqModeParser {
+    type Value = AutoReqMode;
+    fn parse_ref(
+        &self,
+        cmd: &Command,
+        arg: Option<&Arg>,
+        value: &OsStr,
+    ) -> Result<Self::Value, clap::Error> {
+        const VALUES: [(&str, AutoReqMode); 5] = [
+            ("auto", AutoReqMode::Auto),
+            ("disabled", AutoReqMode::Disabled),
+            ("no", AutoReqMode::Disabled),
+            ("builtin", AutoReqMode::Builtin),
+            ("find-requires", AutoReqMode::FindRequires),
+        ];
+
+        let inner = PossibleValuesParser::new(VALUES.iter().map(|(k, _v)| k));
+        match inner.parse_ref(cmd, arg, value) {
+            Ok(name) => Ok(VALUES
+                .iter()
+                .find(|(k, _v)| name.as_str() == (k.as_ref() as &str))
+                .unwrap()
+                .1
+                .clone()),
+            Err(e) if e.kind() == clap::error::ErrorKind::InvalidValue => {
+                let inner = PathBufValueParser::new();
+                match inner.parse_ref(cmd, arg, value) {
+                    Ok(v) => Ok(AutoReqMode::Script(v)),
+                    Err(e) => Err(e),
+                }
             }
-            FindRequires => PossibleValue::new("find-requires")
-                .help("Use the external program specified in RPM_FIND_REQUIRES."),
-            _ => PossibleValue::new("/path/to/find-requires")
-                .help("Use the specified external program."),
-        };
-        Some(val)
+            Err(e) => Err(e),
+        }
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+    #[test]
+    fn verify_cli() {
+        use clap::CommandFactory;
+        Cli::command().debug_assert()
+    }
+
+    #[test]
+    fn test_metadata_overwrite() {
+        let args = Cli::try_parse_from([
+            "",
+            "--metadata-overwrite",
+            "TOML_FILE.toml",
+            "--metadata-overwrite",
+            "TOML_FILE.toml#TOML.PATH",
+        ])
+        .unwrap();
+        assert_eq!(
+            args.metadata_overwrite,
+            vec!["TOML_FILE.toml", "TOML_FILE.toml#TOML.PATH"]
+        );
+    }
+
+    #[test]
+    fn test_set_metadata() {
+        let args = Cli::try_parse_from([
+            "",
+            "-s",
+            "toml \"text1\"",
+            "--set-metadata",
+            "toml \"text2\"",
+        ])
+        .unwrap();
+        assert_eq!(args.set_metadata, vec!["toml \"text1\"", "toml \"text2\""]);
     }
 
-    // Provided method
-    fn from_str(input: &str, ignore_case: bool) -> Result<Self, String> {
-        let lowercase = String::from(input).to_lowercase();
-        let val = if ignore_case { &lowercase } else { input };
-        Ok(match val {
-            "disabled" => Self::Disabled,
-            "builtin" => Self::Builtin,
-            "find-requires" => Self::FindRequires,
-            _ => Self::Script(input.into()),
-        })
+    #[test]
+    fn test_auto_req() {
+        let args = Cli::try_parse_from([""]).unwrap();
+        assert_eq!(args.auto_req, AutoReqMode::Auto);
+        let args = Cli::try_parse_from(["", "--auto-req", "auto"]).unwrap();
+        assert_eq!(args.auto_req, AutoReqMode::Auto);
+        let args = Cli::try_parse_from(["", "--auto-req", "builtin"]).unwrap();
+        assert_eq!(args.auto_req, AutoReqMode::Builtin);
+        let args = Cli::try_parse_from(["", "--auto-req", "find-requires"]).unwrap();
+        assert_eq!(args.auto_req, AutoReqMode::FindRequires);
+        let args = Cli::try_parse_from(["", "--auto-req", "/usr/lib/rpm/find-requires"]).unwrap();
+        assert!(
+            matches!(args.auto_req, AutoReqMode::Script(v) if v == PathBuf::from("/usr/lib/rpm/find-requires"))
+        );
+        let args = Cli::try_parse_from(["", "--auto-req", "no"]).unwrap();
+        assert_eq!(args.auto_req, AutoReqMode::Disabled);
     }
 }

src/config/mod.rs

@@ -302,8 +302,8 @@ impl Config {
 
         let meta_aut_req = metadata.get_str("auto-req")?;
         let auto_req = match (&cfg.args.auto_req, meta_aut_req) {
-            (None, Some("no" | "disabled")) => AutoReqMode::Disabled,
-            (r, _) => r.into(),
+            (crate::cli::AutoReqMode::Auto, Some("no" | "disabled")) => AutoReqMode::Disabled,
+            (v, _) => AutoReqMode::from(v.clone()),
         };
 
         for requires in find_requires(expanded_file_paths, auto_req)? {