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.

Sign up for GitHub

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

Start decoupling X and graphics support with videoDrivers #153808

Closed
wants to merge 1 commit into from
Closed

Start decoupling X and graphics support with videoDrivers #153808

wants to merge 1 commit into from

Conversation

Ericson2314
Copy link
Member

Motivation for this change

Now that Wayland is less unviable, we should begin to refactor our
options so you don't need to use X11-named options just to do graphics
without X11.

I don't really know what I am doing, so someone please advise.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 22.05 Release Notes (or backporting 21.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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Jan 7, 2022
@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 Jan 7, 2022
@aanderse
Copy link
Member

aanderse commented Jan 7, 2022

Awesome. I'm pretty sure @jtojnar had opinions about this change some time ago... 🤔

@jtojnar
Copy link
Member

jtojnar commented Jan 9, 2022

I like this. It is narrow in scope and the new option name fits well.

@Ericson2314
Copy link
Member Author

Do we know what this CI failure is about? I just eyeballed it so I assume it has issues, but the error looks spurious.

Ericson2314
Ericson2314 commented Jan 9, 2022
View reviewed changes
nixos/modules/hardware/graphics.nix
Comment on lines +70 to +73
config = mkIf cfg.enable {


};
Copy link
Member Author

Choose a reason for hiding this comment

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

Note nothing happens yet, because the boot modules are appended per-card.

@jtojnar
Copy link
Member

jtojnar commented Jan 9, 2022 •
edited
Loading

Do we know what this CI failure is about? I just eyeballed it so I assume it has issues, but the error looks spurious.

The new module probably needs the following:

# uses relatedPackages
meta.buildDocsInSandbox = false;

Edit: There is actually an error message that explains it:

Cacheable portion of option doc build failed.
Usually this means that an option attribute that ends up in documentation (eg default or description) depends on the restricted module arguments config or pkgs.

Rebuild your configuration with --show-trace to find the offending location. Remove the references to restricted arguments (eg by escaping their antiquotations or adding a defaultText) or disable the sandboxed build for the failing module by setting meta.buildDocsInSandbox = false.

@Ericson2314
WIP: Start decoupling X and graphics support with videoDrivers
60820ed 
Now that Wayland is less unviable, we should begin to refactor our
options so you don't need to use X11-named options just to do graphics
without X11.

This will be a long process, and I am not really sure where to start,
but `videoDrivers` seems like a decent option.
@Ericson2314 Ericson2314 changed the title WIP: Start decoupling X and graphics support with videoDrivers Start decoupling X and graphics support with videoDrivers Feb 14, 2022
@Ericson2314 Ericson2314 mentioned this pull request Feb 14, 2022
Closed
colemickens
colemickens reviewed Mar 30, 2022
View reviewed changes
nixos/modules/hardware/video/nvidia.nix
@@ -341,7 +341,7 @@ in

# nvidia-uvm is required by CUDA applications.
boot.kernelModules = [ "nvidia-uvm" ] ++
optionals config.services.xserver.enable [ "nvidia" "nvidia_modeset" "nvidia_drm" ];
optionals config.hardware.graphics.enable [ "nvidia" "nvidia_modeset" "nvidia_drm" ];
Copy link
Member

Choose a reason for hiding this comment

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

Just ran into this myself, I think. This is what I was looking for. 👍

@colemickens
Copy link
Member

Well, I've been waiting for this for years, and from a review it looks like it solves one of the issues I specifically hit because I use Wayland without enabling Xserver. Thanks @Ericson2314 !

@colemickens
Copy link
Member

Of course, this just makes me want #158079 too. It would be nice if they were easy to test together.

@Ericson2314
Copy link
Member Author

Yeah hopefully after the RFC these PRs get combined and both merged. @colemickens how about you nominate yourself for NixOS/rfcs#121 ?

@Ericson2314
Copy link
Member Author

Actually too bad I am nominating you :P

@colemickens
Copy link
Member

I feel like looking at this one before the other bigger PR. Have you considered a more option based approach?

I'd like to be able to do:

# based on hostname
hardware.drivers.radeon.enable = true;
hardware.drivers.amdvlk.enable = true;

# based on different hostname (or includes or however you organize nix config)
hardware.drivers.nvidia.enable = true;
# the nvidia module options would then just be under hardware.drivers.nvidia

But I'm wondering if I'm missing a reasoning behind this string-y API? Is it just historical?

@Ericson2314
Copy link
Member Author

@colemickens I just did the minimal thing for this one. I vaguely recall there was some priority stuff the current list of strings approach also worked for, but I do not. I am not opposed to cleaning things up further.

@colemickens
Copy link
Member

@Ericson2314 If it's agreeable to you and @jonringer, I'd prefer to just merge this one, focus on @jonringer's big cleanup, collect thoughts along the way and then take another pass at it after it's merged.

@Ericson2314
Copy link
Member Author

I am fine with such an incremental plan, but it would good to get @jonringer's thoughts here. I don't know much usual overflowing strong opinions with this :)

@berbiche
Copy link
Member

berbiche commented Apr 7, 2022

Should this be merged after the 22.05 release?

@Ericson2314
Copy link
Member Author

Good question. It is backwards compatibility. Hopefully we can discuss in RFC meeting.

@Ericson2314
Copy link
Member Author

Closed in favor of #158079 which is wholly superior!

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

@berbiche berbiche berbiche left review comments

@colemickens colemickens colemickens left review comments

@vcunat vcunat Awaiting requested review from vcunat

@bobby285271 bobby285271 Awaiting requested review from bobby285271

@TredwellGit TredwellGit Awaiting requested review from TredwellGit

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: 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
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Ericson2314 @aanderse @jtojnar @colemickens @berbiche