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

thermald: 2.2 -> 2.3 #97469

Merged
merged 2 commits into from Sep 20, 2020
Merged

thermald: 2.2 -> 2.3 #97469

merged 2 commits into from Sep 20, 2020

Conversation

Emantor
Copy link
Member

@Emantor Emantor commented Sep 8, 2020

Motivation for this change

Supports DPTF, which works in conjunction with the patches in kernel 5.8.

Also cleanup the derivation.

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.

Copy link
Contributor

@drewrisinger drewrisinger left a comment

Choose a reason for hiding this comment

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

https://github.com/NixOS/nixpkgs/pull/97469
1 package built:
thermald

pkgs/tools/system/thermald/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/thermald/default.nix Outdated Show resolved Hide resolved
@Emantor
Copy link
Member Author

Emantor commented Sep 9, 2020

I ran:

$ nix-shell -p nixpkgs-fmt --run "nixpkgs-fmt pkgs/tools/system/thermald/default.nix"
these paths will be fetched (0.90 MiB download, 3.64 MiB unpacked):
  /nix/store/2yv66xci59v5pjl56zmhv5i1rrv1fizb-nixpkgs-fmt-1.0.0
  /nix/store/bgdqr36i8csfda3yfq5asbcnsdkwrklc-bash-interactive-4.4-p23-dev
copying path '/nix/store/bgdqr36i8csfda3yfq5asbcnsdkwrklc-bash-interactive-4.4-p23-dev' from 'https://cache.nixos.org'...
copying path '/nix/store/2yv66xci59v5pjl56zmhv5i1rrv1fizb-nixpkgs-fmt-1.0.0' from 'https://cache.nixos.org'...
0 / 1 have been reformatted

I don't know why no reformatting was applied. Fixed up the sorting and all other issues manually.

@drewrisinger
Copy link
Contributor

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 nixpkgs-fmt, can't tell you what'd be causing that.

Copy link
Contributor

@drewrisinger drewrisinger left a 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

@Ma27
Copy link
Member

Ma27 commented Sep 9, 2020

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 thermald_2_3 in our package set.

@Emantor
Copy link
Member Author

Emantor commented Sep 10, 2020

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 thermald_2_3 in our package set.

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:

$ uname -a
Linux dibooki 5.4.62 #1-NixOS SMP Thu Sep 3 09:27:11 UTC 2020 x86_64 GNU/Linux
$ systemctl status thermald
● thermald.service - Thermal Daemon Service
     Loaded: loaded (/nix/store/ax4g6pyj1psgp158xvj6qcvllcmdf4pq-unit-thermald.service/thermald.service; enabled; vend>
     Active: active (running) since Thu 2020-09-10 06:46:16 CEST; 26s ago
   Main PID: 1197 (thermald)
         IP: 0B in, 0B out
      Tasks: 2 (limit: 4915)
     Memory: 6.9M
        CPU: 46ms
     CGroup: /system.slice/thermald.service
             └─1197 /nix/store/6l8v8k8al8bdvwnw4wyfrikvriij0h7c-thermald-2.3/sbin/thermald --no-daemon --dbus-enable

Sep 10 06:46:16 dibooki systemd[1]: Started Thermal Daemon Service.
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]22 CPUID levels; family:model:stepping 0x6:4e:3 (6:78:3)
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]Polling mode is enabled: 4
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]sensor id 11 : No temp sysfs for reading raw temp
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]sensor id 11 : No temp sysfs for reading raw temp
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]sensor id 11 : No temp sysfs for reading raw temp
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]Using generated /var/run/thermald/thermal-conf.xml.auto
Sep 10 06:46:16 dibooki thermald[1197]: [1599713176][MSG]Using config file /var/run/thermald/thermal-conf.xml.auto

While investigating this, I realized that the adaptive DPTF mode is not enabled by default and requires the new --adaptive CLI option. I'll open another PR with the Nixos module change after this is merged.

@Ma27
Copy link
Member

Ma27 commented Sep 10, 2020

What's the reason to update the module in another PR?

@Emantor
Copy link
Member Author

Emantor commented Sep 10, 2020

Right, I've pushed the module change into this PR as well.

@Emantor
Copy link
Member Author

Emantor commented Sep 14, 2020

@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";
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

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";

Copy link
Member

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?

Copy link
Contributor

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?)

pkgs/tools/system/thermald/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/thermald/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/thermald/default.nix Show resolved Hide resolved
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.
Copy link
Contributor

@jtojnar jtojnar left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks.

@Emantor
Copy link
Member Author

Emantor commented Sep 16, 2020

Thanks for the reviews, I learned a lot and collected bookmarks for guides to adhere to for future PRs 👍

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/235

@gebner gebner merged commit 0c55017 into NixOS:master Sep 20, 2020
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

8 participants