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

Faster make v2 #3724

Merged
merged 7 commits into from
Feb 28, 2020
Merged

Faster make v2 #3724

merged 7 commits into from
Feb 28, 2020

Conversation

dgageot
Copy link
Contributor

@dgageot dgageot commented Feb 24, 2020

This solves multiple slowness issues with the current Makefile:

  • Some variables are eagerly evaluated even if they are not used by the current target.
  • make generate-statik should make its inputs and outputs explicit so that we can avoid the cost of regenerating

The net result is that make and make install will be faster to find out that nothing needs to be rebuilt and will be faster to rebuild changes to the Go code.

@nkubala FYI, this is a fixed version of #3706

This makes the Makefile faster for other targets.

Signed-off-by: David Gageot <[email protected]>
We don’t need a target to go build for every GOOS
since this done by `make cross`.

Signed-off-by: David Gageot <[email protected]>
At release time, we build in a Docker container
so the path is predictable.

Signed-off-by: David Gageot <[email protected]>
@codecov
Copy link

codecov bot commented Feb 24, 2020

Codecov Report

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

@@ -28,10 +28,11 @@ GCP_PROJECT ?= k8s-skaffold
GKE_CLUSTER_NAME ?= integration-tests
GKE_ZONE ?= us-central1-a

SUPPORTED_PLATFORMS := linux-$(GOARCH) darwin-$(GOARCH) windows-$(GOARCH).exe
SUPPORTED_PLATFORMS = linux-$(GOARCH) darwin-$(GOARCH) windows-$(GOARCH).exe
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After going through http://www.gnu.org/software/make/manual/make.html#Flavors,
I feel simply expanded variables and make more sense in this Makefile.

Is there a reason why you changed to "recursively expanded" ?

Makefile Show resolved Hide resolved
@tejal29 tejal29 self-assigned this Feb 27, 2020
@tejal29
Copy link
Contributor

tejal29 commented Feb 28, 2020

Testing notes:
Make is definitely faster

tejaldesai@@skaffold (faster-make-v2)$ time make
hack/generate-statik.sh
Collecting licenses
Collecting schemas
GOOS=linux GOARCH=amd64 CGO_ENABLED=1 go build -tags "osusergo netgo static_build release" -ldflags " -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.version=v1.4.0-11-g15c669938-dirty -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.buildDate=2020-02-27T15:22:04Z -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitCommit=15c6699381e1dca6e5f1faa939df350576912668 -X github.com/GoogleContainerTools/skaffold/pkg/skaffold/version.gitTreeState=dirty -s -w  -extldflags \"-static\"" -o out/skaffold github.com/GoogleContainerTools/skaffold/cmd/skaffold

real	0m26.161s
user	0m48.223s
sys	0m1.393s
tejaldesai@@skaffold (faster-make-v2)$ time make
make: 'out/skaffold' is up to date.

real	0m0.356s
user	0m0.290s
sys	0m0.071s
tejaldesai@@skaffold (faster-make-v2)$ 

Tested the cloudbuild.yaml.

$git clean -fdx cmd/
Removing cmd/skaffold/app/cmd/statik/statik.go

$gcloud builds submit --config deploy/cloudbuild.yaml --substitutions=_RELEASE_BUCKET=tejal-test,COMMIT_SHA=$(git rev-parse HEAD) --project tejal-test
...
Success
TAG_NAME=vtest3724 gcloud builds submit --config deploy/cloudbuild-release.yaml --substitutions=_RELEASE_BUCKET=tejal-test --project tejal-test
..

SUCCESS

Copy link
Contributor

@tejal29 tejal29 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After testing described in #3724 (comment), LGTM!

Thank you!

@dgageot dgageot merged commit 64d8dcb into GoogleContainerTools:master Feb 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants