String flag cannot be set to "-1"

[AkihiroSuda]

my urfave/cli version is

v1.22.1, v1.22.2, v1.22.3 (broken since v1.22.2)

Checklist

• Are you running the latest v1 release? The list of releases is here.
• Did you check the manual for your release? The v1 manual is here
• Did you perform a search about this problem? Here's the Github guide about searching.

Dependency Management

• My project is using go modules.
• My project is using vendoring.
• My project is automatically downloading the latest version.
• I am unsure of what my dependency management setup is.

Describe the bug

This flag can be set to "-1" on v1.22.1 but cannot be set since v1.22.2.

		cli.StringFlag{
			Name:  "memory-swap",
			Usage: "Total memory usage (memory + swap); set '-1' to enable unlimited swap",
		},

https://github.com/opencontainers/runc/blob/be51398a8af0b32b0b580e4da069cc6f704639bf/update.go#L111-L114

Regression in v1.22.1...v1.22.2

To reproduce

Specify urfave/cli version in go.mod in the runc tree, run go mod tidy, go mod vendor, and make integration.

Observed behavior

# runc update test_update --memory-swap -1 (status=1):
# Incorrect Usage: flag provided but not defined: -1

Expected behavior

It should not fail.

Additional context

opencontainers/runc#2268 (comment)

Run go version and paste its output here

For vendoring:

go version go1.14.1 linux/amd64

For running the containerized test suite:

go version go1.12.17 linux/amd64

[coilysiren]

😅 looks like it thinks -1 is a flag called 1. This bug makes sense, thanks for the well written issue @AkihiroSuda!

🙏 PRs are welcome 🙏

[thaJeztah]

Did some quick testing to do a minimal reproducer (LOL, I never worked with urfave/cli, so "code by example" 😅);

package main

import (
	"fmt"
	"os"
	"strings"

	"github.com/urfave/cli"
)

func main() {
	app := cli.NewApp()
	app.Name = "flagtest"

	app.Action = func(c *cli.Context) error {
		fmt.Println("opt:", c.String("opt"))
		fmt.Println("args:", strings.Join(c.Args(), ","))
		return nil
	}
	app.Commands = []cli.Command{{
		Name: "subcommand",
		Action: func(c *cli.Context) error {
			fmt.Println("opt:", c.String("opt"))
			fmt.Println("args:", strings.Join(c.Args(), ","))
			return nil
		},
		Flags: []cli.Flag{
			cli.StringFlag{
				Name:  "opt",
				Usage: "opt; set '-1' to disable",
			},
		},
	}}

	if err := app.Run(os.Args); err != nil {
		fmt.Fprintln(os.Stderr, err)
		os.Exit(1)
	}
}

Testing variations;

go run flagtest.go subcommand
option:
args:

go run flagtest.go subcommand arg1
opt:
args: arg1

go run flagtest.go subcommand arg1 arg2
opt:
args: arg1,arg2

go run flagtest.go subcommand --opt opt-value
option: opt-value
args:

go run flagtest.go subcommand --opt opt-value arg1
opt: opt-value
args: arg1

go run flagtest.go subcommand --opt opt-value arg1 arg2
opt: opt-value
args: arg1,arg2

go run flagtest.go subcommand arg1 --opt opt-value
opt: opt-value
args: arg1

go run flagtest.go subcommand arg1 --opt opt-value arg2
opt: opt-value
args: arg1,arg2

Using -opt-value (leading hyphen (-));

go run flagtest.go subcommand --opt -opt-value
opt: -opt-value
args:

go run flagtest.go subcommand --opt -opt-value arg1
Incorrect Usage: flag provided but not defined: -opt-value


go run flagtest.go subcommand --opt -opt-value arg1 arg2
Incorrect Usage: flag provided but not defined: -opt-value

go run flagtest.go subcommand arg1 --opt -opt-value
Incorrect Usage: flag provided but not defined: -opt-value


go run flagtest.go subcommand arg1 --opt -opt-value arg2
opt: arg2
args: arg1,-opt-value

[thaJeztah]

And a test-case;

func TestParseAndRunHyphenValues(t *testing.T) {
	cases := []struct {
		testArgs     []string
		expectedArgs []string
		expectedOpt string
	}{
		{[]string{"foo", "test", "arg1"}, []string{"arg1"}, ""},
		{[]string{"foo", "test", "arg1", "arg2"}, []string{"arg1", "arg2"}, ""},
		{[]string{"foo", "test", "--opt", "opt-value"}, []string{}, "opt-value"},
		{[]string{"foo", "test", "--opt", "opt-value", "arg1"}, []string{"arg1"}, "opt-value"},
		{[]string{"foo", "test", "--opt", "opt-value", "arg1", "arg2"}, []string{"arg1", "arg2"}, "opt-value"},
		{[]string{"foo", "test", "arg1", "--opt", "opt-value"}, []string{"arg1"}, "opt-value"},
		{[]string{"foo", "test", "arg1", "--opt", "opt-value", "arg2"}, []string{"arg1", "arg2"}, "opt-value"},
		
		{[]string{"foo", "test", "--opt", "-opt-value"}, []string{}, "-opt-value"},
		{[]string{"foo", "test", "--opt", "-opt-value", "arg1"}, []string{"arg1"}, "-opt-value"},
		{[]string{"foo", "test", "--opt", "-opt-value", "arg1", "arg2"}, []string{"arg1", "arg2"}, "-opt-value"},
		{[]string{"foo", "test", "arg1", "--opt", "-opt-value"}, []string{"arg1"}, "-opt-value"},
		{[]string{"foo", "test", "arg1", "--opt", "-opt-value", "arg2"}, []string{"arg1", "arg2"}, "-opt-value"},
		
		{[]string{"foo", "test", "--opt", "--opt-value"}, []string{}, "--opt-value"},
		{[]string{"foo", "test", "--opt", "--opt-value", "arg1"}, []string{"arg1"}, "--opt-value"},
		{[]string{"foo", "test", "--opt", "--opt-value", "arg1", "arg2"}, []string{"arg1", "arg2"}, "--opt-value"},
		{[]string{"foo", "test", "arg1", "--opt", "--opt-value"}, []string{"arg1"}, "-opt-value"},
		{[]string{"foo", "test", "arg1", "--opt", "--opt-value", "arg2"}, []string{"arg1", "arg2"}, "--opt-value"},
	}

	for _, tc := range cases {
		tc := tc
		t.Run(strings.Join(tc.testArgs, "_"), func(t *testing.T){
			var (
				args []string
				opt string
			)

			cmd := Command{
				Name:        "test",
				Usage:       "this is for testing",
				Description: "testing",
				Action: func(c *Context) error {
					args = c.Args()
					opt = c.String("opt")
					return nil
				},
				Flags: []Flag{StringFlag{
					Name:  "opt",
					Usage: "opt; set '-1' to disable",
				}},
			}

			app := NewApp()
			app.Writer = ioutil.Discard
			app.ErrWriter = ioutil.Discard
			app.Commands = []Command{cmd}

			err := app.Run(tc.testArgs)

			expect(t, err, nil)
			expect(t, args, tc.expectedArgs)
			expect(t, opt, tc.expectedOpt)
		})

	}
}

(I'll open a "draft" PR with the test case)

[thaJeztah]

opened #1135 as a draft; I'll try to do a git-bisect and/or look at a fix, but not sure yet if I find time (thought the test-case would be a start to assist in this); anyone else: feel free to pick up the work if I don't come round to it

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[AkihiroSuda]

no stale

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[stale]

This issue or PR has been automatically marked as stale because it has not had recent activity. Please add a comment bumping this if you're still interested in it's resolution! Thanks for your help, please let us know if you need anything else.

[AkihiroSuda]

no stale

[stale]

This issue or PR has been bumped and is no longer marked as stale! Feel free to bump it again in the future, if it's still relevant.

[thaJeztah]

Arf, trying to find time to work on this again, sorry for it taking so long

[coilysiren]

👀

[brandond]

Would love to see a fix for this one.

[AkihiroSuda]

There is a PR, but not merged yet #1135

[kolyshkin]

This is still the case with 1.22.5.

[thockin]

Is this library still alive?

[rliebz]

Is this library still alive?

Yes, but unfortunately I and other maintainers have not been able to dedicate as much time as we would like to. That said, definitely interested in keeping this alive, especially for bug fixes.

From the linked PR, it looks like there are issues with the proposed fix that haven't been addressed. I'm not sure what the best path forward is from here.

[thaJeztah]

From the linked PR, it looks like there are issues with the proposed fix that haven't been addressed. I'm not sure what the best path forward is from here.

Apologies for that; it had been dropping down my list to spend time on diving into it again, and not sure when I'm able to. I will try to find some time for it, but if someone wants to carry my changes from #1135, feel free to do so (feel free to take my patch as a starting point if that helps)

[meatballhat]

IIUC this should now be resolved via #1356 Please holler if that's not true!