Skip to content

Conversation

@happysalada
Copy link
Contributor

Description of changes

adding systemd files in preparation for making a service

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@happysalada happysalada requested a review from NobbZ February 25, 2024 12:47
Copy link
Contributor

@NobbZ NobbZ left a comment

Choose a reason for hiding this comment

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

Don't you think we need to patch them?

They have a hardcoded /usr/bin/rustic in the ExecStart.

Copy link
Member

Choose a reason for hiding this comment

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

Shells are lucky to end in sh!

@ofborg ofborg bot requested a review from NobbZ February 25, 2024 13:11
@ofborg ofborg bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels Feb 25, 2024
@delroth delroth added the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 25, 2024
Copy link
Contributor

@NobbZ NobbZ left a comment

Choose a reason for hiding this comment

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

Please see #291353 (review).

I hit the wrong button, only realized as ofBorg re-requested a review and therefore got this in my inbox again.

@happysalada
Copy link
Contributor Author

@NobbZ thank you for the comment.

I myself didnt know this until recently, but nixpkgs can work with any systemd files, the problematic settings are jusy overriden in the module. For the execstart there is a bit of a weird syntax to override it, but thats how its done in other places.
ExecStart = [ "" "something else" ] is an example of a systemd override

@NobbZ
Copy link
Contributor

NobbZ commented Feb 25, 2024

ExecStart = [ "" "something else" ] is an example of a systemd override

I'm aware of these, and even can kind of explain to myself how this works. Still, I'd prefer if we had some unit files that would work out of the box in systemd.packages without the need to overriding them in the first place.

If it doesn't work that way, we can just write the systemd.services.* etc from scratch using nix.

@happysalada
Copy link
Contributor Author

@NobbZ The benefit of trying to use uptream is that you get upstream knowledge about how things should work.
For example the setting Nice = 19. I'm sure that upstream tested this and in their experience it's the best to set.
We could just copy what they do in nix, but we then loose the benefit of any updates that they do later on to the service.

Personally I just want a service, I was just planning on copying the restic service and testing it works. Since you are the maintainer, if you prefer to close this PR and just have a copy as is of restic (tested of course). That works for me too.

let me know what you like best, happy to go both ways.

@NobbZ
Copy link
Contributor

NobbZ commented Feb 25, 2024

As said, my prefered way was to path the ExecStart, either providing appropriate patch files or using substituteInPlace.

I am fine though with merging as is, and iterating over it.

@happysalada happysalada force-pushed the rustic_add_systemd_files branch from 0918e76 to 9f8e262 Compare February 25, 2024 19:33
@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 Feb 25, 2024
@delroth delroth removed the 12.approvals: 1 This PR was reviewed and approved by one person. label Feb 25, 2024
@ofborg ofborg bot requested a review from NobbZ February 25, 2024 20:19
@ofborg ofborg bot removed the 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. label Feb 25, 2024
@happysalada
Copy link
Contributor Author

Alright I've pushed a copy of the restic module and will test tonight.
In the meanwhile Let me know if you have comments.

@NobbZ I think I understand your point. If we do it with a patch though, we might have to recreate the patch when the systemd file gets updated. I'm not sure how do it with substituteInPlace, were you thinking of just removing that line from the original file ?
What you don't like from the [ "" approach is that it's a little obscure what is happening ?

@happysalada happysalada force-pushed the rustic_add_systemd_files branch from 9f8e262 to 30dd39c Compare March 2, 2024 18:29
@happysalada happysalada force-pushed the rustic_add_systemd_files branch from 30dd39c to de75b33 Compare March 2, 2024 18:54
@NobbZ
Copy link
Contributor

NobbZ commented Mar 3, 2024

What you don't like from the [ "" approach is that it's a little obscure what is happening ?

I do not like that users need to understand

  1. what the issue with the provided units is
  2. then have to understand how systemd works and allows to override the ExecStart that way

I really like if a drvs output is compatible to systemd.packages though then it really needs to work out of the box, and should not just error.

The substituteInPlace could look like this (not remembering exact calling convention and writing this from memory on mobile, therefore paths will be shortened).

substitituteInPlace rustic.service --replace ExecStart=/usr/bin/rustic ExecStart=$out/bin/rustic

I will also start the review in a minute.

@happysalada
Copy link
Contributor Author

@NobbZ no need to start a review yet. Im still changing the module. Ill mark the Pr as draft until im done.

Rustic seems to use more of a config.toml style, so im trying to use that more.

Copy link
Contributor

@NobbZ NobbZ left a comment

Choose a reason for hiding this comment

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

Looks good to me on a first glance, though I am missing support for profiles, which might be needed to get OpenDAL repos configured.

I'm still in favor of getting something "working" in quickly and iterate as we discover additional cases.

@delroth delroth added the 12.approvals: 2 This PR was reviewed and approved by two persons. label Mar 3, 2024
@happysalada happysalada marked this pull request as draft March 9, 2024 15:22
@h7x4 h7x4 added the 8.has: module (new) This PR adds a module in `nixos/` label Jun 1, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 10, 2024
@liberodark
Copy link
Contributor

Any news about this PR ?

@happysalada
Copy link
Contributor Author

Adding the rustic service will need quite a bit of time that i dont have. If you want to take over, yiu can take anything you want.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
@Ekleog
Copy link
Member

Ekleog commented Jan 11, 2025

If anyone is interested, please have a look at #372935 ; I just made a nixos module for it, that includes additional features like automated postgresql dumps to rustic and exporting JSON results as prometheus metrics :)

This being said I only tried it out with the REST server backend, so it's very possible that I'm missing some configuration parameters for OpenDAL and/or rclone. Please let me know if there's anything in the API that would be impossible to support with these; and if not we can hopefully land and incrementally improve on it!

@nixpkgs-ci nixpkgs-ci bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 25, 2025
@nixpkgs-ci nixpkgs-ci bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (new) This PR adds a module in `nixos/` 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants