Minimum Go version is now at least 1.13

[TBBle]

The README still lists "Go 1.9 or newer to build", but it's really Go 1.13, due to use in containerd modules of errors.As and errors.Is, introduced in Go 1.13.

> go1.12.17.exe install .\cmd\wclayer\... .\cmd\containerd-shim-runhcs-v1\... .\cmd\device-util\... .\cmd\runhcs\... .\cmd\shimdiag\... .\cmd\tar2ext4\...
# github.com/Microsoft/hcsshim/vendor/github.com/containerd/containerd/errdefs
vendor\github.com\containerd\containerd\errdefs\errors.go:54:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:59:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:65:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:71:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:76:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:81:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:86:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
vendor\github.com\containerd\containerd\errdefs\errors.go:92:9: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".Is
# github.com/Microsoft/hcsshim/vendor/github.com/containerd/ttrpc
vendor\github.com\containerd\ttrpc\client.go:357:6: undefined: "github.com/Microsoft/hcsshim/vendor/github.com/pkg/errors".As

Containerd claims 1.15 minimum, as seen above, but the same command with go1.13.15.exe doesn't fail to build.

Note that before #946 pulled in the newer containerd, the minimum was 1.12, due to hcsshim's use of exec.ExitError.ExitCode introduced in Go 1.12.

> go1.11.13.exe install .\cmd\wclayer\... .\cmd\containerd-shim-runhcs-v1\... .\cmd\device-util\... .\cmd\runhcs\... .\cmd\shimdiag\... .\cmd\tar2ext4\...
# github.com/Microsoft/hcsshim/internal/cmd
internal\cmd\diag.go:55:18: exiterr.ExitCode undefined (type *exec.ExitError has no field or method ExitCode)

I'd propose to bump the minimum to Go 1.13 or higher, and also as part of that, drop the vendor directories, as keeping those (particularly tests/vendor) up to date is the slowest part of rebasing and cleaning PRs for review; since Go 1.13 has modules on-by-default, there should be no ecosystem pieces that hcsshim depends on which depend on the vendor/ tree once Go 1.13 is the minimum. gometalinter is the only cause for concern in the AppVeyor script, I haven't checked how it works with without vendor/, but it's only doing gofmt anyway, so I don't expect issues there.

go.mod already claims Go 1.13 minimum, but Go 1.12 without modules enabled would have ignored that anyway.

AppVeyor CI currently uses Go 1.15, to support -gcflags=all=-d=checkptr, since #926, and Go 1.13.4 before that, so any claims for support of Go 1.12 or earlier were untested by the public CI.

containerd is 1.15 minimum since 1.4.0, and Docker Engine 19.03, 20.10 and master are all using Go 1.13 in their Dockerfile. I am unaware of what else might be vendoring hcsshim.

Microsoft go-winio may do so in future (microsoft/go-winio#187), but only for a test, not primary functionality, so this should not have a knock-on effect there. They list Go 1.12 minimum in their go.mod.

If we need to regain Go 1.12 support, we have to back out #946, and #947 which depended on it, and reconsider how to implement #901, possibly reinstating the first version of the hard-link support that didn't require #947. This would also block updating to containerd 1.4, which also includes the use of errors.Is.

[TBBle]

I'd propose to bump the minimum to Go 1.13 or higher, and also as part of that, drop the vendor directories,

I note that #956 has gone the other way, and enforced use of the vendor/ directory in CI over modules. >_< I guess at least this way, CI will ensure people haven't forgotten to go mod vendor for the root and test directories, but it's still an annoying timesink in development.

[katiewasnothere]

I'm looking into this :) Will get back to you

[katiewasnothere]

Okay, talking to the team a bit, it sounds like we don't want to switch to modules since having a dedicated vendor/ directory gives us extra protection (such as if a dependency disappears off the internet or somehow is compromised).

Besides that, I see no issue with bumping the min go version to 1.15

[TBBle]

How would you feel about dropping tests/vendor at least? Every time I touch hcsshim, I need to revendor that, because it ends up with a copy of hcsshim, so it leads to very noisy commits.

[dcantah]

I was looking at our go.mod right now and then remembered this issue vaguely 😆 Are we all good to close this one out now that we're on 1.17? (although maybe we could get a new issue to look into "solving" the /tests vendor directory mess..)

[katiewasnothere]

Re the /test vendor mess: I can't really see a way around having the tests/vendor directory "mess" since we need to pull in dependencies for the test suite that we don't want to or can't pull into the main hcsshim module (such as k8s.io/cri-api).

An alternative is to move the test suite out of hcsshim and into a dedicated repository, but I'm not sure we want to do that. Particularly considering that would make the dev experience more difficult -- to run the tests, you'd have to push the code to a remote so that you could then pull those changes via a re-vendor into the test suite's repository, then rebuild. Could also lead to more test breaks since we likely wouldn't update that repository every time we made a change to the hcsshim repository.

I'm open to suggestions, but otherwise I don't see much we can do to improve here right now.

[dcantah]

'Mess' is me exaggerating but it's a pain right now to go mod vendor in /test after almost any change you make. We'd talked about this before amongst the team but yea the alternatives that come to mind also have drawbacks. Just getting rid of the vendor dir has builds still work fine, but we lose the "if a dep disappeared off the face of the earth" security.

[TBBle]

For the tests, I'd take the "a dep might disappear off the face of the earth" risk in order to not have to go mod vendor twice, particularly given how many deps there are in the tests specifically.

I agree that separating the tests into a different repo seems like a worse outcome overall.

I haven't looked at it too closely, but perhaps the new Go Workspaces support in 1.18 would reduce the pain of the separate test/ directory by making the tests/ module more transparent to the various go mod housekeeping tools so it's easier to not forget to run them all twice.

[katiewasnothere]

For the tests, I'd take the "a dep might disappear off the face of the earth" risk in order to not have to go mod vendor twice, particularly given how many deps there are in the tests specifically.

These tests are used to validate code when we make new releases and new features. In a worst-case scenario, a dependency could be compromised suddenly, and we then silently have tests that we don't trust. So I think we're stuck with the test modules to protect against that and the dependencies disappearing.

The go workspaces sound promising from a quick look! We should look into it more.

[slonopotamus]

2 cents: keeping all deps inside hcsshim repo is a poor solution for "all deps are suddenly gone" scenario.

1. MS can just make a mirror of whole github
2. You're not storing a copy of Go itself, aren't you? Why?
3. If each and every Go project started to use this strategy, we would end up with exponential growth of disk usage.
4. hcsshim is... Not a very important project in Go ecosystem, let's be honest. Nothing disastrous would happen if it broke.

[TBBle]

On reflection, my main annoyance with test/vendor is that it vendors github.com/Microsoft/hcsshim, which lives in the same repo (and has a replace github.com/Microsoft/hcsshim => ../ entry in test/go.mod), and hence go mod vendor needs to be rerun for any non-test change, rather than just a change to the dependency graph like go mod vendor in the root does.

And if you forget to go mod vendor in test, and run your tests in -mod=vendor mode, I guess it'll run the tests against the copy of hcsshim in test/vendor, not the actual copy you're working on. But I haven't tried that, so I don't know if that's a common workflow or not, or if it'd actually detect that the vendor directory does not match the source.

Looking at the list of test dependencies, I don't really expect any of them to fall off the Internet, as they're pretty essential to the containers ecosystem.

In fact, a brief survey suggests that only one that isn't also required by hcsshim itself (and hence in its vendor directory) is k8s.io/cri-api. (Which has a huge dependency graph, and I believe is the reason for this module separation in the first place).

So perhaps another idea: Have just the CRI tests (test/cri-containerd) in a separate module, and lift the rest of the tests up into the parent module. Given the purpose of CRI, I suspect that the direct dependency list of test/cri-containerd would come down to k8s.io/cri-api, hcsshim itself, and some test-support utilities and "everyone uses it" stuff like github.com/pkg/errors.

This might also be valuable as an insight for CRI users to see what sort of behaviours, e.g. annotations, os versions, one can expect for a CRI runtime backed by hcsshim, and what packages they need to pull in to access them.

And maybe as a service to users, github.com/Microsoft/hcsshim/pkg/annotations could be a separate module so that depending on that becomes wildly less costly for people who only need the annotation names when calling CRI or OCI APIs.

That might let you get in a situation where you only need to go mod vendor in test/cri-containerd if some public aspect of hcsshim changed such as a new annotation was added, rather than any time you merely stared at hcshim for too long without blinking, as is the (hyperbolic) case now.

[TBBle]

I've put up a PR to lock-in Go 1.17. A separate issue for discussion of the test/vendor directory (or test module structure as I flagged in my last comment) is probably a good idea, give how far this ticket has strayed from my original "Can I use 1.13-specific features now please?".

Although while I think about it, does the Go Proxy system guarantee that existing versions of modules won't fall off the face of the earth, even if their backing repo disappears? See the FAQ:

Whenever possible, the mirror aims to cache content in order to avoid breaking builds for people that depend on your package, so this bad release may still be available in the mirror even if it is not available at the origin. The same situation applies if you delete your entire repository. We suggest creating a new version and encouraging people to use that one instead.

but

proxy.golang.org does not save all modules forever. There are a number of reasons for this, but one reason is if proxy.golang.org is not able to detect a suitable license. In this case, only a temporarily cached copy of the module will be made available, and may become unavailable if it is removed from the original source and becomes outdated. The checksums will still remain in the checksum database regardless of whether or not they have become unavailable in the mirror.