-
Notifications
You must be signed in to change notification settings - Fork 438
Fix dockerfile arch selection #384
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
versions.mk
Outdated
|
|
||
| CUDA_VERSION := 12.3.1 | ||
| GOLANG_VERSION := 1.20.5 | ||
| GOLANG_VERSION := 1.21.5 |
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.
Let me know if we are okay to update this. I am assuming we might run into the glibc version skew here as well
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, we would have to see whether these work in older distributions. I already have issues with 1.20.5 on some systems.
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 can revert it then. We definitely need to think of a solution for this though given 1.20 is close to EOL. Are we dependent on the glibc that comes from the cuda-base-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.
We don't build in the CUDA base images. We build in ubuntu:18.04 and centos:7 (centos:8 for ppc64le).
Let's drop the golang bump and do that as a follow-up.
| x86_64 | amd64) ARCH='amd64' ;; \ | ||
| ppc64el | ppc64le) ARCH='ppc64le' ;; \ | ||
| aarch64) ARCH='arm64' ;; \ | ||
| aarch64 | arm64) ARCH='arm64' ;; \ |
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 know that this is how we do things in other projects, but what about using the TARGETARCH automatic docker build arg instead of running uname -m?
I think this change is useful though.
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.
TARGETARCH is better assuming its value is derived from the docker buildx --platform flag
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. When building with BUILDKIT they're set automatically.
This change includes an arm64 arch check when installing golang. Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com> Signed-off-by: Evan Lezar <elezar@nvidia.com>
elezar
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.
I have updated the PR to remove the Golang version bump for now.
No description provided.