Merge pull request #1637 from sylvestre/clap-install

refactor(install): move to clap

Cargo.lock

@@ -1677,7 +1677,7 @@ dependencies = [
 name = "uu_install"
 version = "0.0.1"
 dependencies = [
- "getopts 0.2.21 (registry+https://github.com/rust-lang/crates.io-index)",
+ "clap 2.33.3 (registry+https://github.com/rust-lang/crates.io-index)",
  "libc 0.2.66 (registry+https://github.com/rust-lang/crates.io-index)",
  "time 0.1.42 (registry+https://github.com/rust-lang/crates.io-index)",
  "uucore 0.0.4",

src/uu/install/Cargo.toml

@@ -18,7 +18,7 @@ edition = "2018"
 path = "src/install.rs"
 
 [dependencies]
-getopts = "0.2.18"
+clap = "2.33"
 libc = ">= 0.2"
 uucore = { version=">=0.0.4", package="uucore", path="../../uucore", features=["mode"] }
 uucore_procs = { version=">=0.0.4", package="uucore_procs", path="../../uucore_procs" }

src/uu/install/src/install.rs

@@ -7,7 +7,7 @@
 
 // spell-checker:ignore (ToDO) rwxr sourcepath targetpath
 
-extern crate getopts;
+extern crate clap;
 extern crate libc;
 
 mod mode;
@@ -15,16 +15,12 @@ mod mode;
 #[macro_use]
 extern crate uucore;
 
+use clap::{App, Arg, ArgMatches};
 use std::fs;
 use std::path::{Path, PathBuf};
 use std::result::Result;
 
-static NAME: &str = "install";
+const DEFAULT_MODE: u32 = 0o755;
-static SUMMARY: &str = "Copy SOURCE to DEST or multiple SOURCE(s) to the existing
- DIRECTORY, while setting permission modes and owner/group";
-static LONG_HELP: &str = "";
-
-const DEFAULT_MODE: u32 = 755;
 
 #[allow(dead_code)]
 pub struct Behavior {
@@ -52,14 +48,179 @@ impl Behavior {
     }
 }
 
+static ABOUT: &str = "Copy SOURCE to DEST or multiple SOURCE(s) to the existing
+ DIRECTORY, while setting permission modes and owner/group";
+static VERSION: &str = env!("CARGO_PKG_VERSION");
+
+static OPT_COMPARE: &str = "compare";
+static OPT_BACKUP: &str = "backup";
+static OPT_BACKUP_2: &str = "backup2";
+static OPT_DIRECTORY: &str = "directory";
+static OPT_IGNORED: &str = "ignored";
+static OPT_CREATED: &str = "created";
+static OPT_GROUP: &str = "group";
+static OPT_MODE: &str = "mode";
+static OPT_OWNER: &str = "owner";
+static OPT_PRESERVE_TIMESTAMPS: &str = "preserve-timestamps";
+static OPT_STRIP: &str = "strip";
+static OPT_STRIP_PROGRAM: &str = "strip-program";
+static OPT_SUFFIX: &str = "suffix";
+static OPT_TARGET_DIRECTORY: &str = "target-directory";
+static OPT_NO_TARGET_DIRECTORY: &str = "no-target-directory";
+static OPT_VERBOSE: &str = "verbose";
+static OPT_PRESERVE_CONTEXT: &str = "preserve-context";
+static OPT_CONTEXT: &str = "context";
+
+static ARG_FILES: &str = "files";
+
+fn get_usage() -> String {
+    format!("{0} [OPTION]... [FILE]...", executable!())
+}
+
 /// Main install utility function, called from main.rs.
 ///
 /// Returns a program return code.
 ///
 pub fn uumain(args: impl uucore::Args) -> i32 {
-    let args = args.collect_str();
+    let usage = get_usage();
 
-    let matches = parse_opts(args);
+    let matches = App::new(executable!())
+        .version(VERSION)
+        .about(ABOUT)
+        .usage(&usage[..])
+        .arg(
+                Arg::with_name(OPT_BACKUP)
+                .long(OPT_BACKUP)
+                .help("(unimplemented) make a backup of each existing destination file")
+                .value_name("CONTROL")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_BACKUP_2)
+            .short("b")
+            .help("(unimplemented) like --backup but does not accept an argument")
+        )
+        .arg(
+            Arg::with_name(OPT_IGNORED)
+            .short("c")
+            .help("ignored")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_COMPARE)
+            .short("C")
+            .long(OPT_COMPARE)
+            .help("(unimplemented) compare each pair of source and destination files, and in some cases, do not modify the destination at all")
+        )
+        .arg(
+            Arg::with_name(OPT_DIRECTORY)
+                .short("d")
+                .long(OPT_DIRECTORY)
+                .help("treat all arguments as directory names. create all components of the specified directories")
+        )
+
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_CREATED)
+                .short("D")
+                .help("(unimplemented) create all leading components of DEST except the last, then copy SOURCE to DEST")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_GROUP)
+                .short("g")
+                .long(OPT_GROUP)
+                .help("(unimplemented) set group ownership, instead of process's current group")
+                .value_name("GROUP")
+        )
+        .arg(
+            Arg::with_name(OPT_MODE)
+                .short("m")
+                .long(OPT_MODE)
+                .help("set permission mode (as in chmod), instead of rwxr-xr-x")
+                .value_name("MODE")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_OWNER)
+                .short("o")
+                .long(OPT_OWNER)
+                .help("(unimplemented) set ownership (super-user only)")
+                .value_name("OWNER")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_PRESERVE_TIMESTAMPS)
+                .short("p")
+                .long(OPT_PRESERVE_TIMESTAMPS)
+                .help("(unimplemented) apply access/modification times of SOURCE files to corresponding destination files")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_STRIP)
+            .short("s")
+            .long(OPT_STRIP)
+            .help("(unimplemented) strip symbol tables")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_STRIP_PROGRAM)
+                .long(OPT_STRIP_PROGRAM)
+                .help("(unimplemented) program used to strip binaries")
+                .value_name("PROGRAM")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_SUFFIX)
+                .short("S")
+                .long(OPT_SUFFIX)
+                .help("(unimplemented) override the usual backup suffix")
+                .value_name("SUFFIX")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_TARGET_DIRECTORY)
+                .short("t")
+                .long(OPT_TARGET_DIRECTORY)
+                .help("(unimplemented) move all SOURCE arguments into DIRECTORY")
+                .value_name("DIRECTORY")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_NO_TARGET_DIRECTORY)
+                .short("T")
+                .long(OPT_NO_TARGET_DIRECTORY)
+                .help("(unimplemented) treat DEST as a normal file")
+
+        )
+        .arg(
+            Arg::with_name(OPT_VERBOSE)
+            .short("v")
+            .long(OPT_VERBOSE)
+            .help("explain what is being done")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_PRESERVE_CONTEXT)
+                .short("P")
+                .long(OPT_PRESERVE_CONTEXT)
+                .help("(unimplemented) preserve security context")
+        )
+        .arg(
+            // TODO implement flag
+            Arg::with_name(OPT_CONTEXT)
+                .short("Z")
+                .long(OPT_CONTEXT)
+                .help("(unimplemented) set security context of files and directories")
+                .value_name("CONTEXT")
+        )
+        .arg(Arg::with_name(ARG_FILES).multiple(true).takes_value(true))
+        .get_matches_from(args);
+
+    let paths: Vec<String> = matches
+        .values_of(ARG_FILES)
+        .map(|v| v.map(ToString::to_string).collect())
+        .unwrap_or_default();
 
     if let Err(s) = check_unimplemented(&matches) {
         show_error!("Unimplemented feature: {}", s);
@@ -73,146 +234,12 @@ pub fn uumain(args: impl uucore::Args) -> i32 {
         }
     };
 
-    let paths: Vec<PathBuf> = {
-        #[allow(clippy::ptr_arg)]
-        fn string_to_path(s: &String) -> &Path {
-            Path::new(s)
-        };
-        let to_owned = |p: &Path| p.to_owned();
-        let arguments = matches.free.iter().map(string_to_path);
-
-        arguments.map(to_owned).collect()
-    };
-
     match behavior.main_function {
-        MainFunction::Directory => directory(&paths[..], behavior),
+        MainFunction::Directory => directory(paths, behavior),
-        MainFunction::Standard => standard(&paths[..], behavior),
+        MainFunction::Standard => standard(paths, behavior),
     }
 }
 
-/// Build a specification of the command line.
-///
-/// Returns a getopts::Options struct.
-///
-fn parse_opts(args: Vec<String>) -> getopts::Matches {
-    let syntax = format!(
-        "SOURCE DEST
- {} SOURCE... DIRECTORY",
-        NAME
-    );
-    app!(&syntax, SUMMARY, LONG_HELP)
-        // TODO implement flag
-        .optflagopt(
-            "",
-            "backup",
-            "(unimplemented) make a backup of each existing destination\n \
-             file",
-            "CONTROL",
-        )
-        // TODO implement flag
-        .optflag(
-            "b",
-            "",
-            "(unimplemented) like --backup but does not accept an argument",
-        )
-        .optflag("c", "", "ignored")
-        // TODO implement flag
-        .optflag(
-            "C",
-            "compare",
-            "(unimplemented) compare each pair of source and destination\n \
-             files, and in some cases, do not modify the destination at all",
-        )
-        .optflag(
-            "d",
-            "directory",
-            "treat all arguments as directory names.\n \
-             create all components of the specified directories",
-        )
-        // TODO implement flag
-        .optflag(
-            "D",
-            "",
-            "(unimplemented) create all leading components of DEST except the\n \
-             last, then copy SOURCE to DEST",
-        )
-        // TODO implement flag
-        .optflagopt(
-            "g",
-            "group",
-            "(unimplemented) set group ownership, instead of process's\n \
-             current group",
-            "GROUP",
-        )
-        .optflagopt(
-            "m",
-            "mode",
-            "set permission mode (as in chmod), instead\n \
-             of rwxr-xr-x",
-            "MODE",
-        )
-        // TODO implement flag
-        .optflagopt(
-            "o",
-            "owner",
-            "(unimplemented) set ownership (super-user only)",
-            "OWNER",
-        )
-        // TODO implement flag
-        .optflag(
-            "p",
-            "preserve-timestamps",
-            "(unimplemented) apply access/modification times\n \
-             of SOURCE files to corresponding destination files",
-        )
-        // TODO implement flag
-        .optflag("s", "strip", "(unimplemented) strip symbol tables")
-        // TODO implement flag
-        .optflagopt(
-            "",
-            "strip-program",
-            "(unimplemented) program used to strip binaries",
-            "PROGRAM",
-        )
-        // TODO implement flag
-        .optopt(
-            "S",
-            "suffix",
-            "(unimplemented) override the usual backup suffix",
-            "SUFFIX",
-        )
-        // TODO implement flag
-        .optopt(
-            "t",
-            "target-directory",
-            "(unimplemented) move all SOURCE arguments into\n \
-             DIRECTORY",
-            "DIRECTORY",
-        )
-        // TODO implement flag
-        .optflag(
-            "T",
-            "no-target-directory",
-            "(unimplemented) treat DEST as a normal file",
-        )
-        .optflag("v", "verbose", "explain what is being done")
-        // TODO implement flag
-        .optflag(
-            "P",
-            "preserve-context",
-            "(unimplemented) preserve security context",
-        )
-        // TODO implement flag
-        .optflagopt(
-            "Z",
-            "context",
-            "(unimplemented) set security context of files and\n \
-             directories",
-            "CONTEXT",
-        )
-        .parse(args)
-}
-
 /// Check for unimplemented command line arguments.
 ///
 /// Either return the degenerate Ok value, or an Err with string.
