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

Should be possible to make help show -- before TrailingVarArg enabled arg #888

Closed
nagisa opened this issue Mar 6, 2017 · 9 comments
Closed
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Milestone

Comments

@nagisa
Copy link

nagisa commented Mar 6, 2017

When TrailingVarArg option is enabled, I feel like it should be possible to make help show the -- before this trailing argument:

i.e.

USAGE:
    cargo-fuzz run <TARGET> -- <ARGS>...

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

ARGS:
    <TARGET>     
    <ARGS>...    

or

USAGE:
    cargo-fuzz run <TARGET> [--] [<ARGS>...]

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

ARGS:
    <TARGET>     
    <ARGS>...    

and not

USAGE:
    cargo-fuzz run <TARGET> [ARGS]

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

ARGS:
    <TARGET>     
    <ARGS>...    

it shows currently. Code here.

@kbknapp
Copy link
Member

kbknapp commented Mar 9, 2017

The TrailingVarArg effectively disables the --, and just makes the final positional argument act like -- was passed.

A hack to make the help generation add the -- is to add a single hidden option which accepts multiple values.

arg(Arg::with_name("_foo").long("_foo").takes_value(true).multiple(true).hidden(true))

@kbknapp
Copy link
Member

kbknapp commented Mar 9, 2017

Also, if the usage string is simple, and you don't have many other args (like the example you gave), you could also just add it manually:

app.usage("cargo-fuzz run <TARGET> [--] [ARGS]...")

The only thing you lose from this is the "context aware" usage strings that happen on error messages.

@kbknapp
Copy link
Member

kbknapp commented Mar 10, 2017

@nagisa I took a look at your link, and two things I noticed

  • The "dummy" arg can actually be dealt with like this:
let app = App::new("cargo-fuzz")
    // Here we lie to clap, saying our binary is actually "cargo" so that our help
    // messages and usage strings are all set correctly
    .bin_name("cargo")
    // Now we add our "fuzz" subcommand
    .subcommand(SubCommand::with_name("fuzz")
        .version(option_env!("CARGO_PKG_VERSION").unwrap_or("0.0.0"))
        .about(option_env!("CARGO_PKG_DESCRIPTION").unwrap_or(""))
        .setting(AppSettings::SubcommandRequiredElseHelp)
        .setting(AppSettings::GlobalVersion)
        .subcommand(SubCommand::with_name("init").about("Initialize the fuzz folder"))
        .subcommand(SubCommand::with_name("run").about("Run the fuzz target in fuzz/fuzzers")
            .setting(AppSettings::TrailingVarArg)
            .arg(Arg::with_name("TARGET").required(true)
                 .help("name of the fuzz target"))
            .arg(Arg::with_name("ARGS").multiple(true)
                 .help("additional libFuzzer arguments passed to the binary"))
        )
        .subcommand(SubCommand::with_name("add").about("Add a new fuzz target")
                    .arg(Arg::with_name("TARGET").required(true)))
        .subcommand(SubCommand::with_name("list").about("List all fuzz targets"))
    )
    .get_matches();
  • The TrailingVarArg isn't really required based off the args you've got. Since it looks like you're using it simply to allow accepting values that may/may not start with a hyphen, I'd suggest using Arg::allow_hyphen_values on the ARGS argument instead.

If you like the usage string with [--] then I'd suggest using the "hidden option" hack. This works because clap inserts the [--] when it detects that there could be an ambiguity between what to parse, and thus hints at using --. Having an option which accepts an unknown number of values and a positional argument that accepts multiple values is such an ambiguity.

@nagisa
Copy link
Author

nagisa commented Mar 10, 2017

Re dummy I want the binary to work as cargo fuzz ... as well as when invoked as cargo-fuzz (e.g. via cargo run). I do not think the proposed subcommand dance handles that.

I'll investigate allow_hyphen_values tonight.

@nagisa
Copy link
Author

nagisa commented Mar 10, 2017

So I tried allow_hyphen_values and it seems to not have the effect I want.

