Update CLI for mullvad-daemon

7 files changed, 145 insertions, 206 deletions

diff --git a/Cargo.lock b/Cargo.lock
index 2da40ab86d..8f546b0bd0 100644
--- a/Cargo.lock
+++ b/Cargo.lock
@@ -472,7 +472,6 @@ dependencies = [
  "bitflags",
  "clap_lex 0.2.4",
  "indexmap",
- "once_cell",
  "strsim 0.10.0",
  "termcolor",
  "textwrap",
@@ -1866,7 +1865,7 @@ dependencies = [
  "android_logger",
  "cfg-if",
  "chrono",
- "clap 3.2.23",
+ "clap 4.2.7",
  "ctrlc",
  "dirs-next",
  "duct",
@@ -1995,7 +1994,7 @@ dependencies = [
 name = "mullvad-problem-report"
 version = "0.0.0"
 dependencies = [
- "clap 3.2.23",
+ "clap 4.2.7",
  "dirs-next",
  "duct",
  "env_logger 0.10.0",
diff --git a/mullvad-daemon/Cargo.toml b/mullvad-daemon/Cargo.toml
index 9c71d57eba..709a061326 100644
--- a/mullvad-daemon/Cargo.toml
+++ b/mullvad-daemon/Cargo.toml
@@ -37,7 +37,7 @@ talpid-platform-metadata = { path = "../talpid-platform-metadata" }
 talpid-time = { path = "../talpid-time" }
 
 [target.'cfg(not(target_os="android"))'.dependencies]
-clap = { version = "3.0", features = ["cargo"] }
+clap = { version = "4.2.7", features = ["cargo"] }
 log-panics = "2.0.0"
 mullvad-management-interface = { path = "../mullvad-management-interface" }
 mullvad-paths = { path = "../mullvad-paths" }
diff --git a/mullvad-daemon/src/cli.rs b/mullvad-daemon/src/cli.rs
index 59223cbc17..00552a611a 100644
--- a/mullvad-daemon/src/cli.rs
+++ b/mullvad-daemon/src/cli.rs
@@ -1,11 +1,67 @@
-use clap::{crate_authors, crate_description, crate_name, App, Arg};
+use clap::Parser;
+
+lazy_static::lazy_static! {
+    static ref ENV_DESC: String = format!(
+"ENV:
+
+    MULLVAD_RESOURCE_DIR       Resource directory (i.e used to locate a root CA certificate)
+                               [Default: {}]
+    MULLVAD_SETTINGS_DIR       Directory path for storing settings. [Default: {}]
+    MULLVAD_CACHE_DIR          Directory path for storing cache. [Default: {}]
+    MULLVAD_LOG_DIR            Directory path for storing logs. [Default: {}]
+    MULLVAD_RPC_SOCKET_PATH    Location of the management interface device.
+                               It refers to Unix domain socket on Unix based platforms, and named pipe on Windows.
+                               [Default: {}]
+
+",
+        mullvad_paths::get_default_resource_dir().display(),
+        mullvad_paths::get_default_settings_dir().map(|dir| dir.display().to_string()).unwrap_or_else(|_| "N/A".to_string()),
+        mullvad_paths::get_default_cache_dir().map(|dir| dir.display().to_string()).unwrap_or_else(|_| "N/A".to_string()),
+        mullvad_paths::get_default_log_dir().map(|dir| dir.display().to_string()).unwrap_or_else(|_| "N/A".to_string()),
+        mullvad_paths::get_default_rpc_socket_path().display());
+}
+
+#[derive(Debug, Parser)]
+#[command(author, version = mullvad_version::VERSION, about, long_about = None, after_help = &*ENV_DESC)]
+struct Cli {
+    /// Set the level of verbosity
+    #[arg(short='v', action = clap::ArgAction::Count)]
+    verbosity: u8,
+    /// Disable logging to file
+    #[arg(long)]
+    disable_log_to_file: bool,
+    /// Don't log timestamps when logging to stdout, useful when running as a systemd service
+    #[arg(long)]
+    disable_stdout_timestamps: bool,
+
+    /// Run as a system service
+    #[cfg(target_os = "windows")]
+    #[arg(long)]
+    run_as_service: bool,
+    /// Register Mullvad daemon as a system service
+    #[cfg(target_os = "windows")]
+    #[arg(long)]
+    register_service: bool,
+
+    /// Initialize firewall to be used during early boot and exit
+    #[cfg(target_os = "linux")]
+    #[arg(long)]
+    initialize_early_boot_firewall: bool,
+
+    /// Check the status of the launch daemon. The exit code represents the current status
+    #[cfg(target_os = "macos")]
+    #[arg(long)]
+    launch_daemon_status: bool,
+}
 
 #[derive(Debug)]
 pub struct Config {
     pub log_level: log::LevelFilter,
     pub log_to_file: bool,
     pub log_stdout_timestamps: bool,
+    #[cfg(target_os = "windows")]
     pub run_as_service: bool,
+    #[cfg(target_os = "windows")]
     pub register_service: bool,
     #[cfg(target_os = "macos")]
     pub launch_daemon_status: bool,
@@ -20,108 +76,26 @@ pub fn get_config() -> &'static Config {
     &CONFIG
 }
 
-pub fn create_config() -> Config {
-    let app = create_app();
-    let matches = app.get_matches();
+fn create_config() -> Config {
+    let app = Cli::parse();
 
-    let log_level = match matches.occurrences_of("v") {
+    let log_level = match app.verbosity {
         0 => log::LevelFilter::Info,
         1 => log::LevelFilter::Debug,
         _ => log::LevelFilter::Trace,
     };
-    let log_to_file = !matches.is_present("disable_log_to_file");
-    let log_stdout_timestamps = !matches.is_present("disable_stdout_timestamps");
-
-    #[cfg(target_os = "linux")]
-    let initialize_firewall_and_exit =
-        cfg!(target_os = "linux") && matches.is_present("initialize-early-boot-firewall");
-    let run_as_service = cfg!(windows) && matches.is_present("run_as_service");
-    let register_service = cfg!(windows) && matches.is_present("register_service");
-    #[cfg(target_os = "macos")]
-    let launch_daemon_status = matches.is_present("launch_daemon_status");
 
     Config {
-        #[cfg(target_os = "linux")]
-        initialize_firewall_and_exit,
         log_level,
-        log_to_file,
-        log_stdout_timestamps,
-        run_as_service,
-        register_service,
+        log_to_file: !app.disable_log_to_file,
+        log_stdout_timestamps: !app.disable_stdout_timestamps,
+        #[cfg(target_os = "windows")]
+        run_as_service: app.run_as_service,
+        #[cfg(target_os = "windows")]
+        register_service: app.register_service,
         #[cfg(target_os = "macos")]
-        launch_daemon_status,
-    }
-}
-
-lazy_static::lazy_static! {
-    static ref ENV_DESC: String = format!(
-"ENV:
-
-    MULLVAD_RESOURCE_DIR       Resource directory (i.e used to locate a root CA certificate)
-                               [Default: {}]
-    MULLVAD_SETTINGS_DIR       Directory path for storing settings. [Default: {}]
-    MULLVAD_CACHE_DIR          Directory path for storing cache. [Default: {}]
-    MULLVAD_LOG_DIR            Directory path for storing logs. [Default: {}]
-    MULLVAD_RPC_SOCKET_PATH    Location of the management interface device.
-                               It refers to Unix domain socket on Unix based platforms, and named pipe on Windows.
-                               [Default: {}]
-
-",
-        mullvad_paths::get_default_resource_dir().display(),
-        mullvad_paths::get_default_settings_dir().map(|dir| dir.display().to_string()).unwrap_or_else(|_| "N/A".to_string()),
-        mullvad_paths::get_default_cache_dir().map(|dir| dir.display().to_string()).unwrap_or_else(|_| "N/A".to_string()),
-        mullvad_paths::get_default_log_dir().map(|dir| dir.display().to_string()).unwrap_or_else(|_| "N/A".to_string()),
-        mullvad_paths::get_default_rpc_socket_path().display());
-}
-
-fn create_app() -> App<'static> {
-    let mut app = App::new(crate_name!())
-        .version(mullvad_version::VERSION)
-        .author(crate_authors!(", "))
-        .about(crate_description!())
-        .after_help(ENV_DESC.as_str())
-        .arg(
-            Arg::new("v")
-                .short('v')
-                .multiple_occurrences(true)
-                .help("Sets the level of verbosity"),
-        )
-        .arg(
-            Arg::new("disable_log_to_file")
-                .long("disable-log-to-file")
-                .help("Disable logging to file"),
-        )
-        .arg(
-            Arg::new("disable_stdout_timestamps")
-                .long("disable-stdout-timestamps")
-                .help("Don't log timestamps when logging to stdout, useful when running as a systemd service")
-        );
-
-    if cfg!(windows) {
-        app = app.arg(
-            Arg::new("run_as_service")
-                .long("run-as-service")
-                .help("Run as a system service. On Windows this option must be used when running a system service"),
-        )
-        .arg(
-            Arg::new("register_service")
-                .long("register-service")
-                .help("Register itself as a system service"),
-        )
-    }
-
-    if cfg!(target_os = "linux") {
-        app = app.arg(
-            Arg::new("initialize-early-boot-firewall")
-                .long("initialize-early-boot-firewall")
-                .help("Initialize firewall to be used during early boot and exit"),
-        )
-    }
-
-    if cfg!(target_os = "macos") {
-        app = app.arg(Arg::new("launch_daemon_status").long("launch-daemon-status").help(
-            "Checks the status of the launch daemon. The exit code represents the current status",
-        ))
+        launch_daemon_status: app.launch_daemon_status,
+        #[cfg(target_os = "linux")]
+        initialize_firewall_and_exit: app.initialize_early_boot_firewall,
     }
-    app
 }
diff --git a/mullvad-jni/src/problem_report.rs b/mullvad-jni/src/problem_report.rs
index f1a7e884db..9943ec4c59 100644
--- a/mullvad-jni/src/problem_report.rs
+++ b/mullvad-jni/src/problem_report.rs
@@ -23,7 +23,7 @@ pub extern "system" fn Java_net_mullvad_mullvadvpn_dataproxy_MullvadProblemRepor
     let output_path_string = String::from_java(&env, outputPath);
     let output_path = Path::new(&output_path_string);
 
-    match mullvad_problem_report::collect_report(&[], output_path, Vec::new(), log_dir) {
+    match mullvad_problem_report::collect_report::<&str>(&[], output_path, Vec::new(), log_dir) {
         Ok(()) => JNI_TRUE,
         Err(error) => {
             log::error!(
diff --git a/mullvad-problem-report/Cargo.toml b/mullvad-problem-report/Cargo.toml
index d516186203..5992b4570f 100644
--- a/mullvad-problem-report/Cargo.toml
+++ b/mullvad-problem-report/Cargo.toml
@@ -23,7 +23,7 @@ talpid-types = { path = "../talpid-types" }
 talpid-platform-metadata = { path = "../talpid-platform-metadata" }
 
 [target.'cfg(not(target_os="android"))'.dependencies]
-clap = { version = "3.0", features = ["cargo"] }
+clap = { version = "4.2.7", features = ["cargo"] }
 env_logger = "0.10.0"
 
 [target.'cfg(target_os = "android")'.dependencies]
diff --git a/mullvad-problem-report/src/lib.rs b/mullvad-problem-report/src/lib.rs
index 9a9b347677..088ceb27df 100644
--- a/mullvad-problem-report/src/lib.rs
+++ b/mullvad-problem-report/src/lib.rs
@@ -106,8 +106,8 @@ pub enum LogError {
     NoLocalAppDataDir,
 }
 
-pub fn collect_report(
-    extra_logs: &[&Path],
+pub fn collect_report<P: AsRef<Path>>(
+    extra_logs: &[P],
     output_path: &Path,
     redact_custom_strings: Vec<String>,
     #[cfg(target_os = "android")] android_log_dir: &Path,
diff --git a/mullvad-problem-report/src/main.rs b/mullvad-problem-report/src/main.rs
index af38f614d2..50a2e0ce18 100644
--- a/mullvad-problem-report/src/main.rs
+++ b/mullvad-problem-report/src/main.rs
@@ -1,8 +1,12 @@
 #![deny(rust_2018_idioms)]
 
-use clap::{crate_authors, crate_name};
+use clap::Parser;
 use mullvad_problem_report::{collect_report, Error};
-use std::{env, path::Path, process};
+use std::{
+    env,
+    path::{Path, PathBuf},
+    process,
+};
 use talpid_types::ErrorExt;
 
 fn main() {
@@ -15,110 +19,70 @@ fn main() {
     })
 }
 
+#[derive(Debug, Parser)]
+#[command(author, version = mullvad_version::VERSION, about, long_about = None)]
+#[command(
+    arg_required_else_help = true,
+    disable_help_subcommand = true,
+    disable_version_flag = true
+)]
+enum Cli {
+    /// Collect problem report to a single file
+    Collect {
+        /// The destination path for saving the collected report
+        #[arg(required = true, long, short = 'o')]
+        output: PathBuf,
+        /// Paths to additional log files to be included
+        extra_logs: Vec<PathBuf>,
+        /// List of strings to remove from the report
+        #[arg(long)]
+        redact: Vec<String>,
+    },
+
+    /// Send collected problem report
+    Send {
+        /// Path to a previously collected report file
+        #[arg(required = true, long, short = 'r')]
+        report: PathBuf,
+        /// Email to attach to the problem report
+        #[arg(long, short = 'e')]
+        email: Option<String>,
+        /// Message to include in the problem report
+        #[arg(long, short = 'm')]
+        message: Option<String>,
+    },
+}
+
 fn run() -> Result<(), Error> {
     env_logger::init();
-    let app = clap::App::new(crate_name!())
-        .version(mullvad_version::VERSION)
-        .author(crate_authors!())
-        .about("Mullvad VPN problem report tool. Collects logs and sends them to Mullvad support.")
-        .setting(clap::AppSettings::SubcommandRequiredElseHelp)
-        .global_setting(clap::AppSettings::DisableHelpSubcommand)
-        .global_setting(clap::AppSettings::DisableVersionFlag)
-        .subcommand(
-            clap::App::new("collect")
-                .about("Collect problem report")
-                .arg(
-                    clap::Arg::new("output")
-                        .help("The destination path for saving the collected report.")
-                        .long("output")
-                        .short('o')
-                        .value_name("PATH")
-                        .allow_invalid_utf8(true)
-                        .takes_value(true)
-                        .required(true),
-                )
-                .arg(
-                    clap::Arg::new("extra_logs")
-                        .help("Paths to additional log files to be included.")
-                        .multiple_occurrences(true)
-                        .multiple_values(true)
-                        .value_name("EXTRA LOGS")
-                        .allow_invalid_utf8(true)
-                        .takes_value(true)
-                        .required(false),
-                )
-                .arg(
-                    clap::Arg::new("redact")
-                        .help("List of words and expressions to remove from the report")
-                        .long("redact")
-                        .value_name("PHRASE")
-                        .multiple_occurrences(true)
-                        .multiple_values(true)
-                        .takes_value(true),
-                ),
-        )
-        .subcommand(
-            clap::App::new("send")
-                .about("Send collected problem report")
-                .arg(
-                    clap::Arg::new("report")
-                        .long("report")
-                        .short('r')
-                        .help("The path to previously collected report file.")
-                        .allow_invalid_utf8(true)
-                        .takes_value(true)
-                        .required(true),
-                )
-                .arg(
-                    clap::Arg::new("email")
-                        .long("email")
-                        .short('e')
-                        .help("Reporter's email")
-                        .takes_value(true)
-                        .required(false),
-                )
-                .arg(
-                    clap::Arg::new("message")
-                        .long("message")
-                        .short('m')
-                        .help("Reporter's message")
-                        .takes_value(true)
-                        .required(false),
-                ),
-        );
-
-    let matches = app.get_matches();
 
-    if let Some(collect_matches) = matches.subcommand_matches("collect") {
-        let redact_custom_strings = collect_matches
-            .values_of_t("redact")
-            .unwrap_or_else(|_| vec![]);
-        let extra_logs = collect_matches
-            .values_of_os("extra_logs")
-            .map(|os_values| os_values.map(Path::new).collect())
-            .unwrap_or_else(Vec::new);
-        let output_path = Path::new(collect_matches.value_of_os("output").unwrap());
-        collect_report(&extra_logs, output_path, redact_custom_strings)?;
+    match Cli::parse() {
+        Cli::Collect {
+            output,
+            extra_logs,
+            redact,
+        } => {
+            collect_report(&extra_logs, &output, redact)?;
 
-        let expanded_output_path = output_path
-            .canonicalize()
-            .unwrap_or_else(|_| output_path.to_owned());
-        println!(
-            "Problem report written to {}",
-            expanded_output_path.display()
-        );
-        println!();
-        println!("Send the problem report to support via the send subcommand. See:");
-        println!(" $ {} send --help", env::args().next().unwrap());
-        Ok(())
-    } else if let Some(send_matches) = matches.subcommand_matches("send") {
-        let report_path = Path::new(send_matches.value_of_os("report").unwrap());
-        let user_email = send_matches.value_of("email").unwrap_or("");
-        let user_message = send_matches.value_of("message").unwrap_or("");
-        send_problem_report(user_email, user_message, report_path)
-    } else {
-        unreachable!("No sub command given");
+            println!("Problem report written to {}", output.display());
+            println!();
+            println!("Send the problem report to support via the send subcommand. See:");
+            println!(" $ {} send --help", env::args().next().unwrap());
+        }
+        Cli::Send {
+            report,
+            email,
+            message,
+        } => {
+            send_problem_report(
+                &email.unwrap_or_default(),
+                &message.unwrap_or_default(),
+                &report,
+            )?;
+        }
     }
+
+    Ok(())
 }
 
 fn send_problem_report(
@@ -128,9 +92,11 @@ fn send_problem_report(
 ) -> Result<(), Error> {
     let cache_dir = mullvad_paths::get_cache_dir().map_err(Error::ObtainCacheDirectory)?;
     mullvad_problem_report::send_problem_report(user_email, user_message, report_path, &cache_dir)
-        .map(|()| println!("Problem report sent"))
         .map_err(|error| {
             eprintln!("{}", error.display_chain());
             error
-        })
+        })?;
+
+    println!("Problem report sent");
+    Ok(())
 }