@@ -221,34 +248,35 @@ fn parse_opts(args: Vec<String>) -> getopts::Matches {
 ///
 /// Error datum is a string of the unimplemented argument.
 ///
-fn check_unimplemented(matches: &getopts::Matches) -> Result<(), &str> {
+///
-    if matches.opt_present("backup") {
+fn check_unimplemented<'a>(matches: &ArgMatches) -> Result<(), &'a str> {
+    if matches.is_present(OPT_BACKUP) {
         Err("--backup")
-    } else if matches.opt_present("b") {
+    } else if matches.is_present(OPT_BACKUP_2) {
         Err("-b")
-    } else if matches.opt_present("compare") {
+    } else if matches.is_present(OPT_COMPARE) {
         Err("--compare, -C")
-    } else if matches.opt_present("D") {
+    } else if matches.is_present(OPT_CREATED) {
         Err("-D")
-    } else if matches.opt_present("group") {
+    } else if matches.is_present(OPT_GROUP) {
         Err("--group, -g")
-    } else if matches.opt_present("owner") {
+    } else if matches.is_present(OPT_OWNER) {
         Err("--owner, -o")
-    } else if matches.opt_present("preserve-timestamps") {
+    } else if matches.is_present(OPT_PRESERVE_TIMESTAMPS) {
         Err("--preserve-timestamps, -p")
-    } else if matches.opt_present("strip") {
+    } else if matches.is_present(OPT_STRIP) {
         Err("--strip, -s")
-    } else if matches.opt_present("strip-program") {
+    } else if matches.is_present(OPT_STRIP_PROGRAM) {
         Err("--strip-program")
-    } else if matches.opt_present("suffix") {
+    } else if matches.is_present(OPT_SUFFIX) {
         Err("--suffix, -S")
-    } else if matches.opt_present("target-directory") {
+    } else if matches.is_present(OPT_TARGET_DIRECTORY) {
         Err("--target-directory, -t")
-    } else if matches.opt_present("no-target-directory") {
+    } else if matches.is_present(OPT_NO_TARGET_DIRECTORY) {
         Err("--no-target-directory, -T")
-    } else if matches.opt_present("preserve-context") {
+    } else if matches.is_present(OPT_PRESERVE_CONTEXT) {
         Err("--preserve-context, -P")
-    } else if matches.opt_present("context") {
+    } else if matches.is_present(OPT_CONTEXT) {
         Err("--context, -Z")
     } else {
         Ok(())
@@ -263,8 +291,8 @@ fn check_unimplemented(matches: &getopts::Matches) -> Result<(), &str> {
 ///
 /// In event of failure, returns an integer intended as a program return code.
 ///
-fn behavior(matches: &getopts::Matches) -> Result<Behavior, i32> {
+fn behavior(matches: &ArgMatches) -> Result<Behavior, i32> {
-    let main_function = if matches.opt_present("directory") {
+    let main_function = if matches.is_present("directory") {
         MainFunction::Directory
     } else {
         MainFunction::Standard
@@ -272,8 +300,8 @@ fn behavior(matches: &getopts::Matches) -> Result<Behavior, i32> {
 
     let considering_dir: bool = MainFunction::Directory == main_function;
 
-    let specified_mode: Option<u32> = if matches.opt_present("mode") {
+    let specified_mode: Option<u32> = if matches.is_present(OPT_MODE) {
-        match matches.opt_str("mode") {
+        match matches.value_of(OPT_MODE) {
             Some(x) => match mode::parse(&x[..], considering_dir) {
                 Ok(y) => Some(y),
                 Err(err) => {
@@ -285,7 +313,7 @@ fn behavior(matches: &getopts::Matches) -> Result<Behavior, i32> {
                 show_error!(
                     "option '--mode' requires an argument\n \
                      Try '{} --help' for more information.",
-                    NAME
+                    executable!()
                 );
                 return Err(1);
             }
@@ -294,27 +322,27 @@ fn behavior(matches: &getopts::Matches) -> Result<Behavior, i32> {
         None
     };
 
-    let backup_suffix = if matches.opt_present("suffix") {
+    let backup_suffix = if matches.is_present(OPT_SUFFIX) {
-        match matches.opt_str("suffix") {
+        match matches.value_of(OPT_SUFFIX) {
             Some(x) => x,
             None => {
                 show_error!(
                     "option '--suffix' requires an argument\n\
                      Try '{} --help' for more information.",
-                    NAME
+                    executable!()
                 );
                 return Err(1);
             }
         }
     } else {
-        "~".to_owned()
+        "~"
     };
 
     Ok(Behavior {
         main_function,
         specified_mode,
-        suffix: backup_suffix,
+        suffix: backup_suffix.to_string(),
-        verbose: matches.opt_present("v"),
+        verbose: matches.is_present(OPT_VERBOSE),
     })
 }
 
@@ -325,15 +353,15 @@ fn behavior(matches: &getopts::Matches) -> Result<Behavior, i32> {
 ///
 /// Returns an integer intended as a program return code.
 ///
-fn directory(paths: &[PathBuf], b: Behavior) -> i32 {
+fn directory(paths: Vec<String>, b: Behavior) -> i32 {
     if paths.is_empty() {
-        println!("{} with -d requires at least one argument.", NAME);
+        println!("{} with -d requires at least one argument.", executable!());
         1
     } else {
         let mut all_successful = true;
 
         for directory in paths.iter() {
-            let path = directory.as_path();
+            let path = Path::new(directory);
 
             if path.exists() {
                 show_info!("cannot create directory '{}': File exists", path.display());
@@ -371,18 +399,21 @@ fn is_new_file_path(path: &Path) -> bool {
 ///
 /// Returns an integer intended as a program return code.
 ///
-fn standard(paths: &[PathBuf], b: Behavior) -> i32 {
+fn standard(paths: Vec<String>, b: Behavior) -> i32 {
     if paths.len() < 2 {
-        println!("{} requires at least 2 arguments.", NAME);
+        println!("{} requires at least 2 arguments.", executable!());
         1
     } else {
-        let sources = &paths[0..paths.len() - 1];
+        let sources = &paths[0..paths.len() - 1]
-        let target = &paths[paths.len() - 1];
+            .iter()
+            .map(PathBuf::from)
+            .collect::<Vec<_>>();
+        let target = Path::new(paths.last().unwrap());
 
         if (target.is_file() || is_new_file_path(target)) && sources.len() == 1 {
-            copy_file_to_file(&sources[0], target, &b)
+            copy_file_to_file(&sources[0], &target.to_path_buf(), &b)
         } else {
-            copy_files_into_dir(sources, target, &b)
+            copy_files_into_dir(sources, &target.to_path_buf(), &b)
         }
     }
 }

tests/by-util/test_install.rs

@@ -10,7 +10,7 @@ fn test_install_help() {
         .succeeds()
         .no_stderr()
         .stdout
-        .contains("Options:"));
+        .contains("FLAGS:"));
 }
 
 #[test]