nixos/acpid: refactor and add initrd support #277898
Conversation
|
Not sure why docs are failing. Building locally with the same command CI runs gives this I did notice that the options listing is missing the |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3239 |
| stage1Cfg = config.boot.initrd.services.acpid; | ||
| stage2Cfg = config.services.acpid; |
There was a problem hiding this comment.
This might be confusing as initrd has stage 1 and 2, too.
There was a problem hiding this comment.
so would initrdCfg / mainCfg work? I'm not sure what to name the one under config.services.
| '' + lib.optionalString (!stage2) '' | ||
|
|
||
| ::: {.note} | ||
| Many events will require kernel modules to be available via `boot.initrd.availableKernelModules`. |
There was a problem hiding this comment.
Are there any other useful information we can give or link how to know which ones are required?
There was a problem hiding this comment.
This is what I found https://wiki.archlinux.org/title/ACPI_modules
| # Acpid requires /var/run to exist but dosen't create it, and no systemd service seems to create it either. | ||
| # Note this is only a problem for stage1. |
There was a problem hiding this comment.
We should rather patch this
There was a problem hiding this comment.
Patch what here? Making acpid create or link /var/run seems out of scope imo.
| ln -s /run /var/run || true | ||
|
|
There was a problem hiding this comment.
| ln -s /run /var/run || true |
| ${cfg.package}/bin/acpid \ | ||
| --foreground \ | ||
| --netlink \ | ||
| --confdir "${acpiConfFor cfg}/handlers" \ |
There was a problem hiding this comment.
Isn't the trailing \ causing problems when logEvents is not appended?
There was a problem hiding this comment.
No, it behaves similarly to running it interactively
> echo hi \
> \
> \
> \
>
hi
>| boot.initrd.services.acpid = options false; | ||
| }; | ||
|
|
||
| config = mkMerge [ |
There was a problem hiding this comment.
We shoudl avoid mkMerge on config. For me that often caused hard to debug infinite recusions but I am not sure if that was only on me.
There was a problem hiding this comment.
I've never experienced this issue and mkMerge assists readability a lot. Would you mind linking relevant issues so I can try to reproduce it or work around it?
|
Not sure if that commit should be squashed or not |
|
it would be so based if this got merged 🥺 |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/3635 |
Description of changes
Adds initrd/stage1 support to the acpid module. Also pretty much a complete refactor.
Things done
Tested stage1 and stage2 support in personal configurations using
powerEventCommandsandhandlers.(self) Reviewed points
mkRemovedOptionModuleAdd a 👍 reaction to pull requests you find important.