-
Notifications
You must be signed in to change notification settings - Fork 89
Add conditional package install in minimal Dockerfile.cpu for s390x #1132
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
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Nishan Acharya <[email protected]>
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Hi @Nash-123. Thanks for your PR. I'm waiting for a opendatahub-io member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the 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-sigs/prow repository. |
/ok-to-test @Nash-123 thank you for this PR! Is there some tracking Jira issue for this change? |
RUN ARCH=$(uname -m) && \ | ||
echo "Detected architecture: $ARCH" && \ | ||
if [ "$ARCH" = "s390x" ]; then \ | ||
dnf install -y mesa-libGL skopeo gcc g++ make openssl-devel autoconf automake libtool cmake; \ |
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 thinking whether doing the build of pyzmq
in a separate build container and only then copying the result in our minimal image wouldn't be a slightly cleaner solution. Something like we are doing for mongocli
in #1087.
But let's get opinion from others too first.
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.
It would be much cleaner, but also much work, sadly, if we wanted to make everything clean and tidy.
There are two options, either build the wheels in builder image and then install them in the output image, or install in the builder image and then copy over the python venv from the builder image. The first option is cleaner, but more work.
Going forward, there's also extra work ensuring that all necessary runtime deps are installed in the output image; but hopefully they are already installed because the x86_64 image hopefully worked, so maybe this is not really an issue.
@jstourac I suggest we create a jira ticket and explore going the separate builder image route first. I hope it will not be too much work. We'd need to test on z390x but hopefully the qemu-user binary emulation will cover it.
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.
Thanks @jstourac and @jiridanek for the comments!
I believe using QEMU binary emulation to build for s390x should work, but it will consume a lot of time to get build.
Given that, I'd like to confirm with you both, which approach out of the two mentioned would you recommend I proceed with for now, considering this is an initial enablement and we'd still want to keep things maintainable?
Thanks again!
This PR enhances the
Dockerfile.cpu
for the minimal runtime to conditionally install additional OS packages when building on thes390x
architecture.This python package
pyzmq
require native compilation and thus depend on development tools and libraries such asgcc
,g++
,make
,openssl-devel
,autoconf
,automake
,libtool
, andcmake
. These are typically not needed on x86_64 but are essential for s390x.Changes
ARCH=$(uname -m)
check to detect the architecture.ARCH == "s390x"
.Impact
This change ensures:
Tested on: s390x
c.c: @modassarrana89 @vibhaKulka