@Ralith, thanks for your PR! By analyzing the history of the files in this pull request, we identified @corngood, @expipiplus1 and @lummax to be potential reviewers.

In the hopes of drastically simplifying future updates, I'm attempting to upstream our patches at KhronosGroup/Vulkan-LoaderAndValidationLayers#1426 and KhronosGroup/glslang#706.

I've been working on graphics (including Vulkan) support lately. Do we really need those patches? I'm not sure if there are any benefits from using XDG_DATA_DIRS for this instead of just patching loader to use mesa.driverLink, i.e.;

  NIX_CFLAGS_COMPILE = [
    "-UEXTRASYSCONFDIR"
    ''-DEXTRASYSCONFDIR="${mesa_noglu.driverLink}/share"''
  ];

EDIT: This is about vulkan-loader patches, not glslang.

@abbradar How would that work for other vulkan drivers? Also using xdg paths allows layers (and drivers, theoretically) to be installed in the user's nix profile.

@corngood What do you mean by "other"? If you talk about non-Nix we can add more paths but right now I think it'll still search in /usr/share/vulkan/icd.d.

Adding drivers in the user's profile would be difficult to implement for multilib (we have /run/opengl-driver-32 for this, but we don't have separate 32-bit profiles). Same problem with layers.

@abbradar Other drivers such as amdgpu-pro, nvidia, and hopefully more in the future.

For multilib, I believe AMD just provides manifests for both and one or the other will fail.

Where would your proposed patch for the loader actually live?

@corngood This patch covers all drivers which are installed to /run/opengl-driver{,-32} (just like other runtime drivers currently, like VDPAU/VA-API) -- it just makes vulkan-icd-loader to search there for ICD manifests. I have tested it for Intel but it should work the same for nvidia and others.

I meant that in Nix drivers are usually not packaged in multilib fashion, but rather separately for 32-bit and 64-bit. They are then separately installed to /run/opengl-driver and /run/opengl-driver-32, so one contains purely 64-bit drivers (pkgs.linuxPackages.nvidia_x11) and other 32-bit (pkgsi686Linux.linuxPackages.nvidia_x11). However from more thought this is a non-issue if 1) absolute paths are used in ICDs and 2) ICDs have different names in 32-bit and 64-bit packages. If both points hold one can just install both packages to his/her profile.

The "patch" is actually those four lines (NIX_CFLAGS_COMPILE) so it doesn't need to live anywhere (it's just a redifinition of a macro).

EDIT: To clarify: I was wrong about multilib so my only point is that all the complexity of the patch may be unjustified -- redefining one macro via NIX_CFLAGS_COMPILE is much simpler.

After yet more thought, even if the patch gets accepted upstream we'd still need to redefine EXTRASYSCONFIDR to point to mesa_noglu.driverLink -- otherwise one needs XDG_DATA_DIRS to point to /run/opengl-driver{,32} for Vulkan to work correctly on NixOS, which seems a bad idea to me.

@abbradar Ok, now I understand. I was thinking you were patching it for to the mesa path in /nix/store, not the driverLink. Here's our discussion from when this was first added:

#19936

@corngood Thanks for the link!

I think that we can leave the patch as is but instead of prepending things to XDG_DATA_DIRS redefine the macro to point to /run/opengl-driver{,32}. This way we get the best of both worlds. Thoughts?

@abbradar If we keep the patch, I don't see what we gain by rolling back XDG_DATA_DIRS.

I still think it would be reasonable to just put the manifests in /run/current-system/sw/share, and leave the XDG paths as they were, but there were fair arguments made against that in the other thread.

@Ralith ?

@corngood We gain simplicity -- we don't need to set environment variables and things just work. This is important for example for FHS chrootenvs but generally I think we want to keep away from global environment variables as much as possible -- hardcoded values are better in cases of well-known paths.

Manifests in /sw/share seem worse to me because then we 1) need to install Vulkan drivers via systemPackages while we use hardware.opengl.{package,extraPackages} for OpenGL/OpenCL/VDPAU/VA-API now; 2) need additional effort to work with multilib.

@abbradar With the patch we maintain the ability to locate ICDs in /sw/share, which could be useful in cases where they aren't tied to the graphics hardware.

