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

nixos: opengl -> impure-drivers.opengl #276558

Closed

Conversation

SomeoneSerge
Copy link
Contributor

@SomeoneSerge SomeoneSerge commented Dec 24, 2023

Description of changes

WIP. Following #275241 and #269475

The motivation is that it's extremely confusing for the new users when instructions for enabling something like CUDA involve setting hardware.opengl.enable and services.xserver.videoDrivers. The module's new name is meant to suggest that it's there to ensure integration with the addDriverRunpath, and it hopefully explains why/when the module has to be enabled (when we're forced to link drivers "impurely").

I think it'd also make sense to introduce an option like hardware.impure-drivers.cuda.enable, which would guide the user through the process of setting up the x11 or datacenter drivers (e.g. through assertions). I'd keep that for a separate PR

Thoughts? @jonringer @Kiskae @NixOS/cuda-maintainers @NixOS/rocm-maintainers

Things done

  • Tested, as applicable:
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 24, 2023
@SomeoneSerge SomeoneSerge added the 6.topic: cuda Parallel computing platform and API label Dec 24, 2023
@github-actions github-actions bot added the 8.has: documentation This PR adds or changes documentation label Dec 24, 2023
@SomeoneSerge SomeoneSerge force-pushed the refactor/nixos/impure-drivers branch from d636249 to ae4b5f8 Compare December 24, 2023 23:06
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Dec 24, 2023
@SomeoneSerge SomeoneSerge force-pushed the refactor/nixos/impure-drivers branch from a60fc1d to c751b97 Compare December 24, 2023 23:31
"L+ /run/opengl-driver-32 - - - - opengl-driver"
else if cfg.driSupport32Bit then
"L+ /run/opengl-driver-32 - - - - ${package32}"
"L+ ${impurePath32} - - - - ${impurePath}"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The target used to be a relative path ("opengl-driver"), now it's absolute ("/run/opengl-driver"). Shouldn't matter

@SomeoneSerge SomeoneSerge changed the title nixos: hardware.opengl -> hardware.impure-drivers.opengl nixos: opengl -> impure-drivers.opengl Dec 24, 2023
Comment on lines 34 to 37
# The old option `hardware.opengl.enable` corresponds to `hardware.impure-drivers.opengl.enable` (defaults to `true`).
# However, in order support transition of the systems have been setting `hardware.opengl.enable`,
# we need to translate it into `hardware.impure-drivers.enable` (defaults to `false`)
(mkRenamedOptionModule [ "hardware" "opengl" "enable" ] [ "hardware" "impure-drivers" "enable" ])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the most questionable decision is this. Maybe I shouldn't be introducing hardware.impure-drivers.opengl.enable at all?

Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps leave hardware.opengl.enable unchanged and make hardware.impure-drivers.opengl.enable depend on its value?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

H'm, how about I keep hardware.opengl.enable, drop hardware.impure-drivers.opengl, but add hardware.impure-drivers.enable = true to hardware.opengl's config?

@SomeoneSerge SomeoneSerge force-pushed the refactor/nixos/impure-drivers branch from c751b97 to 49c8d09 Compare December 25, 2023 02:38
@Tungsten842
Copy link
Member

There is another attempt at this, not sure about the status: #158079.
I personally prefer hardware.drivers compared to hardware.impure-drivers.

@SomeoneSerge
Copy link
Contributor Author

SomeoneSerge commented Dec 27, 2023

There is another attempt at this, not sure about the status: #158079.
I personally prefer hardware.drivers compared to hardware.impure-drivers.

Oooh right, I forgot about those. I'll close this for now, let's see where @jonringer and @Atemu are at?

@Atemu
Copy link
Member

Atemu commented Dec 28, 2023

I haven't had much time to work on it and that likely won't change until April.

The main point I've been stuck on is that I wanted to build a better pattern to select packages that require multiple bitnesses. For example, right now we have this ugly package package32 pattern where you must set two options to change anything and it's possible to set those to different things (other than bitness) or forget to set one.

I've tried a few different things and for now the best I've come up with is building a "package selector" type which is a function from attrset -> drv that takes pkgs as an argument and returns a single package. In the implementation, you'd simply call it with the relevant package set.

Again, I haven't spent as much time on this as I had hoped.

@jonringer
Copy link
Contributor

I've been hopign to do the refactor as well. But wanted to record it to demonstrate how to deprecate old options. just haven't had a single window to free time to do so.

@SomeoneSerge SomeoneSerge mentioned this pull request Jun 23, 2024
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: cuda Parallel computing platform and API 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

5 participants