Use clap in miri-script

[Mandragorian]

Use clap in miri-script, following discussion in #3592.

Simplify the code by using the derive API from clap. This way we can just declare how the cli parser should behave, instead of implementing it ourselves.

This come with a slowdown to the compilation time of miri-script, but it is acceptable compared to the benefits.

[RalfJung]

Thanks for trying this! I agree it is quite elegant.

However, it also doubles our build times for the script. The script hopefully doesn't get built too often, so this is mostly a thing for first-time users -- they get no feedback while miri-script itself is built. Ideally we'd build it on stable Rust so it wouldn't be rebuilt a lot, but we need some nightly cargo features for the diagnostic integration...

Overall I am leaning slightly in favor of this, just to make maintenance of the script easier. I'd still be curious how this would look like without the derive; is there a good example somewhere of what using the clap builder directly looks like?

[RalfJung]

Also @rust-lang/miri what do you all think?

[saethlin]

a much longer compilation time from 2.5s to 5s after the changes.

I see a regression from 2.5s to 3s, which would be fine with me. I like clap, and think it's worth a bit of compile time overhead, but doubling the compile time of miri-script seems a bit steep. I'd quite like to know what the compile time is without using the proc macro (maybe you can just cargo expand miri-script and clean up what comes out?).

[RalfJung]

I am measuring a slowdown from 4s to 5.2s.

[Mandragorian]

what command do you use to build? I can write a small script to measure the build times a bit more systematically. maybe I got an outlier.

[saethlin]

rm -r miri-script/target/ && time ./miri --help

[Mandragorian]

Using this script:

#!/usr/bin/env python
import argparse
import shutil
import subprocess
import sys
import time

parser = argparse.ArgumentParser(
                    prog='ProgramName',
                    description='What the program does',
                    epilog='Text at the bottom of help')
parser.add_argument('-c', '--count', type=int, default=10)


if __name__ == "__main__":
    args = parser.parse_args()
    total_time = 0
    for _ in range(args.count):
        shutil.rmtree('miri-script/target', ignore_errors=True)
        start_time = time.time()
        subprocess.run(['./miri', '--help'], capture_output=True)
        end_time = time.time()
        total_time += end_time - start_time
        print('.', end='')
        sys.stdout.flush()
    print('total time = {}, average execution time = {}'.format(total_time, total_time / args.count))

(don't judge my python skills too much)

I get that without the changes i have

total time = 26.590941667556763, average execution time = 2.6590941667556764

And with the changes

total time = 37.09629225730896, average execution time = 3.709629225730896

So less than 5s but also more than 3s that @saethlin observed (comparing with their results since we seem to have similar baselines).

[RalfJung]

Absolute times are obviously going to differ. What's the relative slowdown? Seems to be 40% for you, 30% for me, 20% for saethlin.

[bors]

☔ The latest upstream changes (presumably #4041) made this pull request unmergeable. Please resolve the merge conflicts.

[Mandragorian]

Yes, these are the relative slowdowns. I compared with saethlin's numbers since we had similar baselines.

What would be the threshold for an acceptable slowdown?

[RalfJung]

IMO that is the wrong question. The right question is, how low can we get the slowdown while still getting most of the benefits of using clap (or some other similar library)?

[Mandragorian]

Ok, then I will try to make a poc using the builder api when I have a bit more time on my hands.

In the meantime I built miri-script with the --timings flag, and I see that building clap_builder is the most heavy addition to the build time at 1.99s. syn is second with 1.5s, but thankfully these two build mostly in parallel. Then clap_derive is at 0.6s.

miri-script itself goes from 0.4s without the changes to 0.5s with the changes.

If I interpret this correctly I don't expect the builder API to save us much time after the first compilation. But then, as long as clap's version doesn't change it won't need to be rebuilt (?).

As for how the builder API looks like, there is an example in the tutorial.

Most notably, we lose some compile time checks (but of course this might be a feature in our case). For example instead of args.argument1, would need to do something like args.get_one::<String>("argument1"), and then check if it returned Some or None.

[RalfJung]

But then, as long as clap's version doesn't change it won't need to be rebuilt (?).

It gets rebuild whenever the compiler changes, which is nightly, so it can change quite frequently.

[RalfJung]

Now that we decided to go with the derive version, the next step would be to clean up and finish this PR. :)

@rustbot author

[Mandragorian]

I see that this now has conflicts. Not sure if you prefer to rebase it now or go through the review first beforehand. I also see that the checks do not run, and I am not sure why.

@rustbot ready

[RalfJung]

Please rebase first. Github doesn't run the checks if there are conflicts (since the checks are run against your PR merged with master).

[bors]

☔ The latest upstream changes (presumably #4089) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

Could you please post what the --help output looks like? We might want to tweak the doc comments now that they are more user-visible.

[RalfJung]

Please use the bail! macro fro anyhow.

[RalfJung]

Also please update the PR title and commit message to make it clear that this is not an experiment any more.

@rustbot author

[Mandragorian]

This is what the help message looks like:

[root@1dfa6c4d75e7 miri]# ./miri help
Usage: miri-script <COMMAND>

Commands:
  install     Installs the miri driver and cargo-miri. Sets up the rpath such that the installed binary should work in any working directory. Note that the binaries are placed in the `miri` toolchain sysroot, to prevent conflicts with other toolchains
  build       Just build miri
  check       Just check miri
  test        Build miri, set up a sysroot and then run the test suite
  run         Build miri, set up a sysroot and then run the driver with the given <flags>. (Also respects MIRIFLAGS environment variable.)
  doc         Build documentation
  fmt         Format all sources and tests
  clippy      Runs clippy on all sources
  bench       Runs the benchmarks from bench-cargo-miri in hyperfine. hyperfine needs to be installed
  toolchain   Update and activate the rustup toolchain 'miri' to the commit given in the `rust-version` file. `rustup-toolchain-install-master` must be installed for this to work. Any extra flags are passed to `rustup-toolchain-install-master`
  rustc-pull  Pull and merge Miri changes from the rustc repo. Defaults to fetching the latest rustc commit. The fetched commit is stored in the `rust-version` file, so the next `./miri toolchain` will install the rustc that just got pulled
  rustc-push  Push Miri changes back to the rustc repo. This will pull a copy of the rustc history into the Miri repo, unless you set the RUSTC_GIT env var to an existing clone of the rustc repo
  help        Print this message or the help of the given subcommand(s)

Options:
  -h, --help  Print help

[RalfJung]

I have resolved the conflicts and done a pass over all the help texts. I noticed some things that were missing (rustc-push lost its default branch name, and the environment variables part of the help was removed without replacement).

Should be good to go now :)

[RalfJung]

FWIW this did change the behavior for some flags. Something like ./miri test foo bar --bless baz used to work, but it doesn't work any more -- --bless has to come first now. @Mandragorian do you think there's any way to keep that working?