-
-
Notifications
You must be signed in to change notification settings - Fork 15.1k
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
libglvnd, ocl-icd, vulkan-loader: Add /run/opengl-driver(-32) to RUNPATH. #60985
Conversation
does this mean we can drop https://github.com/NixOS/nixpkgs/blob/master/nixos/modules/hardware/opengl.nix#L148-L150 too? |
I think so, hope there isn't something else that uses it. |
@abbradar How does that sound to you? |
Actually I think |
For CUDA I suppose we could apply the same RUNPATH correction to any code that dlopen's libcuda, but I am not at all familiar with CUDA things. Do apps normally dlopen libcuda directly, or are they linked to it, or is there a standard helper library that takes care of this? |
Forgot about VDPAU though, where we have I will remove that |
Added patching of some CUDA things. With these patches, I have then removed that I suggest we apply the changes in this PR and also remove that |
I gave this a test run on my system (19.03 with these two commits cherry-picked). Also removed adding to @ambrop72, can you add the commit removing from It should be safe to backport (only) 73f34cb to 19.03, too. cc @samueldr @lheckemann |
Sorry forgot about this. Well now I know that maintainers can push to the PR branch by default :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome idea! Regarding complete drop of LD_LIBRARY_PATH
: I'm not sure now if that would break anything else; I'll try to remember what I encountered when I tried to remove it long time ago but don't count on this much. Bottom line is that there were some problems but maybe they are not relevant anymore.
Nah, my idea is to make it non-propagating and only add it to libglvnd and friends. I just feel we'll have more derivations we'd need to fix so we want to factor out the code.
EDIT: Sorry, I thought I'd answer in thread if I reply by mail :D
…On May 20, 2019 11:50:22 PM GMT+03:00, Ambroz Bizjak ***@***.***> wrote:
ambrop72 commented on this pull request.
> @@ -80,6 +81,16 @@ stdenv.mkDerivation rec {
--prefix PYTHONPATH : ${pythonPackages.numpy}/${python.sitePackages}
'';
+ # Set RUNPATH so that libcuda and libnvrtc in
/run/opengl-driver(-32)/lib can be
Is it really a good idea to add that RPATH entry to all binaries (once
you include the hook)? I remember being bothered by some other hooks,
like rpath stripping, not being configurable in that respect.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#60985 (comment)
--
Nikolay.
|
@abbradar I'm fine with dropping the commit removing Do you want to give the RUNPATH hook a try? |
for program in $out/bin/blender $out/bin/.blender-wrapped; do | ||
isELF "$program" || continue | ||
origRpath=$(patchelf --print-rpath "$program") | ||
patchelf --set-rpath "$origRpath:${libGL_driver.driverLink}/lib" "$program" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we want to put driverLink
path first instead? This way if a package depends on Mesa or nvidia drivers directly it would still load system-wide drivers, not the ones it was linked with -- that's the behavior we arguably expect. It's especially important with nvidia because its driver libraries require exactly the same kernel module version and provide stable ABI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is only one libGL that can be linked to, that is the one from libglvnd
. Neither the mesa_drivers
nor nvidia-x11
provide a libGL that can be linked to, only libGL_vendor
that is dlopen-ed from libglvnd
. I do not see any case where this RPATH adjustment would need to override some default library. Adding the RPATH at the end should reduce the performance impact to normal library loading.
@@ -17,7 +17,7 @@ stdenv.mkDerivation rec { | |||
sha256 = "0zhrwj1gi90x2w8gaaaw5h4b969a8gfy244kn0drrplhhb1nqz3b"; | |||
}; | |||
|
|||
nativeBuildInputs = [ pkgconfig ]; | |||
nativeBuildInputs = [ pkgconfig patchelf ]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
patchelf
is included in stdenv
for Linux by default so no need to add it here.
@flokli I implemented a setup hook and added it everywhere. I only manually tested |
@flokli Regarding |
Issues begin when one tries to run a nix-env installed package with a different version of Nvidia or mesa_drivers than current system. Surely this is a flaky setting but we don't want to break it more than necessary. The same problem can happen if one has e.g. Nvidia beta drivers installed and runs a package which has your usual nvidia_x11 in its RUNPATH.
…On May 21, 2019 6:26:03 PM GMT+03:00, Ambroz Bizjak ***@***.***> wrote:
> I didn't mean libGL, rather any drivers themselves. Nvidia has this
problem as I mentioned if one has nvidia_x11 in their RUNPATH for some
reason, but also mesa_drivers. I think there were several cases where
packages were linked to mesa_drivers or nvidia_x11 but I'll recheck
this.
Well I didn't see any issue with appending RUNPATH. For libglvnd and
other dispatch libraries the only thing that matters is that `dlopen`
will find libraries like `libGL_vendor` and there is only going to be
one package that has those, due to the name.
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#60985 (comment)
--
Nikolay.
|
Yeah, I'll build and check a few things as well. |
0ef193d
to
547c6b6
Compare
Tested that all packages with these R(UN)PATH adjustments build and get the correct R(UN)PATH (using I will now rebase locally on top of channels/nixos-unstable and rebuild and test with my system (including the LD_LIBRARY_PATH removal). |
While testing this I got hit by SDDM breakage in nixos-unstable (not caused by this, happens also on vanilla nixos-unstable, #61840). So I used LightDM instead, but with LightDM one still gets something in I will try removing that xserver |
This hook allows to add NixOS driver libraries path to given ELF objects' RUNPATH. We use it instead of settings RUNPATH manually everywhere. It must be invoked in postFixup so that RUNPATH stripping does not remove the path. It puts the path first instead of last so that system-wide drivers are always preferred.
Previously we were relying on LD_LIBRARY_PATH to discover driver libraries (libGL, ligGLX, libEGL, OpenCL and Vulkan). This has the problem that setuid programs (in particular VirtualBox) ignore LD_LIBRARY_PATH. Fix it by setting RUNPATH in various dispatch libraries. This is not needed for libvdpau because it is already configured to look for libraries in the driver paths. Fixes NixOS#22760.
This is to avoid relying on LD_LIBRARY_PATH for finding the CUDA driver libraries.
547c6b6
to
28a0918
Compare
Rearranged commits, dropped the |
Thanks! |
# actually sets RUNPATH not RPATH, which applies only to dependencies of the binary | ||
# it set on (including for dlopen), so the RUNPATH must indeed be set on these | ||
# libraries and would not work if set only on executables. | ||
addOpenGLRunpath() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a no-op on macOS. We can link to the OpenGL framework there I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be a no-op, or we should link to the OpenGL framework?
This pull request has been mentioned on Nix community. There might be relevant details there: https://discourse.nixos.org/t/software-rendering-using-mesa-not-possible/3023/8 |
# Set RUNPATH so that driver libraries in /run/opengl-driver(-32)/lib can be found. | ||
# See the explanation in addOpenGLRunpath. | ||
postFixup = '' | ||
addOpenGLRunpath $out/lib/libvulkan.so |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should not be needed.
Vulkan Loader doesn't need the openGLRunpath
hardcoded because it will look up libraries not in the opengl path but in whatever the ICD json points to. So vulkan will never look in /run/opengl-driver/lib
,
it will instead look in /nix/store/y99l7729c0157a52y9qfzi3zg4f94b57-mesa-noglu-18.3.4-drivers/lib/libvulkan_intel.so
because the ICD
in /run/opengl-driver/share/vulkan/icd.d
says so
cat /run/opengl-driver/share/vulkan/icd.d/intel_icd.x86_64.json
{
"ICD": {
"api_version": "1.1.90",
"library_path": "/nix/store/y99l7729c0157a52y9qfzi3zg4f94b57-mesa-noglu-18.3.4-drivers/lib/libvulkan_intel.so"
},
"file_format_version": "1.0.0"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently needed for NVidia because the library_path is only a name:
$ cat /run/opengl-driver-32/share/vulkan/icd.d/nvidia.json
{
"file_format_version" : "1.0.0",
"ICD": {
"library_path": "libGLX_nvidia.so",
"api_version" : "1.1.99"
}
}
I think we should patch it and then remove the runpath.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. any other drivers that would need similar fixups? ;-)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened #62870 to set absolute paths in NVidia ICD files.
Also related:
- nvidia-x11: Set 32-bit library paths for 32-bit libraries. #62859: Fix that will enable actually dropping RUNPATH in dispatch libraries and LD_LIBRARY_PATH in
opengl.nix
. - vulkan-loader: Always include /run/opengl-driver(-32)/share in search path. #62869: Will enable also dropping
XDG_DATA_DIRS
inopengl.nix
.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/building-with-cuda-on-nixpkgs/4462/8 |
Previously we were relying on LD_LIBRARY_PATH to discover vendor driver libraries (libGL, ligGLX, libEGL, OpenCL and Vulkan). This has the problem that setuid programs (in particular VirtualBox) ignore LD_LIBRARY_PATH. Fix it by setting RUNPATH in various dispatch libraries to look in /run/opengl-driver(-32).
Setting this on the VirtualBoxVM binary (which would be simpler) would not solve the problem, the RUNPATH must be on the library which performs the dlopen(). The dlopen man page claims that the RUNPATH from the executable is used but that is incorrect, only RUNPATH from the module performing dlopen() is used.
Note that setting RPATH on the executable would work, but patchelf manipulates RUNPATH and not RPATH (RPATH is a legacy feature).
Fixes #22760
Fixes #18457.
Motivation for this change
3D acceleration in VirtualBox does not work with NVidia drivers when hardening is enabled for VirtualBox (the default) (#22760)
Things done
Tested this on 19.03, with this change there is no error when starting a VM with 3D acceleration enabled and the VM log shows that OpenGL was initialized successfully.
sandbox
innix.conf
on non-NixOS)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)