Could use extra eyes on this change; it's a result of the latest commit / fix, but could potentially be a breaking change for users (so at least should be called out in the release notes); I think I interpreted the POSIX spec correctly with my patch, but specs can sometimes be ambiguous, so could use the extra eyes.

I also haven't checked the v2 branch/version yet (possibly the fix is needed there as well, or in another form)

So far, the twitter-verse seems to consolidate to my interpretation being correct 😂 https://twitter.com/thaJeztah/status/1260921988102656002?s=20

I'm having a hard time imagining if this is [ a mundane change, or if it'll cause mass chaos for somebody somewhere ]. Is there a way to keep this behavior as-is???

This is still not quite correct if you wanted to do something like:

% runc run --bundle --some-real-runc-flag ctr

Where it would still re-order things. The least-ugly way I can think of solving this is that we change nextIndexMayContainValue to make use of TakesValue() (with some interface casting) and removing this "skip reordering flags that start with -" logic if it doesn't break anything.

I also hasten to point out that urfave/cli has actually had this behaviour for a very long time and we (speaking collectively in the containers community) have misunderstood how urfave/cli handles this case. I discussed this in #1152 (which I've marked as a duplicate of this) and I've had to work around this in umoci (see opencontainers/umoci#328 for how I ended up hotfixing this -- you just disable argument reordering). Since we want to avoid breaking other users of urfave/cli we might have to migrate runc, umoci, and containerd away from urfave/cli (maybe to spf13/cobra which is what Docker uses)...

A possible patch to do the above might look like the following. This change does pass all of the existing tests...

diff --git a/command.go b/command.go
index f02d3589ff3a..3c8e971a1c53 100644
--- a/command.go
+++ b/command.go
@@ -223,7 +223,7 @@ func (c *Command) useShortOptionHandling() bool {
 func reorderArgs(commandFlags []Flag, args []string) []string {
        var remainingArgs, reorderedArgs []string
 
-       nextIndexMayContainValue := false
+       nextIndexIsValue := false
        for i, arg := range args {
 
                // dont reorder any args after a --
@@ -233,17 +233,29 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
                        remainingArgs = append(remainingArgs, args[i:]...)
                        break
 
-                       // checks if this arg is a value that should be re-ordered next to its associated flag
-               } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") {
-                       nextIndexMayContainValue = false
+                       // if this argument is a value then it isn't a flag and should be
+                       // kept with its associated flag (which was the last flag
+                       // reordered)
+               } else if nextIndexIsValue {
+                       nextIndexIsValue = false
                        reorderedArgs = append(reorderedArgs, arg)
 
                        // checks if this is an arg that should be re-ordered
-               } else if argIsFlag(commandFlags, arg) {
+               } else if flag := findFlagForArg(commandFlags, arg); flag != nil {
                        // we have determined that this is a flag that we should re-order
                        reorderedArgs = append(reorderedArgs, arg)
-                       // if this arg does not contain a "=", then the next index may contain the value for this flag
-                       nextIndexMayContainValue = !strings.Contains(arg, "=")
+
+                       // if the flag takes an argument and doesn't contain a "=" then the
+                       // next index contains a value for this flag -- unfortunately we
+                       // have to depend on the Flag implementing DocGenerationFlag for
+                       // TakesValue().
+                       if docFlag, ok := (*flag).(DocGenerationFlag); ok {
+                               nextIndexIsValue = docFlag.TakesValue() && !strings.Contains(arg, "=")
+                       } else {
+                               // assume that flags take arguments
+                               // XXX: This is quite ugly and I'm not sure what we should do here.
+                               nextIndexIsValue = !strings.Contains(arg, "=")
+                       }
 
                        // simply append any remaining args
                } else {
@@ -254,15 +266,16 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
        return append(reorderedArgs, remainingArgs...)
 }
 
-// argIsFlag checks if an arg is one of our command flags
-func argIsFlag(commandFlags []Flag, arg string) bool {
+// findFlagForArg checks if an arg is one of our command flags and returns the
+// Flag if the arg is a flag (nil otherwise).
+func findFlagForArg(commandFlags []Flag, arg string) *Flag {
        // checks if this is just a `-`, and so definitely not a flag
        if arg == "-" {
-               return false
+               return nil
        }
        // flags always start with a -
        if !strings.HasPrefix(arg, "-") {
-               return false
+               return nil
        }
        // this line turns `--flag` into `flag`
        if strings.HasPrefix(arg, "--") {
@@ -279,12 +292,12 @@ func argIsFlag(commandFlags []Flag, arg string) bool {
                for _, key := range strings.Split(flag.GetName(), ",") {
                        key := strings.TrimSpace(key)
                        if key == arg {
-                               return true
+                               return &flag
                        }
                }
        }
-       // return false if this arg was not one of our flags
-       return false
+       // couldn't find the flag
+       return nil
 }
 
 // Names returns the names including short names and aliases.

@cyphar I see you commented on the first commit; did you check with the second commit as well?

(I was looking at the requested changes to preserve some of the existing behaviour)

No, this was just talking about the behaviour of the first change (and not the fun -- semantics that you quizzed everyone on in May). I /think/ that it should be fairly straight-forward (move the -- check after the "is this an option-value check" as in your current patch) but I agree with both of you that that particular change looks quite scary.

I think the --opt -a case is far more clear-cut.

So, in your example case (runc run --bundle --some-real-runc-flag) both --bundle and --some-real-runc-flag are "known" flags to runc, but the second one should be treated as value for --bundle (so --bundle="--some-real-runc-flag") or as flag? In the former case, wouldn't it have to be used as runc run --bundle -- --some-real-runc-flag? 🤔)

The former (--some-real-runc-flag treated as a avalue for --bundle). The motivating use-case is (aside from POSIX-correctness and compatibility with getopt(3)) similar to --:

% runc run --bundle --foo ctr # where ./--foo is a directory containing an OCI bundle

(Well, for umoci it's more like umoci config --image foo:bar --config.cmd sh --config.cmd -c but that's a slightly more complicated example. But the root cause is the same as your original --memory-limit -1 issue.)

Should mean that --foo is a value for --bundle regardless of whether --foo is a known command-line flag. Currently (and with your patches), urfave/cli will not reorder --bundle --foo (where --foo is not a valid flag for runc) but it will reorder the flags if you do --bundle --systemd-cgroup (where --systemd-cgroup is a supported flag for runc). Even more strangely, --bundle=--systemd-cgroup works without issue because the arguments don't get reordered.

The patch I posted above should make it so that --bundle=--foo acts exactly like --bundle --foo regardless of whether --foo is a valid runc flag.

In the former case, wouldn't it have to be used as runc run --bundle -- --some-real-runc-flag?

I disagree (especially if we take the interpretation that -- should be an option-value in that case). If you use getopt:

% getopt -o '' -l bundle:,systemd-cgroup: -- --bundle --systemd-cgroup container-name
 --bundle '--systemd-cgroup' -- 'container-name'

But also it's the obvious behaviour if you just inject =s:

 getopt -o '' -l bundle:,systemd-cgroup: -- --bundle=--systemd-cgroup container-name
 --bundle '--systemd-cgroup' -- 'container-name'

@thaJeztah PTAL?