In examples, this is what I changed my run subcommand to:

        SubCommand::with_name("run").about("Run the fuzz target in fuzz/fuzzers")
            .arg(Arg::with_name("TARGET").required(true)
                 .help("name of the fuzz target"))
            .arg(Arg::with_name("ARGS").multiple(true).allow_hyphen_values(true)
                 .help("additional libFuzzer arguments passed to the binary"))

Alas, running the help still shows:

USAGE:
    cargo-fuzz run <TARGET> [ARGS]

I also still need the leading -- for something like this work (works just fine without allow_hyphen_values or the setting anyway):

./target/release/cargo-fuzz run target -- -whoops -oi=hey

If you like the usage string with [--] then I'd suggest using the "hidden option" hack.

What is this hack? I tried various ways to construct the DUMMY argument in the following case in attempt to get [--] show up:

        SubCommand::with_name("run").about("Run the fuzz target in fuzz/fuzzers")
            .arg(Arg::with_name("TARGET").required(true)
                 .help("name of the fuzz target"))
            .arg(Arg::with_name("DUMMY"))
            .arg(Arg::with_name("ARGS").multiple(true).allow_hyphen_values(true)
                 .help("additional libFuzzer arguments passed to the binary"))

However, not only [--] does not show up (I tried various combinations of multiple, required, hidden and long, short). In fact I couldn’t make DUMMY argument itself to appear in the USAGE:

USAGE:
    cargo-fuzz run [FLAGS] <TARGET> [ARGS]

Is it even possible to construct something like the following with clap?

cargo-fuzz [fuzz] run <TARGET> [<CORPUS>] [-- <ARGS>...]

(note the non-optional -- if any need to be parsed)

@kbknapp
Copy link
Member

kbknapp commented Mar 10, 2017

What is this hack? I tried various ways to construct the DUMMY argument in the following case in attempt to get [--] show up

Hmm, actually it looks like this doesn't work like I'd thought if you're adding CORPUS 😕

Is it even possible to construct something like the following with clap?

Aaaah OK now I see exactly what you're trying to do. I was misunderstanding, apologies! That'd be a very easy addition, though. Once I get home tonight I'll wrap it up into #893 and get it out with 2.21.0.

With this change the usage string will look like this:

cargo-fuzz [fuzz] run <TARGET> [CORPUS] [-- <ARGS>...]

Once I make these changes, the change to your code will be you can remove the TrailingVarArg (since it won't be necessary in this case), and mark ARGS as allow_hyphen_values(true).last(true) (last(true) is the part I'm adding tonight)

@kbknapp
Copy link
Member

kbknapp commented Mar 10, 2017

Alright, I have this basically implemented and ready for a PR. I just need to finish the final test case tonight.

@kbknapp kbknapp added A-parsing Area: Parser's logic and needs it changed somehow. C: positional args C-enhancement Category: Raise on the bar on expectations and removed T: RFC / question labels Mar 10, 2017
@kbknapp kbknapp added this to the 2.21.0 milestone Mar 10, 2017
kbknapp added a commit that referenced this issue Mar 11, 2017
…st' which means it should be used with `--` syntax and can be accessed early

Marking a positional argument `.last(true)` will allow accessing this argument earlier if the `--` syntax is used (i.e. skipping other positional args)
and also change the usage string to one of the following:

* `$ prog [arg1] [-- <last_arg>]`
* `$ prog [arg1] -- <last_arg>` (if the arg marked `.last(true)` is also marked `.required(true)`)

Closes #888
@kbknapp
Copy link
Member

kbknapp commented Mar 11, 2017

This is implemented in #893. Once that merges I'll put out v2.21.0 on crates.io.

It'll now be possible to implement:

cargo-fuzz [fuzz] run <TARGET> [CORPUS] [-- <ARGS>...]

@nagisa
Copy link
Author

nagisa commented Mar 11, 2017

Awesome!

@homu homu closed this as completed in f966829 Mar 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-parsing Area: Parser's logic and needs it changed somehow. C-enhancement Category: Raise on the bar on expectations
Projects
None yet
Development

No branches or pull requests

2 participants