Port cargo to clap3 #10265

epage · 2022-01-06T15:33:46Z

What does this PR try to resolve?

This moves cargo to the latest major version of clap.

This supersedes #10259 and #10262

How should we test and review this PR?

For testing, I mostly relied on existing tests. I did manually validate that cargo run <non-escaped command args> behaved the same between both

$ cargo run release --help                                               
    Finished dev [unoptimized + debuginfo] target(s) in 0.22s                                                                                      
     Running `target/debug/cargo-release release --help`
cargo-release 0.18.8                                                                                                                               
...

$ cargo run --manifest-path ../cargo/Cargo.toml -- run release --help     
    Finished dev [unoptimized + debuginfo] target(s) in 0.05s
     Running `/home/epage/src/personal/cargo/target/debug/cargo run release --help`
    Finished dev [unoptimized + debuginfo] target(s) in 1.31s
     Running `target/debug/cargo-release release --help`
cargo-release 0.18.8                                                     
...

For reviewing, I split out each deprecation resolution into a separate commit so its easy to focus on more mechanical changes (most of the deprecation fixes) from interesting ones (the port, the Arg::multiple deprecation)

Additional information

One parser change found by cargo_config::includes is that clap 2
would ignore any values after a = for flags.
cargo config --show-origin is a flag but the test passed --show-origin=yes which
happens to give the desired result for that test but is the same as
--show-origin=no or --show-origin=alien-invasion.
ArgMatches now panics when accessing an undefined argument but clap
takes advantage of that for sharing code across commands that have
different subsets of arguments defined. I've extended clap so we can
"look before you leap" and put the checks at the argument calls to
start off with so its very clear what is tenuously shared. This
allows us to go in either direction in the future, either addressing
how we are sharing between commands or by moving this down into the
extension methods and pretending this clap feature doesn't exist
On that topic, a test found External subcommands that alias built-in subccommands cause panics clap-rs/clap#3263. For now, there is a
hack in clap. Depending on how we fix that in clap for clap 4.0, we
might need to re-address things in cargo.
value_of_os now requires setting allow_invalid_utf8, otherwise it
asserts. To help catch this, I updated the argument definitions
associated with lookups reported by:
- rg 'values?_os' src/
- rg 'values?_of_os' src/
clap now reports 2 for usage errors, so we had to bypass clap's
exit call to keep the same exit code.
cargo vendor --sync did not use multi_opt and so it has both
multiple occurrences and multiple values. If we want to deprecate
this, we'll need unstable-grouped to be stablized (or pin our clap
version) and ensure each group has only 1 value.

- One parser change found by `cargo_config::includes` is that clap 2 would ignore any values after a `=` for flags. `cargo config --show-origin` is a flag but the test passed `--show-origin=yes` which happens to give the desired result for that test but is the same as `--show-origin=no` or `--show-origin=alien-invasion`. - The parser now panics when accessing an undefined attribute but clap takes advantage of that for sharing code across commands that have different subsets of arguments defined. I've extended clap so we can "look before you leap" and put the checks at the argument calls to start off with so its very clear what is tenuously shared. This allows us to go in either direction in the future, either addressing how we are sharing between commands or by moving this down into the extension methods and pretending this clap feature doesn't exist - On that topic, a test found clap-rs/clap#3263. For now, there is a hack in clap. Depending on how we fix that in clap for clap 4.0, we might need to re-address things in cargo. - `value_of_os` now requires setting `allow_invalid_utf8`, otherwise it asserts. To help catch this, I updated the argument definitions associated with lookups reported by: - `rg 'values?_os' src/` - `rg 'values?_of_os' src/` - clap now reports `2` for usage errors, so we had to bypass clap's `exit` call to keep the same exit code. BREAKING CHANGE: API now uses clap3

Note: `cargo vendor --sync` did not use `multi_opt` and so it has both multiple occurrences **and** multiple values. If we want to deprecate this, we'll need `unstable-grouped` to be stablized (or pin our clap version) and ensure each group has only 1 value.

rust-highfive · 2022-01-06T15:33:50Z

r? @alexcrichton

(rust-highfive has picked a reviewer for you, use r? to override)

epage · 2022-01-06T15:35:16Z

src/bin/cargo/commands/vendor.rs

+            Arg::new("tomls")
+                .short('s')
                .long("sync")
                .help("Additional `Cargo.toml` to sync and vendor")
                .value_name("TOML")
-                .multiple(true),
+                .allow_invalid_utf8(true)
+                .multiple_occurrences(true)
+                .multiple_values(true),
        )


cargo vendor --sync did not use multi_opt and so it has both
multiple occurrences and multiple values. If we want to deprecate
this, we'll need unstable-grouped to be stablized (or pin our clap
version) and ensure each group has only 1 value.

epage · 2022-01-06T15:35:44Z

src/cargo/lib.rs

+            let exit_code = if clap_err.use_stderr() { 1 } else { 0 };
+            let _ = clap_err.print();
+            std::process::exit(exit_code)


clap now reports 2 for usage errors, so we had to bypass clap's
exit call to keep the same exit code.

epage · 2022-01-06T15:36:40Z

tests/testsuite/cargo_config.rs

+    cargo_process("config get build.rustflags --show-origin -Zunstable-options -Zconfig-include")
+        .cwd(&sub_folder.parent().unwrap())
+        .masquerade_as_nightly_cargo()


One parser change found by cargo_config::includes is that clap 2
would ignore any values after a = for flags.
cargo config --show-origin is a flag but the test passed --show-origin=yes which
happens to give the desired result for that test but is the same as
--show-origin=no or --show-origin=alien-invasion.

epage · 2022-01-06T15:37:15Z

src/cargo/util/command_prelude.rs

+    fn _is_valid_arg(&self, name: &str) -> bool {
+        self.is_valid_arg(name)
+    }


ArgMatches now panics when accessing an undefined argument but clap
takes advantage of that for sharing code across commands that have
different subsets of arguments defined. I've extended clap so we can
"look before you leap" and put the checks at the argument calls to
start off with so its very clear what is tenuously shared. This
allows us to go in either direction in the future, either addressing
how we are sharing between commands or by moving this down into the
extension methods and pretending this clap feature doesn't exist

epage · 2022-01-06T15:38:44Z

src/bin/cargo/cli.rs


-                let (new_cmd, _) = new_args.subcommand();
+                let new_cmd = new_args.subcommand_name().expect("subcommand is required");


clap2 returned "" when no subcommand was present but now returns None.

epage · 2022-01-06T15:39:25Z

src/bin/cargo/cli.rs

@@ -408,22 +408,24 @@ fn cli() -> App {
        "cargo [OPTIONS] [SUBCOMMAND]"
    };
    App::new("cargo")
-        .settings(&[
-            AppSettings::UnifiedHelpMessage,


UnifiedHelpMessage is now built-in

epage · 2022-01-06T15:39:56Z

src/bin/cargo/cli.rs

-        .settings(&[
-            AppSettings::UnifiedHelpMessage,
-            AppSettings::DeriveDisplayOrder,
-            AppSettings::VersionlessSubcommands,


VersionlessSubcommands isn't needed, if a subcommand has no version set then it won't have a --version

epage · 2022-01-06T15:40:57Z

src/bin/cargo/cli.rs

+        .setting(
+            AppSettings::DeriveDisplayOrder
+                | AppSettings::AllowExternalSubcommands
+                | AppSettings::NoAutoVersion,


Looks like clap used to allow you to completely override the --version flag but now you can override its --help but you still get the built-in version behavior unless you opt-out with this flag.

epage · 2022-01-06T15:42:14Z

src/bin/cargo/cli.rs

+#[test]
+fn verify_cli() {
+    cli().debug_assert();
+}


Clap assumes errors are programmer mistakes and reports them through debug_asserrts. It does this lazily, only evaluating the subcommands that the user activates. This test will eagerly run all of the debug asserts, helping to catch problems sooner.

epage · 2022-01-06T15:44:59Z

src/bin/cargo/commands/vendor.rs

        .arg(
-            Arg::with_name("no-delete")
+            Arg::new("path")
+                .allow_invalid_utf8(true)


value_of_os now requires setting allow_invalid_utf8, otherwise it
asserts. To help catch this, I updated the argument definitions
associated with lookups reported by:

I thought it interesting that most clap path arguments don't allow non-UTF-8 arguments but vendor seems to be one of the main exceptions

That's interesting. In general, any path argument we accept should allow non-UTF-8, but that can get fixed in a follow-up PR.

The follow up PR would be two parts

Add arg.allow_invalid_utf8(true)

Call value_of_os rather than value_of

epage · 2022-01-06T15:45:53Z

src/cargo/util/command_prelude.rs

 pub fn optional_multi_opt(
    name: &'static str,
    value_name: &'static str,
    help: &'static str,
-) -> Arg<'static, 'static> {
+) -> Arg<'static> {
    opt(name, help)
        .value_name(value_name)
-        .multiple(true)
+        .multiple_occurrences(true)
+        .multiple_values(true)
        .min_values(0)
        .number_of_values(1)
 }


Didn't dig too deeply into how this works, it might be possible to simplify it like I did multi_opt

optional_multi_opt is supposed to allow cargo cmd --theopt value1 --otheropt and cargo cmd --theopt value1 --theopt value2 --otheropt and cargo cmd --theopt --otheropt, but not cargo cmd --theopt value1 value2 --otheropt.

So its like --color[=WHEN] with multiple.

What confused me is I never realized these args work like this, Playing with some of them it now makes sense; at least in some of the cases, like cargo check --test, its so cargo can list what values are supported.

Also, as far as I can tell, because cargo accepts optional values passed to options, and doesn't require = on those options, we've effectively ruled out the possibility of having positional parameters on various commands.

epage · 2022-01-06T15:47:27Z

src/cargo/util/errors.rs

+impl From<std::io::Error> for CliError {
+    fn from(err: std::io::Error) -> CliError {
+        CliError::new(err.into(), 1)
+    }
+}


cli().print_help()?; now reports IO errors rather than clap errors

alexcrichton · 2022-01-07T15:55:15Z

@ehuss as the original porter to clap (IIRC) would you be up for reviewing this? For me to review this I feel like I'd have to build up a more-complete understanding of clap2, then clap3, then look at this diff, which at least for me would take a significant amount of time.

ehuss · 2022-01-07T18:48:03Z

I think that was matklad. I can review it, but it may take a while.

alexcrichton · 2022-01-07T20:39:34Z

Ah ok no that's my mistake. I'll try to get to this at some point. I can't commit to a particular point in time right now though.

ehuss · 2022-01-11T17:01:07Z

r? @joshtriplett

joshtriplett · 2022-01-11T22:22:14Z

I pulled this down locally, reviewed it in detail, and built it to run various tests. Seems to work exactly as expected, and the code changes look good to me.

I also appreciated the incremental approach here of first doing the minimal port, then migrating away from the deprecated functions one by one.

@bors r+

bors · 2022-01-11T22:22:16Z

📌 Commit 92fa72d has been approved by joshtriplett

bors · 2022-01-11T22:22:22Z

⌛ Testing commit 92fa72d with merge b81ca081d3e901d0cb01abd13af66c8ff5dc1bbf...

bors · 2022-01-11T23:04:29Z

💔 Test failed - checks-actions

ehuss · 2022-01-11T23:47:25Z

@bors retry

The runner has received a shutdown signal. Dunno why that happened.

bors · 2022-01-11T23:47:32Z

⌛ Testing commit 92fa72d with merge 06b9d31...

bors · 2022-01-12T00:25:25Z

☀️ Test successful - checks-actions
Approved by: joshtriplett
Pushing 06b9d31 to master...

Update cargo 6 commits in 358e79fe56fe374649275ca7aebaafd57ade0e8d..06b9d31743210b788b130c8a484c2838afa6fc27 2022-01-04 18:39:45 +0000 to 2022-01-11 23:47:29 +0000 - Port cargo to clap3 (rust-lang/cargo#10265) - feat: support rustflags per profile (rust-lang/cargo#10217) - Make bors ignore the PR template so it doesn't end up in merge messages (rust-lang/cargo#10267) - Be resilient to most IO error and filesystem loop while walking dirs (rust-lang/cargo#10214) - Remove the option to disable pipelining (rust-lang/cargo#10258) - Always ask rustc for messages about artifacts, and always process them (rust-lang/cargo#10255)

Update cargo 16 commits in 358e79fe56fe374649275ca7aebaafd57ade0e8d..95bb3c92bf516017e812e7f1c14c2dea3845b30e 2022-01-04 18:39:45 +0000 to 2022-01-18 17:39:35 +0000 - Error when setting crate type of both dylib and cdylib in library (rust-lang/cargo#10243) - Include `help` in `--list` (rust-lang/cargo#10300) - Add report subcommand to bash completion. (rust-lang/cargo#10295) - Downgrade some log messages. (rust-lang/cargo#10296) - Enable shortcut for triage bot (rust-lang/cargo#10298) - Bump to 0.61.0, update changelog (rust-lang/cargo#10294) - use new cargo fmt option (rust-lang/cargo#10291) - Add `run-fail` to semver-check for docs (rust-lang/cargo#10287) - Use `is_symlink()` method (rust-lang/cargo#10290) - Stabilize namespaced and weak dependency features. (rust-lang/cargo#10269) - Port cargo to clap3 (rust-lang/cargo#10265) - feat: support rustflags per profile (rust-lang/cargo#10217) - Make bors ignore the PR template so it doesn't end up in merge messages (rust-lang/cargo#10267) - Be resilient to most IO error and filesystem loop while walking dirs (rust-lang/cargo#10214) - Remove the option to disable pipelining (rust-lang/cargo#10258) - Always ask rustc for messages about artifacts, and always process them (rust-lang/cargo#10255)

epage added 12 commits January 5, 2022 19:54

refactor: Resolve Arg::with_name deprecation

88a122c

refactor: Resolve Arg::hidden deprecation

5435b26

refactor: Resolve Arg::empty_values deprecation

f3ec865

refactor: Resolve AppSettings::UnifiedHelpMessage deprecation

2aa4e9f

refactor: Resolve App::get_matches_safe deprecation

f9b28cf

refactor: Resolve App::usage deprecation

aed0312

refactor: Resolve App::template deprecation

5cd062e

refactor: Resolve App::help deprecation

0ab4d0c

refactor: Resolve App::settings deprecation

24f301b

refactor: Resolve SubCommand::with_new deprecation

6e08a30

rust-highfive assigned alexcrichton Jan 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2022

epage commented Jan 6, 2022

View reviewed changes

epage force-pushed the clap-port branch from a988a62 to 92fa72d Compare January 6, 2022 15:53

rust-highfive assigned joshtriplett and unassigned alexcrichton Jan 11, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2022

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 11, 2022

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 11, 2022

bors merged commit 06b9d31 into rust-lang:master Jan 12, 2022

This was referenced Jan 12, 2022

Retry git fetch operations when net.git_fetch_with_cli = true is set #9870

Closed

Implement 'multidep' (RFC-3176) #10061

Closed

ehuss mentioned this pull request Jan 12, 2022

Update cargo rust-lang/rust#92807

Merged

epage deleted the clap-port branch January 12, 2022 15:56

ehuss mentioned this pull request Jan 19, 2022

Update cargo rust-lang/rust#93063

Merged

petr-tik mentioned this pull request Feb 7, 2022

Use clap::ArgEnum to auto-generate possible cli arguments from Rust enums #10369

Closed

ehuss added this to the 1.60.0 milestone Feb 14, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Port cargo to clap3 #10265

Port cargo to clap3 #10265

epage commented Jan 6, 2022 •

edited

Loading

rust-highfive commented Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022 •

edited

Loading

epage Jan 6, 2022

joshtriplett Jan 6, 2022

epage Jan 6, 2022

epage Jan 6, 2022 •

edited

Loading

joshtriplett Jan 6, 2022

epage Jan 6, 2022

joshtriplett Jan 7, 2022

epage Jan 6, 2022

alexcrichton commented Jan 7, 2022

ehuss commented Jan 7, 2022

alexcrichton commented Jan 7, 2022

ehuss commented Jan 11, 2022

joshtriplett commented Jan 11, 2022

bors commented Jan 11, 2022

bors commented Jan 11, 2022

bors commented Jan 11, 2022

ehuss commented Jan 11, 2022

bors commented Jan 11, 2022

bors commented Jan 12, 2022


		let (new_cmd, _) = new_args.subcommand();
		let new_cmd = new_args.subcommand_name().expect("subcommand is required");

Port cargo to clap3 #10265

Port cargo to clap3 #10265

Conversation

epage commented Jan 6, 2022 • edited Loading

What does this PR try to resolve?

How should we test and review this PR?

Additional information

rust-highfive commented Jan 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

epage Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexcrichton commented Jan 7, 2022

ehuss commented Jan 7, 2022

alexcrichton commented Jan 7, 2022

ehuss commented Jan 11, 2022

joshtriplett commented Jan 11, 2022

bors commented Jan 11, 2022

bors commented Jan 11, 2022

bors commented Jan 11, 2022

ehuss commented Jan 11, 2022

bors commented Jan 11, 2022

bors commented Jan 12, 2022

epage commented Jan 6, 2022 •

edited

Loading

epage Jan 6, 2022 •

edited

Loading

epage Jan 6, 2022 •

edited

Loading