magefile / mage

a Make/rake-like dev tool using Go
https://magefile.org
Apache License 2.0
4.01k stars 250 forks source link

Bug: Errors aren't created with %w, breaking errors.Is #503

Open rkennedy opened 2 months ago

rkennedy commented 2 months ago

Bug Description

In one of my build targets, I use sh.Output to check the output of a command. For example:

output, err := sh.Output("command", "--version")

But in my case, it's completely normal for "command" not to exist sometimes, so I want to handle that error differently from other failures that sh.Output might return. When the command doesn't exist, the underlying os/exec code will return an fs.PathError, and the idiomatic way to check for that is to use errors.Is:

if err != nil {
    if !errors.Is(err, fs.ErrNotExist) {
        // This is really an error.
        return err
    }
    // File-doesn't-exist is an expected condition that we can handle.
}

But errors.Is returns false for errors returned by sh.Output.

What did you do?

As described above, I tried to use errors.Is to inspect errors returned by sh.Output and other command-executing functions from sh.

What did you expect to happen?

I expected errors.Is to recognize errors from sh functions as any of various common system errors, particularly fs.ErrNotExist.

What actually happened?

errors.Is fails to acknowledge that any sh error is like any other known error.

Environment

Additional context

This happens because errors in sh are constructed using fmt.Errorf("...%v", err). That inserts the value of err.String() but does not actually wrap the error, so any additional information about err is discarded. Changing it to fmt.Errorf("...%w", err) would solve it without affecting any other behavior. Error wrapping was introduced in Go 1.13.

My workaround for this issue is to inspect the text of the error message instead:

if err != nil {
    if !strings.Contains(err.Error(), "no such file or directory") {
        // This is really an error
        return err
    }
    // File-doesn't-exist is an expected condition that we can handle.
}