cli: exit gracefully when no subcommand

[leighmcculloch]

What

Exit from rustup CLI gracefully when no subcommand is provided.

Why

That appears to be the intention because the SubcommandRequiredElseHelp app setting is configured on the CLI, however that is not the behavior because the main function treats the error indicating no subcommand was provided as an error.

rustup/src/cli/rustup_mode.rs

Lines 152 to 159 in 22dc71a

	pub fn cli() -> App<'static, 'static> {
	let mut app = App::new("rustup")
	.version(common::version())
	.about("The Rust toolchain installer")
	.after_help(RUSTUP_HELP)
	.setting(AppSettings::VersionlessSubcommands)
	.setting(AppSettings::DeriveDisplayOrder)
	.setting(AppSettings::SubcommandRequiredElseHelp)

Instead of exiting gracefully the word error: is printed with no error message:

Fixes #2425

Known Limitations

I have not added tests for this change. I'm rather new to rust and this codebase and I could not find tests for this code to modify or change.

[kinnison]

As noted here: #2425 (comment) I think that we might want to investigate why clap isn't returning clap::ErrorKind::HelpDisplayed

[leighmcculloch]

I think clap is returning the correct error, it explicitly returns this error when this feature is enabled:

https://github.com/clap-rs/clap/blob/e8d46f403671ebdd14f4f530f82f6ba6fe9c78cc/src/parse/parser.rs#L750-L756

[leighmcculloch]

clap does the same thing for the similar feature, ArgRequiredElseHelp:

https://github.com/clap-rs/clap/blob/e8d46f403671ebdd14f4f530f82f6ba6fe9c78cc/src/parse/validator.rs#L60-L68

So there's consistency in how it responds with these features.

[kinnison]

Yes, but it would, by default, report these on stderr and exit with failure: https://github.com/clap-rs/clap/blob/v2-master/src/errors.rs#L385

[leighmcculloch]

Interesting, that might be a separate issue though. I don't know.

It's worth noting that ErrKind::MissingArgumentOrSubcommand is not the what is returned if the subcommand is missing and AppSettings::SubcommandRequiredElseHelp is not used. In that case ErrorKind::MissingSubcommand is returned.

It does seem quite confusing.

[kinnison]

You've found a difference in behaviour between 1.21 and 1.22 (the presence of error:) and possibly a bug in how clap-v2 handles this situation. I'm still not convinced this is the right "fix" but I'm not going to reject it yet. Would you care to open a bug on clap discussing this?

[leighmcculloch]

@kinnison See clap-rs/clap#2021.

[kinnison]

Fantastic, let's wait and see what the clap people say about the behaviour.

[leighmcculloch]

@kinnison According to clap-rs/clap#2021 (comment) this would be a breaking change if changed and so if it is changed that will be limited to v3. rustup is using v2.

[leighmcculloch]

Looks like I have some tests to fix, I'll get on that.

[kinnison]

Cool, making sure that over all our behaviour is clean is the goal. You may also want to rebase as I merged another test improvement from Robert earlier today.

[kinnison]

I prefer rebases over merges, it makes for a clearer commit sequence to review. However I can wait for now and we can clean this up later. Are you sure that the changes in behaviour that your test changes imply are desired? Are older rustup behaviours now being changed? I ask because if we've broken how rustup used to behave then we need to be careful if anything might be used in scripting or similar.

[leighmcculloch]

I prefer rebases over merges, it makes for a clearer commit sequence to review.

I force pushed a squash of the commits rebased on master to make the history clean.

Are you sure that the changes in behaviour that your test changes imply are desired? Are older rustup behaviours now being changed? I ask because if we've broken how rustup used to behave then we need to be careful if anything might be used in scripting or similar.

We could keep the error code return without the error: text, but that seems inconsistent.

What do you recommend?

[kinnison]

Sorry it took me so long to get back here. Thank you for the squash, that makes it easier to read the commit. As for behaviour, I personally would prefer that whatever the behaviour was in versions before we started this refactor (so 1.21 or earlier) were preserved for anything where scripting might be used to detect errors. If it's purely in interactive behaviours then it's probably fine to change behaviour, though if the help text goes to stderr then it is opportunistic help and should have an exit code which reflects that (i.e. non-zero in some form).

I don't mind losing the error: text though.

[leighmcculloch]

@kinnison @rbtcollins The PR is updated to maintain the existing exit code when executed without subcommands, while losing the error: prefix on output.

[rbtcollins]

Could you please rebase this to reflect the final changes being done, rather than the review-edit-cycle? Thanks!

[leighmcculloch]

@rbtcollins The PR is updated with a force-push that squashes the commits into a single commit. The diff and the change are identical.