Refactor args.rs to support --help

[davidlattimore]

We don't currently support --help, but it'd be nice to.

We could just write a separate help document and then try to make sure that we keep it in sync with the actual argument parsing, however I think it would be inevitable that the two would diverge.

See also #302 for discussion as to why we can't use something like clap.

EDIT: The suggestion here is superseded by the suggestion to use bpaf instead, so be sure to read the whole issue. Some of what's written here is still relevant though - e.g. don't copy help text. Also, still a good idea to do an initial PR that doesn't add help, then add help texts in subsequent PR(s).

My current thought on how we might approach this is to run the same function when printing help as we run when parsing, but just pass a different argument. Currently the argument passed to args::parse is an iterator over the arguments. We could replace this with something that implements our own trait, of which we'd have two implementations, one for parsing and one for printing help.

To provide a concrete example, let's take a snippet of our argument parsing code:

       } else if long_arg_eq("static") || long_arg_eq("Bstatic") {
            modifier_stack.last_mut().unwrap().allow_shared = false;
        } else if long_arg_eq("Bdynamic") {
            modifier_stack.last_mut().unwrap().allow_shared = true;
        } else if arg == "-o" {
            args.output = input
                .next()
                .map(|a| Arc::from(Path::new(a.as_ref())))
                .context("Missing argument to -o")?;
       }

We might rewrite this something like:

parser
   .declare()
   .long("static")
   .long("Bstatic")
   .help("Disallow linking of shared libraries")
   .execute(|| {
     modifier_stack.last_mut().unwrap().allow_shared = false;
  });

parser
   .declare()
   .long("Bdynamic")
   .help("Allow linking of shared libraries")
   .execute(|| {
      modifier_stack.last_mut().unwrap().allow_shared = true;
   });

parser
   .declare_with_param()
   .short("-o")
   .help("Set the output filename")
   .execute(|a| {
      args.output = Arc::from(Path::new(a.as_ref()));
    });

So the argument parser trait would likely have a couple of associated types that get returned by the declare and declare_with_param methods. That will mean that we'll likely have 3 traits in total with each having two implementations. It'd also be possible to do it all with one trait and no associated types, albeit with slightly more runtime checking.

Some arguments might need to return errors from their closure. For that, we could have a try_execute that takes a closure returning a Result and which itself returns a Result.

Lots of PRs change argument parsing, so to reduce conflicts a bit, I think it'd make sense to do this change as two PRs. The first wouldn't actually add support for help, but would just change the way the existing parser is written. The second would add help support and help messages. We could also potentially add the help messages a bit at a time once the initial help support is added.

I think it's important that we not copy help text from other linkers or man pages, since if we do that, we can't put it under the same license as the rest of the repository and things get messy. So we'd need to write each bit of help text ourselves.

I'll probably open this up as a good-first-issue in a day or two once others have had a chance to comment on the design. When I do, if you'd like to work on this issue, please comment on the issue first, otherwise we risk having two people working on it at the same time without knowing and I don't want to be in the position of having to reject someone's PR because someone else did it. It's perfectly fine to say you're going to work on it, then after a day or to change your mind - just let us know.

[inflation]

bpaf is also an option, and it's pretty flexible. You could write arbitrary parsers since it is a parser combinator under the hood.

[davidlattimore]

bpaf is also an option, and it's pretty flexible. You could write arbitrary parsers since it is a parser combinator under the hood.

Interesting design, and it does look like it's got some flexibility, but I can't see APIs that would allow us to implement --push-stack and --pop-stack, then attach the state of the top of the stack to positional arguments. I'm also not sure if it has a way to make long arguments that support either a single or double dash, which we need to do for all long arguments other than those that start with lowercase 'o'. Our command line parsing requirements are pretty rigid, since we need to conform to what GNU ld does.

[pacak]

I can't see APIs that would allow us to implement --push-stack and --pop-stack, then attach the state of the top of the stack to positional arguments.

Do you have an example?

I'm also not sure if it has a way to make long arguments that support either a single or double dash

You can parse pretty much anything with help of any or literal as long as it doesn't require monadic dependencies between parsed values - that is a value passed to an argument changes what gets parsed next.

Our command line parsing requirements are pretty rigid, since we need to conform to what GNU ld does.

I'm happy to help :)

Hmm... Nice, you have some tests for cli parsing.

[davidlattimore]

I can't see APIs that would allow us to implement --push-stack and --pop-stack, then attach the state of the top of the stack to positional arguments.

Do you have an example?

Some flags such as --as-needed, --static and --whole-archive affect the state of the top of the stack. --push-state copies the top of the stack to a new stack entry and --pop-stack discards the current top stack entry. As each input file is encountered, whatever state is on the top of the stack is associated with that input file.

For example:

ld lib1.a --push-state --whole-archive lib2.a lib3.a --pop-state lib4.so

In that command, the input files will be:

"lib1.a" with whole-archive=false
"lib2.a" with whole-archive=true
"lib3.a" with whole-archive=true
"lib4.a" with whole-archive=false

[pacak]

So... We want to see some events for those flags and files, but I'm assuming there are some flags that don't obey it. Let's parse the events first. I'm going with minimal annotations, we can add them later.

#[derive(Debug, Clone, Bpaf)]
enum Event {
    /// Help for push state
    PushState,
    PopState,
    WholeArchive,
    NoWholeArchive, // I imagine there's a way to disable whole archive state?
    File(PathBuf)
}

Resulting program probably want to see files with annotation and there should be an easy way to convert a vector of events into a vector of files:

struct AnnotatedFile {
    file: PathBuf,
    whole_archive: bool,
}

fn from_events(events: Vec<Event>) -> Vec<AnnotattedFile> {
    todo!("Boring");
}

And with those bits we are ready to make a parser for files and all the annotations:

fn annotated_files() -> impl Parser<Vec<AnnotatedFile>> {
    event()
        .many()
        .map(from_events)
}

And this parser can go into resulting parser with external

#[derive(Debug, Clone, Bpaf)]
#[bpaf(options)]
struct Options {
    // more options go here
    ..
    // since annotated files involve positional items - they must go at the end of
    // `Options` struct, this is a restriction of current version, gone in the next
    // but I'm still working on it.
    #[bpaf(external(annotated_files))]
    inputs: Vec<AnnotatedFile>
}

[davidlattimore]

Ah, neat, that does look like it should work. Thanks!

I'm also not sure if it has a way to make long arguments that support either a single or double dash

You can parse pretty much anything with help of any or literal as long as it doesn't require monadic dependencies between parsed values - that is a value passed to an argument changes what gets parsed next.

What might that look like? Suppose we wanted to be able to parse an option sysroot, that could accept any of the following four forms:

--sysroot /foo
--sysroot=/foo
-sysroot /foo
-sysroot=foo

[pacak]

What might that look like? Suppose we wanted to be able to parse an option sysroot, that could accept any of the following four forms:

Sorry for the delay, got a bit distracted. So we want to parse 4 different forms, can combine several parsers for that.

I made a helper function that takes a name - "sysroot" in your case, a help message and gives a parser. This parser can be used to make a concrete parser (I imagine there's more than just sysroot to be parsed) which can be used in the final product with external if you use derive method or simply in a construct! macro for combinatoric approach.

The idea is to use 3 separate parsers and combine their output;

use std::path::PathBuf;

use bpaf::*;

fn prefixed(name: &'static str, help: &'static str) -> impl Parser<PathBuf> {
    // this parser deals with `--sysroot xxx` and `--sysroot=xxx` variants
    // it will be also used to display help messages as the rest are hidden
    let double = long(name).help(help).argument::<PathBuf>("FILE");

    // this parser deals with `-sysroot=xxx` variant. It looks a bit
    // clunky since it needs to parse any static `name`, for just sysroot
    // it can look like `any(|s| s.strip_prefix("-sysroot"))`
    let single_eq = any::<String, _, _>("X", move |s| {
        let s = s.strip_prefix("-")?;
        let s = s.strip_prefix(name)?;
        let s = s.strip_prefix("=")?;
        Some(PathBuf::from(s.to_string()))
    })
    .hide();

    // this parser deals with `-sysroot xxx` variant, it's secretly two separate
    // parsers, one knows how to parse `-sysroot`, one knows how to parse positional
    // items and there's a trenchcoat telling that they must be adjacent to each other
    // I have to use `any` for the first parser to deal with a custom name, for 
    // statically known names it would look like `literal("-sysroot")`
    let prefix =
        any::<String, _, _>("X", |s| (s.strip_prefix("-") == Some(name)).then_some(())).anywhere();
    let s = positional::<PathBuf>("");
    let single = construct!(prefix, s).adjacent().map(|x| x.1).hide();

    // Once we have all 3 parsers - just combine them together
    construct!([double, single_eq, single])
}

fn main() {
    let a = prefixed("sysroot", "this is sysroot stuff");
    let b = prefixed("something", "this is something else");
    let parser = construct!(a, b).to_options().fallback_to_usage();
    println!("{:?}", parser.run());
}

This should parse any combination of sysroot and something flags, will deal with non-utf8 file names with double dash or with a single dash separated by a space, but will panic if non-utf8 name is passed to -sysroot=NON-UTF8.

Should be reasonably fast.

[davidlattimore]

Thanks! That makes sense. I'm going to open this up as a good-first-issue now. It's suitable for someone without any deep linker knowledge. Might be a bit tricky for someone who's very new to Rust.

Given that it seems that bpaf can be used for this, I think it'd be worthwhile giving it a go.

[yerke]

I will try to implement this if you are not in a hurry.

[davidlattimore]

@yerke great! Don't hesitate to reach out if you have any questions. You can either comment here, or book time in my calendar (see link on my github profile).

[yerke]

@davidlattimore I left some questions for you in #527. Do you mind taking a look when you have time? Thanks.