-
Notifications
You must be signed in to change notification settings - Fork 71
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
Support s390x cross-platform go-build #379
Conversation
Signed-off-by: huoqifeng <[email protected]>
Dockerfile.s390x
Outdated
MAINTAINER LoZ Open Source Ecosystem (https://www.ibm.com/developerworks/community/groups/community/lozopensource) | ||
|
||
ARG MANIFEST_TOOL_VERSION=v1.0.2 | ||
ARG GO111MODULE=auto |
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 think this should always be "on" now
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.
Agree, corrected.
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.
Also appears to still be here
Dockerfile.s390x
Outdated
chmod 0755 /sbin/su-exec; \ | ||
rm /sbin/su-exec.c | ||
|
||
# Install fossa for foss license checks |
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.
fossa can actually be removed - we don't use this any more
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.
Deleted.
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.
Looks like this is still here @huoqifeng
Makefile
Outdated
@@ -74,7 +74,11 @@ image: calico/go-build | |||
calico/go-build: register | |||
# Make sure we re-pull the base image to pick up security fixes. | |||
# Limit the build to use only one CPU, This helps to work around qemu bugs such as https://bugs.launchpad.net/qemu/+bug/1098729 | |||
ifeq ($(BUILDARCH),s390x) | |||
docker build $(DOCKER_BUILD_ARGS) -t $(ARCHIMAGE) -f $(DOCKERFILE) . |
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.
Why no --pull
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.
Drop --pull
for s390x build is because there is no base image calico/bpftool:v5.3-s390x
available in docker hub, so I built it locally. Otherwise, it will fail like below:
# ARCH=s390x make image
# Make sure we re-pull the base image to pick up security fixes.
# Limit the build to use only one CPU, This helps to work around qemu bugs such as https://bugs.launchpad.net/qemu/+bug/1098729
docker build --cpuset-cpus 0 --pull -t calico/go-build:latest-s390x -f Dockerfile.s390x .
Sending build context to Docker daemon 720.9kB
Step 1/30 : FROM calico/bpftool:v5.3-s390x as bpftool
manifest for calico/bpftool:v5.3-s390x not found: manifest unknown: manifest unknown
make: *** [Makefile:77: calico/go-build] Error 1
Maybe, the right approach is to build calico/bpftool
and go-build
for s390x and push them to docker hub as official base image. Could it be achieved after the fix get merged for go-build? Do we have a s390x box in CI? The same reason is for the changes in calico
PRs, like projectcalico/calico#5778
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.
There is no s390x version bpftool here https://hub.docker.com/r/calico/bpftool/tags but it can be built successfully based on https://github.com/projectcalico/bpftool, possible to generate a s390x image? so that we don't need drop --pull
option.
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.
@caseydavenport May I know why there is no s390x images built for calico/bpftool
, go-build
and calico
, I guess travis supports s390x already for github, right?
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.
Yeah, the "correct" fix here would be to publish an s390x image to dockerhub.
I suspect the problem is that we weren't able to cross-build such an image before? We don't have s390x infra ourselves so need to rely on cross compilation.
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.
@caseydavenport I see, so, is it reasonable to not use --pull
option for dependencies when build the s390x image now, so that a local one can be used? Maybe, it could be rollback when there is a s390x build box, like https://github.com/travis-ci/travis-ci.
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.
@caseydavenport are you happy if we drop --pull
option for s390x images because which is not enabled in CI and the required images can not be pull from docker hub, We drop the fix as this so make it possible to build all images locally?
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.
@caseydavenport I enabled cross-platform build also for s390x in this PR, to use --pull
option we'll depends on bpftool s390x image get built and pushed, which is addressed in projectcalico/bpftool#6
We can revise this to get |
Signed-off-by: huoqifeng <[email protected]>
Signed-off-by: huoqifeng <[email protected]>
Given image
|
/sem-approve |
@caseydavenport PR build looks good, may you help review it again? |
@caseydavenport I think the |
@caseydavenport any comments 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.
LGTM but maybe you forgot to push a commit?
@caseydavenport yeah, I copied new contents from latest Dockerfile.amd64, and revised it again. regard |
Signed-off-by: huoqifeng <[email protected]>
Signed-off-by: huoqifeng <[email protected]>
Signed-off-by: huoqifeng <[email protected]>
@caseydavenport is the new commit OK? |
/sem-approve |
@caseydavenport looks the |
All supported architectures are unified in #490. Closing as done. |
Signed-off-by: huoqifeng [email protected]