Update clap dependency from v3 to v4.1 #1053

cardoso · 2023-01-25T15:32:48Z

The current clap version could make it difficult to improve the CLI's DevX with the newest CLI practices.
The update by itself already makes some improvements, but I'm looking forward to using this new version to add new capabilities.

Here's the full changelog: https://github.com/clap-rs/clap/blob/master/CHANGELOG.md#400---2022-09-28

Leaving this as draft for now while I test this further, but it seems to be working fine.
Also decided to depend on #1052 to test these crate updates together rather than after merge.

Signed-off-by: Matheus Cardoso [email protected]

crates/http/src/lib.rs

cardoso · 2023-01-26T09:24:54Z

Tests are failing in CI but passing locally, so I'm opening this up for review while I figure out if it's related to any changes here.

vdice

Thanks for this update PR @cardoso!

Indeed, I think there may be a few spots to adjust per updated clap usage; both of the e2e test failures seem to arise from either a flag now being unexpected (--env) or a value not received (bindle connection string):

test integration_tests::e2e_tests::test_headers_env_routes has been running for over 60 seconds
error: unexpected argument '--env' found

and

Error: Failed to prepare configuration

Caused by:
    Component hello requires a Bindle connection but none was specified
test integration_tests::e2e_tests::test_using_parcel_as_module_source ... FAILED

vdice

Had a couple Qs/comments, otherwise looking great to me. Appreciative of the detail/attention required for this update!

src/commands/new.rs

crates/trigger/src/cli.rs

crates/http/src/lib.rs

vdice

@cardoso took one more close look at the updated cli output and raised a few more comments around notable changes, mostly to check in with others...

src/commands/build.rs

vdice · 2023-01-30T23:47:05Z

src/bin/spin.rs

    version = version(),
 )]
 enum SpinApp {
-    #[clap(subcommand, alias = "template")]
+    #[command(subcommand, alias = "template")]


(Not a comment on this particular line.)

Using this opportunity to point out to other reviewers that clap 4.0.0+ has updated default display order. Whereas in 3.x.x the commands are alphabetically sorted, 4+ appears to defer to the order we declare commands here in code.

Previous to this PR (looking at SUBCOMMANDS):

With this PR (looking at Commands):

@itowlson (and others) -- Do we have a preference on ordering? If so, should we ensure commands are listed here in the order we'd like them displayed? (Or, I imagine there is a clap 4+ setting we might utilize, regardless of order in code.)

Eek, good catch. I'd be inclined to go for alphabetical - some tools (such as Cargo do try to group similar commands together, but I'm not sure if we have a strong enough notion of that at the moment. And if we do it, we should approach it intentionally rather than as part of a package rev.

Should I do anything here? Change the order by hand, force the old way, leave it be for now?

Using a flag would be fighting clap quite a bit as it wants to be WYSIWYG going forward.

New way seems better to make the code 1:1 with the ~~OCD~~ CLI.

New behavior: Sorted order is derived by default Sorting is turned on by cmd.next_display_order(None) This is not recursive, it must be set on each level The display order incrementing is mixed with arguments This does make it slightly more difficult to predict

from: clap-rs/clap#3975

Maybe the best bet is to change the order by hand, assuming we'd like to retain the current alphabetical order.

cardoso · 2023-01-31T09:18:00Z

tests/integration.rs

+            let env_args = spin_app_env.iter().flat_map(|e| ["--env", e]).collect();
+
+            let trigger_args = vec!["--file", manifest_path, "--listen", &url];
+
+            let bindle_args = match bindle_url {
+                Some(url) => vec!["--bindle-server", url],
+                None => vec![],
+            };
+
+            let args = vec![
+                vec!["up"],
+                spin_args.to_vec(),
+                bindle_args,
+                env_args,
+                trigger_args,
+            ]
+            .into_iter()
+            .flatten();


Probably worth mentioning I had to change this test. Clap changed the way it parses, so that --env was being seen as part of spin trigger and not spin up.

I could try to force it, but it's not going to be pretty. The old behavior seemed less correct since --env appeared after other args which were going to spin trigger.

If it's too risky of a change, I could try a workaround

Thanks for the note here. It looks like spin built from main is agnostic about ordering/precedence between default up opts and trigger-specific opts. They can be provided in either order, for example:

$ ./spin-main up -e foo=bar --listen 127.0.0.1:3333 Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello. ^C $ ./spin-main up --listen 127.0.0.1:3333 -e foo=bar Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello.

Additionally, default opts and trigger opts can be intermixed:

$ ./spin-main up --listen 127.0.0.1:3333 -e foo=bar --follow-all Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello.

While spin built from this branch does appear to enforce ordering and won't allow intermixing of opts:

$ spin up -e foo=bar --listen 127.0.0.1:3333 Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello. ^C $ spin up --listen 127.0.0.1:3333 -e foo=bar error: unexpected argument '-e' found Usage: spin trigger http [OPTIONS] For more information, try '--help'. Error: exit status: 2 $ spin up -e foo=bar --listen 127.0.0.1:3333 --direct-mounts error: unexpected argument '--direct-mounts' found Usage: spin trigger http <--listen <ADDRESS>|--tls-cert <TLS_CERT>|--tls-key <TLS_KEY>> For more information, try '--help'. Error: exit status: 2

I'd definitely like to see how we can maintain the current (main) behavior and allow for arbitrary ordering of opts.

vdice · 2023-01-31T16:49:19Z

src/commands/build.rs

    pub up: bool,

-    #[clap(requires = BUILD_UP_OPT)]
+    #[arg(requires = BUILD_UP_OPT)]


Interesting; I'm not quite sure what is amiss, but I'm seeing behavior where spin build --up claims add'l up opts are invalid. It's a breaking change, so we'll want to check this out.

With the examples/http-rust app, using spin built from main:

$ spin build --up --listen 127.0.0.1:3333 Executing the build command for component hello: cargo build --target wasm32-wasi --release Finished release [optimized] target(s) in 0.03s Successfully ran the build command for the Spin components. Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello. ^C

And here using the binary built from this branch:

$ spin build --up --listen 127.0.0.1:3333 error: unexpected argument '--listen' found note: to pass '--listen' as a value, use '-- --listen' Usage: spin build <--file <APP_CONFIG_FILE>|--up|UP_ARGS> For more information, try '--help'.

vdice · 2023-01-31T17:06:40Z

tests/integration.rs

+            let env_args = spin_app_env.iter().flat_map(|e| ["--env", e]).collect();
+
+            let trigger_args = vec!["--file", manifest_path, "--listen", &url];
+
+            let bindle_args = match bindle_url {
+                Some(url) => vec!["--bindle-server", url],
+                None => vec![],
+            };
+
+            let args = vec![
+                vec!["up"],
+                spin_args.to_vec(),
+                bindle_args,
+                env_args,
+                trigger_args,
+            ]
+            .into_iter()
+            .flatten();


Thanks for the note here. It looks like spin built from main is agnostic about ordering/precedence between default up opts and trigger-specific opts. They can be provided in either order, for example:

$ ./spin-main up -e foo=bar --listen 127.0.0.1:3333 Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello. ^C $ ./spin-main up --listen 127.0.0.1:3333 -e foo=bar Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello.

Additionally, default opts and trigger opts can be intermixed:

$ ./spin-main up --listen 127.0.0.1:3333 -e foo=bar --follow-all Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello.

While spin built from this branch does appear to enforce ordering and won't allow intermixing of opts:

$ spin up -e foo=bar --listen 127.0.0.1:3333 Serving http://127.0.0.1:3333 Available Routes: hello: http://127.0.0.1:3333/hello A simple component that returns hello. ^C $ spin up --listen 127.0.0.1:3333 -e foo=bar error: unexpected argument '-e' found Usage: spin trigger http [OPTIONS] For more information, try '--help'. Error: exit status: 2 $ spin up -e foo=bar --listen 127.0.0.1:3333 --direct-mounts error: unexpected argument '--direct-mounts' found Usage: spin trigger http <--listen <ADDRESS>|--tls-cert <TLS_CERT>|--tls-key <TLS_KEY>> For more information, try '--help'. Error: exit status: 2

I'd definitely like to see how we can maintain the current (main) behavior and allow for arbitrary ordering of opts.

itowlson · 2023-02-03T00:05:45Z

src/commands/build.rs

+pub struct BuildUpArgs {
+    #[arg(short, long, group = "UpArgs", required=true)]
    pub up: bool,
+    #[command(flatten, next_help_heading="Up")]
+    pub args: UpArgs
+}


cardoso · 2023-02-06T13:11:48Z

Cargo.toml

@@ -80,12 +82,13 @@ vergen = { version = "7", default-features = false, features = [
 ] }

 [features]
-default = []
+default = ["generate-completions"]


I added completions here just to make sure the less conventional features like build's --up option aren't breaking it.

I'll make sure to remove once this is ready for review again.

Ooh, if this provides us with a better and more consistent way to #1032 then I'd love to have your feedback on that PR (or your own PR). Thanks! (I agree on not trying to shoehorn it into this one, of course.)

Signed-off-by: Matheus Cardoso <[email protected]>

cardoso · 2023-02-23T21:19:08Z

Closing this since it's fallen behind. Will open a new one soon.

vdice · 2023-02-23T22:38:48Z

Thanks @cardoso and apologies -- this has definitely been a busy period for the Spin project and particularly the CLI. Happy to help in any way I can.

cardoso force-pushed the clap4 branch 3 times, most recently from 62384f4 to a71a7b2 Compare January 26, 2023 08:25

cardoso commented Jan 26, 2023

View reviewed changes

crates/http/src/lib.rs Outdated Show resolved Hide resolved

cardoso marked this pull request as ready for review January 26, 2023 09:22

cardoso force-pushed the clap4 branch from a71a7b2 to 8e541c0 Compare January 26, 2023 10:18

vdice reviewed Jan 27, 2023

View reviewed changes

cardoso force-pushed the clap4 branch 22 times, most recently from 3f05911 to 56913e2 Compare January 30, 2023 16:14

vdice reviewed Jan 30, 2023

View reviewed changes

src/commands/new.rs Outdated Show resolved Hide resolved

crates/trigger/src/cli.rs Show resolved Hide resolved

crates/http/src/lib.rs Outdated Show resolved Hide resolved

vdice reviewed Jan 30, 2023

View reviewed changes

cardoso force-pushed the clap4 branch 2 times, most recently from c1d019e to 654eed2 Compare January 31, 2023 09:09

cardoso commented Jan 31, 2023

View reviewed changes

vdice reviewed Jan 31, 2023

View reviewed changes

cardoso force-pushed the clap4 branch from 654eed2 to 88d4cc6 Compare February 2, 2023 23:56

cardoso marked this pull request as draft February 2, 2023 23:57

itowlson reviewed Feb 3, 2023

View reviewed changes

cardoso commented Feb 6, 2023

View reviewed changes

cardoso force-pushed the clap4 branch from 88d4cc6 to d00945f Compare February 20, 2023 10:03

chore: update clap dependency from v3 to v4

be061e1

Signed-off-by: Matheus Cardoso <[email protected]>

cardoso force-pushed the clap4 branch from d00945f to be061e1 Compare February 23, 2023 21:17

cardoso closed this Feb 23, 2023

vdice mentioned this pull request Feb 24, 2023

chore: update clap dependency to 4.1 #1198

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update clap dependency from v3 to v4.1 #1053

Update clap dependency from v3 to v4.1 #1053

cardoso commented Jan 25, 2023 •

edited

Loading

cardoso commented Jan 26, 2023

vdice left a comment •

edited

Loading

vdice left a comment

vdice left a comment

vdice Jan 30, 2023

itowlson Jan 31, 2023

cardoso Jan 31, 2023 •

edited

Loading

vdice Feb 6, 2023

cardoso Jan 31, 2023 •

edited

Loading

vdice Jan 31, 2023

vdice Jan 31, 2023

vdice Jan 31, 2023

itowlson Feb 3, 2023

cardoso Feb 6, 2023

itowlson Feb 6, 2023

cardoso commented Feb 23, 2023

vdice commented Feb 23, 2023

Update clap dependency from v3 to v4.1 #1053

Update clap dependency from v3 to v4.1 #1053

Conversation

cardoso commented Jan 25, 2023 • edited Loading

cardoso commented Jan 26, 2023

vdice left a comment • edited Loading

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

vdice left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Previous to this PR (looking at SUBCOMMANDS):

With this PR (looking at Commands):

Choose a reason for hiding this comment

cardoso Jan 31, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardoso Jan 31, 2023 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cardoso commented Feb 23, 2023

vdice commented Feb 23, 2023

cardoso commented Jan 25, 2023 •

edited

Loading

vdice left a comment •

edited

Loading

Previous to this PR (looking at `SUBCOMMANDS`):

With this PR (looking at `Commands`):

cardoso Jan 31, 2023 •

edited

Loading

cardoso Jan 31, 2023 •

edited

Loading