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

nixos/klipper: preserve SAVE_CONFIG for nixos-managed config #370704

Merged
merged 2 commits into from
Jan 12, 2025

Conversation

kira-bruneau
Copy link
Contributor

@kira-bruneau kira-bruneau commented Jan 3, 2025

Klipper macros that use SAVE_CONFIG (eg. bed mesh calibration, PID tuning, ...) don't currently work with a nixos-managed config.

This can be worked around by using services.klipper.mutableConfig = true, but then you lose out on being able to configure klipper from NixOS.

This changes the default behaviour so that:

  1. The config is stored in services.klipper.configDir instead of /etc
  2. The config is copied instead of symlinked (keeping a timestamped backup of the existing config)
  3. The SAVE_CONFIG section from the backup is copied over into the new config
  4. The backup is deleted if the final config is identical

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.05 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.

@kira-bruneau kira-bruneau requested a review from cab404 January 3, 2025 19:42
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 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: 1-10 labels Jan 3, 2025
@kira-bruneau kira-bruneau force-pushed the klipper-config branch 3 times, most recently from d4e45fb to 88ebe25 Compare January 3, 2025 20:22
@cab404
Copy link
Member

cab404 commented Jan 5, 2025

I do support the «deprecate immutable config» part. However, I do want to retain the ability to change the rest of the config without changing nixos configuration.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jan 6, 2025

Oh yep, you should still be able to do that. If you want to quickly test a change without committing to it in your NixOS config, you can just add something to the SAVE_CONFIG section (although the comment there does point out you probably shouldn't modify that section, because klipper could stamp over anything you change there).

Also, if you wanted to permanently persist something outside of your Nix config, you can add an include section in settings that points to a path that isn't manged by NixOS.

@cab404
Copy link
Member

cab404 commented Jan 8, 2025

Also, if you wanted to permanently persist something outside of your Nix config, you can add an include section in settings that points to a path that isn't manged by NixOS.

But I do want to have a default config!

Maybe this should just replace immutable config instead, as it basically retains the same semantics, yet fixing the SAVE_CONFIG problem?

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jan 8, 2025

But I do want to have a default config!

I'm not really sure what you mean by that, I think there might be a misunderstanding? With this PR, what I mentioned would still allow for a default config.

Maybe this should just replace immutable config instead, as it basically retains the same semantics, yet fixing the SAVE_CONFIG problem?

I think this PR already does this? (but I feel unclear on what you meant earlier, so I might be missing something)

You got me thinking though, maybe it would be simpler to patch klipper to automatically include /etc/printer.cfg as a default config? That way we wouldn't have to do this weird workaround to merge an immutable config into a mutable one.

@cab404
Copy link
Member

cab404 commented Jan 8, 2025

But I do want to have a default config!

I'm not really sure what you mean by that, I think there might be a misunderstanding? With this PR, what I mentioned would still allow for a default config.

What I mean is that I want default config while retaining the option to edit it in arbitrary locations. E.g if I am building an image for a generic printer platform, and I want to provide a default for it, but not to restrict end user in any way.

@kira-bruneau
Copy link
Contributor Author

kira-bruneau commented Jan 8, 2025

Ohhh ok thanks! That makes a lot more sense!

Basically what I wanted was to be able to manage all the settings from NixOS, with the exception of SAVE_CONFIG. I'd only want to modify printer.cfg manually to quickly test something, and then I would move it over to my NixOS config when I was ok with the change.

It's sounds like you designed mutableConfig to basically keep NixOS from touching the config, except for initialization. I think it was wrong for me to put the behaviour I wanted into the mutableConfig option then.

How would you feel about a new managedConfig option that replaces "immutable" configs? Then mutableConfig could be deprecated in favour of setting managedConfig = false.

@cab404
Copy link
Member

cab404 commented Jan 8, 2025

I really do feel that your changes should be enabled by default, as they are strictly better than the previous default — so maybe just use them if mutableConfig = false?

@kira-bruneau
Copy link
Contributor Author

Oh yeah that would be ideal, but the reason I thought of the new option was because the change would be breaking. So we'd need some kind of transitory option between old state versions and new ones.

@cab404
Copy link
Member

cab404 commented Jan 9, 2025

Oh yeah that would be ideal, but the reason I thought of the new option was because the change would be breaking. So we'd need some kind of transitory option between old state versions and new ones.

Hmm, I don't really see how it would be breaking

@kira-bruneau
Copy link
Contributor Author

Oh wait sorry, your right. I'm not sure why I thought it would be earlier 😅

Klipper macros that use `SAVE_CONFIG` (eg. bed mesh calibration, PID
tuning, ...) don't currently work with a nixos-managed config.

This can be worked around by using `services.klipper.mutableConfig =
true`, but then you lose out on being able to configure klipper from
NixOS.

This changes the default behaviour so that:

1. The config is stored in `services.klipper.configDir` instead of `/etc`
2. The config is copied instead of symlinked (keeping a timestamped backup of the existing config)
3. The `SAVE_CONFIG` section from the backup is copied over into the new config
4. The backup is deleted if the final config is identical
@github-actions github-actions bot removed 6.topic: python 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: haskell 8.has: documentation This PR adds or changes documentation 8.has: changelog 6.topic: emacs Text editor 6.topic: printing 6.topic: rust 6.topic: policy discussion 6.topic: vim 6.topic: hardware 6.topic: TeX Issues regarding texlive and TeX in general 6.topic: testing Tooling for automated testing of packages and modules 6.topic: module system About "NixOS" module system internals 6.topic: jitsi 6.topic: java Including JDK, tooling, other languages, other VMs 6.topic: flakes The experimental Nix feature 6.topic: lib The Nixpkgs function library 6.topic: jupyter Interactive computing tooling: kernels, notebook, jupyterlab 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` 6.topic: deepin Desktop environment and its components 6.topic: dotnet Language: .NET 6.topic: nvidia 6.topic: continuous integration Affects continuous integration (CI) in Nixpkgs, including Ofborg and GitHub Actions labels Jan 11, 2025
@NixOS NixOS deleted a comment from nix-owners bot Jan 11, 2025
@kira-bruneau kira-bruneau changed the title nixos/klipper: always update mutable config & deprecate immutable config nixos/klipper: preserve SAVE_CONFIG for nixos-managed config Jan 11, 2025
@kira-bruneau
Copy link
Contributor Author

I really do feel that your changes should be enabled by default, as they are strictly better than the previous default — so maybe just use them if mutableConfig = false?

Ok, it should be good now!

Copy link
Member

@cab404 cab404 left a comment

Choose a reason for hiding this comment

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

LGTM!

Got stuck for a second thinking of removed chmod, but I guess that should work with piping instead of cp <3

@cab404 cab404 merged commit d5493eb into NixOS:master Jan 12, 2025
39 checks passed
@cab404
Copy link
Member

cab404 commented Jan 12, 2025

@kira-bruneau Thank you so much for your PR <3

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: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants