Add some dependency logic around generators. #1966
Conversation
k8s-ci-robot
added
cncf-cla: yes
| $(BINDIR)/lister-gen: .init | ||
| $(DOCKER_CMD) go build -o $@ $(SC_PKG)/vendor/k8s.io/code-generator/cmd/lister-gen | ||
| .PHONY: generators | ||
| generators: $(GENERATORS) |
There was a problem hiding this comment.
what's this target for? not used anywhere
There was a problem hiding this comment.
Mostly a convenience, to be able to say "make generators" from the command line. But I've taken your suggestion to reference it throughout instead of $(GENERATORS).
Makefile
Outdated
|
|
||
| $(BINDIR)/openapi-gen: vendor/k8s.io/code-generator/cmd/openapi-gen | ||
| $(DOCKER_CMD) go build -o $@ $(SC_PKG)/$^ | ||
| $(BINDIR)/%-gen: $$(shell find vendor/k8s.io/code-generator/cmd/$$*-gen vendor/k8s.io/gengo) .init |
There was a problem hiding this comment.
notes, maybe need some comments to quickstart people's minds when reading
depend on something-gen in gengo
Makefile
Outdated
| $(BINDIR)/openapi-gen: vendor/k8s.io/code-generator/cmd/openapi-gen | ||
| $(DOCKER_CMD) go build -o $@ $(SC_PKG)/$^ | ||
| $(BINDIR)/%-gen: $$(shell find vendor/k8s.io/code-generator/cmd/$$*-gen vendor/k8s.io/gengo) .init | ||
| $(DOCKER_CMD) go build -o $@ $(SC_PKG)/vendor/k8s.io/code-generator/cmd/$*-gen |
There was a problem hiding this comment.
I think this is now clear, with the comment I've added above. But let me know if you disagree.
Makefile
Outdated
|
|
||
| .PHONY: $(BINDIR)/e2e.test | ||
| $(BINDIR)/e2e.test: .init | ||
| $(DOCKER_CMD) go test -c -o $@ $(SC_PKG)/test/e2e | ||
|
|
||
| # Regenerate all files if the gen exes changed or any "types.go" files changed | ||
| .generate_files: .init .generate_exes $(TYPES_FILES) | ||
| .generate_files: .init $(GENERATORS) $(TYPES_FILES) |
There was a problem hiding this comment.
why use the variable vs the target defined above?
There was a problem hiding this comment.
No strong reason -- generally I was favoring eliminating indirection, but in practice it's no easier to understand. Done.
|
if this is an optimization people need, I guess I'm fine with it. I'd prefer to let golang handle the "is it necessary to rebuild" decision. It gets better in v1.10, but we can't move to that yet. |
|
@n3wscott was asking for this; IIRC he had an issue where he accidentally committed code generated by stale generator binaries. This should be more correct than the previous implementation. |
This isn't 100% correct, but it's a pretty good approximation. Specifically, it doesn't know about transitive dependencies from other go packages. But it'll rebuild a generator if any code inside its package changes (or build them all if anything in gengo changes).
|
on golang 1.10 and it's handling the binary dependency thing and caching. |
| # on everything under its source tree as well as gengo's. This uses GNU Make's | ||
| # secondary expansion feature to pass $* to `find`. | ||
| $(BINDIR)/%-gen: $$(shell find vendor/k8s.io/code-generator/cmd/$$*-gen vendor/k8s.io/gengo) .init | ||
| $(DOCKER_CMD) go build -o $@ $(SC_PKG)/vendor/k8s.io/code-generator/cmd/$*-gen |
There was a problem hiding this comment.
nice comment. this is neat. I'm wondering if the find is the right practice vs having golang handle it vs handling it like upstream k/k does with the go2mk generator.
|
altogether, I cannot see any reason not to do this. |
k8s-ci-robot
added
lgtm
|
/assign @jberkhahn /assign @jboyd01 |
|
/cc @jberkhahn @jboyd01 |
|
this seems to work fine on Mac /lgtm |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: MHBauer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/retest |
This isn't 100% correct, but it's a pretty good approximation.
Specifically, it doesn't know about transitive dependencies from other
go packages. But it'll rebuild a generator if any code inside its
package changes (or build them all if anything in gengo changes).