Changed dockerfile to work on multi *nix platforms#1569
Changed dockerfile to work on multi *nix platforms#1569TheeNawMan wants to merge 1 commit intoNVIDIA:developfrom
Conversation
Signed-off-by: Joshua Nauman <carnage_auction.2i@icloud.com>
WalkthroughThe HOST_ARCH detection method in the build container script was changed from Changes
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@docker/build_container.sh`:
- Around line 24-25: The script sets HOST_ARCH from uname -m and uses
DOCKER_TARGET_ARCH from it, but Docker --platform requires normalized names;
update the assignment logic for HOST_ARCH/DOCKER_TARGET_ARCH to map raw uname -m
outputs to Docker-compatible values (e.g., map x86_64 -> amd64, aarch64 ->
arm64, arm64 -> arm64, i386 -> 386, armv7l -> arm/v7, etc.) so the --platform
flag (used later) receives a valid architecture; implement a small mapping block
that reads HOST_ARCH=$(uname -m) and then sets
DOCKER_TARGET_ARCH=${DOCKER_TARGET_ARCH:-<mapped-value>} based on those cases.
| HOST_ARCH=$(uname -m) | ||
| DOCKER_TARGET_ARCH=${DOCKER_TARGET_ARCH:-${HOST_ARCH}} |
There was a problem hiding this comment.
Architecture naming mismatch will break Docker builds on Linux systems.
uname -m returns raw kernel architecture names (x86_64, aarch64) while Docker's --platform flag on line 40 expects normalized names (amd64, arm64). This will cause builds to fail on x86_64 and ARM64 Linux systems with an invalid platform error.
A mapping is needed to translate uname -m output to Docker-compatible architecture names.
🐛 Proposed fix with architecture mapping
-HOST_ARCH=$(uname -m)
+# Map uname output to Docker platform architecture names
+case $(uname -m) in
+ x86_64) HOST_ARCH="amd64" ;;
+ aarch64) HOST_ARCH="arm64" ;;
+ arm64) HOST_ARCH="arm64" ;;
+ *) HOST_ARCH=$(uname -m) ;;
+esac
DOCKER_TARGET_ARCH=${DOCKER_TARGET_ARCH:-${HOST_ARCH}}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| HOST_ARCH=$(uname -m) | |
| DOCKER_TARGET_ARCH=${DOCKER_TARGET_ARCH:-${HOST_ARCH}} | |
| # Map uname output to Docker platform architecture names | |
| case $(uname -m) in | |
| x86_64) HOST_ARCH="amd64" ;; | |
| aarch64) HOST_ARCH="arm64" ;; | |
| arm64) HOST_ARCH="arm64" ;; | |
| *) HOST_ARCH=$(uname -m) ;; | |
| esac | |
| DOCKER_TARGET_ARCH=${DOCKER_TARGET_ARCH:-${HOST_ARCH}} |
🤖 Prompt for AI Agents
In `@docker/build_container.sh` around lines 24 - 25, The script sets HOST_ARCH
from uname -m and uses DOCKER_TARGET_ARCH from it, but Docker --platform
requires normalized names; update the assignment logic for
HOST_ARCH/DOCKER_TARGET_ARCH to map raw uname -m outputs to Docker-compatible
values (e.g., map x86_64 -> amd64, aarch64 -> arm64, arm64 -> arm64, i386 ->
386, armv7l -> arm/v7, etc.) so the --platform flag (used later) receives a
valid architecture; implement a small mapping block that reads HOST_ARCH=$(uname
-m) and then sets DOCKER_TARGET_ARCH=${DOCKER_TARGET_ARCH:-<mapped-value>} based
on those cases.
willkill07
left a comment
There was a problem hiding this comment.
Coderabbit's feedback is meaningful and would need to be addressed.
uname -m is not suitable
|
@TheeNawMan I want to clarify my justification: macOS w/ Podman Linux w/ Docker Notice how platform definitions are strict and do not match |
Description
Changed dockerfile to work on multi *nix platforms
Tested on MacBook M1, MacBook Pro M4, DGX-SPARK, and Debian 13.3.0 x86
Label: enhancement
By Submitting this PR I confirm:
Summary by CodeRabbit