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

Add Makefile for development #67

Merged
merged 3 commits into from
Feb 1, 2019
Merged

Add Makefile for development #67

merged 3 commits into from
Feb 1, 2019

Conversation

bufdev
Copy link
Contributor

@bufdev bufdev commented Jan 31, 2019

This adds a simple Makefile for development.

This Makefile:

  • Sets up everything for Golang Module support, regardless of where everything is checked out
  • Sets up some simple linters (the important ones being golint, errcheck, staticcheck)
  • Sets up testing and code coverage

A data race was also fixed.

Note that errcheck and staticcheck do not pass yet but this is a good start. Let me know if this is helpful!

Makefile Outdated
# your terminal as a command to run, and then see the code coverage output locally.
.PHONY: cover
cover:
$(AT) go install golang.org/x/tools/cmd/cover
Copy link
Owner

Choose a reason for hiding this comment

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

Why do we need to install the cover tool?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well what do you know - you don't! Good catch. You used to need this, I've been copying and pasting this for years effectively heh. I deleted it, and also ran go mod tidy within a clean Docker container.

@bojand
Copy link
Owner

bojand commented Feb 1, 2019

Hello, and thanks for the PR! Looks good, just have the one question about the install of cover tool.

@bojand
Copy link
Owner

bojand commented Feb 1, 2019

Hello thanks for the fix. One more thing... I feel like this may be a dumb question but what's the point of the new tools package with the build constraint? Is it to force install of those tools for build automatically (via go.mod)? If so, similar to the above, I also noticed that we install the tools (ie, golint, errcheck, staticcheck) manually via go install so just want to check that's also necessary?

Example:

errcheck:
	$(AT) go install github.com/kisielk/errcheck
	errcheck -ignoretests $(GO_PKGS)

@bufdev
Copy link
Contributor Author

bufdev commented Feb 1, 2019

Yea that one is a little counterintuitive...so:

Golang Modules need to have a reference to every thing you import to pick them up, it's not like Glide or Dep where you can specify something to always install. Luckily, Golang Modules pick everything up regardless of tag. Note that the tools file has +build tools at the top - this is a dummy tag that will only be built into your code if you did go build -tags tools, which means in practice none of your code will have this as a dependency. Having this dummy tag in a file with blank imports is the "officially supported" way of installing Golang tools with modules, there's an issue on golang/go that describes this.

As to the install - this is necessary to build the binary so that it is available. The install part puts it into GOBIN, which you'll see that at the top of the Makefile we set to be .tmp/$(uname -s)/$(uname -m)/bin, which we then add to the PATH. This makes it available to use, so when we go install errcheck, it goes to this directory, and we can then call our vendored/versioned errcheck from go.mod.

@bojand
Copy link
Owner

bojand commented Feb 1, 2019

Oh right that makes sense. Thanks for the explanation and the PR!

@bojand bojand merged commit 0215b2c into bojand:master Feb 1, 2019
@bufdev bufdev deleted the makefile branch February 1, 2019 17:01
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.

2 participants