-
-
Notifications
You must be signed in to change notification settings - Fork 13.8k
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: systemd: split off helper functions into systemd-lib #164317
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -235,4 +236,205 @@ in rec { | |||
''} | |||
''; # */ | |||
|
|||
makeJobScript = name: text: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This function can eventually be removed when replaced by #154154
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 😄 👍 |
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 = true
set 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.sh
to update generated release notes