Enable Go Modules

[gdbelvin]

Go Modules now has a way to pin versions of tools so I thought I'd give things a spin. So far things seem to be working reasonably well.

Checklist

• I have updated the CHANGELOG.
  • Adjust the draft version number according to semantic versioning rules.
• I have updated documentation accordingly.

[gdbelvin]

gometalinter does not support Go Modules. As a result this is blocked on migrating to golangci-lint #1401.

[daviddrysdale]

A few questions before looking at this in detail...

The main question is how/whether this deals with the primary problem we encountered on the last few iterations of looking into dependency management, which is that non-Go files are not dependency-managed. For example: do imported .proto files end up being from a different codebase version than the Go tools & libraries that deal with protobufs, leading to an inability to go generate or (worse) a subtle mismatch of code versions?

The other questions are

• This still seems to have a Go 1.10 build -- how does that work? Does that just fall back to the current some-vendored-most-tip dependencies?
• (following on) Do we get to drop our vendor/ copies as part of this, maybe when we shift to requiring Go 1.11 (with module support on)?

[gdbelvin]

Your question in order:

1. How does this handle dependencies for tools that generate files?

• For go based tools, we make a fake code reference in tools.go. The module system will track the dependency, while a build flag prevents the file from messing up our builds.
• go get <tool path> will fetch the latest version of the tool and record all of it's dependencies in go.mod
• go install <tool path> will only fetch the version of the tool already declared in go.mod

2. Keeping compatibility with go.10.

• Yes, I think the fallback behavior is pull things from tip. This theoretically works according to semantic path versioning, but of course in practice we know that's not true.

3. What happens to vendor?
   In module mode, the vendor directory is ignored completely by default. There are options, however, to a) not ignore it, and b) synchronize it with go.mod.
   I suspect the most interesting edge cases are what happens when a project with a go.mod file gets imported by non-module project. I don't have a definitive answer on this yet. I suspect that those projects would revert to the old vendor behavior.

[daviddrysdale]

Ah, number 1 isn't quite my question. I understand how tools.go works, but when I played with this previously I found there wasn't a straightforward way for (say) trillian.proto to include timestamp.proto and get a version of the latter at exactly the same version as the protobuf Go code.

IIRC the problems were roughly:

• setting a proto include path for the module checkout is a pain (-I${GOPATH}/src/mod/<some-complicated-path-with-a-hardcoded-version)
• go mod vendor doesn't populate non-Go files into vendor/ (cf. cmd/go: go mod -vendor prunes non-package directories golang/go#26366) so wouldn't help.

Maybe the problem has been fixed since I looked at this in September?

[gdbelvin]

That is an interesting question... I don't have the full answer but here's an initial stab:

• Well known types such as timestamp.proto are special cased by protoc. Pinning protoc to a specific version should be sufficient to pin timestamp.proto to a specific version.
• go get still downloads non-go data. Here's a histogram from my go/pkg/mod dir:

 135 sh
 151 mod
 222 textproto
 248 info
 285 proto
 388 md
1097 json
1211 sample
8129 go

• Protos in packages that have no go files such as github.com/googleapis/googleapis.
  go get not working is not a change of behavior. We have to use git checkout for this already.
  There does, however, seem to be an effort to make go get work for these use cases, but I wasn't able to get it to work locally:
  • https://go-review.googlesource.com/c/vgo/+/120995/

[daviddrysdale]

OK, so it looks like we're currently OK for .proto files specifically (because all of the included ones either come bundled with protoc or are in the explicitly-non-Go repo googleapis/googleapis).

But are there any other non-Go files we use in (say) the integration test scripts? I guess if we get a clean Travis build then maybe not.

(But if there are, then we hit the problem I think I encountered last time : go mod vendor doesn't copy non-Go files across into ./vendor/github.com/wherever, and referring to them in the module store involves a hard-coded version (include $GOPATH/pkg/mod/github.com/thing/whatsit@v0.1.7/path/file))

Some other musings:

• Should we make this a mandatory thing, and so force a Go 1.11 version requirement? (Presumably that would allow us to delete the local checked-in vendor/ directory.)
• What happens if some combination of dependencies cannot be reconciled using any published versions? Is there a mechanism to have local-only deltas? (Note that we have needed to make such local changes to our vendor/ code in the past -- e.g. b144701, a1b0c2d, maybe 5bf6fd3)
• What happens to the explicit, deliberate checks in Travis that a Trillian repo change doesn't break a combined build of ct-go with Trillian?
• The PR could do with a README.md update to talk about relevant go mod commands (e.g. for getting started and for syncing with tip ct-go etc).

I think this is all worth wider discussion with the rest of the team in person – but it does look more hopeful than the last few times round the loop (glide, vendor, go dep, vmod, go mod, …).

(By the way, my previous experiment with Go modules is at https://github.com/daviddrysdale/trillian/commits/mod – might be worth a compare/contrast against this PR.)

[gdbelvin]

go/src/mod contains non-go files, but you're right that go mod vendor will not copy non-go files to vendor. I'm not sure if we need the vendor directory after moving to modules though...

I don't think we need to use fancy version paths to refer to things in the module store. That's what go.mod is for. One only needs to be careful about updating the module store with go get.

Getting rid of vendor would be nice, since we wouldn't have to worry about different behaviors.

The not breaking ct-go check will be an interesting point for discussion.

• ct-go could explicitly declare what versions of trillian it depends on and we wouldn't have to constantly worry about breaking it -- only when we update ct-go.
• alternatively, we could preemptively check what the breakage will be.
• generically, there are many other projects (KT, trillian-examples, etc) which have dependencies on the Trillian codebase that are not checked at the moment.

[daviddrysdale]

I don't think we need to use fancy version paths to refer to things in the module store. That's what go.mod is for.

go.mod is irrelevant for non-Go code.

[Martin2112]

Shall we close this and reopen on our 6 monthly "what are go modules doing today" quest?

[gdbelvin]

@paulmattei - Can we tag this and put it in some sort of backlog / code cleanup / fixit week bucket?

[Martin2112]

Modules is a special case as the language support is still somewhat in flux.

[gdbelvin]

@Martin2112 Are there any language support features we are blocked on? Those might be nice gating mechanism.

[Martin2112]

Last time we said 1.12 hadn't been out long enough. There could still be changes during the 1.12.x releases I think. Is that right @daviddrysdale ?

[daviddrysdale]

I think the module stuff has been stable since 1.12 and 1.11.4 (but the go.sum file wouldn't be compatible with earlier Go versions).

[codecov]

Codecov Report

Merging #1400 into master will increase coverage by 0.06%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master    #1400      +/-   ##
==========================================
+ Coverage   68.46%   68.53%   +0.06%     
==========================================
  Files         104      103       -1     
  Lines        8940     8898      -42     
==========================================
- Hits         6121     6098      -23     
+ Misses       2216     2199      -17     
+ Partials      603      601       -2

Impacted Files	Coverage Δ
storage/postgres/log_storage.go	64.4% <0%> (ø)	⬆️
storage/types.go	87.56% <0%> (-0.79%)	⬇️
skylog/core/builder.go
scripts/licenses/licenses/find.go	80.76% <0%> (+4.9%)	⬆️
scripts/licenses/licenses/library.go	68.57% <0%> (+12.32%)	⬆️

---

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 2267e75...e018ce5. Read the comment docs.

[Martin2112]

As discussed, I think we now need this to unblock the skylog work.

[pav-kv]

I couldn't quickly find a spec for this path expression. Just to double check, should there not be a colon before the exclamation mark?

[gdbelvin]

Sharp eye

I don't see any reference to a colon here:
https://git-scm.com/docs/gitignore#_pattern_format

[RJPercival]

That's because this is a pathspec, not a gitignore pattern. The pathspec added here won't work as expected without a colon added, since that's necessary for ! to be interpreted as "exclude".