Cleanup and add some more comments

ian-h-chamberlain · ian-h-chamberlain · commit 6b4fd53f2086 · 2023-08-30T23:02:55.000-04:00
Minor tweaks like fn names, refactoring a little etc.

Also update README to include an install command, with `--locked` as
well.
diff --git a/README.md b/README.md
@@ -2,6 +2,20 @@
 
 Cargo command to work with Nintendo 3DS project binaries. Based on cargo-psp.
 
+## Installation
+
+To install the latest release on <https://crates.io>:
+
+```sh
+cargo install --locked cargo-3ds
+```
+
+To install the current `master` version of `cargo-3ds`:
+
+```sh
+cargo install --locked --git https://github.com/rust3ds/cargo-3ds
+```
+
 ## Usage
 
 Use the nightly toolchain to build 3DS apps (either by using `rustup override nightly` for the project directory or by adding `+nightly` in the `cargo` invocation).
diff --git a/src/command.rs b/src/command.rs
@@ -6,7 +6,7 @@ use std::sync::OnceLock;
 use cargo_metadata::Message;
 use clap::{Args, Parser, Subcommand};
 
-use crate::{build_3dsx, build_smdh, find_cargo, get_metadata, link, print_command, CTRConfig};
+use crate::{build_3dsx, build_smdh, cargo, get_metadata, link, print_command, CTRConfig};
 
 #[derive(Parser, Debug)]
 #[command(name = "cargo", bin_name = "cargo")]
@@ -22,7 +22,7 @@ pub struct Input {
     pub cmd: CargoCmd,
 
     /// Print the exact commands `cargo-3ds` is running. Note that this does not
-    /// set the verbose flag for cargo itself. To set cargo's verbose flag, add
+    /// set the verbose flag for cargo itself. To set cargo's verbosity flag, add
     /// `-- -v` to the end of the command line.
     #[arg(long, short = 'v', global = true)]
     pub verbose: bool,
@@ -165,21 +165,21 @@ impl CargoCmd {
 
                 // We can't run 3DS executables on the host, but we want to respect
                 // the user's "runner" configuration if set.
-                if test.doc {
-                    eprintln!("Documentation tests requested, no 3dsx will be built");
+                //
+                // If doctests were requested, `--no-run` will be rejected on the
+                // command line and must be set with RUSTDOCFLAGS instead:
+                // https://github.com/rust-lang/rust/issues/87022
+                if !test.run_args.use_custom_runner() && !test.doc {
+                    cargo_args.push("--no-run".to_string());
+                }
 
-                    // https://github.com/rust-lang/cargo/issues/7040
-                    cargo_args.append(&mut vec![
-                        "--doc".to_string(),
-                        "-Z".to_string(),
-                        "doctest-xcompile".to_string(),
-                        // doctests don't automatically build the `test` crate,
-                        // so we manually specify it on the command line
-                        "-Z".to_string(),
-                        "build-std=std,test".to_string(),
+                if test.doc {
+                    cargo_args.extend([
+                        "--doc".into(),
+                        // https://github.com/rust-lang/cargo/issues/7040
+                        "-Z".into(),
+                        "doctest-xcompile".into(),
                     ]);
-                } else if !test.run_args.is_runner_configured() {
-                    cargo_args.push("--no-run".to_string());
                 }
 
                 cargo_args
@@ -206,7 +206,7 @@ impl CargoCmd {
         match self {
             CargoCmd::Build(_) => "build",
             CargoCmd::Run(run) => {
-                if run.is_runner_configured() {
+                if run.use_custom_runner() {
                     "run"
                 } else {
                     "build"
@@ -228,23 +228,31 @@ impl CargoCmd {
 
     /// Whether or not this command should build a 3DSX executable file.
     pub fn should_build_3dsx(&self) -> bool {
-        matches!(
-            self,
-            Self::Build(_) | Self::Run(_) | Self::Test(Test { doc: false, .. })
-        )
+        match self {
+            Self::Build(_) | CargoCmd::Run(_) => true,
+            &Self::Test(Test { doc, .. }) => {
+                if doc {
+                    eprintln!("Documentation tests requested, no 3dsx will be built");
+                    false
+                } else {
+                    true
+                }
+            }
+            _ => false,
+        }
     }
 
     /// Whether or not the resulting executable should be sent to the 3DS with
     /// `3dslink`.
     pub fn should_link_to_device(&self) -> bool {
         match self {
-            Self::Test(test) => !(test.no_run || test.run_args.is_runner_configured()),
-            Self::Run(run) => !run.is_runner_configured(),
+            Self::Test(Test { no_run: true, .. }) => false,
+            Self::Run(run) | Self::Test(Test { run_args: run, .. }) => !run.use_custom_runner(),
             _ => false,
         }
     }
 
-    pub const DEFAULT_MESSAGE_FORMAT: &str = "json-render-diagnostics";
+    pub const DEFAULT_MESSAGE_FORMAT: &'static str = "json-render-diagnostics";
 
     pub fn extract_message_format(&mut self) -> Result<Option<String>, String> {
         let cargo_args = match self {
@@ -347,7 +355,7 @@ impl RemainingArgs {
 
         if let Some(split) = args.iter().position(|s| s == "--") {
             let second_half = args.split_off(split + 1);
-            // take off the "--" arg we found
+            // take off the "--" arg we found, we'll add one later if needed
             args.pop();
 
             (args, second_half)
@@ -421,7 +429,7 @@ impl Run {
         // Run the normal "build" callback
         self.build_args.callback(config);
 
-        if !self.is_runner_configured() {
+        if !self.use_custom_runner() {
             if let Some(cfg) = config {
                 eprintln!("Running 3dslink");
                 link(cfg, self, self.build_args.verbose);
@@ -431,45 +439,44 @@ impl Run {
 
     /// Returns whether the cargo environment has `target.armv6k-nintendo-3ds.runner`
     /// configured. This will only be checked once during the lifetime of the program,
-    /// and takes into account the usual ways Cargo looks for
+    /// and takes into account the usual ways Cargo looks for its
     /// [configuration](https://doc.rust-lang.org/cargo/reference/config.html):
     ///
     /// - `.cargo/config.toml`
     /// - Environment variables
     /// - Command-line `--config` overrides
-    pub fn is_runner_configured(&self) -> bool {
+    pub fn use_custom_runner(&self) -> bool {
         static HAS_RUNNER: OnceLock<bool> = OnceLock::new();
 
-        let has_runner = HAS_RUNNER.get_or_init(|| {
-            let mut cmd = find_cargo();
-
-            let config_args = self.config.iter().map(|cfg| format!("--config={cfg}"));
-
-            cmd.args(config_args)
-                .args([
-                    "config",
-                    "-Zunstable-options",
-                    "get",
-                    "target.armv6k-nintendo-3ds.runner",
-                ])
-                .stdout(Stdio::null())
-                .stderr(Stdio::null());
+        let &custom_runner_configured = HAS_RUNNER.get_or_init(|| {
+            let mut cmd = cargo(&self.config);
+            cmd.args([
+                // https://github.com/rust-lang/cargo/issues/9301
+                "-Z",
+                "unstable-options",
+                "config",
+                "get",
+                "target.armv6k-nintendo-3ds.runner",
+            ])
+            .stdout(Stdio::null())
+            .stderr(Stdio::null());
 
             if self.build_args.verbose {
                 print_command(&cmd);
             }
 
+            // `cargo config get` exits zero if the config exists, or nonzero otherwise
             cmd.status().map_or(false, |status| status.success())
         });
 
         if self.build_args.verbose {
             eprintln!(
                 "Custom runner is {}configured",
-                if *has_runner { "" } else { "not " }
+                if custom_runner_configured { "" } else { "not " }
             );
         }
 
-        *has_runner
+        custom_runner_configured
     }
 }
 
@@ -488,13 +495,13 @@ impl Test {
     }
 }
 
-const TOML_CHANGES: &str = "ctru-rs = { git = \"https://github.com/rust3ds/ctru-rs\" }
+const TOML_CHANGES: &str = r#"ctru-rs = { git = "https://github.com/rust3ds/ctru-rs" }
 
 [package.metadata.cargo-3ds]
-romfs_dir = \"romfs\"
-";
+romfs_dir = "romfs"
+"#;
 
-const CUSTOM_MAIN_RS: &str = "use ctru::prelude::*;
+const CUSTOM_MAIN_RS: &str = r#"use ctru::prelude::*;
 
 fn main() {
     ctru::use_panic_handler();
@@ -504,8 +511,8 @@ fn main() {
     let gfx = Gfx::new().unwrap();
     let _console = Console::new(gfx.top_screen.borrow_mut());
 
-    println!(\"Hello, World!\");
-    println!(\"\\x1b[29;16HPress Start to exit\");
+    println!("Hello, World!");
+    println!("\x1b[29;16HPress Start to exit");
 
     while apt.main_loop() {
         gfx.wait_for_vblank();
@@ -516,7 +523,7 @@ fn main() {
         }
     }
 }
-";
+"#;
 
 impl New {
     /// Callback for `cargo 3ds new`.
diff --git a/src/lib.rs b/src/lib.rs
@@ -63,16 +63,14 @@ pub fn run_cargo(input: &Input, message_format: Option<String>) -> (ExitStatus,
 /// For "build" commands (which compile code, such as `cargo 3ds build` or `cargo 3ds clippy`),
 /// if there is no pre-built std detected in the sysroot, `build-std` will be used instead.
 pub fn make_cargo_command(input: &Input, message_format: &Option<String>) -> Command {
-    let cmd = &input.cmd;
+    let cargo_cmd = &input.cmd;
 
-    let mut command = find_cargo();
-    command
-        .args(input.config.iter().map(|cfg| format!("--config={cfg}")))
-        .arg(cmd.subcommand_name());
+    let mut command = cargo(&input.config);
+    command.arg(cargo_cmd.subcommand_name());
 
     // Any command that needs to compile code will run under this environment.
     // Even `clippy` and `check` need this kind of context, so we'll just assume any other `Passthrough` command uses it too.
-    if cmd.should_compile() {
+    if cargo_cmd.should_compile() {
         let rust_flags = env::var("RUSTFLAGS").unwrap_or_default()
             + &format!(
                 " -L{}/libctru/lib -lctru",
@@ -93,33 +91,30 @@ pub fn make_cargo_command(input: &Input, message_format: &Option<String>) -> Com
         let sysroot = find_sysroot();
         if !sysroot.join("lib/rustlib/armv6k-nintendo-3ds").exists() {
             eprintln!("No pre-build std found, using build-std");
-            // TODO: should we consider always building `test` ? It's always needed
-            // if e.g. `test-runner` is a dependency, but not necessarily needed for
-            // production code.
-            command.arg("-Z").arg("build-std");
+            // Always building the test crate is not ideal, but we don't know if the
+            // crate being built uses #![feature(test)], so we build it just in case.
+            command.arg("-Z").arg("build-std=std,test");
         }
     }
 
-    let cargo_args = cmd.cargo_args();
-    command.args(cargo_args);
-
-    if let CargoCmd::Test(test) = cmd {
-        let no_run_flag = if test.run_args.is_runner_configured() {
-            // TODO: should we persist here as well? Or maybe just let the user
-            // add that to RUSTDOCFLAGS if they want it...
+    if let CargoCmd::Test(test) = cargo_cmd {
+        let no_run_flag = if test.run_args.use_custom_runner() {
             ""
         } else {
+            // We don't support running doctests by default, but cargo doesn't like
+            // --no-run for doctests, so we have to plumb it in via RUSTDOCFLAGS
             " --no-run"
         };
 
-        // Cargo doesn't like --no-run for doctests, so we have to plumb it in here
-        // https://github.com/rust-lang/rust/issues/87022
+        // RUSTDOCFLAGS is simply ignored if --doc wasn't passed, so we always set it.
         let rustdoc_flags = std::env::var("RUSTDOCFLAGS").unwrap_or_default() + no_run_flag;
         command.env("RUSTDOCFLAGS", rustdoc_flags);
     }
 
-    if let CargoCmd::Run(run) | CargoCmd::Test(Test { run_args: run, .. }) = &cmd {
-        if run.is_runner_configured() {
+    command.args(cargo_cmd.cargo_args());
+
+    if let CargoCmd::Run(run) | CargoCmd::Test(Test { run_args: run, .. }) = &cargo_cmd {
+        if run.use_custom_runner() {
             command
                 .arg("--")
                 .args(run.build_args.passthrough.exe_args());
@@ -134,10 +129,12 @@ pub fn make_cargo_command(input: &Input, message_format: &Option<String>) -> Com
     command
 }
 
-/// Get the environment's version of cargo
-fn find_cargo() -> Command {
+/// Build a `cargo` command with the given `--config` flags.
+fn cargo(config: &[String]) -> Command {
     let cargo = env::var("CARGO").unwrap_or_else(|_| "cargo".to_string());
-    Command::new(cargo)
+    let mut cmd = Command::new(cargo);
+    cmd.args(config.iter().map(|cfg| format!("--config={cfg}")));
+    cmd
 }
 
 fn print_command(command: &Command) {
@@ -181,7 +178,8 @@ pub fn check_rust_version() {
         eprintln!("cargo-3ds requires a nightly rustc version.");
         eprintln!(
             "Please run `rustup override set nightly` to use nightly in the \
-            current directory."
+            current directory, or use `cargo +nightly 3ds` to use it for a \
+            single invocation."
         );
         process::exit(1);
     }
