Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Go module/1.11.x support, vendored deps, cttestsrv api fixes. #74

Merged
merged 11 commits into from
Dec 18, 2018

Conversation

cpu
Copy link
Contributor

@cpu cpu commented Dec 15, 2018

  • Fixes the cttestsrv API breakage with Trillian master from time source refactoring.
  • Updates the project/CI to use Go 1.11.x
  • Configures a ct-woodpecker module
  • Vendors dependencies[0]
  • Removes errcheck[1] from CI
  • Updates CI to use vendored deps throughout
  • Fixes a few typos flagged by Goreportcard

Resolves #72 and #73 and fixes CI.

[0]: Notably this pins Trillian to d0c8d5f - the tip of master at the time of writing. We're using changes that aren't in the latest v1.2.1 tag. Similarly, Trillian at tip of master requires us to pin golang/protobuf at 1d3f30b - the tip of master for that project at the time of writing. Trillian is using changes in this project that aren't in the latest v1.2.0 tag.

[1]: Errcheck doesn't work with Go modules and our vendored deps. Adding it back is blocked on kisielk/errcheck#155 Personally I think the stability improvements of vendored dependencies merits removing errcheck from Travis and the cron builds. We do still get errcheck coverage per-PR with GolangCI.

cpu added 8 commits December 15, 2018 09:10
Notably this pins Trillian to d0c8d5f - the tip of master at the time
of writing. We're using changes that aren't in the latest v1.2.1
tag.

Similarly, Trillian at tip of master requires us to pin golang/protobuf
at 1d3f30b - the tip of master for that project at the time of writing.
Trillian is using changes in this project that aren't in the latest
v1.2.0 tag.
@cpu cpu self-assigned this Dec 15, 2018
@coveralls
Copy link

coveralls commented Dec 15, 2018

Pull Request Test Coverage Report for Build 484

  • 1 of 1 (100.0%) changed or added relevant line in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.3%) to 67.515%

Totals Coverage Status
Change from base Build 365: 0.3%
Covered Lines: 769
Relevant Lines: 1139

💛 - Coveralls

.travis.yml Outdated
- go get github.com/kisielk/errcheck
- go get ./...
# Install `cover` and `goveralls` without `GO111MODULE` enabled so that we
# don't download project dependencies and just put the tools in our gobin.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think go get with GO111MODULE=off still fetches dependencies, since that's what go get has always done, right? If we wanted to segregate the dependencies of these tools I think we'd need to set GOPATH to some alternate path.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think go get with GO111MODULE=off still fetches dependencies, since that's what go get has always done, right?

It downloads the dependencies needed by these tools, into the default $GOPATH ($HOME/go) and that's what I wanted.

I didn't make it very clear in my comment but with GO111MODULE=on running go get github.com/cpu/some-tool-we-need in the ct-woodpecker project root with Go 1.11+ would download all of the dependencies for ct-woodpecker and some-tool-we-need. I don't really understand why that is the case but what I wanted was to just let a default GOPATH be initialized with the various utils we need and their deps, without fetching ct-woodpecker's deps. I also didn't want to vendor the tools in ct-woodpecker.

Does that make sense? I could try to better capture this in the comment.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohhh I misread "project dependencies" to mean the opposite - the dependecies of cover and goveralls. I think if you just replace "project" with "ct-woodpecker" it becomes clear (though I also don't understand why Go has the behavior you described)

Copy link
Contributor Author

@cpu cpu Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think if you just replace "project" with "ct-woodpecker" it becomes clear

Fixed in 55860dc

I also don't understand why Go has the behavior you described

Thinking about it more it happens because running go get in a module enabled project with GO111MODULE=on means you want go get to be updating go.mod to track the new dependency. I think at the same time it adds the new dependency it fetches the others. I thought about trying to run the go get outside of the project dir instead but while researching the answer to this I see that would have caused its own unique problem specifically because go get would have failed to find the go.mod to update: https://github.com/golang/go/wiki/Modules#why-does-installing-a-tool-via-go-get-fail-with-error-cannot-find-main-module

@jsha jsha merged commit c6dde3c into master Dec 18, 2018
@jsha jsha deleted the cpu-vendor-deps branch December 18, 2018 18:57
@cpu cpu mentioned this pull request Dec 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix cttestsrv breakage from NewSequencer api change
3 participants