Skip to content

All should buld the docker images (and therefore the binaries and protos via dependancies), and not the exes directly. #1399

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
May 20, 2019
Merged

All should buld the docker images (and therefore the binaries and protos via dependancies), and not the exes directly. #1399

merged 4 commits into from
May 20, 2019
+12,276 −17

Conversation

@tomwilkie
Copy link
Contributor

@tomwilkie tomwilkie commented May 18, 2019 •
edited
Loading

Also:

  • reduce the spam when building by hiding the docker run commands.
  • check in the protos so Cortex can be used as a dependancy (fixes Check in proto generated go files #1371).
  • TODO: verify generated proto code in CI

On master, running make builds the binaries twice and enters the build container too often.

>>>> Entering build container: exes
make: Entering directory '/go/src/github.com/cortexproject/cortex'
protoc -I /go/src:./vendor:./pkg/chunk/storage --gogoslick_out=plugins=grpc:./pkg/chunk/storage ./pkg/chunk/storage/caching_index_client.proto
protoc -I /go/src:./vendor:./pkg/ingester/client --gogoslick_out=plugins=grpc:./pkg/ingester/client ./pkg/ingester/client/cortex.proto
protoc -I /go/src:./vendor:./pkg/querier/frontend --gogoslick_out=plugins=grpc:./pkg/querier/frontend ./pkg/querier/frontend/frontend.proto
protoc -I /go/src:./vendor:./pkg/distributor --gogoslick_out=plugins=grpc:./pkg/distributor ./pkg/distributor/ha_tracker.proto
protoc -I /go/src:./vendor:./pkg/ring --gogoslick_out=plugins=grpc:./pkg/ring ./pkg/ring/ring.proto
CGO_ENABLED=0 go build -ldflags "-extldflags \"-static\" -s -w" -tags netgo -o cmd/cortex/cortex ./cmd/cortex
CGO_ENABLED=0 go build -ldflags "-extldflags \"-static\" -s -w" -tags netgo -o cmd/test-exporter/test-exporter ./cmd/test-exporter
make: Leaving directory '/go/src/github.com/cortexproject/cortex'

real	2m31.642s
user	0m0.031s
sys	0m0.016s

>>>> Entering build container: generated
make: Entering directory '/go/src/github.com/cortexproject/cortex'
make: Nothing to be done for 'generated'.
make: Leaving directory '/go/src/github.com/cortexproject/cortex'

real	0m5.574s
user	0m0.033s
sys	0m0.018s

>>>> Entering build container: cmd/cortex/cortex
make: Entering directory '/go/src/github.com/cortexproject/cortex'
CGO_ENABLED=0 go build -ldflags "-extldflags \"-static\" -s -w" -tags netgo -o cmd/cortex/cortex ./cmd/cortex
make: Leaving directory '/go/src/github.com/cortexproject/cortex'

real	0m39.034s
user	0m0.027s
sys	0m0.017s

docker build --build-arg=revision=d0f8944cf7b03fc6a0c1f8a315ecffe28787c8e7 -t quay.io/cortexproject/cortex cmd/cortex/
...

Which this change we don't:

>>>> Entering build container: generated
make: Entering directory '/go/src/github.com/cortexproject/cortex'
protoc -I /go/src:./vendor:./pkg/chunk/storage --gogoslick_out=plugins=grpc:./pkg/chunk/storage ./pkg/chunk/storage/caching_index_client.proto
protoc -I /go/src:./vendor:./pkg/ingester/client --gogoslick_out=plugins=grpc:./pkg/ingester/client ./pkg/ingester/client/cortex.proto
protoc -I /go/src:./vendor:./pkg/querier/frontend --gogoslick_out=plugins=grpc:./pkg/querier/frontend ./pkg/querier/frontend/frontend.proto
protoc -I /go/src:./vendor:./pkg/distributor --gogoslick_out=plugins=grpc:./pkg/distributor ./pkg/distributor/ha_tracker.proto
protoc -I /go/src:./vendor:./pkg/ring --gogoslick_out=plugins=grpc:./pkg/ring ./pkg/ring/ring.proto
make: Leaving directory '/go/src/github.com/cortexproject/cortex'

real	0m12.988s
user	0m0.030s
sys	0m0.020s

>>>> Entering build container: cmd/cortex/cortex
make: Entering directory '/go/src/github.com/cortexproject/cortex'
CGO_ENABLED=0 go build -ldflags "-extldflags \"-static\" -s -w" -tags netgo -o cmd/cortex/cortex ./cmd/cortex
make: Leaving directory '/go/src/github.com/cortexproject/cortex'

real	1m56.313s
user	0m0.032s
sys	0m0.018s

docker build --build-arg=revision=d0f8944cf7b03fc6a0c1f8a315ecffe28787c8e7 -t quay.io/cortexproject/cortex cmd/cortex/
...

Signed-off-by: Tom Wilkie [email protected]

@tomwilkie
All should buld the docker images (and therefore the binaries and pro…
ebcd2d0 
…tos via dependancies), and not the exes directly.

Also, reduce the spam when building by hiding the docker run commands.

Signed-off-by: Tom Wilkie <[email protected]>
@tomwilkie
Copy link
Contributor Author

tomwilkie commented May 18, 2019

Remaining problem is that successive calls to make end up rebuilding when there are no changes. I don't think this can be solved without reverting to building the protos separately.

tomwilkie and others added 2 commits May 20, 2019 10:55
gouthamve
gouthamve approved these changes May 20, 2019
View reviewed changes
Makefile
# Dependencies (i.e. things that go in the image) still need to be explicitly
# declared.
%/$(UPTODATE): %/Dockerfile
@echo
Copy link
Contributor

@gouthamve gouthamve May 20, 2019

Choose a reason for hiding this comment

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

What's this change for?

Copy link
Contributor Author

@tomwilkie tomwilkie May 20, 2019

Choose a reason for hiding this comment

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

I put some new lines in the output of the makefile to make it easier to follow what it was doing. I like them, but not super attached. WDYT?

Copy link
Contributor

@gouthamve gouthamve May 20, 2019

Choose a reason for hiding this comment

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

Makes sense, thanks! LGTM!!!!

Makefile
@mkdir -p $(shell pwd)/.pkg
@mkdir -p $(shell pwd)/.cache
$(SUDO) time docker run $(RM) $(TTY) -i \
@echo
Copy link
Contributor

@gouthamve gouthamve May 20, 2019

Choose a reason for hiding this comment

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

Same here.

Makefile
@mkdir -p $(shell pwd)/.cache
DB_CONTAINER="$$(docker run -d -e 'POSTGRES_DB=configs_test' postgres:9.6)"; \
$(SUDO) docker run $(RM) $(TTY) -i \
@echo
Copy link
Contributor

@gouthamve gouthamve May 20, 2019

Choose a reason for hiding this comment

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

Same here.

@tomwilkie tomwilkie merged commit 345a280 into master May 20, 2019
@tomwilkie tomwilkie deleted the makefile branch May 20, 2019 13:58
@pstibrany pstibrany mentioned this pull request Oct 12, 2020
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@gouthamve gouthamve gouthamve approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Check in proto generated go files

3 participants

@tomwilkie @gouthamve