From a4219b5ba38c98b2743955fedb0916d137cec007 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Wed, 3 Aug 2022 23:48:30 -0700 Subject: [PATCH 1/6] Allow all discovered `c2rust` subcommands and don't do subcommand arg parsing in the `c2rust` binary. --- Cargo.lock | 11 +++ c2rust/Cargo.toml | 6 +- c2rust/src/main.rs | 189 ++++++++++++++++++++++++++++++++++----------- 3 files changed, 159 insertions(+), 47 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6ec14a28ca..40f05eea43 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -144,9 +144,11 @@ version = "0.16.0" dependencies = [ "anyhow", "c2rust-transpile", + "camino", "clap 2.34.0", "env_logger", "git-testament", + "is_executable", "log", "regex", "rustc-private-link", @@ -767,6 +769,15 @@ dependencies = [ "similar", ] +[[package]] +name = "is_executable" +version = "1.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "fa9acdc6d67b75e626ad644734e8bc6df893d9cd2a834129065d3dd6158ea9c8" +dependencies = [ + "winapi", +] + [[package]] name = "itertools" version = "0.10.3" diff --git a/c2rust/Cargo.toml b/c2rust/Cargo.toml index 891f991d74..7e79499948 100644 --- a/c2rust/Cargo.toml +++ b/c2rust/Cargo.toml @@ -18,10 +18,12 @@ azure-devops = { project = "immunant/c2rust", pipeline = "immunant.c2rust", buil [dependencies] anyhow = "1.0" -clap = {version = "2.34", features = ["yaml"]} -log = "0.4" +camino = "1.0" +clap = { version = "2.34", features = ["yaml"] } env_logger = "0.9" git-testament = "0.2.1" +is_executable = "1.0" +log = "0.4" regex = "1.3" shlex = "1.1" c2rust-transpile = { version = "0.16.0", path = "../c2rust-transpile" } diff --git a/c2rust/src/main.rs b/c2rust/src/main.rs index a58220c931..5ca273f987 100644 --- a/c2rust/src/main.rs +++ b/c2rust/src/main.rs @@ -1,56 +1,155 @@ -use clap::{crate_authors, load_yaml, App, AppSettings, SubCommand}; +use anyhow::anyhow; +use camino::Utf8Path; +use clap::{crate_authors, App, AppSettings, Arg}; use git_testament::{git_testament, render_testament}; +use is_executable::IsExecutable; +use std::borrow::Cow; +use std::collections::HashMap; use std::env; use std::ffi::OsStr; -use std::process::{exit, Command}; +use std::path::{Path, PathBuf}; +use std::process; +use std::process::Command; +use std::str; git_testament!(TESTAMENT); -fn main() { - let subcommand_yamls = [load_yaml!("transpile.yaml")]; - let matches = App::new("C2Rust") - .version(&*render_testament!(TESTAMENT)) - .author(crate_authors!(", ")) - .setting(AppSettings::SubcommandRequiredElseHelp) - .subcommands( - subcommand_yamls - .iter() - .map(|yaml| SubCommand::from_yaml(yaml)), - ) - .get_matches(); +/// A `c2rust` sub-command. +struct SubCommand { + /// The path to the [`SubCommand`]'s executable. + path: Option, + /// The name of the [`SubCommand`], i.e. in `c2rust-{name}`. + name: Cow<'static, str>, +} - let mut os_args = env::args_os(); - os_args.next(); // Skip executable name - let arg_name = os_args.next().and_then(|name| name.into_string().ok()); - match (&arg_name, matches.subcommand_name()) { - (Some(arg_name), Some(subcommand)) if arg_name == subcommand => { - invoke_subcommand(subcommand, os_args); +impl SubCommand { + /// Find all [`SubCommand`]s adjacent to the current (`c2rust`) executable. + /// They are of the form `c2rust-{name}`. + pub fn find_all() -> anyhow::Result> { + let c2rust = env::current_exe()?; + let c2rust_name: &Utf8Path = c2rust + .file_name() + .map(Path::new) + .ok_or_else(|| anyhow!("no file name: {}", c2rust.display()))? + .try_into()?; + let c2rust_name = c2rust_name.as_str(); + let dir = c2rust + .parent() + .ok_or_else(|| anyhow!("no directory: {}", c2rust.display()))?; + let mut sub_commands = Vec::new(); + for entry in dir.read_dir()? { + let entry = entry?; + let file_type = entry.file_type()?; + if !(file_type.is_file() || file_type.is_symlink()) { + continue; + } + let file_name = entry.file_name(); + let file_name: &Utf8Path = Path::new(&file_name).try_into()?; + let file_name = file_name.as_str(); + let name = match file_name.strip_prefix(c2rust_name) { + Some(name) => name, + None => continue, + }; + let name = match name.strip_prefix('-') { + Some(name) => name, + None => continue, + }; + let path = entry.path(); + if !path.is_executable() { + continue; + } + sub_commands.push(Self { + path: Some(path), + name: name.to_owned().into(), + }); } - _ => { - eprintln!("{:?}", arg_name); - panic!("Could not match subcommand"); - } - }; + Ok(sub_commands) + } + + /// Get all known [`SubCommand`]s. These have no [`SubCommand::path`]. + /// Even if the subcommand executables aren't there, we can still suggest them. + pub fn known() -> impl Iterator { + ["transpile", "instrument", "pdg", "analyze"] + .into_iter() + .map(|name| Self { + path: None, + name: name.into(), + }) + } + + /// Get all known ([`Self::known`]) and actual, found ([`Self::find_all`]) subcommands, + /// putting the known ones first so that the found ones overwrite them and take precedence. + pub fn all() -> anyhow::Result> { + Ok(Self::known().chain(Self::find_all()?)) + } + + pub fn invoke(&self, args: I) -> anyhow::Result<()> + where + I: IntoIterator, + S: AsRef, + { + let path = self.path.as_ref().ok_or_else(|| { + anyhow!( + "known subcommand not found (probably not built): {}", + self.name + ) + })?; + let status = Command::new(&path).args(args).status()?; + process::exit(status.code().unwrap_or(1)); + } } -fn invoke_subcommand(subcommand: &str, args: I) -where - I: IntoIterator, - S: AsRef, -{ - // Assumes the subcommand executable is in the same directory as this driver - // program. - let cmd_path = std::env::current_exe().expect("Cannot get current executable path"); - let mut cmd_path = cmd_path.as_path().canonicalize().unwrap(); - cmd_path.pop(); // remove current executable - cmd_path.push(format!("c2rust-{}", subcommand)); - assert!(cmd_path.exists(), "{:?} is missing", cmd_path); - exit( - Command::new(cmd_path.into_os_string()) - .args(args) - .status() - .expect("SubCommand failed to start") - .code() - .unwrap_or(-1), - ); +fn main() -> anyhow::Result<()> { + let sub_commands = SubCommand::all()?.collect::>(); + let sub_commands = sub_commands + .iter() + .map(|cmd| (cmd.name.as_ref(), cmd)) + .collect::>(); + + // If the subcommand matches, don't use `clap` at all. + // + // I can't seem to get `clap` to pass through all arguments as is, + // like the ones with hyphens like `--metadata`, + // even though I've set [`Arg::allow_hyphen_values`]. + // This is faster anyways. + // + // Furthermore, doing it this way correctly forwards `--help` through to the subcommand + // instead of `clap` intercepting it and displaying the top-level `--help`. + let mut args = env::args_os(); + args.next(); // skip exe name + let sub_command = args.next(); + let sub_command = sub_command + .as_ref() + .and_then(|arg| arg.to_str()) + .and_then(|name| sub_commands.get(name)); + + if let Some(sub_command) = sub_command { + return sub_command.invoke(args); + } + + // If we didn't get a subcommand, then use `clap` for parsing and error/help messages. + + // For some stupid reason, [`Arg::possible_values`] requires a slice, not an iterator. + // let sub_command_names = sub_commands.keys().copied().collect::>(); + let matches = App::new("C2Rust") + .version(&*render_testament!(TESTAMENT)) + .author(crate_authors!(", ")) + .settings(&[ + AppSettings::SubcommandRequiredElseHelp, + AppSettings::AllowExternalSubcommands, + ]) + .subcommands(sub_commands.keys().map(|name| { + clap::SubCommand::with_name(name).arg( + Arg::with_name("args") + .multiple(true) + .allow_hyphen_values(true), + ) + })) + // .arg(Arg::with_name("subcommand").possible_values(sub_command_names.as_slice())) + .get_matches(); + let sub_command_name = matches + .subcommand_name() + // .value_of("subcommand") + .ok_or_else(|| anyhow!("no subcommand"))?; + sub_commands[sub_command_name].invoke(args) } From 5956ff54772f2ded2253cf17a153bdb5fbf26e4a Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Wed, 3 Aug 2022 23:51:52 -0700 Subject: [PATCH 2/6] Deleted old `clap` code I tried that didn't work and moved the failure explanations to comments. --- c2rust/src/main.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/c2rust/src/main.rs b/c2rust/src/main.rs index 5ca273f987..1ffd5b49c3 100644 --- a/c2rust/src/main.rs +++ b/c2rust/src/main.rs @@ -16,7 +16,9 @@ git_testament!(TESTAMENT); /// A `c2rust` sub-command. struct SubCommand { - /// The path to the [`SubCommand`]'s executable. + /// The path to the [`SubCommand`]'s executable, + /// if it was found (see [`Self::find_all`]). + /// Otherwise [`None`] if it is a known [`SubCommand`] (see [`Self::known`]). path: Option, /// The name of the [`SubCommand`], i.e. in `c2rust-{name}`. name: Cow<'static, str>, @@ -112,6 +114,8 @@ fn main() -> anyhow::Result<()> { // like the ones with hyphens like `--metadata`, // even though I've set [`Arg::allow_hyphen_values`]. // This is faster anyways. + // I also tried a single "subcommand" argument with [`Arg::possible_values`], + // but that had the same problem passing through all arguments as well. // // Furthermore, doing it this way correctly forwards `--help` through to the subcommand // instead of `clap` intercepting it and displaying the top-level `--help`. @@ -128,9 +132,6 @@ fn main() -> anyhow::Result<()> { } // If we didn't get a subcommand, then use `clap` for parsing and error/help messages. - - // For some stupid reason, [`Arg::possible_values`] requires a slice, not an iterator. - // let sub_command_names = sub_commands.keys().copied().collect::>(); let matches = App::new("C2Rust") .version(&*render_testament!(TESTAMENT)) .author(crate_authors!(", ")) @@ -145,11 +146,9 @@ fn main() -> anyhow::Result<()> { .allow_hyphen_values(true), ) })) - // .arg(Arg::with_name("subcommand").possible_values(sub_command_names.as_slice())) .get_matches(); let sub_command_name = matches .subcommand_name() - // .value_of("subcommand") .ok_or_else(|| anyhow!("no subcommand"))?; sub_commands[sub_command_name].invoke(args) } From 3cacb965983105fcf7895a3c382f3b5eb8e91cd9 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 23 Aug 2022 00:16:03 -0700 Subject: [PATCH 3/6] Replaced 2 `args.next()`s with 1 `args.nth(1)` to correctly propagate a `None` on the first `args.next()`. --- c2rust/src/main.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/c2rust/src/main.rs b/c2rust/src/main.rs index 1ffd5b49c3..61eab62e21 100644 --- a/c2rust/src/main.rs +++ b/c2rust/src/main.rs @@ -120,8 +120,7 @@ fn main() -> anyhow::Result<()> { // Furthermore, doing it this way correctly forwards `--help` through to the subcommand // instead of `clap` intercepting it and displaying the top-level `--help`. let mut args = env::args_os(); - args.next(); // skip exe name - let sub_command = args.next(); + let sub_command = args.nth(1); let sub_command = sub_command .as_ref() .and_then(|arg| arg.to_str()) From 18e95f5becbb679a78aea83969fcbbd9177d52f8 Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 23 Aug 2022 00:29:10 -0700 Subject: [PATCH 4/6] Simplified control flow logic for the `.strip_prefix`s and used `Path::to_str` over `UtfPath::to_str` since we just want to skip the file, not error on it. --- c2rust/src/main.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/c2rust/src/main.rs b/c2rust/src/main.rs index 61eab62e21..a064df4c53 100644 --- a/c2rust/src/main.rs +++ b/c2rust/src/main.rs @@ -46,13 +46,11 @@ impl SubCommand { continue; } let file_name = entry.file_name(); - let file_name: &Utf8Path = Path::new(&file_name).try_into()?; - let file_name = file_name.as_str(); - let name = match file_name.strip_prefix(c2rust_name) { - Some(name) => name, - None => continue, - }; - let name = match name.strip_prefix('-') { + let name = match Path::new(&file_name) + .to_str() + .and_then(|name| name.strip_prefix(c2rust_name)) + .and_then(|name| name.strip_prefix('-')) + { Some(name) => name, None => continue, }; From c4e17fd198a1cd43824adb34c3e3c70eea85874c Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 23 Aug 2022 00:34:07 -0700 Subject: [PATCH 5/6] Refactored things to use more `.filter()`s, `.and_then()`s, and `.map()`s to make it cleaner and more concise. --- c2rust/src/main.rs | 19 ++++++++----------- 1 file changed, 8 insertions(+), 11 deletions(-) diff --git a/c2rust/src/main.rs b/c2rust/src/main.rs index a064df4c53..7af5ac9c31 100644 --- a/c2rust/src/main.rs +++ b/c2rust/src/main.rs @@ -42,25 +42,22 @@ impl SubCommand { for entry in dir.read_dir()? { let entry = entry?; let file_type = entry.file_type()?; - if !(file_type.is_file() || file_type.is_symlink()) { - continue; - } - let file_name = entry.file_name(); - let name = match Path::new(&file_name) - .to_str() + let path = entry.path(); + let name = match path.file_name() + .and_then(|name| name.to_str()) .and_then(|name| name.strip_prefix(c2rust_name)) .and_then(|name| name.strip_prefix('-')) + .map(|name| name.to_owned()) + .map(Cow::from) + .filter(|_| file_type.is_file() || file_type.is_symlink()) + .filter(|_| path.is_executable()) { Some(name) => name, None => continue, }; - let path = entry.path(); - if !path.is_executable() { - continue; - } sub_commands.push(Self { path: Some(path), - name: name.to_owned().into(), + name, }); } Ok(sub_commands) From 4ee25824c4c27afa89e19cbf18d2cdcba99a564d Mon Sep 17 00:00:00 2001 From: Khyber Sen Date: Tue, 23 Aug 2022 00:35:05 -0700 Subject: [PATCH 6/6] Replaced the `match` and `continue` with an `if let`. --- c2rust/src/main.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/c2rust/src/main.rs b/c2rust/src/main.rs index 7af5ac9c31..9a447cc843 100644 --- a/c2rust/src/main.rs +++ b/c2rust/src/main.rs @@ -43,22 +43,21 @@ impl SubCommand { let entry = entry?; let file_type = entry.file_type()?; let path = entry.path(); - let name = match path.file_name() + let name = path + .file_name() .and_then(|name| name.to_str()) .and_then(|name| name.strip_prefix(c2rust_name)) .and_then(|name| name.strip_prefix('-')) .map(|name| name.to_owned()) .map(Cow::from) .filter(|_| file_type.is_file() || file_type.is_symlink()) - .filter(|_| path.is_executable()) - { - Some(name) => name, - None => continue, - }; - sub_commands.push(Self { - path: Some(path), - name, - }); + .filter(|_| path.is_executable()); + if let Some(name) = name { + sub_commands.push(Self { + path: Some(path), + name, + }); + } } Ok(sub_commands) }