Add support for Go modules #751

abhinav · 2019-10-29T22:59:08Z

This switches Zap to using Go modules instead of Glide for managing
dependencies, while keeping the glide.yaml around to provide
constraints for legacy ecosystems.

The change ended up being more complicated than a go mod init because
Zap has a few tests that verify its behavior inside GOPATH and a
vendor/ directory. Those tests were passing fine in CI for #742 because
of the go_import_path directive in the Travis configuration which
places it under $GOPATH/src/go.uber.org/zap. This isn't necessary
with Go modules but I've left it in-place for now.

Note that to keep the direct dependencies of Zap small, the benchmarks/
directory was turned into its own Go module with a replace clause
pointing to the local copy of Zap. We will not tag a release of the
benchmarks/ module.

Supersedes #742
Resolves #639

Note to reviewers: Each commit is meant to be reviewed individually.
The change should not be squashed when merged.

This switches Zap to using Go modules. To keep the list of direct dependencies of Zap minimal, benchmarks are placed in their own Go module as per [Multi-Module Repositories][1]. This change deletes the glide.lock but keeps the glide.yaml around for legacy ecosystems. [1]: https://github.com/golang/go/wiki/Modules#faqs--multi-module-repositories

With Go modules, the location of global_test on disk does not have to be in `$GOPATH/src/go.uber.org/zap`, or even in a directory named `zap`. This simplifies the assertion to verify just the file name.

codecov · 2019-10-29T23:45:01Z

Codecov Report

Merging #751 into master will increase coverage by 0.64%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #751      +/-   ##
==========================================
+ Coverage   97.44%   98.09%   +0.64%     
==========================================
  Files          40       42       +2     
  Lines        2117     2150      +33     
==========================================
+ Hits         2063     2109      +46     
+ Misses         46       33      -13     
  Partials        8        8

Impacted Files	Coverage Δ
internal/ztest/writer.go	`100% <0%> (ø)`
internal/ztest/timeout.go	`86.66% <0%> (ø)`
zapcore/console_encoder.go	`100% <0%> (+3.5%)`	⬆️
zapcore/entry.go	`94.5% <0%> (+4.39%)`	⬆️
zapcore/write_syncer.go	`90.47% <0%> (+11.9%)`	⬆️
buffer/buffer.go	`100% <0%> (+12.9%)`	⬆️

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 0065243...39ba1b2. Read the comment docs.

prashantv

lgtm, minor simplication suggest for stacktrace_ext_test

prashantv · 2019-10-29T23:34:21Z

Makefile

 GO_VERSION := $(shell go version | cut -d " " -f 3)
 GO_MINOR_VERSION := $(word 2,$(subst ., ,$(GO_VERSION)))
-LINTABLE_MINOR_VERSIONS := 12
+LINTABLE_MINOR_VERSIONS := 13


optional: should we just the LINT=yes in travis to control this, so lint versions are controlled in the same place go versions we test against are controlled?

Reasonable. That's what we do in the other projects. Switching.

prashantv · 2019-10-29T23:44:11Z

Makefile

 .PHONY: test
 test:
-	go test -race $(PKGS)
+	go test -race ./...


even though the benchmarks module has no tests, in case we add future modules, should this also be a $(foreach dir,$(MODULE_DIRS),...)

otherwise let's add a comment near MODULE_DIRS that we only test the main module

Adding foreach for make test but not for cover because then we'll have to deal with cross-module coverage merging. Adding a comment that we track coverage only for the main module.

prashantv · 2019-10-30T00:17:06Z

stacktrace_ext_test.go

 		cmd := exec.Command("go", "test", "-v", "-run", "TestStacktraceFiltersZap")
 		cmd.Dir = testDir
+		cmd.Env = envWith(t, "GO111MODULE=off")


i think this could be simplified to:

cmd.Env = append(os.Environ(), "GO11MODULE=off")

since duplicate entries causes the last value to be used;

// If Env contains duplicate environment keys, only the last // value in the slice for each duplicate key is used.

https://golang.org/pkg/os/exec/#Cmd

prashantv · 2019-10-30T00:18:29Z

stacktrace_ext_test.go

+func downloadDependencies(t *testing.T) []dependency {
+	cmd := exec.Command("go", "mod", "download", "-json")
+
+	stdout, err := cmd.StdoutPipe()


is streaming worth it here, suggest simplifying to:

stdout, err := cmd.Output() require.NoError(t, err, ...) dec := json.NewDecoder(bytes.NewReader(stdout))

Yeah, streaming is necessary here. The output contains a series of newline-delimited JSON objects.

Per go help mod download,

By default, download reports errors to standard error but is otherwise silent. The -json flag causes download to print a sequence of JSON objects to standard output, describing each downloaded module (or failure), corresponding to this Go struct: type Module struct { Path string // module path Version string // module version Error string // error loading module Info string // absolute path to cached .info file GoMod string // absolute path to cached .mod file Zip string // absolute path to cached .zip file Dir string // absolute path to cached source root directory Sum string // checksum for path, version (as in go.sum) GoModSum string // checksum for go.mod (as in go.sum) }

We need to use json.NewDecoder since the output has multiple objects, but I don't think we need to use a pipe -- we can get all the output at once and then use json.NewDecoder

Ah, I see what you mean. Switching.

abhinav · 2019-10-30T00:34:15Z

Updated based on comments.

This fixes TestStacktraceFiltersVendorZap to work with Go modules. For context, the test previously did the following: - Set up a temporary GOPATH - Copy stacktrace_ext_test.go into it - Symlink Zap and relevant dependencies into a vendor/ directory in the temporary GOPATH - Run specific tests from stacktrace_ext_test.go with Zap inside the vendor/ directory We're no longer using GOPATH or vendor/ directories, but this behavior of Zap when *it* is inside a vendor/ directory should be retained. To that end, this changes TestStacktraceFiltersVendorZap to continue to run with the following changes: - Use `go mod download -json` to retrieve the locations of all of Zap's dependencies on-disk, and symlink them into the temporary GOPATH's vendor/. - Explicitly set `GO111MODULE=off` before running `go test` because we may be running in an environment where `GO111MODULE=on` is set.

The benchmarks package has been placed inside its own module so that its dependencies are not considered Zap's dependencies. This changes the README update script to run the benchmark inside the benchmark module's directory since we can no longer invoke it from Zap's top-level directory.

This switches the Makefile and travis setup to rely on Go modules and `./...` instead of `glide nv`. Note that a tradeoff of making `benchmarks/` its own module is that `./...` in the Zap directory will no longer include the `benchmarks/` directory. So where necessary, we need to run commands separately in that directory as well, which complicates the build script ever so slightly.

Rather than hard-coding the Go version we lint against in the Makefile, this switches to managing that in the .travis.yml -- the same place we manage the versions we're testing against.

This was removed in #751.

abhinav added 2 commits October 29, 2019 15:51

global_test: Fix for modules

b94a57a

With Go modules, the location of global_test on disk does not have to be in `$GOPATH/src/go.uber.org/zap`, or even in a directory named `zap`. This simplifies the assertion to verify just the file name.

abhinav requested a review from prashantv October 29, 2019 22:59

abhinav force-pushed the abg/modules branch 3 times, most recently from 3c19977 to 3c78cc9 Compare October 29, 2019 23:38

prashantv approved these changes Oct 30, 2019

View reviewed changes

abhinav force-pushed the abg/modules branch from 3c78cc9 to 3efe245 Compare October 30, 2019 00:33

abhinav added 4 commits October 29, 2019 17:46

ci: Manage lint decision in travis.yml

39ba1b2

Rather than hard-coding the Go version we lint against in the Makefile, this switches to managing that in the .travis.yml -- the same place we manage the versions we're testing against.

abhinav force-pushed the abg/modules branch from 3efe245 to 39ba1b2 Compare October 30, 2019 00:46

abhinav merged commit 4065717 into master Oct 30, 2019

abhinav deleted the abg/modules branch October 30, 2019 00:54

abhinav mentioned this pull request Oct 30, 2019

modules: include go module information #742

Closed

github-actions bot mentioned this pull request Apr 10, 2022

Link Checker Report scylladb/scylla-manager#3102

Closed

github-actions bot mentioned this pull request Apr 17, 2022

Link Checker Report scylladb/scylla-manager#3121

Closed

abhinav pushed a commit that referenced this pull request Jun 15, 2022

CONTRIBUTING: Remove mention of LINTABLE_MINOR_VERSIONS (#1109)

c58bc94

This was removed in #751.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for Go modules #751

Add support for Go modules #751

abhinav commented Oct 29, 2019

codecov bot commented Oct 29, 2019 •

edited

Loading

prashantv left a comment

prashantv Oct 29, 2019

abhinav Oct 30, 2019

prashantv Oct 29, 2019

abhinav Oct 30, 2019

prashantv Oct 30, 2019

abhinav Oct 30, 2019

prashantv Oct 30, 2019

abhinav Oct 30, 2019

prashantv Oct 30, 2019

abhinav Oct 30, 2019

abhinav commented Oct 30, 2019

Add support for Go modules #751

Add support for Go modules #751

Conversation

abhinav commented Oct 29, 2019

codecov bot commented Oct 29, 2019 • edited Loading

Codecov Report

prashantv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abhinav commented Oct 30, 2019

codecov bot commented Oct 29, 2019 •

edited

Loading