diff --git a/internal/run.go b/internal/run.go index 79b4f049..b5d13958 100644 --- a/internal/run.go +++ b/internal/run.go @@ -9,6 +9,8 @@ import ( "os/exec" "runtime" "strings" + + "github.com/magefile/mage/mg" ) var debug *log.Logger = log.New(ioutil.Discard, "", 0) @@ -52,7 +54,7 @@ func OutputDebug(cmd string, args ...string) (string, error) { if err := c.Run(); err != nil { errMsg := strings.TrimSpace(errbuf.String()) debug.Print("error running '", cmd, strings.Join(args, " "), "': ", err, ": ", errMsg) - return "", fmt.Errorf("error running \"%s %s\": %s\n%s", cmd, strings.Join(args, " "), err, errMsg) + return "", mg.WrapError(err, fmt.Errorf("error running \"%s %s\": %v\n%s", cmd, strings.Join(args, " "), err, errMsg)) } return strings.TrimSpace(buf.String()), nil } diff --git a/mage/main.go b/mage/main.go index 0062bd35..31ef28db 100644 --- a/mage/main.go +++ b/mage/main.go @@ -473,14 +473,14 @@ func listGoFiles(magePath, goCmd, tag string, envStr []string) ([]string, error) if !filepath.IsAbs(magePath) { cwd, err := os.Getwd() if err != nil { - return nil, fmt.Errorf("can't get current working directory: %v", err) + return nil, mg.WrapError(err, fmt.Errorf("can't get current working directory: %v", err)) } magePath = filepath.Join(cwd, magePath) } env, err := internal.SplitEnv(envStr) if err != nil { - return nil, fmt.Errorf("error parsing environment variables: %v", err) + return nil, mg.WrapError(err, fmt.Errorf("error parsing environment variables: %v", err)) } bctx := build.Default @@ -502,7 +502,7 @@ func listGoFiles(magePath, goCmd, tag string, envStr []string) ([]string, error) // Allow multiple packages in the same directory if _, ok := err.(*build.MultiplePackageError); !ok { - return nil, fmt.Errorf("failed to parse go source files: %v", err) + return nil, mg.WrapError(err, fmt.Errorf("failed to parse go source files: %v", err)) } } @@ -530,7 +530,7 @@ func Magefiles(magePath, goos, goarch, goCmd string, stderr io.Writer, isMagefil debug.Println("getting all files including those with mage tag in", magePath) mageFiles, err := listGoFiles(magePath, goCmd, "mage", env) if err != nil { - return nil, fmt.Errorf("listing mage files: %v", err) + return nil, mg.WrapError(err, fmt.Errorf("listing mage files: %v", err)) } if isMagefilesDirectory { @@ -546,7 +546,7 @@ func Magefiles(magePath, goos, goarch, goCmd string, stderr io.Writer, isMagefil debug.Println("getting all files without mage tag in", magePath) nonMageFiles, err := listGoFiles(magePath, goCmd, "", env) if err != nil { - return nil, fmt.Errorf("listing non-mage files: %v", err) + return nil, mg.WrapError(err, fmt.Errorf("listing non-mage files: %v", err)) } // convert non-Mage list to a map of files to exclude. @@ -612,7 +612,7 @@ func GenerateMainfile(binaryName, path string, info *parse.PkgInfo) error { f, err := os.Create(path) if err != nil { - return fmt.Errorf("error creating generated mainfile: %v", err) + return mg.WrapError(err, fmt.Errorf("error creating generated mainfile: %v", err)) } defer f.Close() data := mainfileTemplateData{ @@ -629,16 +629,16 @@ func GenerateMainfile(binaryName, path string, info *parse.PkgInfo) error { debug.Println("writing new file at", path) if err := mainfileTemplate.Execute(f, data); err != nil { - return fmt.Errorf("can't execute mainfile template: %v", err) + return mg.WrapError(err, fmt.Errorf("can't execute mainfile template: %v", err)) } if err := f.Close(); err != nil { - return fmt.Errorf("error closing generated mainfile: %v", err) + return mg.WrapError(err, fmt.Errorf("error closing generated mainfile: %v", err)) } // we set an old modtime on the generated mainfile so that the go tool // won't think it has changed more recently than the compiled binary. longAgo := time.Now().Add(-time.Hour * 24 * 365 * 10) if err := os.Chtimes(path, longAgo, longAgo); err != nil { - return fmt.Errorf("error setting old modtime on generated mainfile: %v", err) + return mg.WrapError(err, fmt.Errorf("error setting old modtime on generated mainfile: %v", err)) } return nil } @@ -675,13 +675,13 @@ func ExeName(goCmd, cacheDir string, files []string) (string, error) { func hashFile(fn string) (string, error) { f, err := os.Open(fn) if err != nil { - return "", fmt.Errorf("can't open input file for hashing: %#v", err) + return "", mg.WrapError(err, fmt.Errorf("can't open input file for hashing: %v", err)) } defer f.Close() h := sha1.New() if _, err := io.Copy(h, f); err != nil { - return "", fmt.Errorf("can't write data to hash: %v", err) + return "", mg.WrapError(err, fmt.Errorf("can't write data to hash: %v", err)) } return fmt.Sprintf("%x", h.Sum(nil)), nil } @@ -690,12 +690,12 @@ func generateInit(dir string) error { debug.Println("generating default magefile in", dir) f, err := os.Create(filepath.Join(dir, initFile)) if err != nil { - return fmt.Errorf("could not create mage template: %v", err) + return mg.WrapError(err, fmt.Errorf("could not create mage template: %v", err)) } defer f.Close() if err := initOutput.Execute(f, nil); err != nil { - return fmt.Errorf("can't execute magefile template: %v", err) + return mg.WrapError(err, fmt.Errorf("can't execute magefile template: %v", err)) } return nil diff --git a/mage/main_test.go b/mage/main_test.go index 07e598e5..8b9aa02b 100644 --- a/mage/main_test.go +++ b/mage/main_test.go @@ -6,6 +6,7 @@ import ( "debug/macho" "debug/pe" "encoding/hex" + "errors" "flag" "fmt" "go/build" @@ -1270,8 +1271,8 @@ func TestCompiledFlags(t *testing.T) { cmd.Stderr = stderr cmd.Stdout = stdout if err := cmd.Run(); err != nil { - return fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", - filename, strings.Join(args, " "), err, stdout, stderr) + return mg.WrapError(err, fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", + filename, strings.Join(args, " "), err, stdout, stderr)) } return nil } @@ -1315,6 +1316,10 @@ func TestCompiledFlags(t *testing.T) { if err == nil { t.Fatalf("expected an error because of timeout") } + var exitError *exec.ExitError + if !errors.As(err, &exitError) { + t.Errorf("Expected wrapped ExitError from process; got %#v", err) + } got = stderr.String() want = "context deadline exceeded" if strings.Contains(got, want) == false { @@ -1357,8 +1362,8 @@ func TestCompiledEnvironmentVars(t *testing.T) { cmd.Stderr = stderr cmd.Stdout = stdout if err := cmd.Run(); err != nil { - return fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", - filename, strings.Join(args, " "), err, stdout, stderr) + return mg.WrapError(err, fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", + filename, strings.Join(args, " "), err, stdout, stderr)) } return nil } @@ -1511,8 +1516,8 @@ func TestSignals(t *testing.T) { cmd.Stderr = stderr cmd.Stdout = stdout if err := cmd.Start(); err != nil { - return fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", - filename, target, err, stdout, stderr) + return mg.WrapError(err, fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", + filename, target, err, stdout, stderr)) } pid := cmd.Process.Pid go func() { @@ -1523,8 +1528,8 @@ func TestSignals(t *testing.T) { } }() if err := cmd.Wait(); err != nil { - return fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", - filename, target, err, stdout, stderr) + return mg.WrapError(err, fmt.Errorf("running '%s %s' failed with: %v\nstdout: %s\nstderr: %s", + filename, target, err, stdout, stderr)) } return nil } diff --git a/magefile.go b/magefile.go index c08ffa37..108e8e06 100644 --- a/magefile.go +++ b/magefile.go @@ -43,12 +43,12 @@ func Install() error { // in GOPATH environment string bin, err := sh.Output(gocmd, "env", "GOBIN") if err != nil { - return fmt.Errorf("can't determine GOBIN: %v", err) + return mg.WrapError(err, fmt.Errorf("can't determine GOBIN: %v", err)) } if bin == "" { gopath, err := sh.Output(gocmd, "env", "GOPATH") if err != nil { - return fmt.Errorf("can't determine GOPATH: %v", err) + return mg.WrapError(err, fmt.Errorf("can't determine GOPATH: %v", err)) } paths := strings.Split(gopath, string([]rune{os.PathListSeparator})) bin = filepath.Join(paths[0], "bin") @@ -56,7 +56,7 @@ func Install() error { // specifically don't mkdirall, if you have an invalid gopath in the first // place, that's not on us to fix. if err := os.Mkdir(bin, 0700); err != nil && !os.IsExist(err) { - return fmt.Errorf("failed to create %q: %v", bin, err) + return mg.WrapError(err, fmt.Errorf("failed to create %q: %v", bin, err)) } path := filepath.Join(bin, name) @@ -72,7 +72,7 @@ var releaseTag = regexp.MustCompile(`^v1\.[0-9]+\.[0-9]+$`) // Generates a new release. Expects a version tag in v1.x.x format. func Release(tag string) (err error) { if _, err := exec.LookPath("goreleaser"); err != nil { - return fmt.Errorf("can't find goreleaser: %w", err) + return mg.WrapError(err, fmt.Errorf("can't find goreleaser: %v", err)) } if !releaseTag.MatchString(tag) { return errors.New("TAG environment variable must be in semver v1.x.x format, but was " + tag) diff --git a/mg/errors.go b/mg/errors.go index 2dd780fe..9bf7fad4 100644 --- a/mg/errors.go +++ b/mg/errors.go @@ -49,3 +49,38 @@ func ExitStatus(err error) int { } return exit.ExitStatus() } + +// wrappedError is an error that supports the Go 1.13+ mechanism of wrapping +// errors. It implements Unwrap to return the underlying error, but it still +// returns the string version of whatever "main" error it represents. +type wrappedError struct { + underlyingError error + stringError error +} + +// WrapError returns an error that implements the Go 1.13 Unwrap interface. The +// Error function returns the value of the "string" error, and the Unwrap +// function returns the "underlying" error. Use this wherever one might +// otherwise use the "%w" format string in [fmt.Errorf]. +// err := doSomething() +// if err != nil { +// return WrapError(err, fmt.Errorf("Could not do something: %v", err)) +// } +// The premise is that the "string" error is not an interesting one to try to +// inspect with [errors.Is] or [errors.As] because it has no other wrapped +// errors of its own, and it will usually be of whatever error type +// `fmt.Errorf` returns. +func WrapError(underlying, str error) error { + return &wrappedError{ + underlyingError: underlying, + stringError: str, + } +} + +func (e *wrappedError) Error() string { + return e.stringError.Error() +} + +func (e *wrappedError) Unwrap() error { + return e.underlyingError +} diff --git a/mg/errors113_test.go b/mg/errors113_test.go new file mode 100644 index 00000000..2cdc0ff3 --- /dev/null +++ b/mg/errors113_test.go @@ -0,0 +1,24 @@ +// The concept of "wrapping" errors was only introduced in Go 1.13, so we only +// want to test that our errors behave like wrapped errors on Go versions that +// support `errors.Is`. +//go:build go1.13 +// +build go1.13 + +package mg + +import ( + "errors" + "testing" +) + +// TestErrorUnwrap checks that [errors.Is] can detect the underlying error in a +// wrappedError. +func TestErrorUnwrap(t *testing.T) { + strError := errors.New("main error") + underlyingError := errors.New("underlying error") + actual := WrapError(underlyingError, strError) + + if !errors.Is(actual, underlyingError) { + t.Fatalf("Expected outer error %#v to include %#v", actual, underlyingError) + } +} diff --git a/mg/errors_test.go b/mg/errors_test.go index ac5e68f6..880e82d9 100644 --- a/mg/errors_test.go +++ b/mg/errors_test.go @@ -1,6 +1,9 @@ package mg -import "testing" +import ( + "errors" + "testing" +) func TestFatalExit(t *testing.T) { expected := 99 @@ -17,3 +20,15 @@ func TestFatalfExit(t *testing.T) { t.Fatalf("Expected code %v but got %v", expected, code) } } + +// TestBasicWrappedError confirms that a wrappedError returns the same string +// as its "str" error (not its "underlying" error). +func TestBasicWrappedError(t *testing.T) { + strError := errors.New("main error") + underlyingError := errors.New("underlying error") + actual := WrapError(underlyingError, strError) + + if actual.Error() != strError.Error() { + t.Fatalf("Expected outer error to have Error() = %q but got %q", strError.Error(), actual.Error()) + } +} diff --git a/mg/fn.go b/mg/fn.go index 3856857a..b8aa5370 100644 --- a/mg/fn.go +++ b/mg/fn.go @@ -37,7 +37,7 @@ func F(target interface{}, args ...interface{}) Fn { } id, err := json.Marshal(args) if err != nil { - panic(fmt.Errorf("can't convert args into a mage-compatible id for mg.Deps: %s", err)) + panic(WrapError(err, fmt.Errorf("can't convert args into a mage-compatible id for mg.Deps: %v", err))) } return fn{ name: funcName(target), diff --git a/parse/parse.go b/parse/parse.go index 34216690..1ae56bac 100644 --- a/parse/parse.go +++ b/parse/parse.go @@ -15,6 +15,7 @@ import ( "time" "github.com/magefile/mage/internal" + "github.com/magefile/mage/mg" ) const importTag = "mage:import" @@ -735,7 +736,7 @@ func getPackage(path string, files []string, fset *token.FileSet) (*ast.Package, pkgs, err := parser.ParseDir(fset, path, filter, parser.ParseComments) if err != nil { - return nil, fmt.Errorf("failed to parse directory: %v", err) + return nil, mg.WrapError(err, fmt.Errorf("failed to parse directory: %v", err)) } switch len(pkgs) { diff --git a/sh/cmd.go b/sh/cmd.go index 312de65a..c2693d93 100644 --- a/sh/cmd.go +++ b/sh/cmd.go @@ -120,7 +120,7 @@ func Exec(env map[string]string, stdout, stderr io.Writer, cmd string, args ...s if ran { return ran, mg.Fatalf(code, `running "%s %s" failed with exit code %d`, cmd, strings.Join(args, " "), code) } - return ran, fmt.Errorf(`failed to run "%s %s: %v"`, cmd, strings.Join(args, " "), err) + return ran, mg.WrapError(err, fmt.Errorf(`failed to run "%s %s: %v"`, cmd, strings.Join(args, " "), err)) } func run(env map[string]string, stdout, stderr io.Writer, cmd string, args ...string) (ran bool, code int, err error) { diff --git a/sh/cmd113_test.go b/sh/cmd113_test.go new file mode 100644 index 00000000..23a201ce --- /dev/null +++ b/sh/cmd113_test.go @@ -0,0 +1,23 @@ +// The concept of "wrapping" errors was only introduced in Go 1.13, so we only +// want to test that our errors behave like wrapped errors on Go versions that +// support `errors.Is`. +//go:build go1.13 +// +build go1.13 + +package sh + +import ( + "errors" + "os" + "testing" +) + +func TestWrappedError(t *testing.T) { + _, err := Exec(nil, nil, nil, os.Args[0]+"-doesnotexist", "-printArgs", "foo") + if err == nil { + t.Fatalf("Expected to fail running %s", os.Args[0]+"-doesnotexist") + } + if !errors.Is(err, os.ErrNotExist) { + t.Fatalf("Expected error to be like ErrNotExist, got %#v", err) + } +} diff --git a/sh/helpers.go b/sh/helpers.go index f5d20a27..1e9542b9 100644 --- a/sh/helpers.go +++ b/sh/helpers.go @@ -4,6 +4,8 @@ import ( "fmt" "io" "os" + + "github.com/magefile/mage/mg" ) // Rm removes the given file or directory even if non-empty. It will not return @@ -13,28 +15,28 @@ func Rm(path string) error { if err == nil || os.IsNotExist(err) { return nil } - return fmt.Errorf(`failed to remove %s: %v`, path, err) + return mg.WrapError(err, fmt.Errorf(`failed to remove %s: %v`, path, err)) } // Copy robustly copies the source file to the destination, overwriting the destination if necessary. func Copy(dst string, src string) error { from, err := os.Open(src) if err != nil { - return fmt.Errorf(`can't copy %s: %v`, src, err) + return mg.WrapError(err, fmt.Errorf(`can't copy %s: %v`, src, err)) } defer from.Close() finfo, err := from.Stat() if err != nil { - return fmt.Errorf(`can't stat %s: %v`, src, err) + return mg.WrapError(err, fmt.Errorf(`can't stat %s: %v`, src, err)) } to, err := os.OpenFile(dst, os.O_CREATE|os.O_WRONLY|os.O_TRUNC, finfo.Mode()) if err != nil { - return fmt.Errorf(`can't copy to %s: %v`, dst, err) + return mg.WrapError(err, fmt.Errorf(`can't copy to %s: %v`, dst, err)) } defer to.Close() _, err = io.Copy(to, from) if err != nil { - return fmt.Errorf(`error copying %s to %s: %v`, src, dst, err) + return mg.WrapError(err, fmt.Errorf(`error copying %s to %s: %v`, src, dst, err)) } return nil } diff --git a/sh/helpers_test.go b/sh/helpers_test.go index 54e78aa1..698eba3f 100644 --- a/sh/helpers_test.go +++ b/sh/helpers_test.go @@ -8,6 +8,7 @@ import ( "path/filepath" "testing" + "github.com/magefile/mage/mg" "github.com/magefile/mage/sh" ) @@ -17,11 +18,11 @@ import ( func compareFiles(file1 string, file2 string) error { s1, err := os.Stat(file1) if err != nil { - return fmt.Errorf("can't stat %s: %v", file1, err) + return mg.WrapError(err, fmt.Errorf("can't stat %s: %v", file1, err)) } s2, err := os.Stat(file2) if err != nil { - return fmt.Errorf("can't stat %s: %v", file2, err) + return mg.WrapError(err, fmt.Errorf("can't stat %s: %v", file2, err)) } if s1.Size() != s2.Size() { return fmt.Errorf("files %s and %s have different sizes: %d vs %d", file1, file2, s1.Size(), s2.Size()) @@ -31,11 +32,11 @@ func compareFiles(file1 string, file2 string) error { } f1bytes, err := ioutil.ReadFile(file1) if err != nil { - return fmt.Errorf("can't read %s: %v", file1, err) + return mg.WrapError(err, fmt.Errorf("can't read %s: %v", file1, err)) } f2bytes, err := ioutil.ReadFile(file2) if err != nil { - return fmt.Errorf("can't read %s: %v", file2, err) + return mg.WrapError(err, fmt.Errorf("can't read %s: %v", file2, err)) } if !bytes.Equal(f1bytes, f2bytes) { return fmt.Errorf("files %s and %s have different contents", file1, file2)