Skip to content

Commit

Permalink
Clean up / optimize makefile, sandbox builds better. (#474)
Browse files Browse the repository at this point in the history
- 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 :)
  • Loading branch information
Groxx authored and yiminc committed May 23, 2018
1 parent 87a4ffa commit bbc380f
Show file tree
Hide file tree
Showing 3 changed files with 249 additions and 73 deletions.
2 changes: 2 additions & 0 deletions .gitignore
Expand Up @@ -14,3 +14,5 @@ bins
test
test.log
/dummy
.build
.bins
148 changes: 75 additions & 73 deletions Makefile
@@ -1,112 +1,115 @@
.PHONY: test bins clean cover cover_ci
PROJECT_ROOT = go.uber.org/cadence

export PATH := $(GOPATH)/bin:$(PATH)

THRIFT_GENDIR=.gen

# default target
default: test

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

PROGS = cadence-client
TEST_ARG ?= -race -v -timeout 5m
BUILD := ./build
# general build-product folder, cleaned as part of `make clean`
BUILD := .build
# general bins folder. NOT cleaned via `make clean`
BINS := .bins

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

define thriftrwrule
THRIFTRW_GEN_SRC += $(THRIFT_GENDIR)/go/$1/$1.go

$(THRIFT_GENDIR)/go/$1/$1.go:: $2
@mkdir -p $(THRIFT_GENDIR)/go
$(ECHO_V)thriftrw --plugin=yarpc --pkg-prefix=$(PROJECT_ROOT)/$(THRIFT_GENDIR)/go/ --out=$(THRIFT_GENDIR)/go $2
endef
# Files that needs to run lint. excludes testify mocks and the thrift sentinel.
LINT_SRC := $(filter-out ./mock% $(THRIFTRW_OUT),$(ALL_SRC))

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

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

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

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

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

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

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

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

copyright: ./internal/cmd/tools/copyright/licensegen.go
go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly

vendor/glide.updated: glide.lock glide.yaml
vendor/glide.updated: glide.lock
glide install
touch vendor/glide.updated

dummy: vendor/glide.updated $(ALL_SRC)
go build -i -o dummy internal/cmd/dummy/dummy.go

test: bins
@rm -f test
@rm -f test.log
@for dir in $(TEST_DIRS); do \
go test -race -coverprofile=$@ "$$dir" | tee -a test.log; \
done;
$(THRIFTRW_OUT): $(THRIFTRW_SRC) $(BINS)/thriftrw $(BINS)/thriftrw-plugin-yarpc
@echo 'thriftrw: $(THRIFTRW_SRC)'
@mkdir -p $(dir $@)
@# needs to be able to find the thriftrw-plugin-yarpc bin in PATH
@PATH="$(BINS)" \
$(BINS)/thriftrw \
--plugin=yarpc \
--pkg-prefix=$(IMPORT_ROOT)/$(THRIFT_GENDIR) \
--out=$(THRIFT_GENDIR) \
$(THRIFTRW_SRC)

bins: thriftc copyright lint dummy
clean_thrift:
rm -rf .gen

cover_profile: clean copyright lint vendor/glide.updated
# `make copyright` or depend on "copyright" to force-run licensegen,
# or depend on $(BUILD)/copyright to let it run as needed.
copyright $(BUILD)/copyright: $(ALL_SRC)
@mkdir -p $(BUILD)
@echo "mode: atomic" > $(BUILD)/cover.out
go run ./internal/cmd/tools/copyright/licensegen.go --verifyOnly
@touch $(BUILD)/copyright

$(BUILD)/dummy: vendor/glide.updated $(ALL_SRC)
go build -i -o $@ internal/cmd/dummy/dummy.go

test $(BUILD)/cover.out: $(BUILD)/copyright $(BUILD)/dummy $(ALL_SRC)
go test ./... $(TEST_ARG)

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

cover: cover_profile
cover: $(BUILD)/cover.out
go tool cover -html=$(BUILD)/cover.out;

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

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