As for the XDG_DATA_DIRS vs EXTRASYSCONFDIR, I don't have really strong feelings either way. It would be nice if vulkan-loader was still portable, but that shouldn't be a big deal.

@corngood Yep, I believe now that patch is still useful -- what I propose is to use EXTRASYSCONFDIR instead of using environment variables in addition to the patch, to omit need of environment variables on NixOS.

It should still be portable because EXTRASYSCONFDIR by default points to /etc giving /etc/vulkan/icd.d while de facto standard is /usr/share/vulkan/icd.d. We could also make it better by patching loader instead to look in driverLink in addition to EXTRASYSCONFDIR (I was too lazy to do this but should be very simple).

EDIT: To clarify -- vulkan-loader uses not only EXTRASYSCONFDIR but also several other paths including /usr/share/vulkan/icd.d.

EXTRASYSCONFDIR isn't meant to be used in this manner; by placing it in NIX_CFLAGS_COMPILE we'd be circumventing the value provided to it by the build system, which is a little bit ugly.

Using XDG_DATA_DIRS confers a variety of benefits. For one, it should Just Work in any chroot, FHS or something more exotic, that sets that variable correctly, which is a prerequisite for many other applications to be well-behaved. Supporting the user's profile would also otherwise require nontrivial patching. Finally, I believe adding /run/opengl-driver{,32} to XDG_DATA_DIRS as we already have done is perfectly legitimate and consistent with the intended semantics of the variable: packages are installed there and their data should show up in searches in general. Vulkan is the first case where we need this (afaik) but it may not be the last. As discussed in the previous issue, having such a location (vs. using the system profile) is justified because hardware drivers often cannot be updated independently of the kernel, and nixos-rebuild switch shouldn't break your system.

I agree that the size and scope of this patch is a maintenance problem, which is why I'm working to upstream it, at which point NixOS (and other non-FHS distros too) will need neither a patch nor any sketchy compile flags. I believe there's a clear case to be made that the new behavior is strictly more portable and correct than the old, so I think it has good odds of being merged before the next time we need to update this package.

As far as hardcoded values vs. environment variables agree, that's a bit of a philosophical question which departs from the immediate practical benefits this patch confers. The XDG basedir spec exists and is widely used, and programs are simpler and more portable in both source and binary form than they once were as a result. Without it, we wouldn't be able to install nontrivial programs in user profiles without immense amounts of patching.

Whoops, looks like discussion moved on while I was writing that.

NixOS will always need XDG_DATA_DIRS as outlined above, and our addition of /run/opengl-driver{,32} to it is reasonable. I don't see what hacking the compile flags to the vulkan loader buys us there. What we have here works and is idiomatic.

This is getting increasingly offtopic (this PR does not change any behavior!), but note also that vulkan-loader searches /etc by default for a reason; hardware drivers on other distros (e.g. the proprietary nVidia driver) do sometimes put their ICDs there. This isn't relevant to NixOS but is harmless and is unlikely to be removed upstream.

@Ralith Thanks for spending your time to write an elaborate response!

First, I agree that following XDG spec is a Good Thing and should be followed as close as possible. I also agree that EXTRASYSCONFDIR is a hack and can be done better (it's just that it has the least amount of effort needed). Finally, I don't have any more issues with maintenance burdens of this patch and agree that it seems a nice idea overall.

Still, I think that hardcoding /run/opengl-driver{,32} as a default path can improve usability -- in the same way we don't use LIBVA_DRIVERS_PATH, DRI_DRIVERS_PATH and other variables even though we can instead of patching corresponding libraries. It's a good thing that programs work without environment variables, and keeping that property is worth it IMO. XDG_DATA_DIRS and XDG spec is somewhat different on this -- it's used by multitude of libraries/programs in their own ways and we don't want to patch them all. This path, on the other hand, can be added to the loader and expected to propagate to nearly all uses.

You are completely right that I've started an offtopic discussion and I'm sorry about that. Let's stop discussing it here; I'm going to make a PR which adds /run/opengl-driver{,32} to the list of default search paths (in more elaborate way than changing EXTRASYSCONFDIR) later as part of some other improvements and we can continue that conversation.

Tested with Mesa drivers; works fine for me.

The vulkan-loader patch has been merged upstream; future updates to this package will no longer need it.

That's great. Thanks for taking care of it.