Replace vendor.conf by go.mod

[praseodym]

Through go mod init github.com/containers/image followed by
go mod tidy.

[praseodym]

Unfortunately CI fails, but I'm not quite sure why.

[mtrmac]

Thanks for the PR.

The CI fails because it now runs the tests as root instead of an unprivileged user.

Overall, this PR is doing rather too many quite unrelated changes. They may well be desirable, but they need to be described, reviewed, and discussed one at a time — if not in separate PRs, at the very least in separate commits.

(Cc: @vrothberg , who has more experience with Go modules.)

[vrothberg]

I concur with @mtrmac. Let's break the changes into easier to digest commits. The main part is replacing trash with go mod. Once that's done and the CI passes, we can improve from there.

[praseodym]

The CI fails because it now runs the tests as root instead of an unprivileged user.

Overall, this PR is doing rather too many quite unrelated changes. They may well be desirable, but they need to be described, reviewed, and discussed one at a time — if not in separate PRs, at the very least in separate commits.

I made those changes to simplify the CI flow in an attempt to fix it, so I would prefer to say that it's failing despite of those changes 🙂

I've now removed these changes and instead added GO111MODULE="on" in the Makefile to force Go modules to be enabled even within GOPATH. Unfortunately Travis has been stuck at "Job received" for about 45 minutes now, so I'll check back later.

[mtrmac]

The CI fails because it now runs the tests as root instead of an unprivileged user.
Overall, this PR is doing rather too many quite unrelated changes. They may well be desirable, but they need to be described, reviewed, and discussed one at a time — if not in separate PRs, at the very least in separate commits.

I made those changes to simplify the CI flow in an attempt to fix it, so I would prefer to say that it's failing despite of those changes 🙂

Well, the flow can only be simplified after it is understood :)

[rhatdan]

LGTM

[mtrmac]

Looking at the build logs:

go get: warning: modules disabled by GO111MODULE=auto in GOPATH/src;
	ignoring go.mod;
	see 'go help modules'
go get: warning: modules disabled by GO111MODULE=auto in GOPATH/src;
	ignoring go.mod;
	see 'go help modules'
git-validation -q -run DCO,short-subject,dangling-whitespace

It would be nice to clean that up; AFAICS it comes from the tools.timestamp rule.

---