lint:
@$(foreach pkg,\
lint: $(BINS)/golint $(ALL_SRC)
$(foreach pkg,\
$(sort $(dir $(LINT_SRC))), \
$(call lint_if_present,$(filter $(wildcard $(pkg)*.go),$(LINT_SRC))) || ERR=1; \
) test -z "$$ERR" || exit 1
Expand All @@ -121,6 +124,5 @@ fmt:
@gofmt -w $(ALL_SRC)

clean:
rm -rf cadence-client
rm -Rf $(BUILD)
rm -f dummy
rm -Rf .gen
172 changes: 172 additions & 0 deletions versioned_go_build.sh
@@ -0,0 +1,172 @@
#!/bin/bash

set -euf -o pipefail

# In a nutshell, this script:
# - makes a tempdir and moves to it
# - go gets the requested bin (but does not install it)
# - cds to the repo
# - checks out the requested version
# - maybe runs `glide install`
# - builds the bin and puts it where you told it to.
#
# Since doing that verbatim is a bit noisy, and pinning tools tends to
# cause them to slowly get out of date, it does two additional things:
# - suppresses output unless `--debug` is passed
# - checks for newer commits / tags than the one you passed, and
# prints the newer sha/version (if any) to stderr so it's always visible.

usage () {
echo 'Installs a specific version of a go-gettable bin to the specified location.'
echo ''
echo 'Usage:'
echo ''
echo -e "\\t$0 [--debug] go-gettable-repo version [path-to-bin-in-repo] install-location"
echo ''
echo 'Examples:'
echo ''
echo -e "\\t$0 go.uber.org/thriftrw 1.10.0 somewhere/thriftrw"
echo -e '\t\tInstalls v1.10.0 of go.uber.org/thriftrw to somewhere/thriftrw.'
echo -e '\t\tNotice that go.uber.org/thriftrw is both the repository AND the binary name.'
echo ''
echo -e "\\t$0 golang.org/x/lint SOME_SHA golint somewhere/golint"
echo -e '\t\tInstalls a specific SHA of e.g. "golang.org/x/lint/golint" to "somewhere/golint",'
echo -e '\t\tNotice that the golint bin is in a subfolder of the golang.org/x/lint repository.'

exit 1
}

num_commits_behind () {
head=$1
git rev-list "..$head" | wc -l | tr -dc '0-9'
}

is_versioned () {
# returns truthy if the arg is version-like.

str=$1
if echo "$str" | grep -q -E "$VERSION_TAG_REGEX"; then
return 0
fi
return 1
}

most_recent_version_tag () {
# using xargs because it's safer (for big lists) + it doesn't result in
# a ton of SHAs in debug output like `git describe --tags $(...)` causes.
#
# in brief:
# - get all tagged shas
# - get their tags
# - grep for version-like tags
# - sort by version
# - return the "biggest"

git rev-list --tags \
| xargs git describe --always --tags 2>/dev/null \
| grep -E "$VERSION_TAG_REGEX" \
| sort -Vr \
| head -n 1
}

# matches "v1[.2[.3[.4]]]" exactly or fails.
# this intentionally does not match "v1.2.3-ae83c2" tags, nor "v1.2-pre", etc,
# as these are not likely "real" release versions.
declare -r VERSION_TAG_REGEX='^v?\d+(\.\d+){0,3}$'

# output control vars
declare DEBUG=
declare TO_DEV_NULL=
declare QUIET=

# build control vars
declare -x GOPATH
declare GO_GETTABLE_REPO
declare VERSION
declare GO_GETTABLE_BIN
declare INSTALL_LOCATION

# handle optional debug (and consume the arg)
if [ "$1" == "--debug" ]; then
shift # consume it
DEBUG=1
else
# otherwise, redirect to /dev/null (requires eval)
TO_DEV_NULL='>/dev/null'
# pass quiet flags where needed (do not quote)
QUIET='--quiet'
fi

# must have 3 or 4 args
[ $# -ge 3 ] || usage
[ $# -le 4 ] || usage

[ -z $DEBUG ] || set -x

#
# Set up some variables / the temp folder
#

# set up gopath, and make sure it's cleaned up regardless of how we exit
GOPATH=$(mktemp -d)
trap 'rm -rf $GOPATH' EXIT

GO_GETTABLE_REPO="$1"
VERSION="$2"
if [ $# -eq 4 ]; then
# bin resides in a sub-folder
GO_GETTABLE_BIN="$1/$3"
INSTALL_LOCATION="$4"
elif [ $# -eq 3 ]; then
# repo == bin
GO_GETTABLE_BIN="$1"
INSTALL_LOCATION="$3"
else
# should be unreachable
usage
fi

#
# Pull the repo, set to the correct version
#

go get -d "$GO_GETTABLE_BIN"
# eval so redirection works when quiet
eval "pushd $GOPATH/src/$GO_GETTABLE_REPO $TO_DEV_NULL"

# save for the version check
HEAD="$(git rev-parse HEAD)"

# silently check out, reduces a lot of default spam
git checkout $QUIET "$VERSION"

#
# Check if we're building an old version, warn if so
#

if is_versioned "$VERSION"; then
# versioned, check for newer tags.
LATEST="$(most_recent_version_tag)"
# use sort to check if VERSION >= LATEST, or warn about the newer version
echo -e "$VERSION\\n$LATEST" | sort -Vrc 2>/dev/null || (>&2 echo -e "\\t$GO_GETTABLE_REPO has a newer tag: $LATEST")
else
# not versioned, check for newer commits
BEHIND="$(num_commits_behind "$HEAD")"
# should be zero, or warn about the newer version
[ "$BEHIND" -eq 0 ] || (>&2 echo -e "\\t$GO_GETTABLE_REPO is $BEHIND commits behind the current HEAD: $HEAD")
fi

#
# Build the bin, install to the correct location
#

# only glide install when there is a glide file, or it tries to install
# to the current repo (not in our current folder)
if [ -f glide.lock ]; then
glide $QUIET install
fi

# eval so redirection works when quiet
eval "popd $TO_DEV_NULL"

go build -o "$INSTALL_LOCATION" "$GO_GETTABLE_BIN"

0 comments on commit bbc380f

Please sign in to comment.