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

flag: exit 0 for default -h/-help option #37533

Closed
sding3 opened this issue Feb 28, 2020 · 29 comments
Closed

flag: exit 0 for default -h/-help option #37533

sding3 opened this issue Feb 28, 2020 · 29 comments

Comments

@sding3
Copy link
Contributor

sding3 commented Feb 28, 2020

When the -help or -h flags are undefined and invoked, the flag package handles this situation as a special case and prints a nice and helpful default help text, but exit the process with exit code 2. This proposal proposes the exit code be 0 by default and configurable for this specific case.

Emphasis: this proposal does not propose/incur any changes to programs which has -h or -help defined.


As a concrete example, using gofmt, which uses the flag package and does not have -help or -h defined, the behavior today is:

$ gofmt -help
usage: gofmt [flags] [path ...]
  -cpuprofile string
        write cpu profile to this file
<...abbreviated...>
$ echo $?
2

The proposed behavior is:

$ gofmt -help
usage: gofmt [flags] [path ...]
  -cpuprofile string
        write cpu profile to this file
<...abbreviated...>
$ echo $?
0
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/221427 mentions this issue: flag: exit 0 when -h or -help invoked but undefined

@sding3 sding3 changed the title flag: improve default exit handling of -help/-h proposal: flag: improve default exit handling of -help/-h Feb 28, 2020
@gopherbot gopherbot added this to the Proposal milestone Feb 28, 2020
@ianlancetaylor ianlancetaylor changed the title proposal: flag: improve default exit handling of -help/-h proposal: flag: exit 0 for default -h/-help option Feb 28, 2020
@bitfield
Copy link

bitfield commented Mar 1, 2020

If a program is run with an undefined flag, that's an error, because it's being invoked incorrectly. Unless we report this as a non-zero error status, the error will never be detected.

On the other hand, if you explicitly ask for help, using a defined -help or -h flag, or any other flag, that's fine. That's a zero exit status situation, because you invoked the program with a valid flag and it did what you asked. But you explicitly say that your proposal excludes that situation.

@sding3
Copy link
Contributor Author

sding3 commented Mar 1, 2020

@bitfield , reading your second paragraph, I think there is a misunderstanding. What I mean is that if the flags -h or -help are defined, then the flag package's treatment of such flags do not change from existing behavior.

The flag package already handles -h and -help, when undefined, as a special case by printing out a help message, which is helpful for the general use case. This proposal only suggests for this case and this case only, which is already handled as a special case in flag.go, to exit with 0 instead of 2.

Maybe if you run gofmt -help, which uses flag, and check the exit code you'll see.

@bitfield
Copy link

bitfield commented Mar 2, 2020

No, we understand one another correctly, I think. We agree that if the program defines the flag -h, then calling the program with it is not an error.

However, if the program does not define -h, then the current behaviour of flag is that this is an error. You are suggesting changing this. I am saying the current behaviour is correct. Calling a program with an undefined command-line flag is calling the program incorrectly.

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

/cc @robpike

@rsc
Copy link
Contributor

rsc commented Mar 4, 2020

The problem here is that although we recognize -h and -help in flag, all that it does is avoid printing "unrecognized flag: -help". Then it calls f.usage, which is user-defined. And essentially all the usage functions in existence do os.Exit(2). I guess those would keep exiting 2. The main flag.Parse loop would see an ErrHelp and I suppose it could treat that case, provided the error handling is set to ExitOnError, as os.Exit(0) instead of os.Exit(2).

It's a bit of a corner case. Do all the other getopt/argparse/etc agree to exit 0 on -help, or is there variation?

@as
Copy link
Contributor

as commented Mar 4, 2020

I don't think it's a good idea to have specific behaviors for -h and -help. There are instances where -h does not mean -help in some command line tools, and this change can possibly modify the behavior of scripts that use command line tools built in Go.

Ignoring the point above, what is the merit, or usefulness, of homogeneously returning 0 when -h is present? What value does it add to tool building or automation?

@sding3
Copy link
Contributor Author

sding3 commented Mar 5, 2020

@as , this proposal does not propose any changes to go programs that have the -h or -help flags defined. Further, this proposal does not propose homogeneously returning 0 when -h is present. Sorry if this wasn't clear from the proposal description. I've added a note in the description above to emphasize this point.

This proposal is useful for programs, which do not have the -h or -help flag defined, and want/need the default usage printing behavior and a zero exit code when such flags are provided. This proposal makes this much easier for these programs (easier, as they basically get it for free by default), yet without changing anything for programs who do have the the -h or -help flag defined.

@sding3
Copy link
Contributor Author

sding3 commented Mar 5, 2020

@rsc, yes, it already is a corner case before this proposal. flag.go has special handling for h and help.

Looking at argparse for python3 and getopt for GNU C, and both of them exit 0 when programs don't explicitly defined a help flag.

GNU C argp

#include <stdlib.h>
#include <argp.h>

const char *argp_program_version =
  "argp-ex2 1.0";
const char *argp_program_bug_address =
  "<bug-gnu-utils@gnu.org>";

static char doc[] =
  "Argp example #2 -- a pretty minimal program using argp";

static struct argp argp = { 0, 0, 0, doc };

int
main (int argc, char **argv)
{
  argp_parse (&argp, argc, argv, 0, 0, 0);
  exit (0);
}

Produces:

$ ./a.out --help
Usage: a.out [OPTION...]
Argp example #2 -- a pretty minimal program using argp

  -?, --help                 Give this help list
      --usage                Give a short usage message
  -V, --version              Print program version

Report bugs to <bug-gnu-utils@gnu.org>.

$ echo $?
0

python3 argparse

import argparse

parser = argparse.ArgumentParser(description='Process some integers.')
parser.add_argument('integers', metavar='N', type=int, nargs='+',
                    help='an integer for the accumulator')
parser.add_argument('--sum', dest='accumulate', action='store_const',
                    const=sum, default=max,
                    help='sum the integers (default: find the max)')

args = parser.parse_args()
print(args.accumulate(args.integers))

Produces:

$ ./flag.py -h
usage: flag.py [-h] [--sum] N [N ...]

Process some integers.

positional arguments:
  N           an integer for the accumulator

optional arguments:
  -h, --help  show this help message and exit
  --sum       sum the integers (default: find the max)

$ echo $?
0

@mattn
Copy link
Member

mattn commented Mar 5, 2020

The exit code of usage is depend on whether the "help" or "-h" is defined for the application.

$ go --help 2> /dev/null
$ echo $?
2

$ go -help 2> /dev/null
$ echo $?
2

$ go -h 2> /dev/null
$ echo $?
2

$ go help > /dev/null
$ echo $?
0

argp or argparse provide "-h" or "--help" automatically. If you want to get exit code 0 with -help, you should define "-h" your self, i think.

@rsc
Copy link
Contributor

rsc commented Mar 11, 2020

@sding3, thanks for checking GNU C argp and Python argparse. @ianlancetaylor says that GNU getopt also exits 0 for help. The only one that I'd want to check that's left is BSD getopt, but really it doesn't have any concept of -h or --help at all. I guess most BSD getopt-using programs just treat -h the same as any other unrecognized option. For example, on a Mac:

$ ls --help
ls: illegal option -- -
usage: ls [-@ABCFGHLOPRSTUWabcdefghiklmnopqrstuwx1%] [file ...]
$ echo $?
1
$

I guess if we're going to go to the trouble of implementing -h and --help as a special case, it would make sense to match all the others.

Does anyone see any argument for not changing the exit status of these to match all other languages we have checked? Thanks.

@rsc
Copy link
Contributor

rsc commented Mar 25, 2020

Based on the discussion above and the lack of objections raised since my last comment, this seems like a likely accept.

@rsc
Copy link
Contributor

rsc commented Apr 1, 2020

No change in consensus, so accepted.

@rsc rsc changed the title proposal: flag: exit 0 for default -h/-help option flag: exit 0 for default -h/-help option Apr 1, 2020
@rsc rsc modified the milestones: Proposal, Backlog Apr 1, 2020
@gopherbot
Copy link
Contributor

Change https://golang.org/cl/227178 mentions this issue: flag: skip TestExitCode on Plan 9

@ianlancetaylor
Copy link
Contributor

@ret394 Dropping the handling of -h and -help in the flag package would be a different proposal. You're welcome to make that one. This isn't the best place to discuss it.

That said, note that I don't think these options impose anything on anyone. Any program can define -h and/or -help itself.

@frankbraun
Copy link

frankbraun commented Aug 11, 2020

Too bad I only see this now, I tripped over it when reading the Go 1.15 release notes.

To me it seems you just changed the command-line API of all Go programs using the flag package with the default set of command-line flags in subtle ways (given that -h and -help are undefined). For example, if you were using such a program in a shell script with any undefined flag (including an undefined -h or -help) it produced an exit code 2. Now suddenly some undefined flags produce an exit code 0.

To recreate old behavior one would now have to switch to a custom flag-set with ContinueOnError that handles flag.ErrHelp as before.

For me that breaks the Go 1 Compatibility Expectations. It wasn't a security issue, unspecified behavior, a specification error, or a bug. It was clearly specified behavior of the standard library that now has been changed in subtle ways for, in my opinion, no good reason. If one wanted the new behavior before it was always possible with the means of the flag package.

For me one of the biggest advantages of Go is that it doesn't change all the time breaking things that used to work before. Introducing subtle changes is even worse than breaking things, because it's so hard to detect.

Like I said, this changes the CLI API of a lot of Go programs out there in the wild. I would even go so far to say this should be flagged as a bug and be reverted for Go 1.15.1.

@ianlancetaylor
Copy link
Contributor

@frankbraun Can you show an example where this new behavior will cause a problem?

The flag package never really defined the behavior of -h and -help (and it still doesn't). Those options already did not act like other undefined options. For an undefined option other than -h or -help the flag package will by default print "flag provided but not defined", but for -h and -help it print that.

@frankbraun
Copy link

frankbraun commented Aug 12, 2020

The default behavior is ExitOnError:

var CommandLine = NewFlagSet(os.Args[0], ExitOnError)

And:

ErrHelp is the error returned if the -help or -h flag is invoked but no such flag is defined.

var ErrHelp = errors.New("flag: help requested")

So before the change the behavior was

       ExitOnError // Call os.Exit(2).

and now it's:

       ExitOnError // Call os.Exit(2) or for -h/-help Exit(0).

Never mind what's printed on stderr (and hasn't been specified). I'm talking about exit codes here which are part of the API of a CLI program.

So let's say I have a CLI program (called example-tool) with two options -foo and -bar:

package main

import (
	"flag"
	"fmt"
)

func main() {
	foo := flag.Bool("foo", false, "Foo option")
	bar := flag.Bool("bar", false, "Bar option")
	flag.Parse()
	// do something with foo and bar options
	if *foo {
		fmt.Println("foo")
	} else if *bar {
		fmt.Println("bar")
	} else {
		fmt.Println("default")
	}
}

And a shell script example.sh that calls it:

#!/bin/sh

if [ $# -ne 1 ]
then
  echo "Usage: $0 switch" >&2
  exit 1
fi

example-tool -$1 2> /dev/null
if [ $? -ne 0 ]
then
  echo "undefined flag"
  exit 1
else
  echo "success"
fi

In Go 1.14 calling example.sh h leads to:

$ ./example.sh h          
undefined flag

And now in Go 1.15 calling example.sh h leads to:

$ ./example.sh
success

So suddenly my example-tool doesn't give me an error code anymore for an option I never defined. But maybe my script depends on that. I only wanted to process valid flags.

So not only did this change break the Go 1.0 Compatibility Expectations (by changing specified behaviour), it also silently changed the CLI API (in terms of error codes, never mind undefined stderr output) of a lot of Go programs.

@ianlancetaylor Does that clarify it?

@ianlancetaylor
Copy link
Contributor

I'm sorry, I should have been more clear.

Can you show me an example of an existing program or script where this new behavior will cause a problem?

@frankbraun
Copy link

frankbraun commented Aug 12, 2020

No, I haven't seen anything in the wild yet, Go 1.15 was just released.

Do you get my point though that this breaks existing CLI APIs and violates the Go 1.0 Compatibility Expectations?

IMHO @rsc shouldn't have put it up for a "consensus discussion", it's an API change that's not cool given the compatibility promise. I know I'm a bit nit-picky here, but this really ticked me off. I have decommissioned all third party Go option parsers for exactly the reason that they keep changing and keep breaking in subtle ways (apart from the fact that the amount of dependencies some of these parsers pull in is just crazy). I would really like if unnecessary stdlib changes don't suddenly change the behavior of my programs.

@ianlancetaylor
Copy link
Contributor

I do get your point that it changes the CLI behavior.

It's not obvious to me that this violates https://golang.org/doc/go1compat. As I said above, the behavior of -h and -help was never really specified, and it still isn't really specified. You've disregarded the fact that the standard error output is different for -h and -help, but I don't see why. Why is the exit status more relevant than the standard error output? Both are program behaviors.

I agree that it's a grey area. But the point of https://golang.org/doc/go1compat is not that we can't change anything in the existing libraries. It's that we won't break existing programs. So for areas like this that seem to me to be grey, I think it's very relevant to ask for examples of programs that break.

I'm sorry this ticked you off.

@artob
Copy link

artob commented Aug 12, 2020

It's not obvious to me that this violates https://golang.org/doc/go1compat. As I said above, the behavior of -h and -help was never really specified, and it still isn't really specified. You've disregarded the fact that the standard error output is different for -h and -help, but I don't see why. Why is the exit status more relevant than the standard error output? Both are program behaviors.

Changes to exit codes will affect Makefiles and Docker builds, as a couple of trivial examples, whereas output on standard error won't.

@frankbraun
Copy link

It's not obvious to me that this violates https://golang.org/doc/go1compat. As I said above, the behavior of -h and -help was never really specified, and it still isn't really specified. You've disregarded the fact that the standard error output is different for -h and -help, but I don't see why. Why is the exit status more relevant than the standard error output? Both are program behaviors.

As I wrote in my second message it was clearly specified in the documentation of flag that using an undefined -h returns an ErrHelp and that with ExitOnError that leads to an os.Exit(2).
There is no gray area there, it clearly specified what it did.

The stderr output difference for -h and -help compared to other options wasn't specified and it doesn't affect error codes. Much less likely that anyone depends on that. I still wouldn't change it, but rather document it.

I agree that it's a grey area. But the point of https://golang.org/doc/go1compat is not that we can't change anything in the existing libraries. It's that we won't break existing programs. So for areas like this that seem to me to be grey, I think it's very relevant to ask for examples of programs that break.

You won't know if this breaks existing programs, it might be deeply buried in some shell script calls.
And then you recompile the binary and suddenly it behaves differently.

In my opinion this sets a dangerous precedent. It also was the correct behavior in the first place: You call a binary with a non existing flag and then you get an error code.

@frankbraun
Copy link

I would suggest don't upgrade to Go 1.15 unless required.

For me the whole point of https://golang.org/doc/go1compat is that I can just upgrade and things generally get better. Not that behavior changes.

Reverting this change would be a real waste of time. The trade-off is worth it in the long run. If all OSS tools could just agree on good conventions, that's really great. Eventually all tools will standardize on more things.

It's nice to have improvements in a general direction. As far as I can tell, this behavior can be overridden if needed in Go apps, doesn't it?

It's a total waste of time if I have to comb through all release notes to check for subtle library behavior changes and then change existing code to stick to old behavior. It's a massive advantage of Go that things keep working as they did before and generally just get better and faster. There are very few modern programming languages where you can take code that was written 8 years ago and just compile and run it successfully with the latest compiler.

@rsc
Copy link
Contributor

rsc commented Aug 12, 2020

Two small points.

  1. We do make changes for bug fixes, and that leads to behavior changes. Bugs are explicitly called out as fixable. This was a bug in that it differed from all other implementations in other languages.
  2. If this causes a significant problem in a real case (I've only seen hypotheticals above), then we will examine that case and decide whether the new behavior is worse than the old. If you do see a case like that, please file a new bug reporting it.

Thanks!

@frankbraun
Copy link

Thanks for the reply. I rest my case until I have more hard evidence. Hopefully I don't have to open a bug report ;)

@twm
Copy link

twm commented Sep 3, 2020

Since you are seeking hard evidence, I'll note that this did cause a test failure for a CLI tool I maintain. The test in question expected an exit status of 2 when passed the -h flag. I don't consider the change in behavior a problem, though. It took much more time to read this thread than update the test expectation.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

11 participants