-
Notifications
You must be signed in to change notification settings - Fork 33
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
Move all build logic and dependencies installs to Makefile #155
Conversation
@@ -3,26 +3,7 @@ language: go | |||
go: | |||
- 1.7 | |||
before_install: | |||
- go get golang.org/x/tools/cmd/cover | |||
- go get github.com/axw/gocov/gocov |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i removed those "magic" installs as they don't seem to be used anymore
SOURCES := $(shell find $(SOURCEDIR) -name '*.go') | ||
APP_SOURCES := $(shell find $(SOURCEDIR) -name '*.go' -not -path '$(SOURCEDIR)/vendor/*') | ||
VERSION=$(shell cat .goxc.json | python -c "import json,sys;obj=json.load(sys.stdin);print obj['PackageVersion'];") | ||
|
||
PATH := $(CURRENTDIR)/bin:$(PATH) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
setting this path once for all tests, thanks to running builds with consul on travis we got +10% to coverage for free ;)
|
||
all: build | ||
|
||
deps: | ||
@./install_consul.sh | ||
|
||
build: deps test | ||
build-deps: deps format test check |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
build runs goimports
before starting build
There is a problem with tarvis build. I restarted it but it failed again. |
...and now it passed. Either we get back to not running |
|
||
check: gometalint-exists $(SOURCES) | ||
check: check-deps $(SOURCES) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should follow standard targets so check should do style checking and testing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ack
@which gover > /dev/null || \ | ||
(go get github.com/modocache/gover) | ||
|
||
test-coverage: coverage-deps $(SOURCES) $(COVERAGE_TARGETS) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer to keep one test target. I think test-coverage
looks better then current test
, the output is cleaner and gives information about coverage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, that's reasonable, it doesn't seem to be slower
I think we should merge it. I think build hang could be not related to changes you made. |
I know its not related to changes, but we might not be happy with builds failing randomly because of consul. |
713cfb8
to
424e3d4
Compare
travis.ci configuration uses make commands to be able to replicate CI builds on local machine
424e3d4
to
26407f5
Compare
@janisz fixed, now we have 2 targets: |
travis.ci configuration uses make commands to be able to replicate CI builds
on local machine