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

Conversation

jonringer
Copy link
Contributor

@jonringer jonringer commented Feb 4, 2022

Would like to move away from referring the gpu and related drivers as "opengl" packages.

It's odd for contributors, newcomers, and external people. Basically everyone.

Rendered: https://github.com/jonringer/rfcs/blob/hardware-driver/rfcs/0121-hardware-driver.md

Continuation of: NixOS/nixpkgs#141803

Nixpkgs PR for NixOS Options: NixOS/nixpkgs#158079
Nixpkgs staging PR for addOpenGLRunpath change: NixOS/nixpkgs#196174

@jonringer jonringer changed the title [0121] Migrate opengl references to api-agnostic terms [0121] Migrate OpenGL References to API-Agnostic Terms Feb 4, 2022
rfcs/0121-hardware-driver.md Outdated Show resolved Hide resolved
@jonringer
Copy link
Contributor Author

The nixpkgs PR for this should be ready to review with exception of release notes: NixOS/nixpkgs#158079

@vcunat
Copy link
Member

vcunat commented Feb 6, 2022

Don't you want compatibility both ways? That is, first add compat symlinks, then wait at least for current stable release to EOL (or backport that and wait somewhat less), and only after that start switching packages to using the new paths?

@jonringer
Copy link
Contributor Author

Don't you want compatibility both ways? That is, first add compat symlinks, then wait at least for current stable release to EOL (or backport that and wait somewhat less), and only after that start switching packages to using the new paths?

If you're talking about users on stable picking packages from unstable, we can can backport the module changes.

I'm also okay with renaming nixpkgs+nixos to the new paradigms, but still using the existing /run/opengl-driver/ path until 22.05

@edolstra
Copy link
Member

edolstra commented Feb 7, 2022

If the only benefit is updating an outdated name with a better one, at the cost of causing a lot of breakage, I'm not really in favor. Unix is full of obsolete names like /usr. But if we keep a /run/opengl-drivers symlink for compatibility, maybe it's not too bad.

@jonringer
Copy link
Contributor Author

But if we keep a /run/opengl-drivers symlink for compatibility, maybe it's not too bad.

This is what is currently done in the PR.

https://github.com/NixOS/nixpkgs/blob/8f256bfa624345c33a0800fac1dc7f4b70b8ddfd/nixos/modules/hardware/drivers.nix#L113

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/include-nixgl-in-nix-flake-app/17588/2

@jonringer
Copy link
Contributor Author

Unix is full of obsolete names like /usr.

We already don't follow FHS, may as well align with better conventions.

@lheckemann lheckemann added the status: open for nominations Open for shepherding team nominations label Feb 9, 2022
@Ericson2314
Copy link
Member

I made NixOS/nixpkgs#153808 a while back that dovetails.

My only question with this is where software rendering fits in, but that's it.

@jonringer
Copy link
Contributor Author

My only question with this is where software rendering fits in, but that's it.

The major difference between our PRs is that my option is hardware.gpu.drivers and yours was hardware.graphics.videoDrivers.

@vcunat
Copy link
Member

vcunat commented Feb 15, 2022

I think the question was around the fact that the current /run/opengl-driver* also contain drivers for purely SW rendering, so it doesn't seem like the new naming is "flawless"... well it's not like I think it must be flawless, but it seems to be a similar kind of discrepancy that this RFC is trying to address, matching some parts better and others worse (compared to what we have now).

EDIT: maybe "video-drivers" would be a better combination, on a quick glance. But it's still not like I'm convinced that renaming is worth it.

@jonringer
Copy link
Contributor Author

contain drivers for purely SW rendering

I was also toying with the idea of system-drivers. NixOS/nixpkgs#141803 (comment). I don't have a strong opinion on one or the other. I leaned toward the side of using hardware as that would probably align more with current user expectations. If people feel that system-driver, or something better should take it's place, I'm fine with updating the rfc and related nixpkgs PR

@roosemberth
Copy link

My two-cents is that hardware-drivers is a much less surprising name, since accelerators are usually hardware-based. This intuition still holds even if the actual implementation just requires a CPU (in case of a sw driver). system-drivers is a bit too vague and give me little to none intuition about what's in there compared to eg. firmware.

@colemickens
Copy link
Member

How about /run/current-system/drivers/ ? Then there's room to grow under /run/current-system. "drivers" can be used as it's sort of contained within current-system, current-system will probably stick out to someone with some nixos familiarity...? Just a thought, all of the other suggestions are just as fine.

@vcunat
Copy link
Member

vcunat commented Feb 18, 2022

That sounds possible, but note that it would change the moment when the link is being switched. (though I'm not entirely sure which moment is best anyway)

@edolstra
Copy link
Member

CPUs are hardware too :-)

But I think drivers is a fine name.

@jonringer
Copy link
Contributor Author

jonringer commented Feb 21, 2022

How about /run/current-system/drivers/ ? Then there's room to grow under /run/current-system. "drivers" can be used as it's sort of contained within current-system, current-system will probably stick out to someone with some nixos familiarity...? Just a thought, all of the other suggestions are just as fine.

I really like this, I can update the RFC if there's more agreement. Please thumbs up this comment for me to change to /run/current-system/drivers/, thumbs down if you want something else

@vcunat
Copy link
Member

vcunat commented Feb 22, 2022

