Makefile improvements #2660
Makefile improvements #2660
Conversation
twz123
force-pushed
the
parallel-make-builds
branch
from
1f3ea52 to
c961ca9
Compare
|
@twz123 Could we keep buildmode=None behavior somehow? |
You mean |
|
@twz123 I was confused with "Remove unused "buildmode" make variable" but it seems to be about other thing, nvm |
| @@ -53,7 +53,7 @@ jobs: | |||
| - name: Build Tool | |||
| run: | | |||
| cd hack/tool | |||
| make | |||
| make --jobs=$(nproc) | |||
There was a problem hiding this comment.
I don't really understand how here and everywhere in the diff -j helps to parallelize jobs, majority of the targets are single external commands and all of the make calls uses only one target.
There was a problem hiding this comment.
This is to ensure that k0s actually supports parallel builds, not so much to speed up things. That might prevent bugs like #2654. Parallel builds are advertised in the README, so we should ensure that there are no regressions.
It's so easy to hack on the Makefiles and do something that works flawlessly in sequential builds, but will break when run in parallel.
There was a problem hiding this comment.
Other option is to change the readme and drop the claim, because in fact we do not support parallel build more than go build toolchain supports it. Makefile in our case basically just a job runner, not the build tool per se. Claiming that we support parallel build by passing -j is not something that exactly reflects the reality.
There was a problem hiding this comment.
It actually speeds things up a bit when building the embedded binaries (subjectively 15-20% on the GH runners). We can also drop the support for parallel builds, but then I think we should explicitly say that in the README.
@ncopa: You added the note about parallel builds. Any thoughts on that?
There was a problem hiding this comment.
15-20% is a significant difference. We could also keep the -j for the embedded binaries call (I guess it's coming from make k0s?) but drop the claim and don't put -j everywhere. On the other hand, I am not sure if claim in the readme has anything to share with our GHA workflows.
There was a problem hiding this comment.
I think that ensuring support for parallel builds will also help us make sure that partial builds always works. I think its worth the extra complexity.
twz123
force-pushed
the
parallel-make-builds
branch
3 times, most recently
from
eb8ab70 to
837f24b
Compare
|
I'll revisit this once changes around #2909 have been sorted out. |
twz123
force-pushed
the
parallel-make-builds
branch
from
837f24b to
8f8cfde
Compare
|
This pull request has merge conflicts that need to be resolved. |
twz123
force-pushed
the
parallel-make-builds
branch
from
479e09c to
b447678
Compare
twz123
force-pushed
the
parallel-make-builds
branch
from
b447678 to
ac54421
Compare
Ensure that the binaries are moved into place, instead of sequentially written. Use a temporary untar folder for that, and touch the file there before moving it. Signed-off-by: Tom Wieczorek <[email protected]>
Signed-off-by: Tom Wieczorek <[email protected]>
Place client-gen and controller-gen generated files into their own temporary folders and move them into their proper places afterwards. Don't delete any files from the source directories. This ensures that parallel builds work as expected and tools won't encounter any "missing" Go packages during builds. (There's still a small potential race for client-gen. The whole directory needs to be replaced, which mv refuses to do. So move the to- be-replaced directory out of the way, move the new directory in place, and delete the old directory afterwards.) Signed-off-by: Tom Wieczorek <[email protected]>
Amongst other things, the generated assets embed the controller-gen generated CRDs into the k0s binary. Ensure that all the CRDs have been generated _before_ generating the embedded assets, so that everything is guaranteed to be up to date, especially when using parallel make builds. Signed-off-by: Tom Wieczorek <[email protected]>
Signed-off-by: Tom Wieczorek <[email protected]>
twz123
force-pushed
the
parallel-make-builds
branch
from
ac54421 to
c0b6ef8
Compare
|
The PR is marked as stale since no activity has been recorded in 30 days |
|
@ncopa would you mind to have another look? |
Description
Improve Docker export in embedded-binaries Makefile
Ensure that the binaries are moved into place, instead of sequentially written. Use a temporary untar folder for that, and touch the file there before moving it.
Remove unused "buildmode" make variable
Don't remove auto-generated files during builds
Place client-gen and controller-gen generated files into their own temporary folders and move them into their proper places afterwards. Don't delete any files from the source directories. This ensures that parallel builds work as expected and tools won't encounter any "missing" Go packages during builds.
(There's still a small potential race for client-gen. The whole directory needs to be replaced, which mv refuses to do. So move the to- be-replaced directory out of the way, move the new directory in place, and delete the old directory afterwards.)
Let generated assets depend on controller-gen output
Amongst other things, the generated assets embed the controller-gen generated CRDs into the k0s binary. Ensure that all the CRDs have been generated before generating the embedded assets, so that everything is guaranteed to be up to date, especially when using parallel make builds.
Let CI use parallel builds
Fixes #2654.
Type of change
How Has This Been Tested?
Checklist: