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 container binaries outside of docker #419

Merged
merged 9 commits into from
Jan 22, 2021

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Jan 21, 2021

Description

Builds the binaries used in the docker images outside of docker.

Why is this needed

Fixes: #368

How Has This Been Tested?

Checked the built binaries with file.

@mmlb mmlb force-pushed the build-container-binaries-outside-docker branch from 71f5d87 to a9be8dd Compare January 21, 2021 19:35
@codecov
Copy link

codecov bot commented Jan 21, 2021

Codecov Report

Merging #419 (24ee42e) into master (f7fc782) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #419   +/-   ##
=======================================
  Coverage   33.08%   33.08%           
=======================================
  Files          24       24           
  Lines        2170     2170           
=======================================
  Hits          718      718           
  Misses       1376     1376           
  Partials       76       76           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f7fc782...24ee42e. Read the comment docs.

@mmlb mmlb changed the title Makefile: Use consistent style for variable deref Build container binaries outside of docker Jan 21, 2021
@mmlb mmlb force-pushed the build-container-binaries-outside-docker branch 4 times, most recently from 46ebdac to 68e87eb Compare January 21, 2021 20:56
@mmlb
Copy link
Contributor Author

mmlb commented Jan 21, 2021

With this PR we get CI result in 1/2 the time!

@mmlb mmlb requested review from gianarb and thebsdbox January 21, 2021 21:04
mmlb added 9 commits January 21, 2021 16:06
I tend to like $() for functions and ${} for variables similar to bash,
but make doesn't differentiate. $() has better syntax highlighting though
so there's that :D.

Signed-off-by: Manuel Mendez <[email protected]>
Change variables to be defined with `:=` so they are "Simply expanded
variables" and not recursive... which basically just means they are
evaluated immediately instead of when used. This is the least surprising
behavior.

While here I changed the names of some variables so its shorter and a
little clearer.

I also dropped use of GOOS as it doesn't seem used and moved CGO_ENABLED
out of the recipe since it'll be re-used soon.

Signed-off-by: Manuel Mendez <[email protected]>
Better settings for a better time.

Signed-off-by: Manuel Mendez <[email protected]>
Use the full name instead of the package path/dir name. Makes things
easier to do the crosscompile case later.

Signed-off-by: Manuel Mendez <[email protected]>
Build all the binaries!

Signed-off-by: Manuel Mendez <[email protected]>
This is much faster than building in docker.

Signed-off-by: Manuel Mendez <[email protected]>
Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb force-pushed the build-container-binaries-outside-docker branch from 68e87eb to 24ee42e Compare January 21, 2021 21:06
Copy link
Contributor

@gianarb gianarb left a comment

Choose a reason for hiding this comment

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

Perfect! Thanks for taking care of this

Copy link
Contributor

@thebsdbox thebsdbox left a comment

Choose a reason for hiding this comment

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

VERY NICE

/lgtm

@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 22, 2021
@mmlb mmlb merged commit d9d9077 into tinkerbell:master Jan 22, 2021
@mmlb mmlb deleted the build-container-binaries-outside-docker branch January 22, 2021 15:44
@mmlb mmlb mentioned this pull request Jan 22, 2021
@mmlb mmlb removed the ready-to-merge Signal to Mergify to merge the PR. label Jan 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Lets go back to building binaries outside of Docker
3 participants