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/tlp: force necessary settings #81346

Closed
wants to merge 1 commit into from

Conversation

lovesegfault
Copy link
Member

Motivation for this change

TLP is very sensitive to certain system settings that we take care to
disable/nullify in the module code. This takes the further step to
mkForce those options to ensure no insane behavior if the user attempts
to set them in disagreement with the module.

cc. @worldofpeace

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@worldofpeace
Copy link
Contributor

I should probably add myself to maintainers of both this module and the package 😄

I'm not sure this provides the best UX though, like what happens if someone tries to set it?
Perhaps we could use an assert too.

@lovesegfault
Copy link
Member Author

Before:
If someone attempts to set those it will blow up saying it's in conflict with TLP, they can then mkForce it and explode themselves.

After:
If someone attempts to set them TLP will mkForce over them and no one gets hurt.

And yes, you should definitely add yourself :D

@worldofpeace
Copy link
Contributor

@lovesegfault Right, but even before, it would say it conflicted with TLP but you had no idea why. Just that it conflicts. Now, I guess it'll just be silently ignored?

@lovesegfault
Copy link
Member Author

I wouldn't say ignored, now it will silently override you and do-the-right-thing:tm:

I agree that it's not ideal that it happens silently. Is there a way to add a warning like "Hey, we're preventing you from being silly, here's my soundcloud"?

@worldofpeace
Copy link
Contributor

I wouldn't say ignored, now it will silently override you and do-the-right-thingtm

I agree that it's not ideal that it happens silently. Is there a way to add a warning like "Hey, we're preventing you from being silly, here's my soundcloud"?

I believe assertion or warning https://nixos.org/nixos/manual/index.html#sec-assertions

TLP is very sensitive to certain system settings that we take care to
disable/nullify in the module code. This takes the further step to
mkForce those options to ensure no insane behavior if the user attempts
to set them in disagreement with the module.
@lovesegfault
Copy link
Member Author

Alright, done @worldofpeace, it will now scream at the user!

@@ -37,6 +37,21 @@ in

###### implementation
config = mkIf cfg.enable {
warnings = with builtins.isNull;
if !isNull config.powerManagement.scsiLinkPolicy then
[ "You've set a non-null value for powerManagement.scsiLinkPolicy. The TLP module will override that decision." ]
Copy link
Member

Choose a reason for hiding this comment

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

This warning seems inaccurate to me.
Either the user didn't use mkForce, and the warning does not fire because the config is overriden by the module.
Or the user used something stronger than mkForce, and the "The TLP module will override that decision" part of the sentence is false.

The same applies below.

@symphorien
Copy link
Member

I think in this case it is better to

  • not use mkForce
  • use real assertions, with links to the part of the tlp documentation which explains why this setting is mandatory.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants