cmd/cue: use stderr and non-zero exit for errors

Exit code is determined from returning an error or if
anything was printed to stderr.

Closes #101
Closes #100

Change-Id: I980798586d8a23d17a32c52d62da49376e7165ef
Reviewed-on: https://cue-review.googlesource.com/c/cue/+/3102
Reviewed-by: Marcel van Lohuizen <mpvl@google.com>

cmd/cue/cmd/add.go

@@ -195,7 +195,7 @@ type originalFile struct {
 func restoreOriginals(cmd *Command, originals []originalFile) {
 	for _, fo := range originals {
 		if err := fo.restore(); err != nil {
-			fmt.Fprintln(cmd.OutOrStderr(), "Error restoring file: ", err)
+			fmt.Fprintln(cmd.Stderr(), "Error restoring file: ", err)
 		}
 	}
 }

cmd/cue/cmd/cmd.go

@@ -211,12 +211,13 @@ An example using pipes:
 
 `,
 		RunE: mkRunE(c, func(cmd *Command, args []string) error {
+			w := cmd.Stderr()
 			if len(args) == 0 {
-				fmt.Println("cmd must be run as one of its subcommands")
+				fmt.Fprintln(w, "cmd must be run as one of its subcommands")
 			} else {
-				fmt.Printf("cmd must be run as one of its subcommands: unknown subcommand %q\n", args[0])
+				fmt.Fprintf(w, "cmd must be run as one of its subcommands: unknown subcommand %q\n", args[0])
 			}
-			fmt.Println("Run 'cue help cmd' for known subcommands.")
+			fmt.Fprintln(w, "Run 'cue help cmd' for known subcommands.")
 			os.Exit(1) // TODO: get rid of this
 			return nil
 		}),

cmd/cue/cmd/cmd_test.go

@@ -41,7 +41,7 @@ func TestCmd(t *testing.T) {
 		c := newRootCmd()
 		run := func(cmd *cobra.Command, args []string) error {
 			stdout = cmd.OutOrStdout()
-			stderr = cmd.OutOrStderr()
+			stderr = c.Stderr()
 
 			tools, _ := buildTools(c, args)
 			cmd, err := addCustom(c, c.root, "command", name, tools)

cmd/cue/cmd/common.go

@@ -74,7 +74,7 @@ func exitOnErr(cmd *Command, err error, fatal bool) {
 	})
 
 	b := w.Bytes()
-	_, _ = cmd.OutOrStderr().Write(b)
+	_, _ = cmd.Stderr().Write(b)
 	if fatal {
 		exit()
 	}
@@ -89,7 +89,6 @@ func buildFromArgs(cmd *Command, args []string) []*cue.Instance {
 }
 
 func loadFromArgs(cmd *Command, args []string) []*build.Instance {
-	log.SetOutput(cmd.OutOrStderr())
 	binst := load.Instances(args, nil)
 	if len(binst) == 0 {
 		return nil

cmd/cue/cmd/common_test.go

@@ -51,7 +51,6 @@ func printConfig(t *testing.T) *errors.Config {
 
 func runCommand(t *testing.T, cmd *cobra.Command, name string, args ...string) {
 	t.Helper()
-	log.SetFlags(0)
 
 	const dir = "./testdata"
 

cmd/cue/cmd/get.go

@@ -36,12 +36,13 @@ The specifics on how dependencies are fechted and converted vary
 per language and are documented in the respective subcommands.
 `,
 		RunE: mkRunE(c, func(cmd *Command, args []string) error {
+			stderr := cmd.Stderr()
 			if len(args) == 0 {
-				fmt.Println("get must be run as one of its subcommands")
+				fmt.Fprintln(stderr, "get must be run as one of its subcommands")
 			} else {
-				fmt.Printf("get must be run as one of its subcommands: unknown subcommand %q\n", args[0])
+				fmt.Fprintf(stderr, "get must be run as one of its subcommands: unknown subcommand %q\n", args[0])
 			}
-			fmt.Println("Run 'cue help get' for known subcommands.")
+			fmt.Fprintln(stderr, "Run 'cue help get' for known subcommands.")
 			os.Exit(1) // TODO: get rid of this
 			return nil
 		}),

cmd/cue/cmd/get_go.go

@@ -337,7 +337,7 @@ func extract(cmd *Command, args []string) error {
 
 	e := extractor{
 		cmd:    cmd,
-		stderr: cmd.OutOrStderr(),
+		stderr: cmd.Stderr(),
 		pkgs:   pkgs,
 		orig:   map[types.Type]*ast.StructType{},
 	}
@@ -474,8 +474,9 @@ func (e *extractor) extractPkg(root string, p *packages.Package) error {
 		b, err := format.Source(w.Bytes())
 		if err != nil {
 			_ = ioutil.WriteFile(filepath.Join(dir, file), w.Bytes(), 0644)
-			fmt.Println(w.String())
-			fmt.Println(dir, file)
+			stderr := e.cmd.Stderr()
+			fmt.Fprintln(stderr, w.String())
+			fmt.Fprintln(stderr, dir, file)
 			return err
 		}
 		err = ioutil.WriteFile(filepath.Join(dir, file), b, 0644)
@@ -894,7 +895,7 @@ func (e *extractor) printType(expr types.Type) {
 
 	case *types.Map:
 		if b, ok := x.Key().Underlying().(*types.Basic); !ok || b.Kind() != types.String {
-			log.Panicf("unsupported map key type %T", x.Key())
+			panic(fmt.Sprintf("unsupported map key type %T", x.Key()))
 		}
 		fmt.Fprintf(e.w, "{ <_>: ")
 		e.printType(x.Elem())

cmd/cue/cmd/get_go_test.go

@@ -42,7 +42,7 @@ func TestGetGo(t *testing.T) {
 	cmd.SetArgs([]string{"./testdata/code/go/..."})
 	err = cmd.Execute()
 	if err != nil {
-		log.Fatal(err)
+		t.Fatal(err)
 	}
 
 	// Packages will generate differently in modules versus GOPATH. Search

cmd/cue/cmd/import.go

@@ -251,8 +251,6 @@ func getExtInfo(ext string) *encodingInfo {
 }
 
 func runImport(cmd *Command, args []string) error {
-	log.SetOutput(cmd.OutOrStderr())
-
 	var group errgroup.Group
 
 	pkgFlag := flagPackage.String(cmd)
@@ -391,7 +389,10 @@ func combineExpressions(cmd *Command, pkg, cueFile string, objs ...ast.Expr) err
 		case os.IsNotExist(err):
 		case err == nil:
 			if !flagForce.Bool(cmd) {
-				log.Printf("skipping file %q: already exists", cueFile)
+				// TODO: mimic old behavior: write to stderr, but do not exit
+				// with error code. Consider what is best to do here.
+				stderr := cmd.Command.OutOrStderr()
+				fmt.Fprintf(stderr, "skipping file %q: already exists\n", cueFile)
 				return nil
 			}
 		default:

cmd/cue/cmd/root.go

@@ -18,7 +18,6 @@ import (
 	"context"
 	"fmt"
 	"io"
-	logger "log"
 	"os"
 	"strings"
 
@@ -39,8 +38,6 @@ import (
 // TODO: documentation of concepts
 //   tasks     the key element for cmd, serve, and fix
 
-var log = logger.New(os.Stderr, "", logger.Lshortfile)
-
 type runFunction func(cmd *Command, args []string) error
 
 func mkRunE(c *Command, f runFunction) func(*cobra.Command, []string) error {
@@ -125,8 +122,37 @@ type Command struct {
 
 	// Subcommands
 	cmd *cobra.Command
+
+	hasErr bool
+}
+
+type errWriter Command
+
+func (w *errWriter) Write(b []byte) (int, error) {
+	c := (*Command)(w)
+	c.hasErr = true
+	return c.Command.OutOrStderr().Write(b)
+}
+
+// Hint: search for uses of OutOrStderr other than the one here to see
+// which output does not trigger a non-zero exit code. os.Stderr may never
+// be used directly.
+
+// Stderr returns a writer that should be used for error messages.
+func (c *Command) Stderr() io.Writer {
+	return (*errWriter)(c)
 }
 
+// TODO: add something similar for Stdout. The output model of Cobra isn't
+// entirely clear, and such a change seems non-trivial.
+
+// Consider overriding these methods from Cobra using OutOrStdErr.
+// We don't use them currently, but may be safer to block. Having them
+// will encourage their usage, and the naming is inconsistent with other CUE APIs.
+// PrintErrf(format string, args ...interface{})
+// PrintErrln(args ...interface{})
+// PrintErr(args ...interface{})
+
 func (c *Command) SetOutput(w io.Writer) {
 	c.root.SetOutput(w)
 }
@@ -136,16 +162,24 @@ func (c *Command) SetInput(r io.Reader) {
 	stdin = r
 }
 
+// ErrPrintedError indicates error messages have been printed to stderr.
+var ErrPrintedError = errors.New("terminating because of errors")
+
 func (c *Command) Run(ctx context.Context) (err error) {
-	log.SetFlags(0)
 	// Three categories of commands:
 	// - normal
 	// - user defined
 	// - help
 	// For the latter two, we need to use the default loading.
 	defer recoverError(&err)
 
-	return c.root.Execute()
+	if err := c.root.Execute(); err != nil {
+		return err
+	}
+	if c.hasErr {
+		return ErrPrintedError
+	}
+	return nil
 }
 
 func recoverError(err *error) {
@@ -199,8 +233,9 @@ func New(args []string) (cmd *Command, err error) {
 	}
 	_, err = addCustom(cmd, rootCmd, commandSection, args[0], tools)
 	if err != nil {
-		fmt.Printf("command %s %q is not defined\n", commandSection, args[0])
-		fmt.Println("Run 'cue help' to show available commands.")
+		stderr := cmd.Stderr()
+		fmt.Fprintf(stderr, "command %s %q is not defined\n", commandSection, args[0])
+		fmt.Println(stderr, "Run 'cue help' to show available commands.")
 		os.Exit(1)
 	}
 	return cmd, nil

cmd/cue/cmd/vet.go

@@ -119,7 +119,7 @@ func doVet(cmd *Command, args []string) error {
 			cue.Optional(true),
 			cue.Hidden(true),
 		}
-		w := cmd.OutOrStderr()
+		w := cmd.Stderr()
 		err := inst.Value().Validate(append(opt, cue.Concrete(concrete))...)
 		if err != nil && !hasFlag {
 			err = inst.Value().Validate(append(opt, cue.Concrete(false))...)

cmd/cue/main.go

@@ -25,7 +25,9 @@ import (
 func main() {
 	err := cmd.Main(context.Background(), os.Args[1:])
 	if err != nil {
-		fmt.Println(err)
+		if err != cmd.ErrPrintedError {
+			fmt.Println(err)
+		}
 		os.Exit(1)
 	}
 }