Skip to content

Overhaul the collector CLI #687

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.

Sign up for GitHub

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

Jump to bottom
Merged
Mark-Simulacrum merged 6 commits into from
Jul 16, 2020
Merged

Overhaul the collector CLI #687

Mark-Simulacrum merged 6 commits into from
Jul 16, 2020

Conversation

@nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Jul 16, 2020

This fixes #683.

Here is a summary of the new subcommand names, arguments, and options.

bench_local
  <RUSTC>               // mandatory
  <ID>                  // mandatory
  --builds <BUILDS>     // default: Check,Debug,Opt
  --cargo <CARGO>       // default: `rustup which cargo --toolchain nightly`
  --db <DB>             // default: results.db
  --exclude <EXCLUDE>   // default: None (i.e. run all benchmarks)
  --include <INCLUDE>   // default: None (i.e. run all benchmarks)
  --runs <RUNS>         // default: All (i.e. Full, IncrFull, IncrUnchanged, IncrPatched)
  --rustdoc <RUSTDOC>   // default: rustdoc next to <RUSTC>
  --self-profile        // default: false

bench_next
  <SITE_URL>            // mandatory
  --db <DB>             // default: results.db
  --self-profile        // default: false

bench_published
  <TOOLCHAIN>           // mandatory
  --db <DB>             // default: results.db

bench_test
  --db <DB>             // default: results.db
  --exclude <EXCLUDE>   // default: None (i.e. run all benchmarks)
  --include <INCLUDE>   // default: None (i.e. run all benchmarks)

profile_local
  <PROFILER>            // mandatory
  <RUSTC>               // mandatory
  <ID>                  // mandatory
  --builds <BUILDS>     // default: Check,Debug,Opt
  --cargo <CARGO>       // default: cargo in PATH
  --exclude <EXCLUDE>   // default: None (i.e. run all benchmarks)
  --include <INCLUDE>   // default: None (i.e. run all benchmarks)
  --out-dir <OUT_DIR>   // default: results/
  --runs <RUNS>         // default: All (i.e. Full, IncrFull, IncrUnchanged, IncrPatched)
  --rustdoc <RUSTDOC>   // default: rustdoc next to <RUSTC>

@nnethercote
Copy link
Contributor Author

nnethercote commented Jul 16, 2020 •
edited
Loading

@Mark-Simulacrum: in your run.sh script, this line:

target/release/collector --db $DATABASE bench_published $x;

will need to change to this:

 target/release/collector bench_published $x --db $DATABASE;

And this line:

target/release/collector --self-profile --db $DATABASE process;

will need to change to this:

target/release/collector bench_next $SITE_URL --self-profile --db $DATABASE;

Mark-Simulacrum
Mark-Simulacrum reviewed Jul 16, 2020
View reviewed changes
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum left a comment

Choose a reason for hiding this comment

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

Looks great! Left some comments, but looks good to merge with them resolved.

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Jul 16, 2020
Closed
@nnethercote nnethercote force-pushed the overhaul-the-collector-CLI branch from 0584d20 to cd66984 Compare July 16, 2020 22:13
@nnethercote
Change --filter to --include.
54f6115 
So that it matches `--exclude`.
@nnethercote
Be more consistent with option handling.
584050b 
- Always use all-caps for the `@arg` key.
- Make sure the `@arg` key matches the option name.
- Only use the `--("foo")` form when necessary.
@nnethercote
Overhaul the collector CLI.
66e434e 
This commit makes the following changes to the invocation and behaviour
of subcommands.

`bench_local`
- rustc is given via a positional argument.
- Cargo is now found via `rustup which cargo` by default.
- rustdoc is now looked for next to rustc by default.
- The default database is now `results.db`.
- `--db`, `--include`, `--exclude`, `--self-profile` are now given after
  the subcommand name.

`bench_published`
- The default database is now `results.db`.
- `--db` is now given after the subcommand name.

`bench_test`
- The default database is now `results.db`.
- `--db`, `--include`, `--exclude` are now given after the subcommand
  name.

`profile`
- Renamed as `profile_local`.
- `--output` option renamed as `--out-dir`.
- Cargo and rustdoc finding behaviour is the same as for
  `bench_local`.
- The default output dir is now `results/`.
- `--include`, `--exclude` are now given after the subcommand name.
- Checks for rustdoc being present when a `Doc` build is requested.
  (Previously this check was only done for a `bench_local` run.)

`process`
- Renamed as `bench_next`.
- The default database is now `results.db`.
- `--db`, `--self-profile` are now given after the subcommand name.
- Now has a `SITE_URL` positional argument instead of accessing the
  `$SITE_URL` env var.

The commit also improves some error messages, e.g.:
- by using `.with_context()` in a few places;
- by making `get_pkgid()` return a `Result` instead of panicking;
- by printing "collector error:" as a prefix on error messages.
@nnethercote
Remove FromNameError and KindError.
62a4e1b 
These can be handled by generic `anyhow` errors.
@nnethercote
Add a helpful message to site start-up.
2339e66
@nnethercote
Fix indenting of the big clap macro.
1fd100d 
It's currently 7 spaces, but should be 8.
@nnethercote nnethercote force-pushed the overhaul-the-collector-CLI branch from cd66984 to 1fd100d Compare July 16, 2020 22:21
@Mark-Simulacrum Mark-Simulacrum merged commit 807246b into rust-lang:master Jul 16, 2020
@nnethercote nnethercote deleted the overhaul-the-collector-CLI branch July 17, 2020 01:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Mark-Simulacrum Mark-Simulacrum Mark-Simulacrum left review comments

+1 more reviewer

@jyn514 jyn514 jyn514 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Command options are a mess

3 participants

@nnethercote @Mark-Simulacrum @jyn514