Skip to content

Commit

Permalink
Makefile: Remove 'clean' prerequisite from build targets
Browse files Browse the repository at this point in the history
'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.
  • Loading branch information
wking committed Aug 17, 2018
1 parent b66af43 commit 15ef313
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 9 deletions.
2 changes: 1 addition & 1 deletion .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,4 @@ before_script:
go:
- 1.10.x
script:
- make clean release
- make release
11 changes: 7 additions & 4 deletions Documentation/development.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,20 +16,23 @@ cd $(go env GOPATH | cut -d: -f1)/src/github.com/kubernetes-incubator/bootkube
Then build:

```
make clean all
make clean
make all
```

## Local Development Environments

To easily launch local vagrant development clusters:
To easily launch local, single-node vagrant development clusters:

```
# Launch a single-node cluster
make clean-vm-single
make run-single
```

You can also launch a multi-node cluster:

```
# Launch a multi-node cluster
make clean-vm-multi
make run-multi
```

Expand Down
8 changes: 4 additions & 4 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ export PATH:=$(PATH):$(PWD)

LOCAL_OS:=$(shell uname | tr A-Z a-z)
GOFILES:=$(shell find . -name '*.go' ! -path './vendor/*')
VENDOR_GOFILES ?= $(shell find vendor -name '*.go')
LDFLAGS=-X github.com/kubernetes-incubator/bootkube/pkg/version.Version=$(shell $(CURDIR)/build/git-version.sh)
TERRAFORM:=$(shell command -v terraform 2> /dev/null)

Expand All @@ -23,7 +24,6 @@ cross: \
_output/bin/linux/s390x/checkpoint

release: \
clean \
check \
_output/release/bootkube.tar.gz \

Expand All @@ -47,7 +47,7 @@ install:
_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)
mkdir -p $(dir $@)
GOOS=$(GOOS) GOARCH=$(GOARCH) go build $(GOFLAGS) -ldflags "$(LDFLAGS)" -o $@ github.com/kubernetes-incubator/bootkube/cmd/$(notdir $@)

Expand All @@ -56,7 +56,7 @@ _output/release/bootkube.tar.gz: _output/bin/linux/bootkube _output/bin/darwin/b
tar czf $@ -C _output bin/linux/bootkube bin/darwin/bootkube bin/linux/checkpoint

run-%: GOFLAGS = -i
run-%: clean-vm-% _output/bin/linux/bootkube _output/bin/$(LOCAL_OS)/bootkube
run-%: _output/bin/linux/bootkube _output/bin/$(LOCAL_OS)/bootkube
@cd hack/$*-node && ./bootkube-up
@echo "Bootkube ready"

Expand All @@ -68,7 +68,7 @@ clean-vm-%:
rm -rf cluster )

#TODO(aaron): Prompt because this is destructive
conformance-%: clean all
conformance-%: all
@cd hack/$*-node && vagrant destroy -f
@cd hack/$*-node && rm -rf cluster
@cd hack/$*-node && ./bootkube-up
Expand Down

0 comments on commit 15ef313

Please sign in to comment.