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

Use faster build for the default minikube target #5749

Merged
merged 3 commits into from
Nov 6, 2019

Conversation

afbjorklund
Copy link
Collaborator

When we are doing the "cross" compile targets, we
force rebuilding all packages (up-to-date or not).

For the default native target "out/minikube", we
can do a much faster build (10x!), for development.

Leaving all the cross target to rebuild, as before.

Also the _test.go files are only used for unit tests.


On my machine it takes about 50 seconds, to rebuild all the 645 packages...

real	0m47.801s
user	2m36.732s
sys	0m8.620s

Doing an incremental build takes only 6 seconds, and is much more usable.

real	0m6.077s
user	0m7.368s
sys	0m0.928s

Interesting flags to go build:

The build flags are shared by the build, clean, get, install, list, run,
and test commands:

	-a
		force rebuilding of packages that are already up-to-date.
	-n
		print the commands but do not run them.
	-v
		print the names of packages as they are compiled.
	-x
		print the commands.

I used -v to get the 645 lines.

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Oct 27, 2019
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: afbjorklund

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 27, 2019
@codecov-io
Copy link

codecov-io commented Oct 27, 2019

Codecov Report

Merging #5749 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #5749   +/-   ##
=======================================
  Coverage   36.58%   36.58%           
=======================================
  Files         110      110           
  Lines        8096     8096           
=======================================
  Hits         2962     2962           
  Misses       4745     4745           
  Partials      389      389

@tstromberg tstromberg added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Oct 28, 2019
@tstromberg
Copy link
Contributor

@minikube-bot OK to test

@tstromberg
Copy link
Contributor

That's a really impressive improvement. I look forward to merging this :)

@medyagh
Copy link
Member

medyagh commented Oct 28, 2019

the mac os test failures I seen on other PRs, not related to this PR

@afbjorklund
Copy link
Collaborator Author

Need to add at least go.mod to the list of dependencies, when not rebuilding everything.

When we are doing the "cross" compile targets, we
force rebuilding all packages (up-to-date or not).

For the default native target "out/minikube", we
can do a much faster build (10x!), for development.

Leaving all the cross target to rebuild, as before.

Also the _test.go files are only used for unit tests.
When not rebuilding everything all the time, we need to check
the go.mod for changes to the list of dependencies (and rebuild).
Since the docker image is always running Linux amd64, we need to
pass the actual local target to build (cross-compile) minikube for.
@afbjorklund
Copy link
Collaborator Author

Also had broken the default build in Docker, i.e. BUILD_IN_DOCKER=y make

@minikube-bot
Copy link
Collaborator

Old binary: [177.933725752 182.630983241 192.264802579]
New binary: [176.728872042 193.192061635 173.00130522]
Average Old: 184.276504
Average New: 180.974080

@medyagh
Copy link
Member

medyagh commented Nov 6, 2019

afbjorklund please merge this when you feel ready !

@afbjorklund afbjorklund merged commit be865e5 into kubernetes:master Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. ok-to-test Indicates a non-member PR verified by an org member that is safe to test. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants