Navigation Menu

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

vulkan-loader: Always include /run/opengl-driver(-32)/share in search path. #62869

Merged
merged 1 commit into from Jun 26, 2019
Merged

vulkan-loader: Always include /run/opengl-driver(-32)/share in search path. #62869

merged 1 commit into from Jun 26, 2019

Conversation

ambrop72
Copy link
Contributor

@ambrop72 ambrop72 commented Jun 8, 2019

Even though FALLBACK_DATA_DIRS is set to include this, it only applies when XDG_DATA_DIRS is not defined, so the NixOS opengl.nix module still had to include these in the search path. Use a simple patch to force a default search path, consulted after all other search paths.

Note that FALLBACK_DATA_DIRS is no longer set, and the default (/usr/local/share:/usr/share) applies.

Motivation for this change

Allow getting rid of adding these driver paths to XDG_DATA_DIRS. That will be a separate PR. Adding these paths to XDG_DATA_DIRS may be hurting application startup time due to every application checking them.

Things done

Tested that vulkaninfo and vkcube work without /run/opengl-driver(-32)/share in XDG_DATA_DIRS (did not work before, unless XDG_DATA_DIRS was not set at all), also 32-bit binary on 64-bit system.

  • 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.

@ambrop72
vulkan-loader: Always include /run/opengl-driver(-32)/share in search…
bc96146 
… path.

Even though FALLBACK_DATA_DIRS is set to include this, it only applies when XDG_DATA_DIRS is not defined, so the NixOS opengl.nix module still had to include these in the search path. Use a simple patch to force a default search path, consulted after all other search paths.

Note that FALLBACK_DATA_DIRS is no longer set, and the default (/usr/local/share:/usr/share) applies.
@ambrop72 ambrop72 mentioned this pull request Jun 8, 2019
Merged
10 tasks
@matthewbauer matthewbauer merged commit 525f0b0 into NixOS:master Jun 26, 2019
@ambrop72
Copy link
Contributor Author

Thanks. Anyway it looks like a slightly different change to vulkan-loader will be merged - we'll need to set -DSYSCONFDIR= instead of -DCUSTOM_SEARCH_PATH.

@ambrop72 ambrop72 mentioned this pull request Jun 27, 2019
Merged
10 tasks
ambrop72 added a commit to ambrop72/nixpkgs that referenced this pull request Jun 28, 2019
@ambrop72
nixos/opengl: Don't set XDG_DATA_DIRS.
c07fb9c 
This was added in NixOS#19936 so that vulkan-loader finds the ICD config files. It is
not needed any more after NixOS#62869 where it was ensured that the loader looks in
/run/opengl-driver(-32)/share.
@ambrop72 ambrop72 mentioned this pull request Jun 28, 2019
Merged
10 tasks
@Ralith
Copy link
Contributor

Ralith commented Jun 28, 2019

As the maintainer, I'd have appreciated being pinged regarding the significant change in approach here.

@matthewbauer
Copy link
Member

@Ralith sorry about that! Ofborg usually pings maintainers but it looks like it didn’t in this case… Feel free to state any issues with the pr.

@Ralith
Copy link
Contributor

Ralith commented Jun 28, 2019

I'm not sure I have full context, but it looks like this is part of a larger effort to allow things linked against vulkan-loader to Just Work without depending on the environment? That seems reasonable and I don't see any issues (and I'm glad to see upstreaming in progress!), it was just a surprise.

@ambrop72
Copy link
Contributor Author

I'm not sure I have full context, but it looks like this is part of a larger effort to allow things linked against vulkan-loader to Just Work without depending on the environment? That seems reasonable and I don't see any issues (and I'm glad to see upstreaming in progress!), it was just a surprise.

We don't really have a way to make things Just Work on non-NixOS, this was mostly to simplifying the configuration and bring it in line with how it works for other GPU-based APIs (OpenGL, opencl, vdpau) - to always look in /run/opengl-driver without needing environment variables. In this case it was actually really strange because the vulkan-loader was configured to look there only as a fallback if XDG_DATA_DIRS was unset, so despite having that path hardcoded, it still needed that path in XDG_DATA_DIRS in the end.

@Ralith
Copy link
Contributor

Ralith commented Jun 28, 2019

Yeah, I wrote that code. The idea was that XDG_DATA_DIRS is the authoritative way to find manifests, but we still want to make a good effort to do the right thing when suid precludes inspecting environment variables. It wasn't clear that this was a vulkan-specific problem, and setting XDG variables for /run/opengl-driver rather than hard-wiring the search path into the driver solves the general case.

Per the upstream PR, searching /run/opengl-driver certainly makes more sense than searching /etc on NixOS, but that change may reduce the compatibility of nix-built Vulkan programs with non-NixOS platforms. Of course, that's only an issue if they're dumping manifests in a /etc instead of a freedesktop config/data dir like they should be, but IIRC nVidia was doing that.

@ambrop72
Copy link
Contributor Author

Per the upstream PR, searching /run/opengl-driver certainly makes more sense than searching /etc on NixOS, but that change may reduce the compatibility of nix-built Vulkan programs with non-NixOS platforms. Of course, that's only an issue if they're dumping manifests in a /etc instead of a freedesktop config/data dir like they should be, but IIRC nVidia was doing that.

Looking in /etc on non-NixOS would only result in trying to load libraries from the host OS which will not work in the majority of cases, due to libraries not being found (since we don't use the host ld.so.cache) and/or conflicts between NixOS and host libraries. Using host libraries is just something we cannot do at the moment. There is some discussion about that in #62177. Actually this hazard still remains due to XDG_DATA_DIRS and XDG_RUNTIME_DIRS though.

@ambrop72
Copy link
Contributor Author

Also I think that if we someday find a solution for loading host libraries (I think we might, with modifications of the dynamic linker), it will almost certainly involve patching of dispatch libraries like vulkan-loader and libglvnd to put the dynamic linker into a mode where it will only look in the host filesystem and isolate the NixOS and host world. But -trying to make the search path include the host world now will just lead to more failures before we support that.

@Ralith
Copy link
Contributor

Ralith commented Jun 29, 2019

Makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthewbauer matthewbauer matthewbauer approved these changes

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 11-100
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@ambrop72 @Ralith @matthewbauer