search

sifive / wake

The SiFive wake build tool
Other
86 stars 28 forks source link

Overhaul Arg Parsing for Wake #1163

Open V-FEXrt opened 1 year ago

V-FEXrt commented 1 year ago

Wake's argument parsing has grown to be a complex and hairy beast. It would be best to overhaul and optimize the system. This could be done via several small steps outlined below.

1) Fix and minimize positional arguments.

Our current arg parser (gopt) doesn't play very nicely with positional arguments. This makes any refactors/changes/attempts as testing very difficult and subject to subtle bugs. With this in mind we should fix wrongly used positional arguments and minimize correctly used ones.

On first pass, the following flags rely on positional arguments timeline, shebang, and targets. I haven't deeply looked into shebang or targets but I believe they are correctly using positional arguments. timeline is insidious as it should use an option argument (--timeline, --timeline job-reflections, or --timeline file-accesses) but instead uses positional arguments. This seems to be caused by GOPT not handling the GOPT_ARGUMENT_OPTIONAL flag properly. When an optional argument option is set the value is put in the positional flags slot instead of being assigned as the argument to the flag

2) Extract arg parsing into a dedicated class

Arg parsing has gotten complex and deeply coupled with wake's main.cpp. This makes it challenging to reason about and impossible to test leading to very strange interactions when setting an uncommon combination of options.

3) Write tests for the arg parser

I don't think I need to argue too hard for more tests, but tl;dr its impossible to make sure functionality isn't subtly changed when editing the arg related code for wake. Writing tests allows us to safeguard ourselves from unintentional API breakages

4) Re-evaluate the UI/UX of our current CLI args.

This is the first proposed breaking change. There are several sharp or confusing edges with the current CLI interface. The most obvious is the fact that --verbose has two meanings but there are several improvements that can be made if we take a step back and reason about our interfaces. Top of mind is splitting the tool into subcommands

wake lsp --std-lib path-to-stdlib
wake db --last --failed --verbose
wake run -x 'some code' --verbose # <- different verbose
wake run some-file.wake

Above is just an example, and likely not exactly what we'd land on. Overall the main goal is to make wake invocations more intuitive.

ag-eitilt commented 1 year ago

wake run some-file.wake

This should probably be wake run some-target (including def) rather than wanting a file. Otherwise, I support all four proposals, at least pending discussion of the actual details.