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
thermald: 2.2 -> 2.3 #97469
thermald: 2.2 -> 2.3 #97469
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Formatting fixes (try
nix-shell -p nixpkgs-fmt --run "nixpkgs-fmt /path/to/file.nix"
) - Add meta.changelog attribute
- Change license to gpl2Only or other appropriate: https://discourse.nixos.org/t/lib-licenses-gpl3-co-are-now-deprecated/8206
- Commits LGTM
- Builds via
nix-review
.
https://github.com/NixOS/nixpkgs/pull/97469
1 package built:
thermald
e0d06cc
to
279e1c8
Compare
I ran:
I don't know why no reformatting was applied. Fixed up the sorting and all other issues manually. |
Strange. Honestly, it's been a while since I've used |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Diff LGTM
- Commits LGTM
- Builds via
nix-review
:
https://github.com/NixOS/nixpkgs/pull/97469
1 package built:
thermald
According to the changelog version 2.3 requires Linux 5.8 or later, so we shouldn't merge it yet IMHO as we use 5.4 as default. Alternatively we could expose this as |
I think this is a mistake in the README file, thermald will work fine for older kernels, however the adaptive DPTF policy will not be supported:
While investigating this, I realized that the adaptive DPTF mode is not enabled by default and requires the new |
What's the reason to update the module in another PR? |
279e1c8
to
57d7430
Compare
Right, I've pushed the module change into this PR as well. |
57d7430
to
73346ca
Compare
@Ma27 Is there something missing to push this forward? |
export PKG_CONFIG_PATH="${dbus.dev}/lib/pkgconfig:$PKG_CONFIG_PATH" | ||
./autogen.sh | ||
''; | ||
preConfigure = "./autogen.sh"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will make configure run twice:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fixed by using preConfigure = "NO_CONFIGURE=1 ./autogen.sh";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't that what we have autoreconfHook
for?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
autoreconfHook
just runs autoreconf -vfi
, which might not be sufficient in some projects. (Does it run gtkdocize
like autogen.sh
does? Is that necessary?)
73346ca
to
705e9f1
Compare
Also cleanup the derivation: - remove unnecessary PKG_CONFIG_PATH export - change gpl2 to gpl2Only - add meta.changelog - reformat inputs and sort alphabetically last three suggested by @drewrisinger.
thermald >=2.3 supports the adaptive DPTF mode, in conjunction with kernel 5.8.
705e9f1
to
0aaa5ad
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
Thanks for the reviews, I learned a lot and collected bookmarks for guides to adhere to for future PRs 👍 |
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: |
Motivation for this change
Supports DPTF, which works in conjunction with the patches in kernel 5.8.
Also cleanup the derivation.
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)