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

Migrate Urfave CLI from v1 to v2 #4499

Closed
wants to merge 4 commits into from

Conversation

dereknola
Copy link

@dereknola dereknola commented Oct 31, 2024

Problem

The Urfave CLI v1 has been deprecated for several years now. v2 is very stable (v3 is even under alpha development now). Moving to v2 would keep runc up to date and enable any new features added in the future (as v1 no longer receives dependency updates or feature work).

Changes

Vendoring/go mod updates is in a separate commit for PR review clarity

Followed the Migration Guide at https://cli.urfave.org/migrate-v1-to-v2/
Some major points from this guide:

  • Aliases are now in a separate field, not integrated into the name
  • All Flags and Commands are now initialized as Pointers

The major changes not pointed out in the migration guide are:

  • context.Args() no longer produces a []slice, so context.Args().Slice() is substituted
  • All cli.Global***** are deprecated (the migration guide is somewhat unclear on this)
  • SkipArgReorder and other Reorder options are deprecated
  • context.NArgs() now gives commands + flags counts, not just command counts. This necessitated the remove of the exactArgs check in favor of a minArgs of 1 check.

Verification

make passes

@cyphar
Copy link
Member

cyphar commented Nov 1, 2024

The reason the tests are failing is that urfave/cli has unfortunate re-ordering behaviour and SkipArgReorder is something we really need in order to maintain compatibility with cases like runc exec ctr command --some-flag-that-runc-also-understands (as well working around urfave/cli#1152 but that issue was fixed in urfave/cli#1356).

To be honest, if urfave/cli v1 is actually deprecated and there is no way of getting this behaviour in newer versions, we might need to migrate to a different cli library.

@dereknola
Copy link
Author

dereknola commented Nov 1, 2024

Thanks for pointing this out.

Based on this line in the urfave/v1 code

// Skip argument reordering which attempts to move flags before arguments,
// but only works if all flags appear after all arguments. This behavior was
// removed n version 2 since it only works under specific conditions so we
// backport here by exposing it as an option for compatibility.
SkipArgReorder bool

It would appear that the Arg Reorder Functionality was removed in v2, so v2 might still work for runc (As every place I see the option used it is true), i.e. runc did not want that Reorder functionality... and v2 doesn't have it at all. I will investigate more.

@dereknola
Copy link
Author

dereknola commented Nov 1, 2024

Hmm I see what you mean, other commands like update seem to rely on that reorder functionality. For example, the integration test has

runc update [test_update --cpu-period 900000]

This expects urfave to reorder the flags to be runc update --cpu-period 900000 test-update which is the "standard" order of COMMAND [FLAGS] [ARGUMENTS]

Urfave v2 will not do this reorder for you. The other popular CLI library Cobra does seem to support some form of what you are after with Persistent Flags.

I will close this PR as this effectively kills this migration without a major overhaul of existing tests / user experience.

@dereknola dereknola closed this Nov 1, 2024
@kolyshkin
Copy link
Contributor

If we can find a way to reorder the arguments before urfave/cli kicks in, this might be revived.

I'm also interested in how the switch to v2 affects the binary size.

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.

3 participants