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

[RFC 0121] Migrate OpenGL References to API-Agnostic Terms #121

Closed
wants to merge 7 commits into from
Closed
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
88 changes: 88 additions & 0 deletions rfcs/0121-hardware-driver.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,88 @@
---
feature: Use a more canonical name for hardware driver path and modules
start-date: 2022-02-03
author: @jonringer
co-authors:
shepherd-team: @edolstra @colemickens @Ericson2314
shepherd-leader: @colemickens
related-issues: https://github.com/NixOS/nixpkgs/issues/141803
---

# Summary
[summary]: #summary

Currently, NixOS mounts video drivers under the path `/run/opengl-driver/`,
but should be mounted under a more generic `/run/current-system/drivers/` path.
Comment on lines +14 to +15
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure if this is actually going in the right direction. There's already the problem where nixpkgs binaries are hardcoded to the /run/opengl-driver path, which prevents them from working on non-NixOS systems by default. With this change, this becomes even more NixOS specific. NixGL was created to fix this problem, but that's only a workaround, not a permanent solution.

So I'd say rather than trying to fix the naming of something that's already problematic, how about we instead focus on fixing the problem at its core, then the misnaming will go away automatically when it's replaced with something better.

Copy link
Contributor Author

@jonringer jonringer Oct 18, 2022

Choose a reason for hiding this comment

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

"Don't let perfect be the enemy of good".

I agree that the solution isn't ideal, but I can't think of a better one which can be achieved in a reasonable of time or significant work on how derivations work.

I just want Nixpkgs/NixOS in a better state. I'm fine delaying the heavier ramifications of using a different path (e.g /run/current-system/sw/driver), which have already been postponed for allowing for a more seamless upgrade path for users.

But I really just want the user facing "APIs" to be more intuitive (e.g. hardware.gpu.drivers = [ "nvidia" ]; rather than services.xserver.videoDrivers = [ "nvidia ];).

This RFC has already been open for ~10 months, it's almost impossible to get anything moved through in this RFC process; this really encourages people to just create a nixpkgs PR, and get a sympathetic committer to merge it and avoid the RFC process altogether.

The usage of opengl explicitly may have been an accurate name given the time
in which NixOS was first created; however, graphic drivers alone include much
more than just userland graphics libraries so this usages is now misaligned with
current paradigms. The misalignment with this path and the related Nixpkgs
utilities are a misnomer in most contexts, and contributes to Nixpkgs'
"weirdness budget".

# Motivation
[motivation]: #motivation

Video drivers ship many hardware acceleration libraries these days: vulkan,
opengl, opencl, video encoding, cuda, and many other userland libraries are
installed. Having all of these libraries placed under a single "opengl" header
is odd for many contributors, especially newcomers. This awkwardness is
compounded with other specialized hardware such as fpgas, tpus, and asics
where their function isn't related to graphics, however, also need to make
use of a "known good" path in which the related userland libraries will be mounted.

To remedy this misalignment, a new convention around placing those libraries
should be used, preferably `/run/current-system/drivers/`.

# Detailed design
[design]: #detailed-design

Most of the implementation of the current paradigms will continue, as there
will always be a need for some "impurity" around hardware based libraries.
The significant changes will be around naming conventions used within
Nixpkgs and NixOS.

Deprecate and move existing `/run/opengl-driver/` logic:
- Rename `hardware.opengl` options to `hardware.drivers` (pre 22.11)
- Rename `pkgs.addOpenGLRunpath` shell hook to `addHardwareRunpath` (pre 22.11)
- Alias `addOpenGLRunpath` to `addHardwareRunpath` for compatibility (pre 22.11)
- Update nixpkgs references of `/run/opengl-driver/` to point to `/run/current-system/drivers/` (post 22.11)
Copy link
Member

@infinisil infinisil Oct 18, 2022

Choose a reason for hiding this comment

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

If /run/current-system is used, we should nest it under /run/current-system/sw at a particular suffix like /run/current-system/sw/nix-support/drivers. This way Nix packages can install drivers into $out/nix-support/drivers and if they're added to environment.systemPackages, they will automatically populate /run/current-system/sw (needs environment.pathsToLink = [ "nix-support/drivers" ] though)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wouldn't this cause issues if someone were to do environment.systemPackages = lib.mkForce [ <some minimal collection of tooling they need ];.

Will also require changes to "driver" builds, looks like only mesa has a driver output currently.

- Update `mesa.driverLink` to point to `/run/current-system/drivers/lib` (post 22.11)

For compatibility with existing nixpkgs packages, `/run/opengl-driver{,-32}/` will
be a symbolic link to `/run/current-system/drivers{,-32}/`. This will likely be
an indefinite change, or else older packages will not work on NixOS. Also,
we need to ensure compatibility with out-of-tree code which may have been built around
the opengl paths, such as [nixGL](https://github.com/guibou/nixGL).
Copy link
Member

@infinisil infinisil Oct 18, 2022

Choose a reason for hiding this comment

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

I think this means that both /run/opengl-drivers and /run/current-system need to be checked for drivers by the binaries right? Because on non-NixOS systems, we can't automatically update the drivers to now go to /run/current-system.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, only one path will be written to the ELF files.

Current PR to change the name of addOpenGLRunpath, but not the implementation (still points to /run/opengl-driver/lib)

NixOS/nixpkgs#196174


# Drawbacks
[drawbacks]: #drawbacks

Technical churn. Doesn't provide any new or added benefit, other than
correcting a stale misnomer.

# Alternatives
[alternatives]: #alternatives

Continue to use `/run/opengl-driver` in it's current state.

# Unresolved questions
[unresolved]: #unresolved-questions

Is there a better name for the path, nixos module options, and nixpkgs hook?

This is also an opportunity to revisit the `hardware.opengl` module. Some
potential improvements could be:
- Move `hardware.opengl.package` + `hardware.opengl.extraPackages` to a single `hardware.drivers.packages`
Copy link
Member

Choose a reason for hiding this comment

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

This makes replacing mesa in particular harder, as now it's a single list. Using an overlay isn't practical as it rebuilds far too much.

Making this a set so you can do hardware.drivers.packages.mesa = myCustomMesa could solve that. Leaving it as is after the rename also works.

I originally commented this on the impl PR instead of the RFC accidentally.

Copy link
Member

Choose a reason for hiding this comment

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

How special is mesa? Does everything use it now?

I will concede that if it is special, that is not just an OpenGL thing, but also something that could be true in a Vulkan / many languages world.

Copy link
Member

Choose a reason for hiding this comment

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

Having it as a set makes it easier to replace any specific package, we'd change anywhere in nixpkgs that sets it to eg do hardware.drivers.packages.amdvlk-pro = something or hardware.drivers.packages.nvidia so it wouldn't be mesa specific and makes overriding any specific package easier.

Mesa is sort of special - any system with working gl/vulkan is expected to have it, even when using other proprietary libs alongside it - but I don't think handling specifically for mesa is the right approach when we could make it easier to override any hardware.drivers package with the right approach.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with the general point here that in the modules system, lists are poorly composable. They can only be appended to, or replaced outright. We don't have the tools to allow a user to remove or replace an element from a configuration system list.

There are too many options already that imo should be migrated to attrsets to let the user be in control. Let's not add more if we can.

Copy link
Member

@Ericson2314 Ericson2314 Nov 20, 2022

Choose a reason for hiding this comment

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

I wouldn't set across the board because names are arbitrary. Like, if I do foo = <Nvidia-drivers-package>; whose to say I'm doing anything wrong?

Copy link
Member

Choose a reason for hiding this comment

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

You aren't doing anything wrong if you do that and it isn't much of a problem if you do that, unless you get it included in nixpkgs I guess.

If you do that alongside something else setting nvidia = <another nvidia drivers package> you could get two version of the same package by mistake. That could be prevented by asserting that the pnames of the packages involved are unique although I'm not sure that's necessary.

Copy link
Member

Choose a reason for hiding this comment

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

I am not sure how express this, but we shouldn't design things around names. Perhaps other modules can make configurable what package they would add to this list, but unless we can characterize "structurally" how these package fit into different roles I would leave it just as is.

Copy link
Member

@LunNova LunNova Nov 21, 2022

Choose a reason for hiding this comment

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

Using a list to avoid the names is even worse, as your only options are to use an overlay and rebuild everything or to mkForce and have to keep track of any other entries that should be in the list.

If we keep it as is and have .package and .extraPackages then we aren't relying on names but we're also only making it easy to override mesa, and it's even easier to accidentally include duplicate packages of different versions in the list as instead of relying on a name there's nothing to prevent dupes.

e: I guess it's at least agreed that we shouldn't combine them into one list and that that's worse than the current approach?

- The current paradigm seems to be a compromise of `package` existing, but needed to adapt for other hardware acceleration libraries.
- Enable option can default to `hardware.drivers.packages != []` ?
- Move `services.xserver.videoDrivers` to `hardware.gpu.drivers`?
- Very awkward for sway users to need to set something in `services.xserver`

# Future work
[future]: #future-work

- Execute the actions outlined in the detailed design
- Update release notes
- Update documentation which mentions older usages of the shell hook or nixos options.