Makefile: Remove clean prerequisites from build targets #950
Conversation
|
Can one of the admins verify this patch? |
3 similar comments
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
|
Can one of the admins verify this patch? |
k8s-ci-robot
added
size/XS
|
Actually, hold off on this. We may want to keep |
wking
force-pushed
the
travis-drop-clean
branch
from
e32afd2
to
fb4eb8a
Compare
k8s-ci-robot
added
size/S
wking
changed the title
And it does introduce a possibly-breaking race in parallel I've updated the PR title to reflect the broader change, and I'm back to thinking this is ready for review ;). |
|
ok to test |
| @@ -26,7 +27,6 @@ cross: \ | |||
| _output/bin/linux/s390x/checkpoint | |||
|
|
|||
| release: \ | |||
| clean \ | |||
There was a problem hiding this comment.
Doesn't removing this prevent clean from running on all releases?
There was a problem hiding this comment.
Doesn't removing this prevent
cleanfrom running on all releases?
Yup, and that avoids a parallel-make race. You can always call make clean first, but I don't think we need to (more on why not in 3136f29).
There was a problem hiding this comment.
The intent here is to always ensure release is running in a clean workspace. We should keep this dependency, though calling make clean release is redundant in the Travis script.
I'm confused about why this would cause a race. Dependencies always resolve before the block executes, so release will always wait on clean to finish.
There was a problem hiding this comment.
The intent here is to always ensure release is running in a
cleanworkspace.
I think the release prereq tree is sufficient for this, but if you want to be extra safe we can drop some/all of 3136f29. Let me know how you want that handled.
I'm confused about why this would cause a race. Dependencies always resolve before the block executes, so
releasewill always wait oncleanto finish.
Right, but release has no recipe. All of its prereqs will finish before that no-op recipe, but make makes no guarantees about the order in which the prereqs are resolved. For an example using sleep to slant the race odds the other way, see the 9e18c38 commit message.
| @@ -47,7 +47,7 @@ install: _output/bin/$(LOCAL_OS)/bootkube | |||
| _output/bin/%: GOOS=$(word 1, $(subst /, ,$*)) | |||
| _output/bin/%: GOARCH=$(word 2, $(subst /, ,$*)) | |||
| _output/bin/%: GOARCH:=amd64 # default to amd64 to support release scripts | |||
| _output/bin/%: $(GOFILES) | |||
| _output/bin/%: $(GOFILES) $(VENDOR_GOFILES) | |||
There was a problem hiding this comment.
Instead of adding VENDOR_GOFILES we can probably just drop the dependencies. go build already caches everything and detects what needs to be recompiled.
There was a problem hiding this comment.
Feel free to leave it as is, but I may do a follow up to remove make dependencies.
|
I dunno what's going on with the Calico failure; looks loke it's missing from S3. |
|
Rebased onto master (no conflicts) with 15fad99 -> e5ab3ec, in case a retry gets past the previous Calico flake. |
|
Getting around to all the old PRs. Mind resolving conflict? |
'clean' has been a prerequisite for: * conformance-%, since the target landed in 0520bdf (hack: add conformance tests for hack builds, 2016-05-26, kubernetes-retired#45). * release, since the target landed in 04848d1 (Add release build process, 2016-05-27, kubernetes-retired#47). clean-vm-% target has been a prerequisite for run-% since the target landed in 9e2e89e (run make target, 2016-09-23, kubernetes-retired#141). But there are two problems with including cleaners are prerequisites of build steps: * They are inefficient. Make is good at rebuilding only things that need rebuilding, provided you inform it of your dependencies. Cleaning before every build will often lead to unnecessary rebuilds of up-to-date content. * They are racy. For example, a parallel make invocation may process several prerequisites at once. The current 'clean' recipe will execute quickly, which is why it hasn't been much of a problem yet, but you can see the race by inserting a delay: clean: sleep 60 rm -rf _output and running: $ make -j8 release sleep 60 mkdir -p _output/bin/darwin/ GOOS=darwin GOARCH=amd64 go build ... ... mkdir -p _output/release/ tar czf _output/release/bootkube.tar.gz -C _output bin/linux/bootkube bin/darwin/bootkube bin/linux/checkpoint rm -rf _output with the other preqrequisites completing first, clean's rm ends up removing everything release generated. In this commit, I've removed the clean prerequisites from the build targets to resolve the above issues. We could also update our recommended make invocations to add clean as an explicit earlier step, and I've done that for 'all' and 'run-' to hint at the existence of the clean targets. I don't think we need separate clean calls for 'release' or 'conformance-%' * No need to 'make clean' before calling 'make release' For Travis, the test environment should not include anything in _output at this point, so there's no need to call clean there. The 'clean' target has been included in .travis.yml since 1f34600 (Add travis tests, 2016-05-27, kubernetes-retired#46). And 'clean' had been a 'release' prerequisite since 04848d1 (Add release build process, 2016-05-27, kubernetes-retired#47). But neither of those motivate the Travis use-case, so I think they were just being overly cautious. And for RELEASING, walking the prerequisite chain for release shows that a clean all would have no impact: * check is verifying the source, and does not care about _output. * _output/release/bootkube.tar.gz knows about its dependencies. * _output/bin/% (used for _output/bin/.../bootkube) knows about its GOFILES and (new in this commit) VENDOR_GOFILES dependencies. This is *overly* broad, because it includes cmd/checkpoint/*.go and other non-dependencies, but overly broad is still safe. * The new VENDOR_GOFILES ensures we rebuild _output/bin/... if a vendored dependency is bumped even if no bootkube-specific code is bumped. * No need to 'make clean' before calling 'make conformance-...' The hack/ content only depends on _output for _output/bin/${local_os}/bootkube and _output/bin/linux/bootkube. conformance-% only uses hack/ content, and it depends on all. 'all', in turn, depends on both _output/bin/$(LOCAL_OS)/bootkube and _output/bin/linux/bootkube, and each of those fully specifies their build dependencies. So there's no need for a separate clean step before running the conformance-% targets; Make will rebuild anything that needs rebuilding already.
wking
force-pushed
the
travis-drop-clean
branch
from
e5ab3ec
to
15ef313
Compare
The
cleantarget has been included there since 1f34600 (#46). Butcleanhas been areleaseprerequisite since 04848d1 (#47), so the.travis.ymlentry is redundant.