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

Jump to bottom

tlp: 1.2.2 -> 1.3.1 #81087

Merged
merged 3 commits into from Feb 27, 2020
Merged

tlp: 1.2.2 -> 1.3.1 #81087

merged 3 commits into from Feb 27, 2020

Conversation

lovesegfault
Copy link
Member

@lovesegfault lovesegfault commented Feb 26, 2020 •
edited

Motivation for this change

I noticed that tlp was out of date and that it's nixos module had some spiderwebs as well. This bumps tlp to the latest 1.3.1 and updates the relevant module.

Fixes #81049

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.

@lovesegfault
Copy link
Member Author

@GrahamcOfBorg build tlp

@ofborg ofborg bot requested a review from abbradar February 26, 2020 04:17
@lovesegfault lovesegfault force-pushed the tlp-1.3.1 branch 3 times, most recently from 7d35335 to 317ba9b Compare February 26, 2020 05:09
@lovesegfault lovesegfault marked this pull request as ready for review February 26, 2020 05:14
@lovesegfault lovesegfault force-pushed the tlp-1.3.1 branch 6 times, most recently from a0aef53 to dd19486 Compare February 26, 2020 06:44
@worldofpeace
Copy link
Contributor

Running this now, it seems pretty hot. but that's the only thing that should be hot cuz it wouldn't be working if my system was on fire.

@worldofpeace
Copy link
Contributor 

diff --git a/nixos/modules/services/hardware/tlp.nix b/nixos/modules/services/hardware/tlp.nix
index 8e47d63edc8..bd8c072cf39 100644
--- a/nixos/modules/services/hardware/tlp.nix
+++ b/nixos/modules/services/hardware/tlp.nix
@@ -65,6 +65,9 @@ in
       # FIXME: Should this only happen when enableRDW = true?
       sockets.systemd-rfkill.enable = false;
       services.systemd-rfkill.enable = false;
+
+      # Have to set manually when using systemd.packages
+      services.tlp.wantedBy = [ "multi-user.target" ];
     };
   };
 }

@lovesegfault lovesegfault force-pushed the tlp-1.3.1 branch 6 times, most recently from 62807ef to 7c6f41e Compare February 26, 2020 17:59
@worldofpeace worldofpeace mentioned this pull request Feb 26, 2020
Open
@lovesegfault
Copy link
Member Author

This still needs a fix for the sleep (suspend/resume) unit which I am working on.

@lovesegfault lovesegfault force-pushed the tlp-1.3.1 branch 3 times, most recently from 442ba2d to 3ef201a Compare February 27, 2020 05:49
@lovesegfault
Copy link
Member Author

Alright, this is now complete and everything works!

worldofpeace
worldofpeace approved these changes Feb 27, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I believe everything is 🔥

@worldofpeace worldofpeace merged commit 76f4f6b into NixOS:master Feb 27, 2020
@lovesegfault lovesegfault mentioned this pull request Feb 27, 2020
Closed
dtzWill pushed a commit to dtzWill/nixpkgs that referenced this pull request Feb 27, 2020
@worldofpeace @dtzWill
Merge pull request NixOS#81087 from lovesegfault/tlp-1.3.1
e3000ef 
tlp: 1.2.2 -> 1.3.1
(cherry picked from commit 76f4f6b)
jtojnar
jtojnar reviewed May 28, 2020
View reviewed changes
pkgs/tools/misc/tlp/default.nix
Comment on lines +41 to +49
# XXX: While [1] states that DESTDIR should not be used, and that the correct
# variable to set is, in fact, PREFIX, tlp thinks otherwise. The Makefile for
# tlp concerns itself only with DESTDIR [2] (possibly incorrectly) and so we set
# that as opposed to PREFIX, despite what [1] says.
#
# [1]: https://github.com/NixOS/nixpkgs/issues/65718
# [2]: https://github.com/linrunner/TLP/blob/ab788abf4936dfb44fbb408afc34af834230a64d/Makefile#L4-L46
makeFlags = [
"DESTDIR=${outRef}"
"TLP_SBIN=${outRef}/bin"
"TLP_BIN=${outRef}/bin"
"TLP_TLIB=${outRef}/share/tlp"
"TLP_FLIB=${outRef}/share/tlp/func.d"
"TLP_ULIB=${outRef}/lib/udev"
"TLP_NMDSP=${outRef}/etc/NetworkManager/dispatcher.d"
"TLP_SHCPL=${outRef}/share/bash-completion/completions"
"TLP_MAN=${outRef}/share/man"
"TLP_META=${outRef}/share/metainfo"
"DESTDIR=${placeholder "out"}"
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the upstream uses DESTDIR correctly (as demonstrated by fix-makefile-sed.patch) and the issue is that it does not support GNU installation directories. Keeping this in mind, the correct solution would be using ${placeholder "out"} in place of ${outRef} and asking upstream to make the path overridability more granular as per GNU guidelines.

@jtojnar jtojnar mentioned this pull request May 28, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar left review comments

@worldofpeace worldofpeace worldofpeace approved these changes

@abbradar abbradar Awaiting requested review from abbradar

Assignees
No one assigned
Labels
6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

tlp: bump to 1.3.1 needed
3 participants
@lovesegfault @worldofpeace @jtojnar