replace deprecated package github.com/pkg/errors

[mroth]

Replaces github.com/pkg/errors, which is now marked as archived and no longer maintained on github, with native >=go1.13 standard library functions. This preserves the error wrapping previously handled via errors.Wrap via the new %w directive in fmt.Errorf.

This makes the minimum go version go1.13, so thus update the go.mod to reflect and also updates the test matrix here to test from go1.13-go1.18 inclusive (previously tested go1.14-go1.15).

[codecov-commenter]

Codecov Report

Merging #28 (a648e02) into master (9c98bf4) will not change coverage.
The diff coverage is 50.00%.

@@           Coverage Diff           @@
##           master      #28   +/-   ##
=======================================
  Coverage   79.47%   79.47%           
=======================================
  Files           5        5           
  Lines         268      268           
=======================================
  Hits          213      213           
  Misses         38       38           
  Partials       17       17           

Impacted Files	Coverage Δ
strftime.go	86.20% <40.00%> (ø)
specifications.go	70.58% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c98bf4...a648e02. Read the comment docs.

[lestrrat]

Hi, thanks, but I'm not sure this is backwards compatible. If there were people relying on github.com/pkg/errors.Cause(), I think this would break their code -- i.e. I think we'd have to bump major versions go with this change.

So that's a showstopper to merging this PR, at least not at this point, and not to main

[mroth]

Hi, thanks, but I'm not sure this is backwards compatible. If there were people relying on github.com/pkg/errors.Cause(), I think this would break their code -- i.e. I think we'd have to bump major versions go with this change.

I was thinking about that, my understanding of how errors.Cause works is based on this documentation:

errors.Cause will recursively retrieve the topmost error which does not implement causer, which is assumed to be the original cause.

In which case, it should return the outermost error. Is there a place where an underlying error type is made public? On my examination of the package it appeared there were no existing exported error types (or any other types) as package variables or constants. If there's a way someone was using Cause() to get at an underlying type, totally agree, I just couldn't find that possibility when looking through the code, but very possible I missed something.

---

UPDATE: Looking at the archived code for pkg/errors, I think this case might actually be handled natively in pkg/errors. In pkg/errors#215 looks like pkg/errors introduced support for go1.13 native wrapped error chains to errors.Cause(), it walks the Unwrap chain to the original error, including a chain with mixed alternating stdlib and pkg/errors wrappers.

[lestrrat]

https://go.dev/play/p/Yn-BWN7QIqg

[mroth]

https://go.dev/play/p/Yn-BWN7QIqg

Ah yes, it looks like the fix was later reverted in pkg/errors#220. Sorry I missed that.

Whats your personal appetite like on eventually rolling a v2 semantic major version release to avoid dependency on an archived/deprecated package? Could target bundling that along with any other potential compatibility changes you'd want to make. Fully understand if it doesn't seem desirable to you.

[lestrrat]

From my PoV:

• I don't (and won't ever) know if there are people who rely on github.com/pkg/errors behavior, so I have to assume the worst
• You haven't shown me a concrete flaw, like wrong behavior, security incident, company policy not allowing you to use github.com/pkg/errors, etc

So I'm not against v2 or your change per se, but ATM I don't have a compelling reason to bump the version or to eradicate github.com/pkg/errors

The other possibility to eradicate github.com/pkg/errors that I can think of is to gradually change the behavior -- perhaps with build tags:

• for version N we provide both implementations of github.com/pkg/errors and fmt.Errorf but make github.com/pkg/errors the default
• for version N+1 make fmt.Errorf the default but keep pkg/errors, and
• finally in version N+2 remove github.com/pkg/errors.

I guess this approach would be up to me, but again, I don't have a compelling reason.

So I guess it comes down to: Is there a concrete problem that you are facing by this module using github.com/pkg/errors, other than the fact that it's archived?

[mroth]

So I guess it comes down to: Is there a concrete problem that you are facing by this module using github.com/pkg/errors, other than the fact that it's archived?

For my general professional use case, we do audit all dependencies, and the active maintenance/governance of a project is one portion the a selection criteria (as well as generally try to reduce the dependency chain where possible to minimize risk). In this particular instance however, my usage was currently for a side project and thus I was only trying to help clean things up for best practice -- prior to recognition of the reverse incompatibility issue. Thus I don't believe my usage should be a particular factor in your decision (and I could always just maintain my own fork if really needed).

A v2 bump would likely be most compelling if there were other changes you wanted to make at the same time.

[lestrrat]

fair enough. let's see if we can do a gradual transition then.

would you be willing to do the following? (feel free to tell me no if you're not up to it)

• separate the PR into

  1. eradicating github.com/pkg/errors
  2. cleaning up github actions and upping go version

• for 1:

  • create an internal package errors which contain pkg.go and native.go, for code using pkg/errors and fmt respectively
  • add a build constraint for strftime_native_errors in native.go
  • chanhe strftime code to use new internal package

• for 2:

  • file separate PRs for upping go version and updating GitHub Actions on top of PR for 1

[mroth]

Sure, I'll take a stab at that. (Might have to chip away at it here and there as I'm traveling for business next few weeks, but will try to make incremental progress.)

[mroth]

I foolishly based this original PR off of master in my fork, and GH won't let you change the source branch of an existing PR, so to preserve the comment history here I'm going to do some git history reconstruction and force push to try to get this branch/PR back to a working state for the eventual work towards the internal errors package and convert back to a draft.

[mroth]

Okay, this PR now represents the work for task "one" above. It cherry-picked contains my original commit for factoring out pkg/errors, which will be updated to reflect the new strategy in subsequent commits. This also makes visible in CI that we would be breaking go1.12 compatibility where depending on the new stdlib errors functionality, so that can be planned for and discussed.

I'll start on the internal errors package in the future (but feel free to push changes to this PR directly if you don't want to wait for me).

[lestrrat]

Sure. I'll take it from here then