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

Add a fmt build target and make use of a go build cache. #1558

Merged
merged 2 commits into from
Dec 17, 2018

Conversation

ijc
Copy link
Contributor

@ijc ijc commented Dec 10, 2018

A couple of (unrelated) tweaks to the build.

Firstly add a make -f dockerfile.Makefile fmt target which runs gofmt on all the non-vendored Go code as a developer convenience.

Secondly add (optional, but defaulted on) support for using a go build cache when using the dockerfile.Makefile targets. This speeds up repeated builds for me by 6x (over 1m to less than 10s), according to time make build -f docker.Makefile DOCKER_BUILDKIT=1 GO_BUILD_CACHE=y (or =n).

Signed-off-by: Ian Campbell <ijc@docker.com>
@codecov-io
Copy link

codecov-io commented Dec 10, 2018

Codecov Report

Merging #1558 into master will decrease coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1558      +/-   ##
==========================================
- Coverage   55.24%   55.23%   -0.02%     
==========================================
  Files         289      289              
  Lines       19381    19391      +10     
==========================================
+ Hits        10707    10710       +3     
- Misses       7978     7984       +6     
- Partials      696      697       +1

@silvin-lubecki
Copy link
Contributor

I'm not sure about the fmt target, which I think should be an IDE configuration for all golangs projects.
But 👍 for the go build cache.

@ijc
Copy link
Contributor Author

ijc commented Dec 10, 2018

The main benefit of the fmt target is it always uses the right version of gofmt from within the container (which matches the lint target etc), whereas an IDE might be picking up a different one from the host system.

docker.Makefile Outdated Show resolved Hide resolved
docker.Makefile Outdated Show resolved Hide resolved
With a docker build cache already primed with the build image I am seeing
`time make build -f docker.Makefile DOCKER_BUILDKIT=1 GO_BUILD_CACHE=n` takes
more than 1 minute.

By contrast `time make build -f docker.Makefile DOCKER_BUILDKIT=1
GO_BUILD_CACHE=y` takes less than 10s with a hot cache irrespective of whether
the source tree has changed

Signed-off-by: Ian Campbell <ijc@docker.com>
@ijc
Copy link
Contributor Author

ijc commented Dec 14, 2018

Is this mergeable @thaJeztah @silvin-lubecki or are there other changes you'd like to see?

@@ -21,6 +21,10 @@ test: test-unit ## run tests
test-coverage: ## run test coverage
./scripts/test/unit-with-coverage $(shell go list ./... | grep -vE '/vendor/|/e2e/')

.PHONY: fmt
fmt:
go list -f {{.Dir}} ./... | xargs gofmt -w -s -d
Copy link
Member

Choose a reason for hiding this comment

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

oh, actually; do we need to exclude vendor here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not according to my testing when I wrote it:

$ go list -f {{.Dir}} ./... | grep vendor
$

Copy link
Member

Choose a reason for hiding this comment

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

Interesting; wonder why it's needed in the other parts of the Makefile then 🤔. Maybe if they're running from inside a different directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it wasn't always the case with older versions of the go tooling. I suspect the two places which grep the vendor dir out are just out of date, but they both also want to exclude e2e so there isn't a whole lot of worthwhile cleanup to be done.

Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@thaJeztah thaJeztah merged commit 298c423 into docker:master Dec 17, 2018
@GordonTheTurtle GordonTheTurtle added this to the 19.03.0 milestone Dec 17, 2018
@ijc ijc deleted the build-tweaks branch January 7, 2019 15:39
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.

5 participants