Consider dropping dependency on github.com/pkg/errors

[imjasonh]

https://github.com/pkg/errors is deprecated, archived, and in maintenance mode, since Go errors natively support wrapping since Go 1.13.

There are about 200 files in this repo that depend on this package (excluding vendor/):

git grep -l github.com/pkg/errors | grep -v ^vendor | wc -l
     196

It should be a fairly mechanical change:

• errors.Wrap(err, "oh no") becomes fmt.Errorf("oh no: %w", err)
• errors.New("oh no") becomes errors.New("oh no"), using the stdlib errors package.
• errors.Errorf("oh %q", "no") becomes fmt.Errorf("oh %q", "no")

It may even be somewhat possible to automate, using x/tools/go/analysis (see sigstore/cosign#1887 (comment)).

Let me know if you'd be interested in considering a PR to make this change.

[thaJeztah]

Unfortunately, Go's native error-wrapping is not a full replacement for pkg/errors, as it doesn't preserve a stack, which limits its usefulness.

We are considering to propose maintainers for the https://github.com/pkg/errors package (pkg/errors#245 TBD); overall the package is "feature complete", so likely not much maintenance will be needed. /cc @tonistiigi

Was there a specific reason you want the package to be removed?

[imjasonh]

Was there a specific reason you want the package to be removed?

Nothing specific exactly, but two broad categories of reasons:

1. an unmaintained repo, even a "feature complete" one, presents a (small, theoretical) risk of having unaddressed vulnerabilities, compared to a well supported solution in Go's stdlib. I'm sure if there was a pkgerrors4shell-style vulnerability the original maintainers or the community would step up to fix it, but it would likely be much slower to seep out into the community than something in Go itself.

2. Go tooling isn't aware that errors.Errorf takes a format string, and that any %s in it must be paired with a later arg. errors.Errorf("oh %s") is considered perfectly valid, though it would result in oh %!s(MISSING). Compare this with fmt.Errorf("oh %s"), which will fail when exercised during tests to alert callers to missing args, or extra args, etc. This also trickles out into IDE support for detecting missing args.

In any case, it's not an urgent need, and your answer is perfectly satisfactory. Especially so if you take on maintainership for it, since that would go toward solving (1) above.

If you ever become interested in this, my offer to send a PR stands. 😄

[mitar]

Shameless plug: you can switch it to drop-in replacement gitlab.com/tozd/go/errors. It fixes many issues, is maintained, and supports modern Go's error patterns (sentinel errors, %w formatting, joined errors, etc.). It also provides some nice utility functions and structured details so that it is easy to extract dynamic data out of errors (instead of trying to get them out of formatted strings). Has improved error formatting and JSON marshaling of errors. It is interoperable with other errors packages and does not require a dependency on itself to extract data (e.g., stack trace) from errors.

BTW, go vet does complain on your example errors.Errorf("oh %s") for gitlab.com/tozd/go/errors.

[kolyshkin]

I concur that this needs to be removed, as the https://github.com/pkg/errors repository is now archived.

As for the stack trace functionality, I guess, we need to re-assess the need of. If there is a consensus that it is really needed, come up with a solution which explicitly uses Go runtime package to get those and embed into errors. We should consider that getting stack traces is somewhat expensive and in most cases should not really be required.

[thaJeztah]

There's a longer discussion here;

• decide on the future use of `errors`, `pkg/errors` and other errors moby/moby#46358

Currently BuildKit enforces the use of pkg/errors, as it's essential for some error handling, but some discussion is on going to consider moving this functionality into https://github.com/containerd/errdefs. w.r.t. the https://github.com/pkg/errors repository being archived; I concur that's not ideal, OTOH, ISTR it was archived as "feature complete", so not necessarily problematic (currently).