nixos: systemd: split off helper functions into systemd-lib#164317
nixos: systemd: split off helper functions into systemd-lib#164317roberth merged 1 commit intoNixOS:masterfrom
Conversation
github-actions
bot
added
6.topic: nixos
ofborg
bot
added
10.rebuild-darwin: 0
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
Some ideas.
Systemd module code could be made less odd by making better use of the module system; techniques such as imports in submodules, and internal options that perform "computation" like generating unit files and scripts.
| ''} | ||
| ''; # */ | ||
|
|
||
| makeJobScript = name: text: |
There was a problem hiding this comment.
This function can eventually be removed when replaced by #154154
There was a problem hiding this comment.
It's only used in one module, so you could put it in a let binding there. (See the other comment about extracting to files)
There was a problem hiding this comment.
I'd like to reuse these functions (or something better that can be reused). Since I'd like to move forward with systemd in initramfs I wanted to make the minimal amount of changes while still allowing me to reuse functionality.
There was a problem hiding this comment.
I looked at #154154. It looks nice, but it hasn't had activity since januari 7th. I can't wait for that one to be worked on before I can do this PR, which is needed to work on an alternative to #120015 that will be easier to review/merge. #120015 is also stalled, probably because of hard to review/merge and maybe lack of time/motivation.
| }); | ||
| in "${out}/bin/${scriptName}"; | ||
|
|
||
| unitConfig = { config, options, ... }: { |
There was a problem hiding this comment.
This is a module. If you put it in a file, it will be more recognizable in error messages.
The same applies to other modules in this file.
You could consider moving the correspond mkOption as well, but only if it's always used with this config-only module.
There was a problem hiding this comment.
I fully agree with your comments. However, this PR was made to have make it easier to merge. It is the minimal amount of changes that I need to reuse systemd options for systemd in initramfs.
This PR was split off from #164016. It was suggested to me to keep the PRs as small as possible to avoid long review a long review process and avoid it going out-of-date quickly. I think that is indeed the right way to go, so I'm going to keep creating smallish PRs to move towards the goal of having a cleaner systemd module as well as reusing parts in systemd.user and initramfs.
For your interest, I have also tried to create individual 'submodules' for each part of the common systemd options. units, services, paths, mounts, etc. That work is already done, but I'm going to wait to create a PR for that because I need as least the changes in this PR to accomplish it.
See bobvanderlinden@e5ad5b3. I'm also very interested in feedback on this one... though I do know it will be a long while before that can even be considered for reviewing.
There was a problem hiding this comment.
See bobvanderlinden@e5ad5b3. I'm also very interested in feedback on this one...
I'd try to avoid functions to modules; instead turning the parameters into internal options.
Instead of ${unitType} =, I'd try to make a generic common module and import it in each "normal" unit type submodule.
I guess this isn't as commonplace as it should be because of #156533
There was a problem hiding this comment.
The internal options would be ideal. I started out with systemd = mkOption { type = submoduleWith { modules = [ units ... ]; } }, but that resulted in problems. Iteratively I came to the solution in the branch. Can you explain how #156533 can make the branch better?
roberth
left a comment
roberth
left a comment
There was a problem hiding this comment.
This is just moving internal things to a different file.
It's not a cleanup but serves a purpose as mentioned.
Incremental changes are good.
✔️ nixosTests.systemd still evaluates to the same drv.
|
Awesome! Thanks a lot 😄 👍 |
2 participants
Description of changes
This PR was split off from #164016. It moves the (reusable) functions in systemd.nix to systemd-lib.nix. This seemed like the most useful change from #164016.
After this PR, these functions could be reused in other systemd-related modules. Related to #120015.
This is a refactoring, no changes to the outputs should have occurred. I verified this using:
Things done
sandbox = trueset innix.conf? (See Nix manual)nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)nixos/doc/manual/md-to-db.shto update generated release notes