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.

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

Fix binary names #122

Merged
merged 7 commits into from
Jan 20, 2021
Merged

Fix binary names #122

merged 7 commits into from
Jan 20, 2021

Conversation

mmlb
Copy link
Contributor

@mmlb mmlb commented Jan 20, 2021 •
edited
Loading

Description

This PR fixes the binary names to be both correct and also "unsurprising".

Why is this needed

Avoids needing to COPY all the binaries, only to keep 1. Also avoids glaringly misnamed binaries :D.

For example these are the binaries built from master:

file boots-* | sed 's|, statically.*||'
boots-linux-aarch64: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV)
boots-linux-amd64:   ELF 64-bit LSB executable, x86-64, version 1 (SYSV)
boots-linux-arm64:   ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV)
boots-linux-armv7l:  ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV)
boots-linux-x86_64:  ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV)

notice the 32bit arm v6 binary is named aarch64 and x86_64 is actually 386!

Here is from this PR:

[~/go/src/github.com/tinkerbell/boots]─[manny@dellnix]> 
file boots-* | sed 's|, statically.*||'
boots-linux-386:   ELF 32-bit LSB executable, Intel 80386, version 1 (SYSV)
boots-linux-amd64: ELF 64-bit LSB executable, x86-64, version 1 (SYSV)
boots-linux-arm64: ELF 64-bit LSB executable, ARM aarch64, version 1 (SYSV)
boots-linux-armv6: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV)
boots-linux-armv7: ELF 32-bit LSB executable, ARM, EABI5 version 1 (SYSV)

How Has This Been Tested?

Inspecting each file with file, docker build works.

How are existing users impacted? What migration steps/scripts do we need?

Any user using the cross compiled binaries directly will be impacted, but this should most likely be 0 users since they will likely use plain-old boots binary instead.

mmlb added 7 commits January 19, 2021 17:25
@mmlb
Docker: only uploade the crosscompile'd binaries
ce62242 
No need for all the other files to bust the cache nor waste time being
uploaded.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
git: Ignore crosscompile'd binaries
c7ca5ed 
Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
Dockerfile: Only COPY binary for TARGET
b40ecfe 
No need to copy all the binaries only to keep one of them. This was also
already buggy as `uname -m` was showing x86_64 for both the x86_64 and
386 builds.

This is making use of the buildx multi-platform provided ARGS, a normal
`docker build` user would have to pass these values in manually. This
seems like a step backwards compared to `uname -m` but as I mentioned
previously, that was already broken.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
Makefile: Extract some reused envvars to be DRY
310fd9b 
Less copy/paste is better than more.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
Makefile: Break out crosscompile's recipe to separate deps
bc203af 
This way we have just one recipe but can make use of parallal make
during local development.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
Makefile: Fix the crosscompile binary names
7e83933 
These names matches the variables used by Go and buildx.

Signed-off-by: Manuel Mendez <[email protected]>
@mmlb
Makefile: Sort the crosscompile binaries and deps
4984a29 
Signed-off-by: Manuel Mendez <[email protected]>
@mmlb mmlb requested a review from gianarb January 20, 2021 01:23
@mmlb mmlb added the ready-to-merge Signal to Mergify to merge the PR. label Jan 20, 2021
@codecov
Copy link

codecov bot commented Jan 20, 2021

Codecov Report

Merging #122 (4984a29) into master (0498419) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #122   +/-   ##
=======================================
  Coverage   40.90%   40.90%           
=======================================
  Files          40       40           
  Lines        2056     2056           
=======================================
  Hits          841      841           
  Misses       1130     1130           
  Partials       85       85

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 0498419...4984a29. Read the comment docs.

@gianarb
Copy link
Contributor

gianarb commented Jan 20, 2021

Thank you for making it right! I have to admit, we need a bit of consistency.
Can you keep the same binary name we already use in the other project? have a look here.
https://github.com/tinkerbell/sandbox/releases/tag/v0.3.0

Otherwise, can you apply the same criteria to the other projects?

@mmlb mmlb changed the title Docker: only uploade the crosscompile'd binaries Fix binary names Jan 20, 2021
@mmlb
Copy link
Contributor Author

mmlb commented Jan 20, 2021

@gianarb I think those names in sandbox should be changed to -armv6 and armv7 as that seems to be the most common scheme in go projects I can find releases for (caddy, traefik, golangci-lint) and also has the easiest scheme $name-$os-$cpu

@mmlb mmlb mentioned this pull request Jan 20, 2021
Merged
@mmlb
Copy link
Contributor Author

mmlb commented Jan 20, 2021

Also, @gianarb only boots does cross compiling outside of docker so there isn't really anything to do for any of the other projects right?

@gianarb
Copy link
Contributor

gianarb commented Jan 20, 2021

I see what you are saying.. Yep right

@mergify mergify bot merged commit 62ed4b8 into tinkerbell:master Jan 20, 2021
@mmlb mmlb deleted the fix-binary-names branch January 20, 2021 15:01
mergify bot added a commit to tinkerbell/playground that referenced this pull request Jan 20, 2021
@mergify
getbinariesfromquay: Remove - separating arch from variant (#38)
ce7a79b 
## Description

Renames binaries to be more consistent in and of itself and also compared to other Go projects that provide multi-arch binaries.

## Why is this needed

@gianarb asked me to rename the binaries in tinkerbell/smee#122 to match this scheme, but I think that this PR is the better directon.
@mmlb mmlb mentioned this pull request Jan 21, 2021
Closed
ttwd80 pushed a commit to ttwd80/tinkerbell-playground that referenced this pull request Sep 7, 2024
@mergify
getbinariesfromquay: Remove - separating arch from variant (tinkerbel…
eea2d2b 
…l#38)

## Description

Renames binaries to be more consistent in and of itself and also compared to other Go projects that provide multi-arch binaries.

## Why is this needed

@gianarb asked me to rename the binaries in tinkerbell/smee#122 to match this scheme, but I think that this PR is the better directon.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gianarb gianarb gianarb approved these changes

Assignees
No one assigned
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@mmlb @gianarb