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

tlpui: init at 1.6.5 #305278

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

LordGrimmauld
Copy link
Contributor

@LordGrimmauld LordGrimmauld commented Apr 19, 2024

Description of changes

  • link the empty default config of tlp into the legacy config path of tlp to make it work even without explicit config in /etc/tlp.conf
  • move nixos tlp module to use /etc/tlp.d/ as to not break the tlp config created imperatively with tlpui in /etc/tlp.conf
  • init tlpui at 1.6.5

This PR reuses work by @GeorgesAlkhouri in #188278, but with an updated nix package to use pyproj and an updated patchset to use the tlp default config in the tlp store path if the explicit tlp config in /etc/tlp.conf does not yet exist.

I tested this PR on my local system and it works well as far as i can tell.

I understand this is a relatively big scale for tlp as a whole, not just a simple package addition, feedback is welcome.

Edit: i adjusted this pull request. It no longer needs to modify the tlp nixos module.
Instead, the tlp nixos module is now completely untouched and still applies its config in the global config location of /etc/tlp.conf. tlpui places its config files in /etc/tlp.d/30-tlpui.conf instead.
This approach has a couple interesting quirks but is an overall improvement:

  • currently, it outright crashes if /etc/tlp.d does not yet exist as a directory.
  • the changes made in tlpui do persist even after uninstalling tlpui
  • nix settings for tlp takes precedence over tlpui, which means at least the end result can still be fully determined by nix even with left-over config files.
    Ideally, I'd write a nixos module for tlpui instead, with tlpui writing to a special location and the nixos module symlinking the config result into place ensuring both tlp.d exists and is being properly cleaned up on uninstall.

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.

@LordGrimmauld
Copy link
Contributor Author

TODO: I do need to do more testing in terms of how tlpui reads default values. At a quick glance everything seemed fine, but i can't quite tell. I need to especially crossreference with a tlpui install on a known-good distro.

@LordGrimmauld
Copy link
Contributor Author

Update:
I ran this ever since implementing without issues. It is not possible to reset to "clean" defaults however. Config fields that were at any point defined in tlpui will always be defined in tlpui. Further, uninstalling tlpui does not remove the config fields defined by it. This might in itself be an issue in regards to nix inherent concept to only have things affect the system that are also listed in the config.
Likely the cleaner option would be to have nix still define the config in /etc/tlp.conf and somehow figure out how to coerce tlpui to place its configs in /etc/tlp.d. This would still mean uninstalling tlpui would have the tlpui config file remain on the system, but the nix defined config would take priority.

If anyone familiar enough with tlpui to know how we can set it to use a config in /etc/tlp.d/* ? One option would be to patch the path lookup in https://github.com/d4nj1/TLPUI/blob/master/tlpui/settingshelper.py#L23 but that might not be the best of ideas

@Aleksanaa
Copy link
Member

This might in itself be an issue in regards to nix inherent concept to only have things affect the system that are also listed in the config.

Yes, it's intended. Sorry I cannot test it for now.

@LordGrimmauld
Copy link
Contributor Author

All good, take your time. These are chaotic and turbulent times and this is not a pressing issue. I can run this PR just fine until we have a better method and a more stable situation overall....

@LordGrimmauld
Copy link
Contributor Author

Alright, i adjusted this pull request. It no longer needs to modify the tlp nixos module.
Instead, the tlp nixos module is now completely untouched and still applies its config in the global config location of /etc/tlp.conf. tlpui places its config files in /etc/tlp.d/30-tlpui.conf instead.
This approach has a couple interesting quirks but is an overall improvement:

  • currently, it outright crashes if /etc/tlp.d does not yet exist as a directory.
  • the changes made in tlpui do persist even after uninstalling tlpui
  • nix settings for tlp takes precedence over tlpui, which means at least the end result can still be fully determined by nix even with left-over config files.
    Ideally, I'd write a nixos module for tlpui instead, with tlpui writing to a special location and the nixos module symlinking the config result into place ensuring both tlp.d exists and is being properly cleaned up on uninstall.

Copy link
Member

@Aleksanaa Aleksanaa left a comment

Choose a reason for hiding this comment

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

Also should avoid double wrapping (See #303848)

substituteAll,
tlp,
usbutils,
wrapGAppsHook,
Copy link
Member

Choose a reason for hiding this comment

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

It has been renamed to wrapGAppsHook3 to avoid confusion.

@Aleksanaa
Copy link
Member

currently, it outright crashes if /etc/tlp.d does not yet exist as a directory.

This is logically correct, but it is better to mention it in longDescription.

the changes made in tlpui do persist even after uninstalling tlpui

This is also acceptable.

By the way, I'd recommend always adding new module after package, in two separate PRs, to improve the efficiency of reviewing.

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.

4 participants