-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
Parameterize the binary path and host arch for the hyperkube image #47776
Parameterize the binary path and host arch for the hyperkube image #47776
Conversation
Hi @jianzhangbjz. Thanks for your PR. I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
/assign @eparis |
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.
I'm sorry, but this is working exactly as expected.
Just copy _output/local/bin/linux/${ARCH}/hyperkube
to _output/dockerized/bin/linux/${ARCH}/hyperkube
if you want to build the hyperkube image. Using only make
isn't a supported build, using make release
(dockerized build) is.
The reason we're using the dockerized directory is because the official builds are located there and we won't change that.
And if we changed the ifeq
clause to the ifneq
you have there it would break the release scripts :/
I'm very sorry, but I can't take this PR
I'm open to a |
@luxas AFAIK, |
@luxas Ok, I will create the |
@luxas Update, added |
@luxas Needs to revise? Or something else I missed? |
This operation still can't success, because the image for ppc64le and amd64 are the same in |
cluster/images/hyperkube/Makefile
Outdated
@@ -74,6 +76,8 @@ endif | |||
ifeq ($(ARCH),amd64) | |||
# When building "normally" for amd64, remove the whole line, it has no part in the amd64 image | |||
cd ${TEMP_DIR} && ${SED_CMD} "/CROSS_BUILD_/d" Dockerfile | |||
else ifeq ($(ARCH),ppc64le) |
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.
This will break the release scripts, as ppc64le must be cross-built when on amd64.
What we should do instead is check the host platform and the target platform. If they equal,
cd ${TEMP_DIR} && ${SED_CMD} "/CROSS_BUILD_/d" Dockerfile
should be run.
If they aren't the same, the second case should be run.
So add a HOSTARCH?=amd64
variable and compare ifeq($(ARCH),$(HOSTARCH))
above.
Then you set HOSTARCH=ppc64le
when running the script, and it will work
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.
Got it. This variable has been added. :)
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, but please squash your commits
@k8s-bot ok to test |
/assign @ixdy |
7db6b69
to
ad552e5
Compare
@luxas Squashed it, thanks. :) |
cluster/images/hyperkube/Makefile
Outdated
@@ -71,7 +74,7 @@ ifeq ($(CACHEBUST),1) | |||
cd ${TEMP_DIR} && sed -i.back "s|CACHEBUST|$(shell uuidgen)|g" Dockerfile | |||
endif | |||
|
|||
ifeq ($(ARCH),amd64) | |||
ifeq ($(ARCH),$(HOSTARCH)) | |||
# When building "normally" for amd64, remove the whole line, it has no part in the amd64 image |
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.
maybe update this 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.
yes, it has been updated.
ad552e5
to
5660043
Compare
/retest |
/retest |
/lgtm |
/approve no-issue |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
5 similar comments
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: eparis, ixdy, jianzhangbjz Associated issue requirement bypassed by: eparis The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these OWNERS Files:
You can indicate your approval by writing |
Automatic merge from submit-queue |
As the cluster/images/hyperkube/README.md shows, I run the command:
make build VERSION=test ARCH=ppc64le
, but got the below errors, so this PR will fix it.