Conversation
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR updates the Vulkan SDK install process on Linux ARM64 by adding missing utilities, revising extraction steps, and introducing an on‐the‐fly SDK build path for non-x86_64 architectures; it also removes an unused build argument in the Dockerfile. Flow diagram for updated Vulkan SDK installation process on Linux/ARM64flowchart TD
A["Start Vulkan SDK install"] --> B["Detect architecture (uname -m)"]
B --> C["Install wget and xz-utils"]
C --> D["Download Vulkan SDK tarball"]
D --> E["Extract tarball to /tmp"]
E --> F{"Is architecture x86_64?"}
F -- Yes --> G["Move /tmp/x86_64/* to /opt/vulkan/"]
F -- No --> H["Install build dependencies"]
H --> I["Build Vulkan SDK in /tmp/<vulkan_version> with vulkansdk script"]
I --> J["Move /tmp/<vulkan_version>/<arch>/* to /opt/vulkan/"]
G --> K["Clean up /tmp"]
J --> K
K --> L["End"]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
Summary of ChangesHello @p1-0tr, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical issue preventing the successful installation of the Vulkan SDK on Linux ARM64 environments. It modifies the installation script to detect the architecture and, for non-x86_64 systems, installs all required build dependencies before compiling the SDK from source. This ensures broader compatibility and correct functionality across different hardware platforms. Additionally, a minor cleanup was performed in the Dockerfile by removing an unnecessary argument. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Consider using a multi‐stage build or explicitly purging the build‐time dependencies after
./vulkansdkto avoid bloating your final image. - Removing the
TARGETARCHARG in the Dockerfile will break theLLAMA_BINARY_PATHexpansion—either reintroduce it or update the path logic accordingly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider using a multi‐stage build or explicitly purging the build‐time dependencies after `./vulkansdk` to avoid bloating your final image.
- Removing the `TARGETARCH` ARG in the Dockerfile will break the `LLAMA_BINARY_PATH` expansion—either reintroduce it or update the path logic accordingly.
## Individual Comments
### Comment 1
<location> `scripts/apt-install.sh:27-28` </location>
<code_context>
+ clang-format qtbase5-dev qt6-base-dev
+ pushd /tmp/"${vulkan_version}"
+ # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here
+ ./vulkansdk --no-deps -j $(nproc)
+ fi
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** No error handling for build failures.
Add error checking after the build step to stop the script if the build fails.
```suggestion
# TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here
./vulkansdk --no-deps -j $(nproc)
if [ $? -ne 0 ]; then
echo "ERROR: Vulkan SDK build failed."
exit 1
fi
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
scripts/apt-install.sh
Outdated
| # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here | ||
| ./vulkansdk --no-deps -j $(nproc) |
There was a problem hiding this comment.
suggestion (bug_risk): No error handling for build failures.
Add error checking after the build step to stop the script if the build fails.
| # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here | |
| ./vulkansdk --no-deps -j $(nproc) | |
| # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here | |
| ./vulkansdk --no-deps -j $(nproc) | |
| if [ $? -ne 0 ]; then | |
| echo "ERROR: Vulkan SDK build failed." | |
| exit 1 | |
| fi |
Signed-off-by: Piotr Stankiewicz <piotr.stankiewicz@docker.com>
p1-0tr
force-pushed
the
ps-fix-vulkan-linux-arm64
branch
from
e83fcd0 to
5279c60
Compare
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix the Vulkan SDK installation on linux/arm64 by adding a build-from-source step for non-x86_64 architectures. The changes in scripts/apt-install.sh correctly identify the architecture and trigger the build, but they also introduce some issues. Specifically, there's a critical bug due to a missing popd command, and the build dependencies are not uninstalled, which will lead to a significantly larger Docker image. I've provided suggestions to fix these issues and improve the script's robustness and efficiency. The change in the Dockerfile is a minor cleanup and looks good.
| if [ "$arch" != "x86_64" ]; then | ||
| # TODO: uninstall build time deps after building the SDK | ||
| apt-get install -y libglm-dev cmake libxcb-dri3-0 libxcb-present0 libpciaccess0 \ | ||
| libpng-dev libxcb-keysyms1-dev libxcb-dri3-dev libx11-dev g++ gcc \ | ||
| libwayland-dev libxrandr-dev libxcb-randr0-dev libxcb-ewmh-dev \ | ||
| git python-is-python3 bison libx11-xcb-dev liblz4-dev libzstd-dev \ | ||
| ocaml-core ninja-build pkg-config libxml2-dev wayland-protocols python3-jsonschema \ | ||
| clang-format qtbase5-dev qt6-base-dev | ||
| pushd /tmp/"${vulkan_version}" | ||
| # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here | ||
| ./vulkansdk --no-deps -j "$(nproc)" | ||
| fi |
There was a problem hiding this comment.
The current implementation for building the Vulkan SDK on non-x86_64 architectures has a few issues that should be addressed:
- Missing
popd: There's apushdwithout a correspondingpopd, which can cause subsequent commands to fail. This is a critical bug. - Build dependencies are not removed: This will significantly bloat the final Docker image. The
TODOcomment highlights this. - Missing
--no-install-recommends: Using this flag withapt-get installis a best practice for Docker images to minimize their size. - Readability: The long list of packages can be stored in a variable to improve readability.
Here is a suggested replacement that addresses these points.
| if [ "$arch" != "x86_64" ]; then | |
| # TODO: uninstall build time deps after building the SDK | |
| apt-get install -y libglm-dev cmake libxcb-dri3-0 libxcb-present0 libpciaccess0 \ | |
| libpng-dev libxcb-keysyms1-dev libxcb-dri3-dev libx11-dev g++ gcc \ | |
| libwayland-dev libxrandr-dev libxcb-randr0-dev libxcb-ewmh-dev \ | |
| git python-is-python3 bison libx11-xcb-dev liblz4-dev libzstd-dev \ | |
| ocaml-core ninja-build pkg-config libxml2-dev wayland-protocols python3-jsonschema \ | |
| clang-format qtbase5-dev qt6-base-dev | |
| pushd /tmp/"${vulkan_version}" | |
| # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here | |
| ./vulkansdk --no-deps -j "$(nproc)" | |
| fi | |
| if [ "$arch" != "x86_64" ]; then | |
| local build_deps="libglm-dev cmake libxcb-dri3-0 libxcb-present0 libpciaccess0 \ | |
| libpng-dev libxcb-keysyms1-dev libxcb-dri3-dev libx11-dev g++ gcc \ | |
| libwayland-dev libxrandr-dev libxcb-randr0-dev libxcb-ewmh-dev \ | |
| git python-is-python3 bison libx11-xcb-dev liblz4-dev libzstd-dev \ | |
| ocaml-core ninja-build pkg-config libxml2-dev wayland-protocols python3-jsonschema \ | |
| clang-format qtbase5-dev qt6-base-dev" | |
| apt-get install -y --no-install-recommends $build_deps | |
| pushd /tmp/"${vulkan_version}" | |
| # TODO: we don't need the whole SDK to run stuff, so eventually only build necessary targets here | |
| ./vulkansdk --no-deps -j $(nproc) | |
| popd | |
| apt-get purge -y --auto-remove $build_deps | |
| fi |
| local vulkan_version=1.4.321.1 | ||
| local arch | ||
| arch=$(uname -m) | ||
| apt-get install -y wget xz-utils |
There was a problem hiding this comment.
This change introduces an additional apt-get install command. The script now contains multiple apt-get install calls (here, for build dependencies, and for ca-certificates). For better efficiency and to optimize Docker image layers, it's recommended to consolidate all package installations into a single apt-get install command at the top of the script, right after apt-get update.
|
Does this match with how we build in the other repo? |
Yep, the only difference is here I went with building the full SDK for now. The runtime repo only builds the components that it needs. |
2 participants
Summary by Sourcery
Fix Vulkan SDK installation on linux/arm64 by installing missing tools, adjusting extraction steps, and building the SDK on non-x86_64 architectures
Bug Fixes:
Enhancements: