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

build: Few minor makefile and dockerfile improvements #10970

Merged
merged 5 commits into from Apr 16, 2020

Conversation

errordeveloper
Copy link
Contributor

No description provided.

@errordeveloper errordeveloper added kind/cleanup This includes no functional changes. area/bugtool Impacts gathering of data for debugging purposes. area/CI-improvement Topic or proposal to improve the Continuous Integration workflow release-note/ci This PR makes changes to the CI. labels Apr 14, 2020
@errordeveloper errordeveloper requested a review from a team as a code owner April 14, 2020 15:08
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 14, 2020
@errordeveloper

This comment has been minimized.

@errordeveloper errordeveloper force-pushed the pr/errordeveloper/makefile-improvements branch from f33c4b8 to e754069 Compare April 14, 2020 15:12
@errordeveloper

This comment has been minimized.

@coveralls
Copy link

coveralls commented Apr 14, 2020

Coverage Status

Coverage increased (+0.02%) to 46.791% when pulling 64aee22 on pr/errordeveloper/makefile-improvements into 3e5ced9 on master.

@errordeveloper errordeveloper force-pushed the pr/errordeveloper/makefile-improvements branch from e754069 to 87b6e14 Compare April 14, 2020 18:23
@errordeveloper errordeveloper requested a review from a team April 14, 2020 18:23
@errordeveloper errordeveloper requested a review from a team as a code owner April 14, 2020 18:23
@errordeveloper errordeveloper requested a review from a team April 14, 2020 18:23
@errordeveloper
Copy link
Contributor Author

test-me-please

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

LGTM, regarding copyright headers I think @joestringer was looking into having a smaller version of them

Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Cool, any metrics yet on whether this changes anything? Build times anywhere? Binary sizes? I realize this isn't intended as an optimization, but it'd be nice to know that we don't see any kind of regressions.

A few minor comments attached.

cilium-docker-plugin.Dockerfile Show resolved Hide resolved
cilium-docker-plugin.Dockerfile Show resolved Hide resolved
Documentation/Makefile Show resolved Hide resolved
Makefile.defs Show resolved Hide resolved
Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

The Hubble related changes look good to me! Great cleanup! A +1 for me on the SPDX identifiers.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Apr 15, 2020

Cool, any metrics yet on whether this changes anything? Build times anywhere? Binary sizes?

The fact that we got rid of dependency declaration will have some effect on build times, I should go and find out what it is, but I suspect the effect to be marginal, but it may as well end up being equal to time it takes to call find :) In the best case scenario, the local incremental builds will be faster, but CI is unlikely to be affected. We will need to do more to make use of Go build cache in CI, and in Dockerized builds more generally.

@errordeveloper
Copy link
Contributor Author

errordeveloper commented Apr 15, 2020

Ok, so more on build performance.

So here is an example of how things look on master now:

$ time make -C operator clean
rm -f cilium-operator
CGO_ENABLED=0 go clean 
make -C operator clean  1.03s user 1.23s system 651% cpu 0.347 total
$ time make -C operator      
CGO_ENABLED=0 go build  -ldflags '-X "github.com/cilium/cilium/pkg/version.Version=1.7.90 263fbd5df 2020-04-10T17:43:59+02:00 go version go1.14 darwin/amd64" -s -w -X "github.com/cilium/cilium/pkg/envoy.RequiredEnvoyVersionSHA=a3385205ad620550b35d3b0b651e40898386e6e3" -X "github.com/cilium/cilium/pkg/datapath/loader.DatapathSHA="'  -o cilium-operator
make -C operator  4.82s user 2.36s system 247% cpu 2.892 total
$ time make -C operator 
make: `cilium-operator' is up to date.
make -C operator  0.23s user 0.05s system 100% cpu 0.283 total
$ 

With this change it looks more like this:

 $ time make -C operator clean
rm -f cilium-operator
go clean 
make -C operator clean  0.97s user 1.18s system 673% cpu 0.320 total
$ time make -C operator      
CGO_ENABLED=0 go build  -ldflags ' -X "github.com/cilium/cilium/pkg/version.Version=1.7.90 10674d0b2 2020-04-15T19:08:26+01:00 go version go1.14 darwin/amd64" -s -w -X "github.com/cilium/cilium/pkg/envoy.RequiredEnvoyVersionSHA=a3385205ad620550b35d3b0b651e40898386e6e3" -X "github.com/cilium/cilium/pkg/datapath/loader.DatapathSHA=" '  -o cilium-operator
make -C operator  4.21s user 2.36s system 249% cpu 2.626 total
$ time make -C operator 
CGO_ENABLED=0 go build  -ldflags ' -X "github.com/cilium/cilium/pkg/version.Version=1.7.90 10674d0b2 2020-04-15T19:08:26+01:00 go version go1.14 darwin/amd64" -s -w -X "github.com/cilium/cilium/pkg/envoy.RequiredEnvoyVersionSHA=a3385205ad620550b35d3b0b651e40898386e6e3" -X "github.com/cilium/cilium/pkg/datapath/loader.DatapathSHA=" '  -o cilium-operator
make -C operator  1.62s user 1.90s system 677% cpu 0.520 total
$

So there is no more make: cilium-operator' is up to date.`, but in my view that was flawed anyway.

The way dependencies are checked is largely inaccurate and hard to maintain (i.e. makefile of each of the components looked for .go files in some set of directories (probably most of the cases just initial set of deps that never got updated), and it ignore all of the external dependencies.

We could fix the nature of dependency calculation and base it on go list, but that shouldn't make any practical different over relying on go build, which does the same thing for us already.

@errordeveloper errordeveloper force-pushed the pr/errordeveloper/makefile-improvements branch from 87b6e14 to 447b0a1 Compare April 15, 2020 18:49
@errordeveloper
Copy link
Contributor Author

test-me-please

- remove unused `V` argument
- remove unused `PKG_BUILD` variable declaration
- `CGO_ENABLED=0` is now the default
- `GOOS=linux` is redundant
- `-a` is meaningless in contaniner build
- `-installsuffix cgo` was a red herring

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
- ensure the copyright header is present
- improve variable naming in `Makefile.defs`
- standardise go invocations with `GO_BUILD`, `GO_TEST`, etc
- remove dependency declaration as `go build` manages build
  cache much more intelligently now and makefile maintenance
  overhead is unnecessary
- ensure all of the common rules are defined consistently,
  so these can be sourced into a common file in the future
- remove trailing and doubly blank lines

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
- ensure the copyright header is present
- remove trailing and doubly blank lines

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
Replace APL header with `SPDX-License-Identifier: Apache-2.0`

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/makefile-improvements branch from 447b0a1 to 64aee22 Compare April 16, 2020 12:47
@errordeveloper
Copy link
Contributor Author

test-me-please

@errordeveloper
Copy link
Contributor Author

test-with-kernel

@errordeveloper
Copy link
Contributor Author

@joestringer this is good to go now, I believe? If so, would you mind approving and merging please?

@joestringer joestringer merged commit 38e3063 into master Apr 16, 2020
1.8.0 automation moved this from In progress to Merged Apr 16, 2020
@joestringer joestringer deleted the pr/errordeveloper/makefile-improvements branch April 16, 2020 21:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bugtool Impacts gathering of data for debugging purposes. area/CI-improvement Topic or proposal to improve the Continuous Integration workflow kind/cleanup This includes no functional changes. release-note/ci This PR makes changes to the CI.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

6 participants