Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Upgrade to clap4, add extra args and help strings #26

Merged

Conversation

ian-h-chamberlain
Copy link
Member

@ian-h-chamberlain ian-h-chamberlain commented Oct 16, 2022

Addresses #13

Leaving this as draft because I want to get some opinions first, but basically with Clap 4 there are some changes which should make it work a lot nicer to pass trailing varargs to cargo. This builds off the work @SteveCookTU did to update the CLI and allows us to expose the 3dslink args mentioned in the issue linked above.

Example help text
$ cargo-3ds help
Usage: cargo <COMMAND>

Commands:
  3ds   Cargo wrapper for developing Rust-based Nintendo 3DS homebrew apps
  help  Print this message or the help of the given subcommand(s)

Options:
  -h, --help  Print help information
$ cargo 3ds help
Cargo wrapper for developing Rust-based Nintendo 3DS homebrew apps

Usage: cargo 3ds [CARGO_OPTIONS]... <COMMAND>

Commands:
  build
          Builds an executable suitable to run on a 3DS (3dsx)
  check
          Equivalent to `cargo check`
  clippy
          Equivalent to `cargo clippy`
  doc
          Equivalent to `cargo doc`
  run
          Builds an executable and sends it to a device with `3dslink`
  test
          Builds a test executable and sends it to a device with `3dslink`
  help
          Print this message or the help of the given subcommand(s)

Arguments:
  [CARGO_OPTIONS]...
          Pass additional options through to the `cargo` command.
          
          To pass flags that start with `-`, you must use `--` to separate `cargo 3ds` options from cargo options. All args after `--` will be passed through to cargo unmodified.
          
          If one of the arguments is itself `--`, the args following that will be considered as args to pass to the executable, rather than to `cargo` (only works for the `run` or `test` commands). For example, if you want to pass some args to the executable
          directly it might look like:
          
          > cargo 3ds run -- -- arg1 arg2

Options:
  -h, --help
          Print help information (use `-h` for a summary)

  -V, --version
          Print version information
$ cargo 3ds test --help
Builds a test executable and sends it to a device with `3dslink`.

This can be used with `--test` for integration tests, or `--lib` for unit tests (which require a custom test runner).

Usage: cargo 3ds test [OPTIONS] [CARGO_OPTIONS]...

Arguments:
  [CARGO_OPTIONS]...
          Pass additional options through to the `cargo` command.
          
          To pass flags that start with `-`, you must use `--` to separate `cargo 3ds` options from cargo options. All args after `--` will be passed through to cargo unmodified.
          
          If one of the arguments is itself `--`, the args following that will be considered as args to pass to the executable, rather than to `cargo` (only works for the `run` or `test` commands). For example, if you want to pass some args to the executable
          directly it might look like:
          
          > cargo 3ds run -- -- arg1 arg2

Options:
      --no-run
          If set, the built executable will not be sent to the device to run it

  -a, --address <ADDRESS>
          Specify the IP address of the device to send the executable to.
          
          Corresponds to 3dslink's `--address` arg, which defaults to automatically finding the device.

  -0, --argv0 <ARGV0>
          Set the 0th argument of the executable when running it. Corresponds to 3dslink's `--argv0` argument

  -s, --server
          Start the 3dslink server after sending the executable. Corresponds to 3dslink's `--server` argument

      --retries <RETRIES>
          Set the number of tries when connecting to the device to send the executable. Corresponds to 3dslink's `--retries` argument

  -h, --help
          Print help information (use `-h` for a summary)
Dump of debug output when varargs are passed
$ cargo 3ds run --  --release foo bar
[src/main.rs:13] &cmd = Root {
    cmd: Run(
        Run {
            address: None,
            argv0: None,
            server: false,
            retries: None,
        },
    ),
    cargo_options: [
        "--release",
        "foo",
        "bar",
    ],
}
[src/main.rs:14] cmd.cargo_options() = [
    "--release",
    "foo",
    "bar",
]
[src/main.rs:15] cmd.executable_args() = []
$ cargo 3ds test --address 192.168.0.167 -- --test baz 
[src/main.rs:14] &input = Input {
    cmd: Test(
        Test {
            no_run: false,
            run_args: Run {
                address: Some(
                    "192.168.0.167",
                ),
                argv0: None,
                server: false,
                retries: None,
            },
        },
    ),
    dry_run: false,
    cargo_options: [
        "--test",
        "baz",
    ],
}
[src/main.rs:15] input.cargo_opts() = [
    "--test",
    "baz",
]
[src/main.rs:16] input.exe_args() = []

