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

Clean up / optimize makefile, sandbox builds better. #474

Merged
merged 3 commits into from
May 23, 2018

Conversation

Groxx
Copy link
Contributor

@Groxx Groxx commented May 18, 2018

  • 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 :)

@Groxx Groxx force-pushed the cleaned branch 3 times, most recently from 61105d9 to 3d7fc1f Compare May 18, 2018 03:16
@Groxx
Copy link
Contributor Author

Groxx commented May 18, 2018

... huh. ./versioned_go_build.sh: line 104: DEBUG: unbound variable
bash version difference maybe? can't reproduce. fixed anyway.
and I wonder if grep is old or something on travis, it seems to be interpreting the regex wrong (version tags are being detected as non-versioned). trying a fix and rerunning from what appear to be unrelated test failures.

@coveralls
Copy link

coveralls commented May 18, 2018

Coverage Status

Coverage decreased (-0.08%) to 65.462% when pulling 22c013c on Groxx:cleaned into 87a4ffa on uber-go:master.

@Groxx
Copy link
Contributor Author

Groxx commented May 18, 2018

I'm thinking of making a bins folder, actually. something that's not cleaned, so versioned-bins don't need to be redownloaded just because someone wanted to rebuild.

@Groxx Groxx force-pushed the cleaned branch 2 times, most recently from e0bd209 to 2538985 Compare May 18, 2018 04:11
@Groxx
Copy link
Contributor Author

Groxx commented May 18, 2018

tests are flaky :(

2018-05-18T04:15:25.608Z ERROR internal/internal_task_handlers.go:655 Replay and history mismatch. {"error": "nondeterministic workflow: missing replay decision for ActivityTaskScheduled: (ActivityId:0, Input:[])"}

- 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 :)
Copy link
Contributor

@yiminc-zz yiminc-zz left a comment

Choose a reason for hiding this comment

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

Awesome, thanks for doing this!

@yiminc-zz yiminc-zz merged commit bbc380f into uber-go:master May 23, 2018
yiminc-zz pushed a commit that referenced this pull request May 23, 2018
purplesusan pushed a commit to purplesusan/cadence-client-1 that referenced this pull request May 30, 2018
- 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 :)
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.

None yet

4 participants