linuxPackages.nvidia_x11.libXNVCtrl: make the shared library available by aidalgol · Pull Request #312811 · NixOS/nixpkgs

aidalgol · 2024-05-19T06:05:33Z

Description of changes

The libXNVCtrl library is used by several programs in nixpkgs to get Nvidia GPU stats, and they all load the library dynamically, but the libXNVCtrl package only builds a static library. There is no build flag to enable this, so we have to patch the Makefile to produce a shared library. We then add the package to hardware.opengl.extraPackages in the NixOS module hardware/video/nvidia.nix so that it is available on Nvidia-enabled systems.

Based on a patch by @negativo17 for Fedora: https://github.com/negativo17/nvidia-settings/blob/master/nvidia-settings-libXNVCtrl.patch

Things done

Add a 👍 reaction to pull requests you find important.

ConnorBaker

One minor nit; I can try the builds later to verify they build.

Do you have an easy way to verify this is working as intended?

Also, would you mind adding yourself to the list of maintainers for pkgs/os-specific/linux/nvidia-x11/settings.nix?

SomeoneSerge · 2024-05-19T15:24:33Z

Looking at https://github.com/NVIDIA/nvidia-settings/blob/4e4bb3fecbbdd902d6f4bc6bbfb9902043c5c7f6/src/libXNVCtrl/Makefile#L39-L53, it seems that we could build the .so just by invoking make with appropriate arguments. I think that'd be more maintainable than patching Makefile per-release.

If we wish to build both the static and the shared object in a single derivation we'd still have to define a custom buildPhase, but I think that's OK.

There's no make rule for a .so file in xnvctrl.mk (or anywhere that I can see), so I don't think we can do this via make arguments. We could possibly just bypass make entirely and invoke the commands directly in a custom buildPhase, but I'm not sure whether that would be more maintainable.

Oh I didn't notice the $(AR) part, I was naive to think they had some kind of a generic rule for linkage...

I'd still like to get rid of the conditional patches. A part of the diff doesn't seem to be essential to the derivation, e.g. the clean (no-op for nix) and all (we have makeFlags/makeTargets for that) rules. Maybe omitting those we could have a single patch that successfully applies to all releases: adding nothing but the .so rule.

I was able to simplify down to two patches, because xnvctrl.mk does not exist in the 3xx versions.

aidalgol · 2024-05-19T18:47:20Z

Do you have an easy way to verify this is working as intended?

The program I was trying to get to work with my nvidia GPU was MangoHUD, so running vkcube with that was how I was testing my fix.

Before changes:

❯ mangohud vkcube
Selected GPU 0: NVIDIA GeForce RTX 3080, type: DiscreteGpu
[2024-05-20 06:44:07.254] [MANGOHUD] [error] [loader_nvctrl.cpp:39] Failed to open 64bit libXNVCtrl.so.0: libXNVCtrl.so.0: cannot open shared object file: No such file or directory
[2024-05-20 06:44:07.254] [MANGOHUD] [error] [nvctrl.cpp:48] XNVCtrl loader failed to load

After changes:

❯ mangohud vkcube
Selected GPU 0: NVIDIA GeForce RTX 3080, type: DiscreteGpu

(With gpu_stats in my ~/.config/MangoHud/MangoHud.conf.)

SomeoneSerge

I've some general comments left, in the line of "are we solving the right problem":

It seems that patching Makefiles takes approximately the same amount of code as shipping our own ad hoc CMakeLists.txt would have...
It seems that upstream does not distribute shared libraries, so it feels kind of arbitrary that MangoHUD should use it as one.

Further on MangoHUD, these probably should be in buildInputs:

nixpkgs/pkgs/tools/graphics/mangohud/default.nix

Lines 190 to 194 in dbeb624

    
           # Only the headers are used from these packages 
        
           # The corresponding libraries are loaded at runtime from the app's runpath 
        
           libXNVCtrl 
        
           wayland 
        
           libX11

That said, if you @aidalgol are convinced this is the way to address your use-case, I'll merge

aidalgol · 2024-05-20T11:40:25Z

It seems that patching Makefiles takes approximately the same amount of code as shipping our own ad hoc CMakeLists.txt would have...

Do you mean for all of nvidia_x11, or just libXNVCtrl?

It seems that upstream does not distribute shared libraries, so it feels kind of arbitrary that MangoHUD should use it as one.

MangoHUD is not the only package in nixpkgs that references libXNVCtrl.

psensor
nvfancontrol
monitor (yes, it's really called just "monitor")
vibrantlinux
conky

But I don't know whether any of those try to link dynamically. It sounds as if Conky does: Installing libxnvctrl (525.60.11-2) breaks dependency ‘libXNVCtrl.so=0-64’ required by conky.

Looking at other Linux distributions, at least Arch patches libXNVCtrl similarly.

Further on MangoHUD, these probably should be in buildInputs:

nixpkgs/pkgs/tools/graphics/mangohud/default.nix

Lines 190 to 194 in dbeb624

# Only the headers are used from these packages

# The corresponding libraries are loaded at runtime from the app's runpath

libXNVCtrl

wayland

libX11

That I'm not too sure about. If someone is using mangohud installed via "standalone" home-manager, and they update that more frequently than their system configuration, wouldn't mangohud have a newer version than the system's driver?

SomeoneSerge · 2024-05-20T11:42:54Z

Do you mean for all of nvidia_x11, or just libXNVCtrl?

libXNVCtrl

Looking at other Linux distributions, at least Arch patches libXNVCtrl similarly.

Seems like the patch belongs in NVIDA/nvidia-settings then, let's aim to eventually move it there.

aidalgol · 2024-05-20T11:44:56Z

Do you mean for all of nvidia_x11, or just libXNVCtrl?

libXNVCtrl

In that case, I would not be opposed to instead replacing the upstream Makefile, if you think that would be simpler or easier to maintain.

Looking at other Linux distributions, at least Arch patches libXNVCtrl similarly.

Seems like the patch belongs in NVIDA/nvidia-settings then, let's aim to eventually move it there.

You mean upstream it?

SomeoneSerge · 2024-05-20T13:22:50Z

You mean upstream it?

Yes

In that case, I would not be opposed to instead replacing the upstream Makefile, if you think that would be simpler or easier to maintain.

I'd be in favour of that. But also if we're upstreaming a patch, we'll want to switch to fetchpatch, and it's safer to avoid trying to impose cmake (or anything) on people...

If you need time to choose but you're committed to following up on this, I can merge the current PR as a temporary fix

aidalgol · 2024-05-21T05:40:48Z

I have submitted a PR upstream and have changed the libXNVCtrl derivation to use the diff from the PR, except for the 3xx legacy version.

SomeoneSerge · 2024-05-21T07:38:16Z

Iirc the pull/xxxx/commits/yyyy URL may get invalidated after force-pushing or even rebase-merging (I may be wrong)

The fetchpatch now has an updated all rule, but the 3xx patch doesn't and the new commit omits makeTargets

I think you're right. I thought that would make it robust against PR changes, but I think GitHub does invalidate commit URLs on a force push, at least after a while. I think I should back out this commit until the PR is merged.

The libXNVCtrl library is used by several programs in nixpkgs to get Nvidia GPU stats, and they all load the library dynamically, but the libXNVCtrl package only builds a static library. There is no build flag to enable this, so we have to patch the Makefile to produce a shared library. Based on a patch by @negativo17 for Fedora. https://github.com/negativo17/nvidia-settings/blob/master/nvidia-settings-libXNVCtrl.patch

SomeoneSerge · 2024-05-24T13:33:49Z

          '';
          hardware.opengl = {
-            extraPackages = [ nvidia_x11.out ];
+            extraPackages = [ nvidia_x11.out nvidia_x11.settings.libXNVCtrl ];


I had another look at the derivation and it actually doesn't seem to depend on nvidia_x11, except through nvidia_x11.settingsVersion. This makes me question if we actually have to deploy this impurely rather than link directly

github-actions · 2024-05-24T13:35:46Z

Backport failed for release-23.11, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-23.11
git worktree add -d .worktree/backport-312811-to-release-23.11 origin/release-23.11
cd .worktree/backport-312811-to-release-23.11
git switch --create backport-312811-to-release-23.11
git cherry-pick -x b57aa88291910689aeaa83ff40dd02c50ab8199b 4e353b67f667766b1c6489f23c20b920ab0c82f5 e8c66dc41dd95c70718ba9f5ab34e9ebe23e49c4

github-actions · 2024-05-24T13:35:57Z

Backport failed for release-24.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally and resolve any conflicts.

git fetch origin release-24.05
git worktree add -d .worktree/backport-312811-to-release-24.05 origin/release-24.05
cd .worktree/backport-312811-to-release-24.05
git switch --create backport-312811-to-release-24.05
git cherry-pick -x b57aa88291910689aeaa83ff40dd02c50ab8199b 4e353b67f667766b1c6489f23c20b920ab0c82f5 e8c66dc41dd95c70718ba9f5ab34e9ebe23e49c4

megheaiulian · 2024-06-01T19:56:03Z

          '';
          hardware.opengl = {
-            extraPackages = [ nvidia_x11.out ];
+            extraPackages = [ nvidia_x11.out nvidia_x11.settings.libXNVCtrl ];


❗ This will break things. Depending on config ( useSettings or nvidiaSettings ) nvidia_x11.settings might be a empty set. See https://github.com/NixOS/nixpkgs/blob/nixos-unstable/pkgs/os-specific/linux/nvidia-x11/generic.nix#L215
This should handle that case or be reverted.

Submitted #316516 to address this.

@megheaiulian thanks for the heads up!

github-actions Bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels May 19, 2024

aidalgol requested a review from a team May 19, 2024 06:06

ofborg Bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 11-100 This PR causes between 11 and 100 packages to rebuild on Linux. labels May 19, 2024

ConnorBaker reviewed May 19, 2024

View reviewed changes

Comment thread pkgs/os-specific/linux/nvidia-x11/settings.nix Outdated

SomeoneSerge reviewed May 19, 2024

View reviewed changes

aidalgol requested review from ConnorBaker, SomeoneSerge and abbradar May 19, 2024 18:56

SomeoneSerge approved these changes May 20, 2024

View reviewed changes

wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 20, 2024

aidalgol requested a review from SomeoneSerge May 21, 2024 05:41

SomeoneSerge reviewed May 21, 2024

View reviewed changes

wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2024

SomeoneSerge reviewed May 23, 2024

View reviewed changes

Comment thread pkgs/os-specific/linux/nvidia-x11/settings.nix Outdated

SomeoneSerge added backport release-23.11 labels May 23, 2024

SomeoneSerge reviewed May 23, 2024

View reviewed changes

Comment thread nixos/modules/hardware/video/nvidia.nix Outdated

aidalgol added 3 commits May 24, 2024 15:52

nixos/nvidia: make libXNVCtrl available on nvidia systems

4e353b6

nvidia-settings: add aidalgol as maintainer

e8c66dc

ofborg Bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label May 24, 2024

SomeoneSerge reviewed May 24, 2024

View reviewed changes

SomeoneSerge changed the title ~~Make libXNVCtrl shared library available~~ linuxPackages.nvidia_x11.libXNVCtrl: make the shared library available May 24, 2024

SomeoneSerge merged commit 67d54c2 into NixOS:master May 24, 2024

SomeoneSerge mentioned this pull request May 24, 2024

linuxPackages.nvidia_x11.settings.libXNVCtrl: reconsider impure deployment #314289

Closed

aidalgol deleted the libxnvctrl-shared branch May 24, 2024 21:48

This was referenced May 24, 2024

Backport 312811 to release 24.05 #314402

Closed

Backport 312811 to release 23.11 #314403

Closed

megheaiulian reviewed Jun 1, 2024

View reviewed changes

This was referenced Jun 1, 2024

nixos/nvidia: add missing check for nvidia settings enabled #316516

Merged

Use libXNVCtrl purely #318092

Merged

aidalgol mentioned this pull request Jun 12, 2024

Multiple libXNVCtrl in system after update #319167

Open

	# Only the headers are used from these packages
	# The corresponding libraries are loaded at runtime from the app's runpath
	libXNVCtrl
	wayland
	libX11

Uh oh!

Conversation

aidalgol commented May 19, 2024

Description of changes

Things done

Uh oh!

ConnorBaker left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomeoneSerge May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SomeoneSerge May 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aidalgol commented May 19, 2024

Uh oh!

SomeoneSerge left a comment

Choose a reason for hiding this comment

Uh oh!

aidalgol commented May 20, 2024

Uh oh!

SomeoneSerge commented May 20, 2024

Uh oh!

aidalgol commented May 20, 2024

Uh oh!

SomeoneSerge commented May 20, 2024

Uh oh!

aidalgol commented May 21, 2024

Uh oh!

SomeoneSerge May 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented May 24, 2024

Uh oh!

github-actions Bot commented May 24, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

SomeoneSerge May 20, 2024 •

edited

Loading

SomeoneSerge May 20, 2024 •

edited

Loading

SomeoneSerge May 21, 2024 •

edited

Loading