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

Jump to bottom

nixos: Don't set LD_LIBRARY_PATH for graphics drivers that don't need it. #61981

Merged
merged 1 commit into from Jul 11, 2019
Merged

nixos: Don't set LD_LIBRARY_PATH for graphics drivers that don't need it. #61981

merged 1 commit into from Jul 11, 2019

Conversation

ambrop72
Copy link
Contributor

@ambrop72 ambrop72 commented May 23, 2019 •
edited

A new option hardware.opengl.setLdLibraryPath is added which controls if LD_LIBRARY_PATH should be set to /run/opengl-driver(-32)/lib. It is false by default and is meant to be set to true by any driver which requires it. If this option is false, then opengl.nix and xserver.nix will not set LD_LIBRARY_PATH.

Currently Mesa and NVidia drivers don't set setLdLibraryPath because they work with libglvnd and do not override libraries, while amdgpu-pro, ati and parallels-guest set it to true (the former two really need it, the last one doesn't build so is presumed to).

Additionally, the libPath attribute within entries of services.xserver.drivers is removed. This made xserver.nix add the driver path directly to the LD_LIBRARY_PATH for the display manager (including X server). Not only is it redundant when the driver is added to hardware.opengl.package (assuming that hardware.opengl.enable is true), in fact all current drivers except ati set it incorrectly to the package path instead of package/lib.

This removal of LD_LIBRARY_PATH could break certain packages using CUDA, but only those that themselves load libcuda or other NVidia driver libraries using dlopen (not if they just use cudatoolkit). A few have already been fixed but it is practically impossible to test all because most packages using CUDA are libraries/frameworks without a simple way to test.

Fixes #11434 if only Mesa or NVidia graphics drivers are used.

Motivation for this change

LD_LIBRARY_PATH needs to go away.

Things done

Tested this with my desktop system with NVidia and observed no problems, including running various programs using OpenGL, Vulkan, VDPAU, CUDA and OpenCL. It would benefit from testing with other drivers.

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

abbradar
abbradar approved these changes May 25, 2019
View reviewed changes
Copy link
Member

@abbradar abbradar left a comment

Choose a reason for hiding this comment

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

I wanted us to arrive to this for quite some time, nice!

nixos/modules/hardware/opengl.nix Show resolved Hide resolved
@ambrop72
nixos: Don't set LD_LIBRARY_PATH for graphics drivers that don't need…
370d3af 
… it.

A new internal option `hardware.opengl.setLdLibraryPath` is added which controls if `LD_LIBRARY_PATH` should be set to `/run/opengl-driver(-32)/lib`. It is false by default and is meant to be set to true by any driver which requires it. If this option is false, then `opengl.nix` and `xserver.nix` will not set `LD_LIBRARY_PATH`.

Currently Mesa and NVidia drivers don't set `setLdLibraryPath` because they work with libglvnd and do not override libraries, while `amdgpu-pro`, `ati` and `parallels-guest` set it to true (the former two really need it, the last one doesn't build so is presumed to).

Additionally, the `libPath` attribute within entries of `services.xserver.drivers` is removed. This made `xserver.nix` add the driver path directly to the `LD_LIBRARY_PATH` for the display manager (including X server). Not only is it redundant when the driver is added to `hardware.opengl.package` (assuming that `hardware.opengl.enable` is true), in fact all current drivers except `ati` set it incorrectly to the package path instead of package/lib.

This removal of `LD_LIBRARY_PATH` could break certain packages using CUDA, but only those that themselves load `libcuda` or other NVidia driver libraries using `dlopen` (not if they just use `cudatoolkit`). A few have already been fixed but it is practically impossible to test all because most packages using CUDA are libraries/frameworks without a simple way to test.

Fixes NixOS#11434 if only Mesa or NVidia graphics drivers are used.
@ambrop72
Copy link
Contributor Author

Updated to make the option internal.

@infinisil
Copy link
Member

What is the problem with setting LD_LIBRARY_PATH for drivers that don't need it?

@ambrop72
Copy link
Contributor Author

What is the problem with setting LD_LIBRARY_PATH for drivers that don't need it?

Unnecessary overhead looking for libraries in those folders (on 64-bit systems, this may involve actually finding the library and then figuring out it's not for the right architecture). Potential to result in overriding libraries that were not intended to be. And generally we shouldn't be setting environment variables that are not needed.

@ambrop72
Copy link
Contributor Author

ambrop72 commented Jun 8, 2019

This change caused breakage of 32-bit apps due to incorrect patchelf-ing in nvidia-x11, will be fixed by #62859.

@matthewbauer
Copy link
Member

@ambrop72 Is this one ready to go?

@ambrop72
Copy link
Contributor Author

@matthewbauer Mostly, but I figured no one tested if mesa drivers still work so let me first do a sanity test on a different machine. I can't test ati drivers though. But in all theory it should not break.

@matthewbauer
Copy link
Member

Yeah I wish we had better automated tests for this stuff, but I don't think it would be possible without actually having the hardware available.

@vcunat
Copy link
Member

vcunat commented Jun 26, 2019

I can test "amdgpu", including 32-bit.

@vcunat
Copy link
Member

vcunat commented Jun 26, 2019

For now I tried various stuff just by unsetting the variable when running them. There's this risk that older versions (that still require the variable) will stop working for people, but that sounds OK-ish, as it's a long time now IIRC and people can work around simply by setting the variable manually or finally updating.

@ambrop72
Copy link
Contributor Author

I will test this with Intel and AMD graphics very soon, however, we first need to merge #62870, or NVidia will break.

@ambrop72
Copy link
Contributor Author

Sorry that was not completely correct because those dispatch libraries currently use RUNPATH that would prevent a problem, though I am planning to get rid of that RUNPATH in a future PR.

@ambrop72
Copy link
Contributor Author

Tested these drivers (services.xserver.videoDrivers) with master and this PR cherry-picked:

  • ati: glxinfo, es2_info, vdpauinfo (opencl and vulkan don't work here anyway).
  • amdgpu: glxinfo, es2_info, vdpauinfo, vulkaninfo (opencl don't work here anyway).
  • intel: glxinfo, es2_info, vulkanifo (opencl and vdpau don't work here anyway).
    Also everything works with NVidia driver (tested that earlier). I think this is ready.

@vcunat
Copy link
Member

vcunat commented Jun 27, 2019

Nitpick: I wonder if all the glvnd-related changes are worth a summary sentence in release notes, perhaps describing all together. (Perhaps later when RUNPATH is included.)

@ambrop72
Copy link
Contributor Author

Agree. Also pending is removal of XDG_DATA_DIRS from opengl.nix which can be done because of #62869.

@ambrop72
Copy link
Contributor Author

ambrop72 commented Jul 1, 2019

I think this should be merged. Everything I could possibly test works perfectly.

@worldofpeace worldofpeace merged commit a6ce6c1 into NixOS:master Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@abbradar abbradar abbradar approved these changes

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

drop LD_LIBRARY_PATH for opengl programs
6 participants
@ambrop72 @infinisil @matthewbauer @vcunat @abbradar @worldofpeace