I wonder if there was some rationale for the (OpenGL) drivers to switch on display-manager restart and not immediately on nixos-rebuild switch. Perhaps that should be tested with DEs relying on OpenGL. (EDIT: I mean, you can change the current link manually.) Still, I don't really expect any issues from that, beyond the steps needed for forward and backward compatibility.

@edolstra edolstra changed the title [0121] Migrate OpenGL References to API-Agnostic Terms [RFC 0121] Migrate OpenGL References to API-Agnostic Terms Feb 23, 2022
@kevincox
Copy link
Contributor

This RFC is open for nomination. Please nominate yourself or others!

Co-authored-by: Sandro <sandro.jaeckel@gmail.com>
@Ericson2314
Copy link
Member

I'll nominate myself; that way we make sure https://github.com/NixOS/nixpkgs/pull/153808/files is adopted to the agreed-upon plan and merged too.

@jonringer
Copy link
Contributor Author

I mean, there was a reference PR, and all that was needed was meeting, which never came to be.

cc @edolstra @Ericson2314 @colemickens

@Ericson2314
Copy link
Member

Yeah we should have the meeting about this finally. It would be good to finish this! :)

@jonringer
Copy link
Contributor Author

@edolstra @Ericson2314 @colemickens and I met today, we are good with moving forward with this as-is. @colemickens would also like to clean up the "stringly typed api" for drivers.

The implementation PR should just need a rebase.

@jonringer
Copy link
Contributor Author

Fixed some minor formatting or typos in paths

@vcunat
Copy link
Member

vcunat commented Oct 5, 2022

Before going forward with this, I'd feel safer if someone confirmed that they've looked into when/how it's best to switch the opengl libs (and perhaps others). This RFC will couple it to current-system for longer future and that will be difficult to undo. If noone used nixos-rebuild switch and just boot, it'd be simple. For example, booted-system might be an alternative, as the user-space drivers are somewhat coupled to kernel side AFAIK (at least in edge cases like nixos-rebuild switch between nvidia and nouveau).

@jonringer
Copy link
Contributor Author

Before going forward with this, I'd feel safer if someone confirmed that they've looked into when/how it's best to switch the opengl libs (and perhaps others). This RFC will couple it to current-system for longer future and that will be difficult to undo. If noone used nixos-rebuild switch and just boot, it'd be simple. For example, booted-system might be an alternative, as the user-space drivers are somewhat coupled to kernel side AFAIK (at least in edge cases like nixos-rebuild switch between nvidia and nouveau).

Currently the /run/opengl-driver/ will be present indefinitely for backwards compatibility, we can just hold off on referring to the new location for a release cycle when packaging (addHardwareRunpath will continue to use /run/opengl-driver/lib for 22.11) . I agree though, that a hard transition will not go well outside of a nixos-rebuild switch && reboot workflow.

@jonringer
Copy link
Contributor Author

@vcunat I decided to not change anything on the nixpkgs side other than addOpenGLRunpath is now deprecated for addHardwareRunpath, so that all the packages are still referencing the existing /run/opengl-driver/lib. I'll create a future-work item to transition this, but use the 22.11 release a transition period where the implementation is the old paradigm and new paradigm, but the API is the new one. Post 22.11 branch off, we can update nixpkgs to reference the new driver path.

@jonringer
Copy link
Contributor Author

Updated RFC to include timeline for the changes to be rolled-out fff5f1d

@vcunat
Copy link
Member

vcunat commented Oct 17, 2022

OK, pre-21.11 steps sound very safe to me.

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

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

Comment on lines +14 to +15
Currently, NixOS mounts video drivers under the path `/run/opengl-driver/`,
but should be mounted under a more generic `/run/current-system/drivers/` path.
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.

@jonringer
Copy link
Contributor Author

I think there's a way to slice this RFC into two smaller pieces, one less controversial, and one more controversial:

  • The user facing API parts:
    • 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)
  • The "NixOS" implementation parts:
    • Update nixpkgs references of /run/opengl-driver/ to point to /run/current-system/drivers/ (post 22.11)
    • Update mesa.driverLink to point to /run/current-system/drivers/lib (post 22.11)

I think @Ericson2314 @colemickens and I really want to see the user facing parts move away from usages of opengl and xserver, as it's awkward in many scenarios ( wayland, cuda, etc). The NixOS parts I'm fine with deferring if people (e.g. @infinisil ) feels like there's a better implementation that hasn't been realized.

In other words, I'm fine with moving the nixos mounted paths to future work, and just scaling back the scope of this RFC to just moving away from stale nixos options.

@infinisil
Copy link
Member

@jonringer That sounds great, I'd be in favor of this RFC if it is limited to that part.

  • The user facing API parts
  • The "NixOS" implementation parts

I think the distinction there is NixOS-exclusive vs non-NixOS-exclusive: Renaming these options is something we can do easily since they're exclusive to NixOS and we have full control over it. Comparatively, the runtime paths are not exclusive to NixOS since the packages that use them can also be used on other systems.

@colemickens
Copy link
Member

@infinisil Is this basically in reference to something like NixGL? I'm think I'm unaware of other non-NixOS users of these paths.

@jonringer
Copy link
Contributor Author

Any other feedback? I had started rebasing all of my original changes, but didn't want to be pushed into a different direction.

@Ericson2314
Copy link
Member

I think that is the only feedback. Let's finish this!


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?

@jonringer
Copy link
Contributor Author

Scoping this RFC down to just module changes does not require an RFC.

Closing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.