More importantly, the go get commands above, when run outside of GOPATH actually add the three commands as dependencies in go.mod; that seems quite undesirable (consumers of the library neither need nor want to include our testing infrastructure). Sadly, there isn’t an obvious way to bypass that (at least quickly skimming golang/go#27643 and several of the linked discussions).

I guess the closest to working is the https://stackoverflow.com/questions/53368187/go-modules-installing-go-tools solution of using an unrelated temporary working directory (except that it should use mktemp -d instead of hard-coding a single shared=possibly malicious directory).

[praseodym]

I think adding an explicit GO111MODULE=off should fix it.

[mtrmac]

For now; containers/skopeo#695 says that Go 1.13 ignores that variable. But this is not my area of expertise.

[praseodym]

Oh great :(

[praseodym]

It looks like Go 1.13 only changes the behaviour of GO111MODULE=auto though:
https://github.com/golang/go/blob/ad4ed87f80c33f23bdd3767ef3208f15a1fb5c90/doc/go1.13.html#L181-L190

So explicitly setting GO111MODULE=off should still work with Go 1.13.

[mtrmac]

Thanks, looking at the code GO111MODULE=off does seem to set at least the local flags in Go’s src/cmd/go/internal/modload/init.go and src/go/build/build.go to turn off modules, so this code should be fine.

Yet, GO111MODULE=off was exactly what Skopeo was using before containers/skopeo#695 , so I don’t quite know what is going on. Hopefully @vrothberg can help.

[vrothberg]

Starting with Go v1.13 the variable will be ignored (since go modules are declared stable) but we still need it for backwards compat with 1.11 and 1.12.

As @mtrmac has mentioned, any go get and other go commands can potentially alter the module, so we need to be careful. What we do in buildah, for instance, is to keep the CI dependencies in a separate (sub) go module: https://github.com/containers/buildah/tree/master/tests/tools

What we do there is have a dedicated go module containing the tools for the CI that will be compiled on-demand. The code could also be downloaded on-demand, if desired. To trick go into believing we have dependencies on the tools, we have a dummy .go file with import _ "github.com/.../..." lines (https://github.com/containers/buildah/blob/master/tests/tools/tools.go).

I can take care of doing that once this PR is merged.

[mtrmac]

Starting with Go v1.13 the variable will be ignored (since go modules are declared stable) but we still need it for backwards compat with 1.11 and 1.12.

That’s not what the code actually does AFAICT. There are quite a few checks for os.Getenv("GO111MODULE") == "off" in the codebase, even on master. Or have they dropped that check from some important place that affects these projects?

[vrothberg]

Thanks for clarifying. I didn't actually check the go code but based my statement on the various build issues in Fedora when flipping it to v1.13. So it seems that GO111MODULE can still be turned off explicitly ... although it did not help with go build apparently.

[mtrmac]

(Note for the record: This is an upgrade from v1.0.0.)

[mtrmac]

(Note for the record: This is an update from an untagged post-v1.1.4 commit.)

[mtrmac]

(Note for the record: all of the /x/* above have been upgraded, fairly significantly.)

[mtrmac]

LGTM, but I’d love to see an ACK by @vrothberg , especially (but not exclusively) WRT the GO111MODULE variable discussed above.

[vrothberg]

Thanks for the ping!

I very much prefer keeping the ./vendor directory and the dependencies locally. Currently, all github.com/containers/ projects use go mod only for dependency management and not for deduping and caching builds. For backwards compat and to avoid regression with v1.13+, we need to add a conditional GO_BUILD to the Makefile (see https://github.com/containers/buildah/blob/master/Makefile#L16-L21).

[vrothberg]

I very much prefer keeping the vendor target. All other projects on github.com/containers do something as follows:

https://github.com/containers/skopeo/blob/481bb94c5f8e65bc83b97539e7e4e6ccb8428a73/Makefile#L178-L182

[vrothberg]

I also want to preserve the having the code in ./vendor (via go mod vendor). The workflow of many folks is used to that. If some don't want it, they are free to not use make vendor.

[vrothberg]

Starting with Go v1.13 the variable will be ignored (since go modules are declared stable) but we still need it for backwards compat with 1.11 and 1.12.

As @mtrmac has mentioned, any go get and other go commands can potentially alter the module, so we need to be careful. What we do in buildah, for instance, is to keep the CI dependencies in a separate (sub) go module: https://github.com/containers/buildah/tree/master/tests/tools

What we do there is have a dedicated go module containing the tools for the CI that will be compiled on-demand. The code could also be downloaded on-demand, if desired. To trick go into believing we have dependencies on the tools, we have a dummy .go file with import _ "github.com/.../..." lines (https://github.com/containers/buildah/blob/master/tests/tools/tools.go).

I can take care of doing that once this PR is merged.

[mtrmac]

I very much prefer keeping the ./vendor directory and the dependencies locally.

In what cases does that make a difference? I could guess non-module-aware IDEs, perhaps.

Currently, all github.com/containers/ projects use go mod only for dependency management and not for deduping and caching builds.

Note that the others are applications, and we distribute the vendor subdirectories in official releases; c/image is a library and it never did that.

[vrothberg]

I very much prefer keeping the ./vendor directory and the dependencies locally.

In what cases does that make a difference? I could guess non-module-aware IDEs, perhaps.

Some IDEs are going wild and I found it helpful when updating/analyzing dependencies; sometimes tracking down bugs.

Currently, all github.com/containers/ projects use go mod only for dependency management and not for deduping and caching builds.

Note that the others are applications, and we distribute the vendor subdirectories in official releases; c/image is a library and it never did that.

I wasn't suggesting to commit the ./vendor folder but to preserve the make vendor target which downloads the dependencies - and preserves the previous behaviour. I feel it makes sense to have an opinionated way for dealing with go mod as there are many ways to do something wrong.

[mtrmac]

Sure, I’m fine with preserving the make vendor target, as an option.

OTOH consumers of c/image will just list it in their go.mod files, so the CI should run a module-aware build without relying on any vendor subdirectory, the way consumers would do.

I don’t know whether that means we should preserve the build/build-internal dichotomy, and modify the CI to call the non-vendoring versions, or whether we should never call the vendor target automatically. I don’t feel strongly about it, so far I’m leaning towards the latter for simplicity.

[vrothberg]

For the build to use ./vendor we need to add -mod=vendor to go build. So we could add make build-vendor target and use both in the CI?

[mtrmac]

I don’t quite see the point of doing that in the CI, the CI is going to start from a fresh checkout and it is going to have to populate ./vendor immediately before relying on it. We would be mostly testing whether go build and go mod vendor are internally consistent—and, well, that the Makefile rules were not broken; I suppose that’s something.

[vrothberg]

@mtrmac, I am not sure I can follow then. Do you want preserve make vendor and do vendor-builds or fully rely on go mod?

[mtrmac]

I think that definitely the CI, and probably the primary local build method, should rely on go mod the way our consumers do.

I don’t personally see preserving make vendor of anything about the vendor directory as important, but if you want make vendor (and make build-vendor?) to exist, that’s perfectly fine with me.

As for testing the vendor build in the CI in addition to the module-based build, I see that as even less important than preserving the vendor targets at all, but I’m fine with that as well, as long as it does not significantly increase the CI run time.

[vrothberg]

Thanks for clarifying! Let's drop the make vendor target then. I prefer having one way rather than multiple ways to build the project.

[vrothberg]

@praseodym, are you still working on it?

[praseodym]

@praseodym, are you still working on it?

Oops, I missed the review comments in rest of the discussion. I've just rebased this and addressed the one comment.

[mtrmac]

LGTM. Thanks!

@vrothberg I’ll leave this up to you to merge, in case there were some tricky aspects WRT updating consumers of c/image that already use Go modules.

[vrothberg]

LGTM, thanks for contributing, @praseodym!

@vrothberg I’ll leave this up to you to merge, in case there were some tricky aspects WRT updating consumers of c/image that already use Go modules.

To the best of my knowledge, nothing will change. Consumers of c/image already marked it as +incompatible since we are not doing the version-path dance. The nice thing now is that vendoring c/image will be a bit easier as go mod will fetch the potentially updated dependencies as well.

After the merge here, I will iterate over the dependencies and check for new versions to make sure we're releasing an up to date module.