Skip to content

Conversation

@NiklasGollenstede
Copy link

Description of changes

This lets the flake.nix export the modules as paths instead of importing them first. This should work just as well for anyone doing imports = [ inputs.nixos-hardware.nixosModules.foo ], but the key difference is that the module system knows the names of the modules imported that way.
This is important for several reasons. At least it let's the module system:

  • report error locations properly (instead of <importing module>:anon-X)
  • deduplicate multiple imports of the same module
  • make disabledModules work.
    I ran into each of those problems at some point or another. There are workarounds like specifying the file path manually instead of using the indirection of the table exported via the flake.nix, ... but that then also bypasses the abstraction that is supposed to be provided there.
Implementation

One could of course remove the 280+ imports in flake.nix, but that is quite a lot of line changes, and the git-blame on this file quite nicely answers the "when was that hardware added?" question (I know that commits can be excluded from the line history, but that is not exactly elegant either).
So instead this simply shadows the import function with the id function.

An alternative implementation is this:

import = path: { _file = "${path}"; key = "${path}"; imports = [ (builtins.import path) ]; };

But that is more verbose and don't see how it is better.

Any implementation is going to break dependents who do some sort of manual transformation of the exported modules (which I have also done in the past), but that is not part of the defined outputs API anyway.
Medium to long term I am pretty sure this will avoid weird bugs and breakage.

Things done
  • Tested the changes in your own NixOS Configuration
  • Tested the changes end-to-end by using your fork of nixos-hardware and
    importing it via <nixos-hardware> or Flake input

@superherointj
Copy link
Contributor

Rebase to master to fix CI.

@superherointj
Copy link
Contributor

You need to squash commits...

@NiklasGollenstede
Copy link
Author

That was not an enjoyable process. I guess I should not have used GitHub's "sync Branch" button to start with.
Anyway. It's now again based on the current HEAD of master.

@superherointj
Copy link
Contributor

superherointj commented Jul 19, 2024

That was not an enjoyable process. I guess I should not have used GitHub's "sync Branch" button to start with. Anyway. It's now again based on the current HEAD of master.

Learning is hard. But you do it once.

I have this alias for git rebase last 2 commits:

alias gr2='git rebase -i HEAD~2'

You learned something new. Great!

@NiklasGollenstede
Copy link
Author

I did try exactly that. Merge master into my branch (online, then local pull), and then git rebase -i HEAD~2.
The output of my git history alias led me to believe that that did not do what I wanted (all commits on master since by branch-off to be spliced in at the branch-off point).
What I then did (with that exact result) was discard my merge commit, update my master from upstream, and git rebase master.

Anyway, thanks for your suggestion!

@superherointj
Copy link
Contributor

I did try exactly that. Merge master into my branch (online, then local pull), and then git rebase -i HEAD~2. The output of my git history alias led me to believe that that did not do what I wanted (all commits on master since by branch-off to be spliced in at the branch-off point). What I then did (with that exact result) was discard my merge commit, update my master from upstream, and git rebase master.

Anyway, thanks for your suggestion!

You are right. git rebase master would have been best. So, now I refreshed my git knowledge. We are always learning. :-)

@Mic92
Copy link
Member

Mic92 commented Jul 19, 2024

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jul 19, 2024

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 3501b9c

@mergify mergify bot merged commit 3501b9c into NixOS:master Jul 19, 2024
@mexisme
Copy link
Contributor

mexisme commented Jul 21, 2024

I think this PR has broken my importing of nixos-hardware.nixosModules.asus-zephyrus-ga402x.amdgpu, which was working beforehand, but now gives the error:

error: value is a path while a set was expected

This really confused me until (after some digging) I realised that nixos-hardware.nixosModules.asus-zephyrus-ga402x now pointed to a separate source path, rather than a direct import of the default.nix at //asus/zephyrus/ga402x/default.nix, which does export a amdgpu and nvidia attr.

This made it easy to switch between AMD only and Nvidia boot specialisations, because disabling Nvidia completely saved some battery.

EDIT:
It's been a while, but I also seem to remember choosing to do it this way as (at the time) the nixos-hardware module for NVidia enablement is a bit inflexible and assumes you want it all enabled.

If this style is going to be the way forward, I think I'll have to add some new options to allow importing various GPU enablement modules, while also switching their behaviour(s) off?

@mexisme
Copy link
Contributor

mexisme commented Jul 21, 2024

I think this PR has broken my importing of nixos-hardware.nixosModules.asus-zephyrus-ga402x.amdgpu, which was working beforehand, but now gives the error:

error: value is a path while a set was expected

This really confused me until (after some digging) I realised that nixos-hardware.nixosModules.asus-zephyrus-ga402x now pointed to a separate source path, rather than a direct import of the default.nix at //asus/zephyrus/ga402x/default.nix, which does export a amdgpu and nvidia attr.

This made it easy to switch between AMD only and Nvidia boot specialisations, because disabling Nvidia completely saved some battery.

I've opened an issue to track this: #1052

@NiklasGollenstede
Copy link
Author

NiklasGollenstede commented Jul 21, 2024

Yeap. Sorry. In addition to

is going to break dependents who do some sort of manual transformation of the exported modules

this also breaks cases where the export is/was not complying to the outputs.nixosModules API (see #1053 (comment)).


error: value is a path while a set was expected

BTW: Newer versions of nix would have told you which path it is you got instead of the expected set, which would probably have been useful.

mexisme added a commit to mexisme/nixos-hardware that referenced this pull request Jul 23, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR NixOS#1046
- Add deprecation assertion for lenovo-yoga-7-14ARH7
mexisme added a commit to mexisme/nixos-hardware that referenced this pull request Jul 24, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR NixOS#1046
- Add deprecation assertion for asus-zephyrus-ga402x
mexisme added a commit to mexisme/nixos-hardware that referenced this pull request Jul 24, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR NixOS#1046
- Add deprecation assertion for lenovo-yoga-7-14ARH7
mexisme added a commit to mexisme/nixos-hardware that referenced this pull request Jul 24, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR NixOS#1046
- Add deprecation assertion for lenovo-yoga-7-14ARH7
mergify bot pushed a commit that referenced this pull request Jul 25, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR #1046
- Add deprecation assertion for lenovo-yoga-7-14ARH7
mexisme added a commit to mexisme/nixos-hardware that referenced this pull request Jul 25, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR NixOS#1046
- Add deprecation assertion for asus-zephyrus-ga402x
mergify bot pushed a commit that referenced this pull request Jul 25, 2024
…tries

- Previous attr-set style providing "amdgpu" and "nvidia" is broken by PR #1046
- Add deprecation assertion for asus-zephyrus-ga402x
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants