Skip to content

Commit

Permalink
Makefile cleanup / codegen revamp / gobin removed (#3903)
Browse files Browse the repository at this point in the history
A few "safe" / simple changes to the Makefile, in prep for future changes.

Specifically, this includes:
- made `// ` (with trailing space) license comments into `//` so they do not conflict with `goimports`
- moving "build" to ".build" so it's hidden and automatically ignored by most things.
  - in particular this _slightly_ improves the odds that the first build (no build-folder at parse time) and the Nth build (build folder already exists) behave the same, as e.g. `ALL_SRC` will implicitly ignore it now.
- `ifndef` -> `?=` because that's basically what they're doing.
  - this does mean that `go env GOOS` may run multiple times, as `?=` acts like `=` and not `:=`, but it's fast enough to not be noticeable (single-digit ms).
  - I did not change `TEST_TAG`, as the `override` is needed to modify the var when it's set via `make TEST_TAG=adsf targets...`
- `ALL_SRC` now uses a find query that does not even descend into vendor / etc, rather than filtering them out afterward.
  - this is visibly faster when the files are not in caches, and will essentially remain fast regardless of how large the project grows.
  - future changes can just modify the find filter (it's pretty simple) or add a `grep -v` to the end like before.
- got rid of `TOOLS_SRC` because it was completely incorrect: the tools bins depend on cadence source outside of the tools folder.
  - the copyright bin is the only exception, and its target dependency doesn't use `TOOLS_SRC` anyway.
- revamped how thrift sources are generated:
  - `*.thrift` files are found rather than listed.  guaranteed to be complete + smaller = perhaps better than a handwritten list.
  - switched to using a "normal" multi-target rule rather than define+foreach+eval+call.  hopefully this is simpler to understand and modify.
  - `--no-recurse` + separate target files = you can safely generate thrift in parallel!  try `touch idls/thrift/cadence.thrift` + `make .gen/go/cadence/cadence.go -j8` to see this in action.
    - `make -j8` in general does not yet work though, as other dependencies are not declared correctly, e.g. copyright is not run when appropriate.  that's a goal for later commits...
- got rid of `gobin`, now using go.mod
  - this approach keeps runtime-library-versions in sync with codegen-library-versions, which is important for some libraries, like yarpc and mockgen.
    - other tools _could_ be moved to a separately-versioned tools.mod for greater flexibility, but that is not done here.
  - I have not yet adapted the protobuf changes, but will do that soon
  - to update tools, `go get -u` them, like normal.  to add tools, add them to `cmd/server/tools.go` (this file will probably be moved soon)
- some changes to protoc to align it with everything else
- minor cleanup of the buildkite dockerfile + adding `unzip` because now it's needed.  I suspect there's a lot more possible here, but I haven't explored.

After pulling, you'll need to manually delete your `build` folder.  Generated code / copyright headers / etc should all be identical after this change, though the actual golint / yarpc / etc binaries are not necessarily _identical_ due to incomplete versioning before.
  • Loading branch information
Groxx authored Jan 22, 2021
1 parent fe4b48d commit 49553cb
Show file tree
Hide file tree
Showing 82 changed files with 486 additions and 365 deletions.
18 changes: 9 additions & 9 deletions .buildkite/pipeline.yml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ steps:
docker: "*"
command: "make cover_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -25,7 +25,7 @@ steps:
docker: "*"
command: "make cover_integration_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -40,7 +40,7 @@ steps:
docker: "*"
command: "make cover_integration_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -55,7 +55,7 @@ steps:
docker: "*"
command: "make cover_ndc_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -70,7 +70,7 @@ steps:
docker: "*"
command: "make cover_integration_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -85,7 +85,7 @@ steps:
docker: "*"
command: "make cover_ndc_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -100,7 +100,7 @@ steps:
docker: "*"
command: "make cover_integration_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand All @@ -115,7 +115,7 @@ steps:
docker: "*"
command: "make cover_ndc_profile"
artifact_paths:
- "build/coverage/*.out"
- ".build/coverage/*.out"
retry:
automatic:
limit: 1
Expand Down Expand Up @@ -150,4 +150,4 @@ steps:
plugins:
- docker-login#v2.0.1:
username: ubercadence
password-env: DOCKER_LOGIN_PASSWORD
password-env: DOCKER_LOGIN_PASSWORD
2 changes: 1 addition & 1 deletion .dockerignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
build/
.build/
cadence
cadence-server
cadence-canary
Expand Down
12 changes: 7 additions & 5 deletions .gen/go/admin/admin.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 8 additions & 6 deletions .gen/go/admin/adminserviceclient/client.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions .gen/go/admin/adminservicefx/client.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .gen/go/admin/adminservicefx/doc.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions .gen/go/admin/adminservicefx/server.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions .gen/go/admin/adminserviceserver/server.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

8 changes: 5 additions & 3 deletions .gen/go/admin/adminservicetest/client.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 6 additions & 4 deletions .gen/go/cadence/cadence.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

12 changes: 7 additions & 5 deletions .gen/go/cadence/workflowserviceclient/client.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions .gen/go/cadence/workflowservicefx/client.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions .gen/go/cadence/workflowservicefx/doc.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading

0 comments on commit 49553cb

Please sign in to comment.