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

systemd: support systemd.tmpfiles.settings #148

Merged
merged 3 commits into from
Nov 8, 2024

Conversation

phanirithvij
Copy link
Contributor

@phanirithvij phanirithvij commented Oct 31, 2024

copied verbatim from nixpkgs

since this is writing to a single file, I am adding comments for each settings section to distinguish it. edit: it is now writing to different files, one per settings option.

cc: @r-vdp

#146

@r-vdp
Copy link
Member

r-vdp commented Oct 31, 2024

Thanks!

Did you copy this code over from nixpkgs or you wrote it from scratch?
In the long run, it would be nice if we could factor out this code and reuse it between here and nixpkgs. But that's a longterm goal.

I'm also wondering if we should actually generate multiple files, like we do in nixpkgs, which gives more control to users, and it makes sure that modules that rely on this, are portable. You think that that would be feasible?

@r-vdp
Copy link
Member

r-vdp commented Oct 31, 2024

A test would also be nice.

@phanirithvij phanirithvij marked this pull request as draft November 1, 2024 03:00
@phanirithvij
Copy link
Contributor Author

phanirithvij commented Nov 1, 2024

Yes I copied it from nixpkgs, as I don't know how to reuse it.

I'll see if I can get it working with multiple files same as in nixpkgs. And test it as well.

@phanirithvij phanirithvij force-pushed the tmpfiles-settings branch 3 times, most recently from 9edc70b to 126da17 Compare November 1, 2024 09:16
@phanirithvij
Copy link
Contributor Author

Let me preface this by saying I don't have much experience with rust.

In tmp_files.rs activate function

    let mut cmd = process::Command::new("systemd-tmpfiles");
    cmd.arg("--create")
        .arg("--remove")
        .arg("/etc/tmpfiles.d/00-system-manager.conf");

Here, I need to change it to something like systemd-tmpfiles --create --remove /etc/tmpfiles.d/*.conf
But the existing ubuntu (host os) already has some services and tmpfiles configured, so it affects the existing tmpfiles.

  • So I read from etc_tree and got the tmpfiles.d entry and looped over the files inside it.

I heard unwrap should be avoided, because it panics at runtime.
In this case since /etc/tmpfiles.d/00-system-manager.conf is guaranteed to exist, meaning /etc/tmpfiles.d for sure will be in etc_tree, and thus we shouldn't panic at all?

exit 1
)
done
'';
Copy link
Contributor Author

@phanirithvij phanirithvij Nov 1, 2024

Choose a reason for hiding this comment

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

Here it differs from nixpkgs version

orig nixpkgs version https://github.com/NixOS/nixpkgs/blob/01ddc67fd61124e3c9df1438e6874731af6d3f06/nixos/modules/system/boot/systemd/tmpfiles.nix#L240-L243

        '' + concatMapStrings (name: optionalString (hasPrefix "tmpfiles.d/" name) ''
          rm -f $out/${removePrefix "tmpfiles.d/" name}
        '') config.system.build.etc.passthru.targets;
      }) + "/*";
  • there is no support for globs in system-manager's environment.etc.source so + "/*"; is out.
  • I don't know what config.system.build.etc.passthru.targets is tbh, can't find it in system-manager codebase

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  etc' = lib.filter (f: f.enable) (lib.attrValues config.environment.etc);

  etc = pkgs.runCommandLocal "etc" {
    # This is needed for the systemd module
    passthru.targets = map (x: x.target) etc';
  } /* sh */ ''

from nixos/modules/system/etc/etc.nix

I don't know if it is needed for system-manager?

@phanirithvij phanirithvij marked this pull request as ready for review November 1, 2024 12:49
Copy link
Member

@r-vdp r-vdp left a comment

Choose a reason for hiding this comment

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

One small change to avoid having to call unwrap, besides that, this looks good to me!

It's really unfortunate that we need to copy all this code from nixpkgs, but I don't think we have a real alternative currently. If at some point this project gets more traction, we could have a discussion in the community to see if we can have some sort of re-usable modules that we can share to lower the maintenance burden.

src/activate/tmp_files.rs Outdated Show resolved Hide resolved
@r-vdp r-vdp linked an issue Nov 8, 2024 that may be closed by this pull request
@phanirithvij phanirithvij requested a review from r-vdp November 8, 2024 13:07
@r-vdp r-vdp merged commit c80b30c into numtide:main Nov 8, 2024
20 checks passed
@phanirithvij phanirithvij deleted the tmpfiles-settings branch November 9, 2024 06:09
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.

support systemd.tmpfiles.settings
2 participants