-
Notifications
You must be signed in to change notification settings - Fork 853
internal/mkcw/embed: cross-compile using Go #6471
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
Conversation
|
Ephemeral COPR build failed. @containers/packit-build please check. |
2395f99 to
2c09bf9
Compare
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a great idea to reuse go, I wonder do we have any test for this binary at all? I don;t see the messages matched anywhere in our tests? I mean we could only run it on amd64 but still I guess it would be nice to know the binary is functional?
| internal/mkcw/embed/entrypoint_arm64.gz: internal/mkcw/embed/entrypoint_arm64 | ||
| gzip -k9nf $^ | ||
| internal/mkcw/embed/entrypoint_ppc64le.gz: internal/mkcw/embed/entrypoint_ppc64le | ||
| gzip -k9nf $^ | ||
| internal/mkcw/embed/entrypoint_s390x.gz: internal/mkcw/embed/entrypoint_s390x | ||
| gzip -k9nf $^ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do we need the other arches for if we only ever embed the amd64 one? Seems like unnecessary churn having to maintain a assembly version for each arch that we don't ever end up using.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't build them, but the confidential workload folks have been making noises about supporting other architectures for a while now.
2c09bf9 to
d09eca9
Compare
flouthoc
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
Had to add the prebuilt binary back to avoid breaking people who vendor us as a library. |
How is the code reachable? Only via buildah mkcw or also directly via the normal podman/buildah build commands? IF it is not needed for build it might make sense to try to split the packages so it does not get leaked into podman then? |
|
Ack, so then I guess we just need to figure out a way how to rebuild this binary as part of the podman rpm then. Since buildah has top level code the Makefile is actually part of vendor/. So I suppose the simple way would be to cd into vendor and just rerun the target like we do here. cc @lsm5 |
lsm5
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
spec change looks good.
LGTM. Thanks a lot @nalind
ack. Once this is vendored we can update the podman spec. |
d09eca9 to
72d61d6
Compare
|
Native |
7659349 to
2841c21
Compare
internal/mkcw/embed/check.sh
Outdated
| @@ -0,0 +1,12 @@ | |||
| #!/bin/bash | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: scripts tend to use #!/usr/bin/env bash here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We seem to be inconsistent about it, but sure, changing it.
internal/mkcw/embed/check.sh
Outdated
| arm64) QEMUARCH=aarch64;; | ||
| ppc64le|s390x) QEMUARCH=$GOARCH;; | ||
| esac | ||
| test "$(qemu-$QEMUARCH ./entrypoint_$GOARCH)" = "$msg" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this need to add a || echo "./entrypoint_$GOARCH is not working" show easy which arch failed?
Right now the script is not set -e either so this could fail silently and the script exit code could still be 0 if the latest s390x arch works.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the tool outputs the error message and exits with an error code, so it's a little trickier. Changed it.
Use the Go toolchain to cross-compile the "This image is designed to be run as a confidential workload using libkrun." entrypoint that we add to confidential workload images. It's bigger than it was before, but easier to port and can be built from source every time when desired. Signed-off-by: Nalin Dahyabhai <[email protected]>
2841c21 to
b6098a2
Compare
Luap99
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: flouthoc, lsm5, Luap99, nalind The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
70220fa
into
containers:main
What type of PR is this?
/kind other
What this PR does / why we need it:
Use the Go toolchain to cross-compile the "This image is designed to be run as a confidential workload using libkrun." entrypoint that we add to confidential workload images. It's bigger than it was before, but easier to port and can be built from source every time.
How to verify it
Check that we can still build on non-amd64 architectures.
Which issue(s) this PR fixes:
Special notes for your reviewer:
Sets up #6469, if we need to stop checking in a pre-built binary.
Does this PR introduce a user-facing change?