Skip to content
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.

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

Update to clap 4 #2851

Closed
wants to merge 2 commits into from
Closed

Update to clap 4 #2851

wants to merge 2 commits into from

Conversation

rylev
Copy link
Collaborator

@rylev rylev commented Sep 19, 2024

On the heels of #2847, this updates to Clap 4 which gets rid of some duplicate dependencies in the dependency tree. I did some light testing, but I'm not sure the best way to verify that this is functionally equivalent to the previous version.

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev rylev requested a review from itowlson September 19, 2024 16:41
@itowlson
Copy link
Contributor

A couple of things I noticed while testing:

  • The spin --help commands are no longer sorted alphabetically. It would be good to restore the sorting.

  • There are obvious formatting differences in help output, some of which are neutral but some of which make it harder to read. For example, here is a fragment of spin up --help with clap 3:

image

and with clap 4:

image

The way clap 4 leaves line wrapping to the terminal, especially combined with the removal of colour highlighting, seems like a step backward. I don't know if clap 4 provides us with any knobs to control this behaviour though.

  • Okay this is super duper weird but when I do spin up --follow foo it now panics with "thread 'main' panicked at /home/ivan/.cargo/registry/src/index.crates.io-6f17d22bba15001f/clap_builder-4.5.2/src/builder/debug_asserts.rs:143:13:
    Command http: Argument or group 'tls-key' specified in 'requires*' for 'tls_cert' does not exist". This doesn't happen with Spin canary. --disable-wasmtime-cache , --listen, and presumably other trigger options, have the same problem. Heaven knows if there are any other bear traps like this, I only found it by accident when testing "multiple occurrence" flags...!

Signed-off-by: Ryan Levick <ryan.levick@fermyon.com>
@rylev
Copy link
Collaborator Author

rylev commented Sep 20, 2024

@itowlson I've addressed your feedback. Unfortunately, I've run into the same issue found in this previous attempt: #1198. It seems clap 4 doesn't lazily evaluate args like clap 3 did, and once it discovers an arg that can only go into the trigger_args bucket of otherwise unknown args, it starts slurping up everything else and putting it into trigger_args.

This means the following used to work but no longer does:

spin up --some-arg-meant-for-the-trigger --env "foo=bar"

This does not work because as soon as --some-arg-meant-for-the-trigger is seen, everything gets lumped into trigger_args. If --env is put first, than things work as expected.

@rylev rylev closed this Oct 14, 2024
@rylev rylev deleted the update-clap branch October 14, 2024 09:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants