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

snapcraft: Add nvidia-ctk as part of the binary tool for Container Device Interface spec generation #470

Merged

Conversation

gabrielmougard
Copy link
Contributor

related to: canonical/lxd#13562

Copy link
Member

@simondeziel simondeziel left a comment

Choose a reason for hiding this comment

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

Aside from the build-snaps directive that's missing, I think this part needs to be added to the after list of the strip part.

@gabrielmougard gabrielmougard force-pushed the feat/nvidia-cdi-support branch from 82d1295 to 9926572 Compare June 24, 2024 07:22
@gabrielmougard
Copy link
Contributor Author

Aside from the build-snaps directive that's missing, I think this part needs to be added to the after list of the strip part.

done :)

simondeziel
simondeziel previously approved these changes Jun 25, 2024
snapcraft.yaml Outdated
- go
plugin: make
override-prime: |-
[ "$(uname -m)" != "x86_64" ] && [ "$(uname -m)" != "aarch64" ] && exit 0
Copy link
Member

Choose a reason for hiding this comment

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

Does this only work on amd64 and arm64 then?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually, it seems to also be supported for ppc64le

Copy link
Contributor Author

Choose a reason for hiding this comment

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

... and s390x (it was not clearly stated in the repo but after a bit of digging I found this: https://rpmfind.net/linux/rpm2html/search.php?query=nvidia-container-toolkit)

Copy link
Member

Choose a reason for hiding this comment

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

its just Go right?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with limiting it if you think there is a reason that this tool wouldn't be used on those architectures, even if it can be built. Can iGPUs be used on ppc64le and s390x?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I asked @elezar

Copy link

Choose a reason for hiding this comment

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

I'm OOTO. Will get to this on Tuesday.

Copy link

@elezar elezar Jul 30, 2024

Choose a reason for hiding this comment

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

It is my understanding that iGPUs are aarch64 only. For discrete GPUs our main platforms are x86_64 and aarch64. We do also build ppc64le packages, but it is my understanding that driver support for these architectures is being deprecated https://docs.nvidia.com/cuda/cuda-toolkit-release-notes/index.html#deprecated-or-dropped-architectures

Note that this does not mean that CDI should not be supported on ppc64le and s360x platforms, but just that the tooling to generate NVIDIA CDI specifications -- which relies on the driver -- may not work there if the driver is not available. One should still be able to build the go binary on these platforms though as device and library discovery using the driver is a runtime concern.

Copy link

Choose a reason for hiding this comment

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

Note that the linked c file should not be relevant on these platforms either since these are specific to discovering the location of driver libraries on WSL2-based systems. If the dxcore.so shared libraries are not present, these are never used.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @elezar

@gabrielmougard are you able to address this please?

snapcraft.yaml Outdated Show resolved Hide resolved
snapcraft.yaml Outdated Show resolved Hide resolved
…Device Interface spec generation

Signed-off-by: Gabriel Mougard <[email protected]>
@gabrielmougard gabrielmougard force-pushed the feat/nvidia-cdi-support branch from 64fdad6 to 2b7129f Compare July 26, 2024 10:08
@tomponline tomponline merged commit 99d3c73 into canonical:latest-edge Jul 26, 2024
5 checks passed
Comment on lines +671 to +673
make binaries
mkdir -p "${CRAFT_PART_INSTALL}/bin/"
cp nvidia-ctk "${CRAFT_PART_INSTALL}/bin/"
Copy link

Choose a reason for hiding this comment

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

Note: If you're only interested in the nvidia-ctk binary (or the nvidia-cdi-hook, I would assume), you should be able to run make cmd-nvidia-ctk or make cmd-nvidia-cdi-hook instead of make binaries to only build the single binary.

Copy link
Member

Choose a reason for hiding this comment

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

@gabrielmougard what do you think?

thanks @elezar !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants