Skip to content
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

packages/libnvidia-container-custom: init #1089

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

msanft
Copy link
Contributor

@msanft msanft commented Dec 19, 2024

This adds a custom implementation of libnvidia-container that can resolve binaries (like nvidia-smi) correctly on NixOS. This needs to know the location of the binaries. Unfortunately, it is not possible to supply the package build with the correct paths from the Nix store, as the binaries are built in the driver package, which is specific to the host (i.e. the kernel), and thus is only known in the NixOS build, where we already need to have the package, creating a circular dependency. This adds a vendored version of said package with a workaoround that consist of just having it search in the NixOS PATH (i.e. /run/current-system/sw), which is fine, since we only use this package in NixOS-scenarios, but can't be upstreamed since it's incompatible with other distributions.

@msanft msanft added the no changelog PRs not listed in the release notes label Dec 19, 2024
@msanft msanft requested a review from burgerdev December 19, 2024 09:20
@msanft msanft requested a review from katexochen as a code owner December 19, 2024 09:20
@msanft msanft force-pushed the msanft/libnvida-container-custom branch 2 times, most recently from b11f561 to d3cec84 Compare December 19, 2024 09:24
@msanft msanft changed the title [3/x] packages/libnvidia-container-custom: init packages/libnvidia-container-custom: init Dec 19, 2024
This adds a custom implementation of libnvidia-container that can resolve binaries (like `nvidia-smi`) correctly on NixOS. This needs to know the location of the binaries. Unfortunately, it is not possible to supply the package build with the correct paths from the Nix store, as the binaries are built in the driver package, which is specific to the host (i.e. the kernel), and thus is only known in the NixOS build, where we already need to have the package, creating a circular dependency. This adds a vendored version of said package with a workaoround that consist of just having it search in the NixOS PATH (i.e. `/run/current-system/sw`), which is fine, since we only use this package in NixOS-scenarios, but can't be upstreamed since it's incompatible with other distributions.
Make `libnvidia-container-custom` the default `libnvidia-container` in our NixOS build.
@msanft msanft force-pushed the msanft/libnvida-container-custom branch from d3cec84 to b1e2cff Compare December 19, 2024 13:51
From 8799541f99785d2bd881561386676fb0985e939e Mon Sep 17 00:00:00 2001
From: Moritz Sanft <58110325+msanft@users.noreply.github.com>
Date: Thu, 10 Oct 2024 14:32:42 +0200
Subject: [PATCH] fix library resolving
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why this needs to be fixed in C code. Can't we just symlink the required binaries from an FHS directory?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at this again, we should be able to get rid from this patch if we patch the PATH of the OCI hook that calls nvidia-container-cli. I will check that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should be able to drop it if we can get rid of this line upstream and use PATH instead.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no changelog PRs not listed in the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants