Update clap to v3 1 #1140
Update clap to v3 1 #1140
Conversation
|
Less tired now so I pushed some cleanup. From an outside user's perspective things should seem nicer because I tried pushing as much validation onto clap as possible so we get the nice error messages from it. From a contributor's perspective the changes use the derive API and I think the code looks more approachable. Since I forgot to mention it before this requires #1137 for the MSRV bump and is part of getting #1110 finished off I still haven't got the Here's an overview comparing things (just for reference the new cli stuff has color while the old stuff doesnt)
|
|
Thinking on it more there are still issues present with how |
|
As promised here is the fix. The issue was that the following wouldn't be allowed on $ SCCACHE_START_SERVER=1 sccache --start-serverAlong with that error messages from The fix clears up error messages when an arg conflict occurs and $ SCCACHE_START_SERVER=1 ./sccache --start-server
sccache: `SCCACHE_START_SERVER=1` can't be used with other commands
$ SCCACHE_START_SERVER=1 ./sccache some command
sccache: `SCCACHE_START_SERVER=1` can't be used with other commandsAlong with that the error with |
Codecov Report
@@ Coverage Diff @@
## main #1140 +/- ##
==========================================
- Coverage 35.11% 34.91% -0.21%
==========================================
Files 47 47
Lines 12774 12794 +20
Branches 6637 6640 +3
==========================================
- Hits 4486 4467 -19
- Misses 3986 3990 +4
- Partials 4302 4337 +35
Continue to review full report at Codecov.
|
|
Thanks for the patch! Looks good so far, but I'll do a more complete review tomorrow most likely. The only concern I have off the top of my head is that the new, non-compact help menu is a bit of a bummer, but you've already noticed and addressed this:
|
|
I'll poke around at some more settings to see if I can get things to be more compact again |
|
So enabling the |
There was a problem hiding this comment.
So enabling the wrap_help feature and switching the value name in package_toolchain from EXECUTABLE to EXE condenses down the help message when the terminal is a reasonable width and falls back to long help on narrower terminals.
This looks great! The touch of colour is pretty nice too 👍
Another way of handling this could be to separate out --package-toolchain and --stats-format to a different heading like how it is by default in clap v2.
I think that separating "options" from "flags" was a very pedantic (and potentially confusing) distinction, so I really like how you've set it up.
Too many commands - sccache --start-server --stop-server
I really like how you've made this more obvious: rather than a confusing "Too many commands specified" error (which is ambiguous with the "you have provided an unexpectedly large number of command-like arguments"), it tells you specifically which two arguments are unacceptably provided together. Nice 👍
I think that the improvements you've made in updating clap and strengthening the restrictions we define (such as start-server and stop-server mutual exclusivity) are strong enhancements. However, the third, significant change that you've made in this patch - the migration to clap's "derive" API - isn't something that I'm confident is an improvement for sccache.
This project is midsized, and is starting to sprout weird behaviours that are harder to represent with the "derive" interface: you're seeing this yourself with how much legwork you had to do to make SCCACHE_START_SERVER play nicely with the other options. The flexibility offered by the "builder" API is something that sccache is already leveraging, and may need to lean on even more in the future.
The other part that makes "derive" tricky is that it is very magical. This can make debugging and tweaking clap's behaviour a bit of a headache. For example, it took me some nontrivial digging to figure out what the attributes on parse::Opts was. Posted here for convenience:
#[derive(Parser)]
#[clap(version)]
#[clap(propagate_version = true)]
struct Opts { ... }
Two parts here were confusing:
- What does
clap(version)do? Looking upversionondocs.rsdidn't point me in the right direction. I had to find the "Derive Reference", at which point the documented behaviour of "magic attributes" became clear. I think that this journey would be much less treacherous with the more traditional API. - Where does
clap(propagate_version = true)affect? Fortunately,propagate_versionhad an entry in the documentation, but I still second-guessed myself for a minute because the function definition I found was associated with a deprecated struct.
I can see the appeal of leveraging the "attribute" language feature to try to tighten the coupling between parsing and the generated struct containing the refined arguments. However, I think that its costs aren't worth the benefits today.
Would you be interested in simplifying this patch to be specifically about updating clap and improving our handling of specific incompatible arguments, such as --start-server, --stop-server, and the others?
|
Thanks for all the feedback! I can push out changes to address your comments (including switching back to the builder API) either later today or tomorrow |
The last push is all I have time for atm. It implements the above where we are back to the bulider api (barring some Things should still follow the same strictness as was enforced by the derive implementation. The A notable difference from the I did take the time to start implementing some unit tests for the arg parsing. I only had time to go through a decent bit of |
|
That sounds great, thanks! I'll take a look at this tomorrow most likely.
I appreciate that, thanks! There isn't a rush to get this landed, especially considering that it's blocked on our next release (which should be going out in the next week or so). |
There was a problem hiding this comment.
but I opted for just building off of Args since I was running into some annoyances with arg!() that made it seem like it wasn't really pulling its weight
Would you mind elaborating here in this PR with a comment for what specific annoyances you were running into?
Three other things:
- I'll still need to do one more pass to look at the non-dist handling, but I'll handle it after you've addressed this first round here
- For future patches, I'd highly recommend trying to keep your diff smaller. Right now, you've significantly changed the
clapconfiguration logic, but you've also moved a bunch of code. It would be much easier to review if that was split into either two, self-contained commits, or two separate PRs. - Thanks for the patch, this is looking great so far 😁
Sure! Just for reference here is the current docs section for
// Doesn't work
clap::arg!(--some-flag);
// Works
clap::arg!(--"some-flag");
All of this just ended up leading to a very bumpy start, so it seemed easiest to just avoid the macro-magic and stick with the straight-forward (and well documented) builder pattern
👍 All the changes seem more than reasonable! I'll work through them when I get back to OSS again |
|
I believe the recent commits address all your current requested changes, so this should be good for another review whenever you are able! |
There was a problem hiding this comment.
Looks good to me, nice work! Thanks again for this patch and your collaboration, this is great!
I'm going to wait to land this until v0.3.0, which is unfortunately taking longer than I had hoped. I'm looking forward to merging this :)
| #[derive(Clone, Copy)] | ||
| enum LogLevel { | ||
| Error, | ||
| Warn, | ||
| Info, | ||
| Debug, | ||
| Trace, | ||
| } | ||
|
|
||
| impl LogLevel { | ||
| fn as_str(&self) -> &'static str { | ||
| match self { | ||
| Self::Error => "error", | ||
| Self::Warn => "warn", | ||
| Self::Info => "info", | ||
| Self::Debug => "debug", | ||
| Self::Trace => "trace", | ||
| } | ||
| } | ||
|
|
||
| fn values() -> &'static [Self] { | ||
| &[ | ||
| Self::Error, | ||
| Self::Warn, | ||
| Self::Info, | ||
| Self::Debug, | ||
| Self::Trace, | ||
| ] | ||
| } | ||
| } | ||
|
|
||
| impl FromStr for LogLevel { | ||
| type Err = anyhow::Error; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let variant = match s { | ||
| "error" => Self::Error, | ||
| "warn" => Self::Warn, | ||
| "info" => Self::Info, | ||
| "debug" => Self::Debug, | ||
| "trace" => Self::Trace, | ||
| _ => bail!("Unknown log level: {:?}", s), | ||
| }; | ||
|
|
||
| Ok(variant) | ||
| } | ||
| } | ||
|
|
||
| impl From<LogLevel> for log::LevelFilter { | ||
| fn from(log_level: LogLevel) -> Self { | ||
| match log_level { | ||
| LogLevel::Error => Self::Error, | ||
| LogLevel::Warn => Self::Warn, | ||
| LogLevel::Info => Self::Info, | ||
| LogLevel::Debug => Self::Debug, | ||
| LogLevel::Trace => Self::Trace, | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
There is the crate https://crates.io/crates/clap-verbosity-flag that already does this :)
There was a problem hiding this comment.
TIL, I'll switch over to that!
There was a problem hiding this comment.
Took a look at clap-verbosity-flag and noticed that it is for the more traditional -q and -v* style of verbosity flags while sccache-dist currently takes --syslog <LEVEL> instead. This means that switching to clap-verbosity-flag would be a breaking change and as v0.3 was released under a month ago, I'm guessing that another breaking change isn't intended quite this soon (and could merit looking into potentially revamping sccache's CLI structure as well)
There was a problem hiding this comment.
You're probably right one that, I am tempted to defer that change to another PR, but still strongly in favor of making it. I am not aware of other programs using that naming.
|
@drahnr this is slightly tangential but is there a reason why It seems very confusing to see Here is POSIX documentation talking about flags/options:
docopt makes the same distinction |
|
@drahnr Thanks for the review! I'm stretched a bit thin this week, but should be able to address everything over this weekend |
I'll look into this more closely, thanks for noting! |
|
I'll give this another go early next week :) The only thing with not using the macros, is that now the logic is split into declaration and evaluation, which makes it harder to maintain imho. I am aware that this opposes what @mitchhentges suggested. Let me dig into this one last time, and please don't force push so we can use the "old" macro based approach just in case. |
|
@drahnr are you still working on it? thanks |
|
I am a bit short on time recently. I'm wary of incompatibilities, other than that, it looks good. Sorry for the delay. I'll fix it up and get it done in the next few days. |
|
I reviewed this again and #1354 is hence ready to merge @sylvestre |
This is my initial attempt at updating
clapto v3.1 (and switching over to theDeriveAPI for it)I kept the same external
Commands for the parsed format and just used intermediate values internally. To keep the separation cleaner this involved changingparse()to be infallible where the error displaying and exiting early is now done internally withinparse()sccacheappears to be fine and tests pass withcargo testfor it.sccache-distis compiling and from a surface level check things seem to be fine, but the tests for it don't pass for me onmainor this branch. This PR is marked as a draft until that gets sorted out. Below is an example of the error I get:Also just for comparison here is a comparison of the new and old help messages
sccacheoldsccachenewsccache-distoldsccache-distnewSo
sccache's is a bit more spaced out and has different headings. I can set custom headings for arguments to be under if we want that to match. I tried setting the output to be more compact too, but I think the line--package-toolchainmay be overriding that. Other than that things are pretty much all the same aside from color with the new output and more parsing being enforced byclapinstead of bubbled up bysccache