Move tool dependencies to a submodule

[sagikazarmark]

Multierr currently brings a bunch of packages as dependencies which are not required for the library itself (lint, license tools, etc).

This PR relocates those tools into a separate Go module, so they are not installed for the consumers of multierr.

The alternative would be just creating a new mod file (eg. tools.mod) and adding the dependencies (without a tools_test.go file)

[codecov]

Codecov Report

Merging #38 into master will not change coverage.
The diff coverage is n/a.

@@            Coverage Diff            @@
##            master       #38   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            2         2           
  Lines          123       123           
=========================================
  Hits           123       123           

---

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 1637846...27de85e. Read the comment docs.

[sagikazarmark]

Not sure how the CI break is related, but Go 1.11 is quite an old version.

[sagikazarmark]

Ping @abhinav

[abhinav]

Hello! Thanks for the contribution and apologies for the delay. This is a reasonable change. @prashantv and I considered doing this originally but found that renaming to tools_test.go was good enough for at least the go mod vendor case. However, it sounds like these constraints are still a problem, so we are in favor of moving tools dependencies to a submodule.

Would you mind rebasing on master so that we have a green build?

Thanks!

[abhinav]

Also, I suspect tools_test.go can now be called tools/tools.go

[sagikazarmark]

@abhinav rebased and renamed. Thanks!

[sagikazarmark]

Can you also cut a release once it's merged?

[abhinav]

Thanks! Yes, we'll cut a release when this is merged.

I see that we still have an /x/tools dependency which is coming from uber-go/atomic.
We're going to fix and release that first: uber-go/atomic#78.

[abhinav]

@sagikazarmark can you clarify how this issue is manifesting for you?
We did some local experimentation, and it doesn't look like test dependencies are added to the consumer.

For example, I just did:

$ mkdir example && cd example
$ go mod init github.com/abhinav/example
$ go get go.uber.org/multierr
go: go.uber.org/multierr upgrade => v1.5.0
$ cat > main.go
package main

import _ "go.uber.org/multierr"

func main() {
}
$ go mod tidy

Following this, the go.mod only contains go.uber.org/multierr and not /x/tools or /x/lint.

 $ cat go.mod
module github.com/abhinav/example

go 1.15

require go.uber.org/multierr v1.5.0

And if we run go mod vendor, only multierr and its dependency atomic are present in the output:

$ go mod vendor
$ cat vendor/modules.txt
# go.uber.org/atomic v1.6.0
go.uber.org/atomic
# go.uber.org/multierr v1.5.0
## explicit
go.uber.org/multierr

[sagikazarmark]

Dependencies are added to go.sum which means go mod download will download them. go mod download is the only way to download dependencies upfront (to warm Docker cache for example) and downloading unnecessary dependencies makes it take longer and makes the cache larger.

See for example https://github.com/emperror/errors/blob/master/go.sum

[abhinav]

Ah! Yes, this seems accurate. Tried locally with the same example as before:

$ cat go.mod
module github.com/abhinav/example

go 1.15

require go.uber.org/multierr v1.5.0

$ mkdir cache
$ GOMODCACHE=$(pwd)/cache go mod download
$ ls cache
cache  github.com  go.uber.org  golang.org  gopkg.in  honnef.co

That includes test dependencies too.

CC @prashantv

[prashantv]

Makes sense, thanks for the additional context @sagikazarmark

Looking at the documentation for go mod download, it's not clear this is intentional:

With no arguments, download applies to all dependencies of the main module.

I wouldn't expect test dependencies of a dependency of the main module to be downloaded, I wonder if this is a bug or a feature request to avoid downloading transitive dependencies.

Given the behaviour, and only slightly increased complexity of the solution, I think this makes sense to do.

[abhinav]

We'll update, merge and release this after uber-go/atomic#80 lands and is released.
Since we prefer to avoid cutting releases late on a Friday, this will have to wait until Monday.

Thanks!