fix(cdi): expose compat32 NVIDIA libraries - #11
Draft
Eli Bosley (elibosley) wants to merge 1 commit into
Draft
Conversation
Signed-off-by: Eli Bosley <eli@bosley.dev>
📝 WalkthroughWalkthroughThe plugin version is updated to ChangesCompat32 CDI support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Installer
participant generate_cdi
participant nvidia_ctk
participant nvidia_container_cli
participant CDI_JSON
Installer->>generate_cdi: Generate CDI specification
generate_cdi->>nvidia_ctk: Create staged JSON
generate_cdi->>nvidia_container_cli: Discover compat32 libraries
generate_cdi->>CDI_JSON: Validate and augment mounts and hooks
generate_cdi-->>Installer: Publish result or return failure
Installer-->>Installer: Abort installation on failure
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nvidia-driver.plg (1)
343-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract a shared cleanup helper to remove duplicated error-handling code.
The failure branches inside
generate_cdirepeat the same three-line cleanup sequence at least seven times: log to stderr,rm -f "${generated}" "${transformed}",rmdir "${staging}" 2>/dev/null || true, thenreturn 1. Extract a helper function so a future edit to the cleanup logic does not need to touch every branch, and so a new failure branch cannot forget the cleanup step.♻️ Proposed refactor to extract a shared cleanup helper
generate_cdi() { local cdi_path="${NVIDIA_CDI_PATH:-/etc/cdi/nvidia.yaml}" local staging generated transformed compat_output compat_json folders_json local library canonical candidate header folder scanned target local -a compat_libraries=("") local -a compat_folders=("") local -a compat_targets=("") local -a scanned_folders=("") + + fail_cdi() { + echo "$1" >&2 + rm -f "${generated}" "${transformed}" + rmdir "${staging}" 2>/dev/null || true + return 1 + } if ! mkdir -p "$(dirname "${cdi_path}")" ; then echo "Failed to create NVIDIA CDI directory" >&2 return 1 fi staging="$(mktemp -d "${cdi_path}.stage.XXXXXX")" || return 1 generated="${staging}/generated.json" transformed="${staging}/transformed.json" if ! nvidia-ctk cdi generate --format=json --output="${generated}" >/dev/null 2>&1 ; then - echo "Failed to generate NVIDIA CDI specification" >&2 - rm -f "${generated}" "${transformed}" - rmdir "${staging}" 2>/dev/null || true - return 1 + fail_cdi "Failed to generate NVIDIA CDI specification" ; return 1 fiApply the same substitution to the remaining branches that discover, validate, and publish the spec.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nvidia-driver.plg` around lines 343 - 497, In generate_cdi, extract the repeated generated/transformed file and staging-directory removal into a local cleanup helper, such as cleanup_cdi_generation, that returns failure after performing cleanup. Update every failure branch for CDI generation, compat32 discovery/validation, JSON augmentation, and publication to log its specific error, invoke the helper, and avoid duplicating the cleanup commands; preserve the existing successful cleanup path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/probe-compat32-vulkan.sh`:
- Around line 25-31: Validate the captured status immediately after the
timeout/docker probe command and before parsing output; when status is nonzero,
exit with failure so "${probe}" failures or timeouts cannot be reported as
successful regression probes. Preserve the existing output parsing only for a
successful status.
---
Nitpick comments:
In `@nvidia-driver.plg`:
- Around line 343-497: In generate_cdi, extract the repeated
generated/transformed file and staging-directory removal into a local cleanup
helper, such as cleanup_cdi_generation, that returns failure after performing
cleanup. Update every failure branch for CDI generation, compat32
discovery/validation, JSON augmentation, and publication to log its specific
error, invoke the helper, and avoid duplicating the cleanup commands; preserve
the existing successful cleanup path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9f67b325-a0f4-4895-9b04-24f8f4e1e2dc
📒 Files selected for processing (5)
nvidia-driver.plgpackages/nvidia-driver-2026.08.01.txzpackages/nvidia-driver-2026.08.01.txz.md5tests/generate-cdi-test.shtests/probe-compat32-vulkan.sh
Comment on lines
+25
to
+31
| set +e | ||
| output="$( | ||
| timeout 20 docker exec -u "${probe_user}" -e DISPLAY="${display}" "${container}" \ | ||
| "${probe}" --c 1 --suppress_popups 2>&1 | ||
| )" | ||
| status=$? | ||
| set -e |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require a successful Vulkan probe exit status.
The script records status but never validates it. If vkcube selects NVIDIA and then fails or times out, Lines 35-46 report a successful regression probe.
Exit before parsing output when status is nonzero.
Proposed fix
status=$?
set -e
printf '%s\n' "${output}"
+if [ "${status}" -ne 0 ]; then
+ echo "The ELF32 Vulkan probe failed (exit ${status})" >&2
+ exit 1
+fi
+
selected="$(printf '%s\n' "${output}" | sed -n 's/^Selected GPU [^:]*: \([^,]*\).*/\1/p' | head -1)"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/probe-compat32-vulkan.sh` around lines 25 - 31, Validate the captured
status immediately after the timeout/docker probe command and before parsing
output; when status is nonzero, exit with failure so "${probe}" failures or
timeouts cannot be reported as successful regression probes. Preserve the
existing output parsing only for a successful status.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
NVIDIA CDI generation currently omits the host's 32-bit driver stack, causing 32-bit Steam and Proton workloads to fall back to software rendering; this temporarily augments the generated spec until NVIDIA's upstream fix is available.
Why This Exists
The NVIDIA Container Toolkit 1.19.1 CDI generator exposes the 64-bit NVIDIA libraries but not the compatible 32-bit libraries discovered by
nvidia-container-cli list --libraries --compat32. A Games on Whales Steam container on Unraid therefore selected llvmpipe for a 32-bit Vulkan probe despite the host's NVIDIA GPU and 32-bit driver libraries being present.Resolution
Generate CDI into a staging file, discover NVIDIA's compat32 libraries through
nvidia-container-cli, validate them as readable ELF32 objects, add both canonical libraries and resolving symlink aliases as read-only mounts, and add their directories to the existingupdate-ldcachehook. The transformed spec is published atomically only after validation succeeds.The augmentation is idempotent: paths and ld-cache folders already emitted by a future toolkit release are not duplicated. This makes the shim a no-op for upstream-provided entries and gives us a straightforward removal point after NVIDIA/nvidia-container-toolkit#1968 ships in Unraid.
Reviewer Considerations
jqlogic tonvidia-driver.plg.2026.08.01artifact./usr/libmounts. All were verified as ELF32, and the existing 40/usr/lib64mounts remain intact.Behavior Changes
update-ldcachehook includes the discovered compat32 library directories.Implementation Summary
Verification
./tests/generate-cdi-test.sh— passed all fixture cases.bash -n tests/generate-cdi-test.sh tests/probe-compat32-vulkan.sh— passed.git diff --check— passed.nvidia-driver.plgas XML — passed.packages/nvidia-driver-2026.08.01.txzagainst its MD5 and confirmed its contents match the plugin source assets.devgen.local, generated and validated a candidate from this exact commit: 48 ELF32/usr/libmounts, 40/usr/lib64mounts, critical CUDA/NVML/GLX libraries present,/usr/libincluded in the ld-cache hook, and all six CDI devices still enumerated.llvmpipe (LLVM 20.1.2, 256 bits), confirming the regression. A newly created Wolf session is required to consume the corrected CDI spec.Risk
Medium. Driver-library discovery varies by installed driver and host layout. Readability and ELF-class validation, idempotent merging, atomic publication, fail-closed behavior, fixture coverage, and real-host validation limit the rollout risk.
Summary by CodeRabbit
New Features
Bug Fixes