Skip to content

Commit bbc380f

Browse files
Groxxyiminc
authored andcommitted
Clean up / optimize makefile, sandbox builds better. (#474)
- Removed a bunch of cruft from overly-generic makefile copypasta (many of it doesn't do anything here, and makes it harder to read). - Corrected a number of dependency chains. Let me know if I missed something! I've tried to be thorough, but it's hard to be sure. in any case, now it shouldn't re-do work unless it's necessary, and it can even init in parallel (`make -j4`) from a fresh clone without any problems I've seen. - I was unable to trick Glide into including the necessary packages / versions to reliably build things out of vendor... so now there's a `versioned_go_build.sh` script to do that. It'll also warn you if there's a newer version of tool X available. - Dep can do this pretty easily, add the target command to `require`... though I'm not sure what that implies for users-of-this-lib. Can investigate later. - If you change the version vars, it'll automatically build the new version and rerun as needed. And they're not removed with `make clean`, which helps keep you from needing to be online to build tools. - Last but not least: `go test ./...` is equivalent to (or better than) testing each package separately, and is often *much* faster because it doesn't spend time rebuilding everything. And now that we're on Go 1.10, it can do coverage across all packages too! --- Before all changes: ``` $ make clean rm -rf cadence-client rm -Rf ./build rm -f dummy $ time make rm -rf .gen go get './vendor/go.uber.org/thriftrw' go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc' thriftrw --plugin=yarpc --pkg-prefix=go.uber.org/cadence/.gen/go/ --out=.gen/go idl/github.com/uber/cadence/cadence.thrift thriftrw --plugin=yarpc --pkg-prefix=go.uber.org/cadence/.gen/go/ --out=.gen/go idl/github.com/uber/cadence/shared.thrift go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly go build -i -o dummy internal/cmd/dummy/dummy.go ok go.uber.org/cadence/internal 4.175s coverage: 66.6% of statements ok go.uber.org/cadence/internal/common/backoff 1.046s coverage: 93.2% of statements ok go.uber.org/cadence/internal/common/cache 1.462s coverage: 73.1% of statements ok go.uber.org/cadence/internal/common/metrics 1.055s coverage: 85.0% of statements ok go.uber.org/cadence/internal/common/util 1.024s coverage: 5.2% of statements ok go.uber.org/cadence/mocks 1.037s coverage: 6.7% of statements real 0m20.177s user 0m11.994s sys 0m5.099s $ time make rm -rf .gen go get './vendor/go.uber.org/thriftrw' go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc' thriftrw --plugin=yarpc --pkg-prefix=go.uber.org/cadence/.gen/go/ --out=.gen/go idl/github.com/uber/cadence/cadence.thrift thriftrw --plugin=yarpc --pkg-prefix=go.uber.org/cadence/.gen/go/ --out=.gen/go idl/github.com/uber/cadence/shared.thrift go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly ok go.uber.org/cadence/internal 4.177s coverage: 66.8% of statements ok go.uber.org/cadence/internal/common/backoff 1.046s coverage: 93.2% of statements ok go.uber.org/cadence/internal/common/cache 1.460s coverage: 73.1% of statements ok go.uber.org/cadence/internal/common/metrics 1.051s coverage: 85.0% of statements ok go.uber.org/cadence/internal/common/util 1.027s coverage: 5.2% of statements ok go.uber.org/cadence/mocks 1.038s coverage: 6.7% of statements real 0m18.736s user 0m10.713s sys 0m4.605s ``` After all changes: ``` $ make clean rm -Rf .build rm -Rf .gen $ time make ./versioned_go_build.sh go.uber.org/thriftrw v1.11.0 .build/thriftrw ./versioned_go_build.sh go.uber.org/yarpc v1.29.1 encoding/thrift/thriftrw-plugin-yarpc .build/thriftrw-plugin-yarpc go.uber.org/yarpc has a newer tag: v1.30.0 thriftrw: idl/github.com/uber/cadence/cadence.thrift go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly touch .build/copyright go build -i -o .build/dummy internal/cmd/dummy/dummy.go go test ./... -coverprofile=.build/cover.out -race ? go.uber.org/cadence [no test files] ? go.uber.org/cadence/activity [no test files] ? go.uber.org/cadence/client [no test files] ? go.uber.org/cadence/encoded [no test files] ok go.uber.org/cadence/internal 4.188s coverage: 66.8% of statements ? go.uber.org/cadence/internal/cmd/dummy [no test files] ? go.uber.org/cadence/internal/cmd/tools/copyright [no test files] ? go.uber.org/cadence/internal/common [no test files] ok go.uber.org/cadence/internal/common/backoff 1.060s coverage: 93.2% of statements ok go.uber.org/cadence/internal/common/cache 1.490s coverage: 73.1% of statements ok go.uber.org/cadence/internal/common/metrics 1.065s coverage: 85.0% of statements ok go.uber.org/cadence/internal/common/util 1.037s coverage: 5.2% of statements ok go.uber.org/cadence/mocks 1.048s coverage: 6.7% of statements ? go.uber.org/cadence/testsuite [no test files] ? go.uber.org/cadence/worker [no test files] ? go.uber.org/cadence/workflow [no test files] real 0m34.329s user 0m25.309s sys 0m18.131s $ time make go test ./... -coverprofile=.build/cover.out -race ? go.uber.org/cadence [no test files] ? go.uber.org/cadence/activity [no test files] ? go.uber.org/cadence/client [no test files] ? go.uber.org/cadence/encoded [no test files] ok go.uber.org/cadence/internal 4.175s coverage: 66.8% of statements ? go.uber.org/cadence/internal/cmd/dummy [no test files] ? go.uber.org/cadence/internal/cmd/tools/copyright [no test files] ? go.uber.org/cadence/internal/common [no test files] ok go.uber.org/cadence/internal/common/backoff 1.062s coverage: 93.2% of statements ok go.uber.org/cadence/internal/common/cache 1.500s coverage: 73.1% of statements ok go.uber.org/cadence/internal/common/metrics 1.060s coverage: 85.0% of statements ok go.uber.org/cadence/internal/common/util 1.034s coverage: 5.2% of statements ok go.uber.org/cadence/mocks 1.042s coverage: 6.7% of statements ? go.uber.org/cadence/testsuite [no test files] ? go.uber.org/cadence/worker [no test files] ? go.uber.org/cadence/workflow [no test files] real 0m6.635s user 0m8.372s sys 0m2.819s ``` So the net effect is a slightly slower initial build, and a much faster second+ build. Seems worth it :)
1 parent 87a4ffa commit bbc380f

File tree

3 files changed

+249
-73
lines changed

3 files changed

+249
-73
lines changed

.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14,3 +14,5 @@ bins
1414
test
1515
test.log
1616
/dummy
17+
.build
18+
.bins

Makefile

Lines changed: 75 additions & 73 deletions
Original file line numberDiff line numberDiff line change
@@ -1,112 +1,115 @@
11
.PHONY: test bins clean cover cover_ci
2-
PROJECT_ROOT = go.uber.org/cadence
3-
4-
export PATH := $(GOPATH)/bin:$(PATH)
5-
6-
THRIFT_GENDIR=.gen
72

83
# default target
94
default: test
105

11-
# define the list of thrift files the service depends on
12-
# (if you have some)
13-
THRIFTRW_SRCS = \
14-
idl/github.com/uber/cadence/cadence.thrift \
15-
idl/github.com/uber/cadence/shared.thrift \
6+
IMPORT_ROOT := go.uber.org/cadence
7+
THRIFT_GENDIR := .gen/go
8+
THRIFTRW_SRC := idl/github.com/uber/cadence/cadence.thrift
9+
# one or more thriftrw-generated file(s), to create / depend on generated code
10+
THRIFTRW_OUT := $(THRIFT_GENDIR)/cadence/idl.go
11+
TEST_ARG ?= -coverprofile=$(BUILD)/cover.out -race
1612

17-
PROGS = cadence-client
18-
TEST_ARG ?= -race -v -timeout 5m
19-
BUILD := ./build
13+
# general build-product folder, cleaned as part of `make clean`
14+
BUILD := .build
15+
# general bins folder. NOT cleaned via `make clean`
16+
BINS := .bins
2017

21-
THRIFT_GEN=$(GOPATH)/bin/thrift-gen
18+
# Automatically gather all srcs + a "sentinel" thriftrw output file (which forces generation).
19+
ALL_SRC := $(THRIFTRW_OUT) $(shell \
20+
find . -name "*.go" | \
21+
grep -v \
22+
-e vendor/ \
23+
-e .gen/ \
24+
-e .build/ \
25+
)
2226

23-
define thriftrwrule
24-
THRIFTRW_GEN_SRC += $(THRIFT_GENDIR)/go/$1/$1.go
25-
26-
$(THRIFT_GENDIR)/go/$1/$1.go:: $2
27-
@mkdir -p $(THRIFT_GENDIR)/go
28-
$(ECHO_V)thriftrw --plugin=yarpc --pkg-prefix=$(PROJECT_ROOT)/$(THRIFT_GENDIR)/go/ --out=$(THRIFT_GENDIR)/go $2
29-
endef
27+
# Files that needs to run lint. excludes testify mocks and the thrift sentinel.
28+
LINT_SRC := $(filter-out ./mock% $(THRIFTRW_OUT),$(ALL_SRC))
3029

31-
$(foreach tsrc,$(THRIFTRW_SRCS),$(eval $(call \
32-
thriftrwrule,$(basename $(notdir \
33-
$(shell echo $(tsrc) | tr A-Z a-z))),$(tsrc))))
30+
THRIFTRW_VERSION := v1.11.0
31+
YARPC_VERSION := v1.29.1
32+
GOLINT_VERSION := 470b6b0bb3005eda157f0275e2e4895055396a81
3433

35-
# Automatically gather all srcs
36-
ALL_SRC := $(shell find . -name "*.go" | grep -v -e Godeps -e vendor \
37-
-e ".*/\..*" \
38-
-e ".*/_.*")
34+
# versioned tools. just change the version vars above, it'll automatically trigger a rebuild.
35+
$(BINS)/versions/thriftrw-$(THRIFTRW_VERSION):
36+
./versioned_go_build.sh go.uber.org/thriftrw $(THRIFTRW_VERSION) $@
3937

40-
# Files that needs to run lint, exclude testify mock from lint
41-
LINT_SRC := $(filter-out ./mock%,$(ALL_SRC))
38+
$(BINS)/versions/yarpc-$(YARPC_VERSION):
39+
./versioned_go_build.sh go.uber.org/yarpc $(YARPC_VERSION) encoding/thrift/thriftrw-plugin-yarpc $@
4240

43-
# all directories with *_test.go files in them
44-
TEST_DIRS := $(sort $(dir $(filter %_test.go,$(ALL_SRC))))
41+
$(BINS)/versions/golint-$(GOLINT_VERSION):
42+
./versioned_go_build.sh golang.org/x/lint $(GOLINT_VERSION) golint $@
4543

46-
vendor:
47-
glide install
44+
# stable tool targets. depend on / execute these instead of the versioned ones.
45+
# this versioned-to-nice-name thing is mostly because thriftrw depends on the yarpc
46+
# bin to be named "thriftrw-plugin-yarpc".
47+
$(BINS)/thriftrw: $(BINS)/versions/thriftrw-$(THRIFTRW_VERSION)
48+
@ln -fs $(CURDIR)/$< $@
4849

49-
yarpc-install: vendor
50-
go get './vendor/go.uber.org/thriftrw'
51-
go get './vendor/go.uber.org/yarpc/encoding/thrift/thriftrw-plugin-yarpc'
50+
$(BINS)/thriftrw-plugin-yarpc: $(BINS)/versions/yarpc-$(YARPC_VERSION)
51+
@ln -fs $(CURDIR)/$< $@
5252

53-
clean_thrift:
54-
rm -rf .gen
53+
$(BINS)/golint: $(BINS)/versions/golint-$(GOLINT_VERSION)
54+
@ln -fs $(CURDIR)/$< $@
5555

56-
thriftc: clean_thrift yarpc-install $(THRIFTRW_GEN_SRC)
56+
vendor: vendor/glide.updated
5757

58-
copyright: ./internal/cmd/tools/copyright/licensegen.go
59-
go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly
60-
61-
vendor/glide.updated: glide.lock glide.yaml
58+
vendor/glide.updated: glide.lock
6259
glide install
6360
touch vendor/glide.updated
6461

65-
dummy: vendor/glide.updated $(ALL_SRC)
66-
go build -i -o dummy internal/cmd/dummy/dummy.go
67-
68-
test: bins
69-
@rm -f test
70-
@rm -f test.log
71-
@for dir in $(TEST_DIRS); do \
72-
go test -race -coverprofile=$@ "$$dir" | tee -a test.log; \
73-
done;
62+
$(THRIFTRW_OUT): $(THRIFTRW_SRC) $(BINS)/thriftrw $(BINS)/thriftrw-plugin-yarpc
63+
@echo 'thriftrw: $(THRIFTRW_SRC)'
64+
@mkdir -p $(dir $@)
65+
@# needs to be able to find the thriftrw-plugin-yarpc bin in PATH
66+
@PATH="$(BINS)" \
67+
$(BINS)/thriftrw \
68+
--plugin=yarpc \
69+
--pkg-prefix=$(IMPORT_ROOT)/$(THRIFT_GENDIR) \
70+
--out=$(THRIFT_GENDIR) \
71+
$(THRIFTRW_SRC)
7472

75-
bins: thriftc copyright lint dummy
73+
clean_thrift:
74+
rm -rf .gen
7675

77-
cover_profile: clean copyright lint vendor/glide.updated
76+
# `make copyright` or depend on "copyright" to force-run licensegen,
77+
# or depend on $(BUILD)/copyright to let it run as needed.
78+
copyright $(BUILD)/copyright: $(ALL_SRC)
7879
@mkdir -p $(BUILD)
79-
@echo "mode: atomic" > $(BUILD)/cover.out
80+
go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly
81+
@touch $(BUILD)/copyright
82+
83+
$(BUILD)/dummy: vendor/glide.updated $(ALL_SRC)
84+
go build -i -o $@ internal/cmd/dummy/dummy.go
85+
86+
test $(BUILD)/cover.out: $(BUILD)/copyright $(BUILD)/dummy $(ALL_SRC)
87+
go test ./... $(TEST_ARG)
8088

81-
@echo Testing packages:
82-
@for dir in $(TEST_DIRS); do \
83-
mkdir -p $(BUILD)/"$$dir"; \
84-
go test "$$dir" $(TEST_ARG) -coverprofile=$(BUILD)/"$$dir"/coverage.out || exit 1; \
85-
cat $(BUILD)/"$$dir"/coverage.out | grep -v "mode: atomic" >> $(BUILD)/cover.out; \
86-
done;
89+
bins: $(ALL_SRC) $(BUILD)/copyright lint $(BUILD)/dummy
8790

88-
cover: cover_profile
91+
cover: $(BUILD)/cover.out
8992
go tool cover -html=$(BUILD)/cover.out;
9093

91-
cover_ci: cover_profile
94+
cover_ci: $(BUILD)/cover.out
9295
goveralls -coverprofile=$(BUILD)/cover.out -service=travis-ci || echo -e "\x1b[31mCoveralls failed\x1b[m";
9396

9497
# golint fails to report many lint failures if it is only given a single file
95-
# to work on at a time. and we can't exclude files from its checks, so for
96-
# best results we need to give it a whitelist of every file in every package
97-
# that we want linted.
98+
# to work on at a time, and it can't handle multiple packages at once, *and*
99+
# we can't exclude files from its checks, so for best results we need to give
100+
# it a whitelist of every file in every package that we want linted, per package.
98101
#
99102
# so lint + this golint func works like:
100-
# - iterate over all dirs (outputs "./folder/")
103+
# - iterate over all lintable dirs (outputs "./folder/")
101104
# - find .go files in a dir (via wildcard, so not recursively)
102105
# - filter to only files in LINT_SRC
103106
# - if it's not empty, run golint against the list
104107
define lint_if_present
105-
test -n "$1" && golint -set_exit_status $1
108+
test -n "$1" && $(BINS)/golint -set_exit_status $1
106109
endef
107110

108-
lint:
109-
@$(foreach pkg,\
111+
lint: $(BINS)/golint $(ALL_SRC)
112+
$(foreach pkg,\
110113
$(sort $(dir $(LINT_SRC))), \
111114
$(call lint_if_present,$(filter $(wildcard $(pkg)*.go),$(LINT_SRC))) || ERR=1; \
112115
) test -z "$$ERR" || exit 1
@@ -121,6 +124,5 @@ fmt:
121124
@gofmt -w $(ALL_SRC)
122125

123126
clean:
124-
rm -rf cadence-client
125127
rm -Rf $(BUILD)
126-
rm -f dummy
128+
rm -Rf .gen

versioned_go_build.sh

Lines changed: 172 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,172 @@
1+
#!/bin/bash
2+
3+
set -euf -o pipefail
4+
5+
# In a nutshell, this script:
6+
# - makes a tempdir and moves to it
7+
# - go gets the requested bin (but does not install it)
8+
# - cds to the repo
9+
# - checks out the requested version
10+
# - maybe runs `glide install`
11+
# - builds the bin and puts it where you told it to.
12+
#
13+
# Since doing that verbatim is a bit noisy, and pinning tools tends to
14+
# cause them to slowly get out of date, it does two additional things:
15+
# - suppresses output unless `--debug` is passed
16+
# - checks for newer commits / tags than the one you passed, and
17+
# prints the newer sha/version (if any) to stderr so it's always visible.
18+
19+
usage () {
20+
echo 'Installs a specific version of a go-gettable bin to the specified location.'
21+
echo ''
22+
echo 'Usage:'
23+
echo ''
24+
echo -e "\\t$0 [--debug] go-gettable-repo version [path-to-bin-in-repo] install-location"
25+
echo ''
26+
echo 'Examples:'
27+
echo ''
28+
echo -e "\\t$0 go.uber.org/thriftrw 1.10.0 somewhere/thriftrw"
29+
echo -e '\t\tInstalls v1.10.0 of go.uber.org/thriftrw to somewhere/thriftrw.'
30+
echo -e '\t\tNotice that go.uber.org/thriftrw is both the repository AND the binary name.'
31+
echo ''
32+
echo -e "\\t$0 golang.org/x/lint SOME_SHA golint somewhere/golint"
33+
echo -e '\t\tInstalls a specific SHA of e.g. "golang.org/x/lint/golint" to "somewhere/golint",'
34+
echo -e '\t\tNotice that the golint bin is in a subfolder of the golang.org/x/lint repository.'
35+
36+
exit 1
37+
}
38+
39+
num_commits_behind () {
40+
head=$1
41+
git rev-list "..$head" | wc -l | tr -dc '0-9'
42+
}
43+
44+
is_versioned () {
45+
# returns truthy if the arg is version-like.
46+
47+
str=$1
48+
if echo "$str" | grep -q -E "$VERSION_TAG_REGEX"; then
49+
return 0
50+
fi
51+
return 1
52+
}
53+
54+
most_recent_version_tag () {
55+
# using xargs because it's safer (for big lists) + it doesn't result in
56+
# a ton of SHAs in debug output like `git describe --tags $(...)` causes.
57+
#
58+
# in brief:
59+
# - get all tagged shas
60+
# - get their tags
61+
# - grep for version-like tags
62+
# - sort by version
63+
# - return the "biggest"
64+
65+
git rev-list --tags \
66+
| xargs git describe --always --tags 2>/dev/null \
67+
| grep -E "$VERSION_TAG_REGEX" \
68+
| sort -Vr \
69+
| head -n 1
70+
}
71+
72+
# matches "v1[.2[.3[.4]]]" exactly or fails.
73+
# this intentionally does not match "v1.2.3-ae83c2" tags, nor "v1.2-pre", etc,
74+
# as these are not likely "real" release versions.
75+
declare -r VERSION_TAG_REGEX='^v?\d+(\.\d+){0,3}$'
76+
77+
# output control vars
78+
declare DEBUG=
79+
declare TO_DEV_NULL=
80+
declare QUIET=
81+
82+
# build control vars
83+
declare -x GOPATH
84+
declare GO_GETTABLE_REPO
85+
declare VERSION
86+
declare GO_GETTABLE_BIN
87+
declare INSTALL_LOCATION
88+
89+
# handle optional debug (and consume the arg)
90+
if [ "$1" == "--debug" ]; then
91+
shift # consume it
92+
DEBUG=1
93+
else
94+
# otherwise, redirect to /dev/null (requires eval)
95+
TO_DEV_NULL='>/dev/null'
96+
# pass quiet flags where needed (do not quote)
97+
QUIET='--quiet'
98+
fi
99+
100+
# must have 3 or 4 args
101+
[ $# -ge 3 ] || usage
102+
[ $# -le 4 ] || usage
103+
104+
[ -z $DEBUG ] || set -x
105+
106+
#
107+
# Set up some variables / the temp folder
108+
#
109+
110+
# set up gopath, and make sure it's cleaned up regardless of how we exit
111+
GOPATH=$(mktemp -d)
112+
trap 'rm -rf $GOPATH' EXIT
113+
114+
GO_GETTABLE_REPO="$1"
115+
VERSION="$2"
116+
if [ $# -eq 4 ]; then
117+
# bin resides in a sub-folder
118+
GO_GETTABLE_BIN="$1/$3"
119+
INSTALL_LOCATION="$4"
120+
elif [ $# -eq 3 ]; then
121+
# repo == bin
122+
GO_GETTABLE_BIN="$1"
123+
INSTALL_LOCATION="$3"
124+
else
125+
# should be unreachable
126+
usage
127+
fi
128+
129+
#
130+
# Pull the repo, set to the correct version
131+
#
132+
133+
go get -d "$GO_GETTABLE_BIN"
134+
# eval so redirection works when quiet
135+
eval "pushd $GOPATH/src/$GO_GETTABLE_REPO $TO_DEV_NULL"
136+
137+
# save for the version check
138+
HEAD="$(git rev-parse HEAD)"
139+
140+
# silently check out, reduces a lot of default spam
141+
git checkout $QUIET "$VERSION"
142+
143+
#
144+
# Check if we're building an old version, warn if so
145+
#
146+
147+
if is_versioned "$VERSION"; then
148+
# versioned, check for newer tags.
149+
LATEST="$(most_recent_version_tag)"
150+
# use sort to check if VERSION >= LATEST, or warn about the newer version
151+
echo -e "$VERSION\\n$LATEST" | sort -Vrc 2>/dev/null || (>&2 echo -e "\\t$GO_GETTABLE_REPO has a newer tag: $LATEST")
152+
else
153+
# not versioned, check for newer commits
154+
BEHIND="$(num_commits_behind "$HEAD")"
155+
# should be zero, or warn about the newer version
156+
[ "$BEHIND" -eq 0 ] || (>&2 echo -e "\\t$GO_GETTABLE_REPO is $BEHIND commits behind the current HEAD: $HEAD")
157+
fi
158+
159+
#
160+
# Build the bin, install to the correct location
161+
#
162+
163+
# only glide install when there is a glide file, or it tries to install
164+
# to the current repo (not in our current folder)
165+
if [ -f glide.lock ]; then
166+
glide $QUIET install
167+
fi
168+
169+
# eval so redirection works when quiet
170+
eval "popd $TO_DEV_NULL"
171+
172+
go build -o "$INSTALL_LOCATION" "$GO_GETTABLE_BIN"

0 commit comments

Comments
 (0)