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.

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

Move systemd-lib.nix and systemd-unit-options.nix into utils #146815

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

ElvishJerricco
Copy link
Contributor

There are several modules that redundantly import the systemd-lib.nix and systemd-unit-options.nix files. I thought it'd be good if they all got them from the same place.

Tested with:

nix-build nixos/tests/systemd-*.nix nixos/tests/restic.nix

All test results were fetched from cache, so eval is unchanged.

@ElvishJerricco ElvishJerricco requested a review from dasJ as a code owner November 20, 2021 22:56
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Nov 20, 2021
@ElvishJerricco
Copy link
Contributor Author

ElvishJerricco commented Nov 20, 2021

One minor concern I have for this PR was that I struggled for a little while with an infinite recursion issue. Turns out my issue was putting with systemdUtils.lib; after with lib;. If you put it before, then name resolution for e.g. config = mkIf will check lib.mkIf first, and not the unprepared utils expression (the module system is quirky like this).

I was thinking about removing all the uses of with on these attrsets just to discourage people from making the same mistake by looking at this code as an example of using systemdUtils. Would that be worthwhile? The networkd module could be a chore to do this to...

@ElvishJerricco
Copy link
Contributor Author

Note: This is part of my effort to put systemd in initrd; as I'm about to make another module that uses these utils.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux labels Nov 20, 2021
@Artturin
Copy link
Member

If it's not too much trouble I'd recommend using inherit instead of with
https://nix.dev/anti-patterns/language#with-attrset-expression

@ElvishJerricco
Copy link
Contributor Author

@Artturin Yea, there was a discussion about that (regarding this very code) on matrix earlier today. While I generally agree with the sentiment, it's worth noting that e.g. the networkd module would have a really gross let inherit. I suppose I could just prefix all the function calls with foo., but that makes for a frustratingly massive git diff.

@Artturin
Copy link
Member

You could add a comment warning users above the with and use inherit elsewhere

@roberth
Copy link
Member

roberth commented Nov 21, 2021

A lot of this could be refactored into modules. We have some underappreciated possibilities in the module system could be quite helpful for keeping things together.

  • Submodules can import modules.
  • Options with internal = true can be used to perform computation in submodules rather than outside of them.
    For example the derivation that writes a unit can be a module. Similarly, the derivation that writes an entire systemd configuration (many units) can be a module.

This way, you won't need "utils" files as much.

@roberth
Copy link
Member

roberth commented Nov 21, 2021

Here's an example of this kind of refactoring #82751 (comment)

@ElvishJerricco
Copy link
Contributor Author

@roberth I'm not exactly following what you're suggesting. How could the module system be used in this case?

@roberth
Copy link
Member

roberth commented Nov 21, 2021

generateUnits could go into an abstract module that reads config.units and some internal options for the other parameters.

config.systemd and config.systemd.user will have to become submodules for that to work, but I don't know if that's feasible.
Currently though, adding a submodule can be disruptive to other modules if they declare options in that part of the tree, see #146882. Might not be a problem, but I'm not sure.

It's probably best to wait for that issue to be resolved first. It seems that such a refactoring would make some of the rewiring in this PR obsolete, but this PR does not make the that change any harder, so there's no need to block this.

Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

It seems to solve a problem for you.

@@ -1,4 +1,4 @@
pkgs: with pkgs.lib;
{ lib, config, pkgs }: with lib;
Copy link
Member

Choose a reason for hiding this comment

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

It's not great that there's more wiring now, but I think we'll have to wait with systemd refactoring, as mentioned in the earlier comment.
I guess that's just the faith of "utils", to grow all sorts of weird dependencies, because everyone wants to use them for their own distinct purposes. I hope we'll remove it altogether in the future.

@ajs124 ajs124 requested a review from flokli December 7, 2021 12:07
@ajs124 ajs124 merged commit eee45bb into NixOS:master Dec 8, 2021
@ajs124
Copy link
Member

ajs124 commented Dec 27, 2021

lol, turns out this broke eval internally for all our systems on the unstable channel, because we have some cursed code that does systemdLib = import "${modulesPath}/system/boot/systemd-lib.nix" { inherit config lib pkgs; }; @dasJ

Edit: changed it to systemdLib = if utils ? systemdUtils then utils.systemdUtils.lib else import "${modulesPath}/system/boot/systemd-lib.nix" { inherit config lib pkgs; }; for added cursedness.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 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: 0 This PR does not cause any packages to rebuild on Linux
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants