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.

Sign up for GitHub

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

Jump to bottom

Consider dropping dependency on github.com/pkg/errors #3618

Open
imjasonh opened this issue May 17, 2022 · 5 comments · May be fixed by #5525
Open

Consider dropping dependency on github.com/pkg/errors #3618

imjasonh opened this issue May 17, 2022 · 5 comments · May be fixed by #5525

Comments

@imjasonh
Copy link
Contributor

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
Copy link
Member

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
Copy link
Contributor Author

imjasonh commented May 17, 2022 •
edited
Loading

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
Copy link

mitar commented Oct 9, 2023

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
Copy link
Contributor

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.

kolyshkin added a commit to kolyshkin/hcsshim that referenced this issue Oct 9, 2024
@kolyshkin
Switch from pkg/errors to Go 1.13+ error wrapping
1cabea0 
The github.com/pkg/errors is mostly obsoleted since Go 1.13 introduced
%w-style error wrapping. It is also not maintained and is now archived
by the owner.

Some part of this change was done manually, and some changes were done
by using github.com/AkihiroSuda/go-wrap-to-percent-w tool.

In a few places this also:
 - changes '%s' or \"%s\" to %q;
 - removes extra context from the error message (such as, errors from os
   functions dealing with files do contain the file name already, and
   strconv.Atoi errors already contains the string which it failed to
   parse).

Note that there is a single place which uses StackTrace functionality of
pkg/errors, which is removed by this commit.

A few remaining users of pkg/errors vendored here (directly and
indirectly) are:
 - github.com/containerd/go-runc (needs to be bumped to v1.1.0);
 - github.com/microsoft/didx509go (needs microsoft/didx509go#19);
 - github.com/docker/cli (needs docker/cli#3618 fixed);
 - github.com/docker/docker (?)
 - github.com/linuxkit/virtsock (needs linuxkit/virtsock#69 merged);

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin mentioned this issue Oct 9, 2024
Open
@thaJeztah
Copy link
Member

There's a longer discussion here;

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).

@kolyshkin kolyshkin linked a pull request Oct 9, 2024 that will close this issue
Draft
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Switch from github.com/pkg/errors to %w-style wrap kolyshkin/cli
4 participants
@imjasonh @mitar @thaJeztah @kolyshkin