containerd / continuity

A transport-agnostic, filesystem metadata manifest system
https://containerd.io
Apache License 2.0
138 stars 66 forks source link

Remove Windows' Readlink fork #151

Closed kolyshkin closed 4 years ago

kolyshkin commented 4 years ago

This reverts the part of commit 8100e750d (PR #113) that added the fork of Readlink() for Windows.

At that time the fork was done to work around the bug in golang's implementation of os.Readlink() for Windows.

The above bug was never reported upstream, but fortunately it was independently found, reported [1], and fixed [2]. The fix made its way into go-1.13 (there is no mention of that in release notes, so I checked it in git history).

[1] https://github.com/golang/go/issues/30463 [2] https://go-review.googlesource.com/c/go/+/164201

PS since this requires go 1.13 now, add the appropriate guard so the package won't compile on earlier golang

kolyshkin commented 4 years ago

@crosbymichael @estesp @dmcgowan PTAL

kolyshkin commented 4 years ago

@cpuguy83 PTAL

cpuguy83 commented 4 years ago

I like the change but I'm not sure I like that we are dropping go1.12 support so quickly.

kolyshkin commented 4 years ago

I like the change but I'm not sure I like that we are dropping go1.12 support so quickly.

  1. this is windows only
  2. go 1.12 is an unsupported release :skull: anyway, since 1.14 is out

so when this change is disruptive, it means someone is using an unsupported golang release

cpuguy83 commented 4 years ago

Understood that go1.12 is no longer a supported release from the go team, but we still use this in docker 19.03 (for now).

kolyshkin commented 4 years ago

So this one is blocked by https://github.com/moby/moby/pull/40592

kolyshkin commented 4 years ago

we still use this in docker 19.03 (for now).

@cpuguy83 we just stopped doing it :) PTAL

crosbymichael commented 4 years ago

LGTM