Let me know what y'all think, and if it seems good I'll go ahead and wire up the command parser into the actual logic. After / with this change I was also thinking of adding some CLI tests with trycmd or something like that which could print the commands that the tool would run, although it will probably take a bit of mocking and stuff to get that working.

Copy link
Member

@AzureMarker AzureMarker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall looks like an improvement!

src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
@SteveCookTU
Copy link
Contributor

I haven't gotten to look at the general changes for Clap4 but out of curiosity does the global attribute cause the need for the '--' to pass to cargo? I believe in the previous version, having the vec as the last input accepted any number of args that would be forwarded to Cargo without the need of the separator

@ian-h-chamberlain
Copy link
Member Author

out of curiosity does the global attribute cause the need for the '--' to pass to cargo?

Not quite – global on args means they apply to all subcommands, so you can do e.g.

cargo 3ds --foo run --bar

and both --foo and --bar could be registered just once at the top level.

However, the combination of allow_hyphen_values and trailing_var_arg does allow the kind of input you're talking about. It has a weird side effect, though: once any unknown arg is passed, all subsequent args are included in the trailing vararg, even if they are registered as a known option:

$ cargo 3ds test --foo --no-run -- bar 
[src/main.rs:14] &input = Input {
    cmd: Test(
        Test {
            no_run: false,
            run_args: Run {
                address: None,
                argv0: None,
                server: false,
                retries: None,
            },
        },
    ),
    dry_run: false,
    cargo_options: [
        "--foo",
        "--no-run",
        "--",
        "bar",
    ],
}
[src/main.rs:15] input.cargo_opts() = [
    "--foo",
    "--no-run",
    "--",
]
[src/main.rs:16] input.exe_args() = [
    "bar",
]

I thought that behavior might be more confusing than having to pass the extra -- to clearly delineate. I suppose we could manually look through cargo_opts for matches again.

I just found that there is also last = true but that looks like it would have a similar problem.

@ian-h-chamberlain ian-h-chamberlain requested a review from a team October 30, 2022 22:41
@ian-h-chamberlain ian-h-chamberlain marked this pull request as ready for review October 30, 2022 22:41
@ian-h-chamberlain
Copy link
Member Author

ian-h-chamberlain commented Oct 30, 2022

Ok, I've pushed a set of changes that hooks everything up, and I tested a variety of flag combinations manually as well as added a couple unit tests on the custom parsing logic.

Here's an example of running it with the 3dslink args (one slightly awkward caveat is that 3dslink args must always come before cargo args, which I suppose might be worth documenting somewhere):

$ cargo 3ds run --server --address 192.168.0.167 --example output-3dslink

[snip]

Getting metadata
Building smdh:/Users/ianchamberlain/Documents/Development/3ds/ctru-rs/target/armv6k-nintendo-3ds/debug/examples/output-3dslink.smdh
Building 3dsx: /Users/ianchamberlain/Documents/Development/3ds/ctru-rs/target/armv6k-nintendo-3ds/debug/examples/output-3dslink.3dsx
Adding RomFS from /Users/ianchamberlain/Documents/Development/3ds/ctru-rs/ctru-rs/examples/romfs
Running 3dslink
Sending output-3dslink.3dsx, 1695260 bytes
497493 sent (29.35%), 39 blocks
starting server
server active ...
Hello 3dslink!
Press Start on the device to disconnect and exit.
exiting ... 

I also verified cargo 3ds clippy, cargo 3ds doc etc. work as expected (as mentioned in #27).

There may be some weird edge cases missing here but hopefully some additional testing and actually using the tool could catch those, so I think this is ready for review and we can always come back and bugfix stuff later.

src/command.rs Outdated Show resolved Hide resolved
src/command.rs Outdated Show resolved Hide resolved
src/command.rs Show resolved Hide resolved

impl Run {
/// Get the args to pass to `3dslink` based on these options.
pub fn get_3dslink_args(&self) -> Vec<String> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to add tests for this? Not needed for the merge at least.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure, I can add it whenever I try to add CI / other test assertions in a future PR

src/lib.rs Outdated Show resolved Hide resolved
@Meziu
Copy link
Member

Meziu commented Nov 10, 2022

This is exactly what I had in mind for this tool! Thank you so much for the work done.

@Meziu Meziu merged commit 83032c6 into rust3ds:master Nov 10, 2022
@ian-h-chamberlain ian-h-chamberlain deleted the feature/clap4-upgrade-extra-args branch March 17, 2023 15:05
@Techie-Pi Techie-Pi mentioned this pull request Apr 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants