Skip to content

chore(deps): upgrade urfave/cli from v1 to v3 - #5184

Merged
kolyshkin merged 2 commits into
opencontainers:mainfrom
lifubang:use-urfave-cli-v3
May 20, 2026
Merged

kolyshkin merged 2 commits into
opencontainers:mainfrom
lifubang:use-urfave-cli-v3

Conversation

@lifubang

@lifubang lifubang commented Mar 18, 2026 •

Copy link
Copy Markdown
Member

Migrate from urfave/cli v1 (maintenance mode) to v3 to benefit from active development,
improved features, and long-term support.

There are two key configuration options helps this migration success:

	// Stop parsing flags after the first positional argument (command)
	// This allows passing flags like -c to the command being executed
	StopOnNthArg: intPtr(1),

	// Disable comma as separator for slice flags
	// This allows cgroup controller lists like "cpu,cpuacct:subcpu".
	DisableSliceFlagSeparator: true,

@cyphar

cyphar commented Mar 18, 2026

Copy link
Copy Markdown
Member

We should update this, but I'm a little cautious about this change.

There were a few features (bugs?) we depend on in v1 that (after discussing this with upstream many years ago) I suspect were broken in later versions -- parsing of arguments for runc exec for instance (we support not using -- to separate the command and I think urfave/cli broke this in a particular way and seemed to not see our usecase as important).

I don't think we have any real regression tests for that. 🤔

@lifubang

lifubang commented Mar 18, 2026 •

Copy link
Copy Markdown
Member Author

we support not using -- to separate the command and I think urfave/cli broke this in a particular way and seemed to not see our usecase as important

They later introduced a configuration option called StopOnNthArg to support this exact scenario.

@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch from 4a05b5d to 6106733 Compare March 18, 2026 04:21
@cyphar

cyphar commented Mar 18, 2026

Copy link
Copy Markdown
Member

Ah okay, I haven't looked at this in quite a while (as you can probably tell). Sounds great then. 😸

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR migrates the codebase from urfave/cli v1 to urfave/cli v3, updating command/flag wiring and adopting v3-specific configuration needed to preserve existing CLI behavior.

Changes:

  • Upgrade dependency to github.com/urfave/cli/v3 and refactor commands/actions to the v3 *cli.Command + context.Context API.
  • Add v3 migration settings (StopOnNthArg, DisableSliceFlagSeparator) to preserve argument/flag parsing semantics.
  • Update build configuration to drop the legacy urfave_cli_no_docs build tag and remove now-unneeded indirect deps.

Reviewed changes

Copilot reviewed 26 out of 125 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
utils_linux.go Refactors helper functions to use *cli.Command accessors for args/flags.
utils.go Updates arg-checking/help display for cli v3 and adds an intPtr helper used by v3 options.
update.go Converts update command definition/flags/action to cli v3 patterns.
tests/cmd/remap-rootfs/remap-rootfs.go Migrates the test helper CLI app to cli v3 Command.Run(ctx, args).
tests/cmd/recvtty/recvtty.go Migrates the test helper CLI app and flags to cli v3.
tests/cmd/pidfd-kill/pidfd-kill.go Migrates the test helper CLI app and flags to cli v3.
state.go Converts state command to cli v3 and sets v3 parsing option(s).
start.go Converts start command to cli v3 and sets v3 parsing option(s).
spec.go Converts spec command to cli v3, updates flags (aliases), and sets slice parsing behavior.
run.go Converts run command to cli v3, updates flags (aliases), and sets slice parsing behavior.
rootless_linux.go Switches rootless cgroup manager option lookup to cli v3 accessors.
restore.go Converts restore command to cli v3 and updates flag definitions/aliases.
ps.go Converts ps command to cli v3 and adds StopOnNthArg to forward ps flags correctly.
pause.go Converts pause/resume commands to cli v3 and sets slice parsing behavior.
notify_socket.go Refactors notify socket helpers to use *cli.Command for root flag reading.
main.go Reworks root command setup for cli v3, including Before signature and logging config.
list.go Converts list command to cli v3 and updates flags (aliases) + output writer usage.
kill.go Converts kill command to cli v3 and adds StopOnNthArg to prevent flag parsing of signal args.
go.mod Upgrades urfave/cli to v3 and removes indirect markdown-man dependencies.
features.go Converts features command to cli v3 and uses command writer for JSON output.
exec.go Converts exec command to cli v3, sets StopOnNthArg, and updates slice flag defaults.
events.go Converts events command to cli v3, adjusts flag definitions, and uses v3 duration accessors.
delete.go Converts delete command to cli v3 and updates flag aliases + root access.
create.go Converts create command to cli v3 and updates flag aliases.
checkpoint.go Converts checkpoint command and CRIU option helpers to cli v3 APIs.
Makefile Removes urfave_cli_no_docs build tag (no longer applicable with cli v3).

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread rootless_linux.go Resolved
Comment thread tests/cmd/pidfd-kill/pidfd-kill.go Resolved
Comment thread ps.go Resolved
Comment thread kill.go Resolved
Comment thread tests/cmd/pidfd-kill/pidfd-kill.go Outdated
Name: "pid-file",
Value: "",
Usage: "Path to write the pidfd-kill process ID to",
Usage: "Path to write to pidfd-kill process ID to",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like the original text was grammatically correct

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, the correct message is "Path to write the pidfd-kill process ID to" and it should not be changed.

@kolyshkin

Copy link
Copy Markdown
Contributor

Would be interesting to see the runc binary size comparison (hoping the new one is smaller).

This also reminds me I spent a few days trying to get rid of urfave/cli and gave up after a few days 🫨

@kolyshkin

Copy link
Copy Markdown
Contributor

Would be interesting to see the runc binary size comparison (hoping the new one is smaller).

Alas it's not

[kir@kir-tp1 runc]$ size runc.before runc
   text	   data	    bss	    dec	    hex	filename
6491412	4613585	 229128	11334125	 acf1ed	runc.before
6801036	4888809	 229576	11919421	 b5e03d	runc

Can't call it a huge difference but it's not small either (570K). This is with go1.25.7

@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch 2 times, most recently from efa9fd4 to 86219e6 Compare March 19, 2026 08:50
@abitrolly

abitrolly commented Mar 20, 2026 •

Copy link
Copy Markdown

@kolyshkin interesting to know what contributes the most to the size. Maybe it is some new dependency, or just bloated output templates?

@kolyshkin

Copy link
Copy Markdown
Contributor

It seems that the biggest part of it (more than 50%) is re-introduction of regexp dependency (which was eliminated in #3460).

@kolyshkin

Copy link
Copy Markdown
Contributor

It seems that the biggest part of it (more than 50%) is re-introduction of regexp dependency (which was eliminated in #3460).

Opened urfave/cli#2288.

Local testing shows it saves ~427 KB

@kolyshkin

Copy link
Copy Markdown
Contributor

If we are to switch to urfave v3, we may as well use it's shell completion (to replace ./contrib/completions/)

@kolyshkin

Copy link
Copy Markdown
Contributor

If we are to switch to urfave v3, we may as well use it's shell completion (to replace ./contrib/completions/)

For that, we need

index 3ee1009a..6056d479 100644
--- a/main.go
+++ b/main.go
@@ -82,7 +82,9 @@ value for "bundle" is the current directory.`
 )
 
 func main() {
-       app := &cli.Command{}
+       app := &cli.Command{
+               EnableShellCompletion: true,
+       }
        app.Name = "runc"
        app.Version = strings.TrimSpace(version) + extraVersion
        app.Usage = usage

(which doesn't increase binary size)

and then something like

./runc completion bash > runc-bash-completion

(and perhaps the same for fish and zsh). And then it just works.

@lifubang

Copy link
Copy Markdown
Member Author

Thanks for the suggestion. I agree that leveraging urfave/cli v3's built-in completion is the right long-term direction.

However, for this PR, I propose keeping the existing runc script and treating the shell completion migration as a follow-up task.
Currently, we lack command-specific ShellComplete handlers, so an immediate switch would likely regress some value-level completions.
My goal is to keep this PR strictly focused on the v1 -> v3 migration with full behavior parity.

Proposed Plan:

  1. This PR: Retain the current installed completion script.
  2. Follow-up PR: Implement ShellComplete handlers for key commands and arguments.
  3. Final Step: Once parity is achieved, update installation/docs to use runc completion bash|zsh|fish and remove the legacy script."

@kolyshkin

Copy link
Copy Markdown
Contributor

This LGTM once a release containing urfave/cli#2288 is created.

@kolyshkin

Copy link
Copy Markdown
Contributor

Still LGTM from me (but now needs a rebase, PTAL @lifubang)

@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch 2 times, most recently from 6fed29e to 9ec8045 Compare April 16, 2026 12:02
@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch from 9ec8045 to 7b89249 Compare May 18, 2026 14:45
@lifubang

Copy link
Copy Markdown
Member Author

@opencontainers/runc-maintainers PTAL, I think If we can merge this PR sooner, we'll have more time on the main branch to get community testing and validation before the 1.6.0-rc.1 release.

@lifubang
lifubang requested a review from thaJeztah May 19, 2026 02:18
@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch from 7b89249 to 389afbe Compare May 19, 2026 04:18

@thaJeztah thaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Comment thread utils.go Outdated
Comment on lines +133 to +136
// intPtr is a helper function to create int pointer.
func intPtr(i int) *int {
return &i
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like we can replace this with new() once we drop go1.25

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also have mkPtr in update.go which works with current Go. But yes, ultimately both should be replaced with new.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lifubang actually you need to drop this and use mkPtr.

Comment thread exec.go Outdated
# runc exec <container-id> ps`,
// Stop parsing flags after the first positional argument (command)
// This allows passing flags like -c to the command being executed
StopOnNthArg: intPtr(1),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: mkPtr(1)

@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch 2 times, most recently from bfe3bb9 to f2b573d Compare May 20, 2026 00:58
Comment thread exec.go Outdated
Comment on lines +32 to +33
// Stop parsing flags after the first positional argument (command)
// This allows passing flags like -c to the command being executed

@kolyshkin kolyshkin May 20, 2026 •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: missing periods.

Suggested change
// Stop parsing flags after the first positional argument (command)
// This allows passing flags like -c to the command being executed
// Stop parsing flags after the first positional argument (command).
// This allows passing flags like -c to the command being executed.

Comment thread exec.go Resolved
@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch from f2b573d to 297d10e Compare May 20, 2026 01:48
Comment thread utils.go Resolved
Comment thread list.go
list, err := os.ReadDir(root)
if err != nil {
if errors.Is(err, os.ErrNotExist) && context.IsSet("root") {
if errors.Is(err, os.ErrNotExist) && cmd.IsSet("root") {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was a bug in here, previously reported but still not fixed (see #5203, #5207).

It looks like this bug is being fixed here just by switching to v3. Maybe that's the best way to fix it (as the test case in #5707 is ridiculously complex).

Comment thread tests/cmd/pidfd-kill/pidfd-kill.go Outdated
Name: "pid-file",
Value: "",
Usage: "Path to write the pidfd-kill process ID to",
Usage: "Path to write to pidfd-kill process ID to",

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lifubang this ^^^ is still an issue

@kolyshkin kolyshkin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, just one issue.

The correct usage for --pid-file should be Path to write the pidfd-kill process ID to. In other words, please keep it intact.

lifubang added 2 commits May 20, 2026 05:26
Migrate from urfave/cli v1 (maintenance mode) to v3 to benefit from
active development, improved features, and long-term support.

Signed-off-by: lifubang <lifubang@acmcoder.com>
After migrate from urfave/cli v1 (maintenance mode) to v3,
we don't need this build tag anymore.

Signed-off-by: lifubang <lifubang@acmcoder.com>
@lifubang
lifubang force-pushed the use-urfave-cli-v3 branch from 297d10e to 098a2b4 Compare May 20, 2026 05:27
@kolyshkin
kolyshkin enabled auto-merge May 20, 2026 05:33
@kolyshkin
kolyshkin merged commit 3e802d1 into opencontainers:main May 20, 2026
55 checks passed
@lifubang
lifubang deleted the use-urfave-cli-v3 branch May 20, 2026 10:16
@kolyshkin kolyshkin modified the milestones: 1.6.0, 1.6.0-rc.1 Aug 26, 2026
@kolyshkin kolyshkin mentioned this pull request Sep 26, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants