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

linuxPackages.cpupower-gui: init at 1.0.0 #100120

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@unode
Copy link
Member

@unode unode commented Oct 10, 2020

Motivation for this change

This is round 2 of #94676 now including the necessary dbus / polkit modules for use as regular user.

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.
@unode
Copy link
Member Author

@unode unode commented Oct 11, 2020

@jonringer as author of the original pull request, would you mind reviewing?

@unode unode requested a review from worldofpeace Oct 11, 2020
nixos/modules/services/desktops/cpupower-gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
Copy link
Member

@worldofpeace worldofpeace left a comment

The main issue is we need to rewrite this to use buildPythonApplication.
Though I do remember you mentioning a problem with wrapPython on IRC, so maybe you already did this and found the pythonEnv to be simpler. Let me know what you think.

pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
@unode unode force-pushed the unode:cpupower-gui branch 4 times, most recently from 4b1c563 to 2ff45e7 Oct 12, 2020
@unode
Copy link
Member Author

@unode unode commented Oct 12, 2020

Thanks everyone for all the feedback. I think I included all the proposed changes.
Redid all local tests and it seems to be working as intended.

If there are no additional remarks, please merge when possible.

@unode unode force-pushed the unode:cpupower-gui branch 2 times, most recently from 52efad2 to 84e627b Oct 12, 2020
@unode unode force-pushed the unode:cpupower-gui branch 2 times, most recently from ea3db02 to d75e7e8 Oct 12, 2020
@unode unode force-pushed the unode:cpupower-gui branch from d75e7e8 to 9ce0e3c Oct 12, 2020
@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Oct 12, 2020

What you've done with ${out} is actually the interpolation syntax for nix. So that will likely fail eval. It's most simple to do $out for the bash variable.

@unode unode force-pushed the unode:cpupower-gui branch from 9ce0e3c to d6c757d Oct 12, 2020
@unode
Copy link
Member Author

@unode unode commented Oct 12, 2020

Such a subtle difference. Was not aware that there is a distinction there.
Now using $out.

@unode unode changed the title linuxPackages.cpupower-gui: init at 0.8.0 linuxPackages.cpupower-gui: init at 0.9.0 Oct 15, 2020
@unode unode force-pushed the unode:cpupower-gui branch from d6c757d to 7f4dc3f Oct 15, 2020
@unode
Copy link
Member Author

@unode unode commented Oct 15, 2020

Updating this pull request since @vagnum08 released 0.9.0 in the meanwhile.

@unode unode force-pushed the unode:cpupower-gui branch from 7f4dc3f to 0ca6657 Oct 15, 2020
@unode unode changed the title linuxPackages.cpupower-gui: init at 0.9.0 linuxPackages.cpupower-gui: init at 0.9.1 Oct 15, 2020
@jonringer
Copy link
Contributor

@jonringer jonringer commented Oct 15, 2020

Such a subtle difference. Was not aware that there is a distinction there.
Now using $out.

in normal bash, ${out} is valid syntax for referencing the value of a variable. If you don't want nix to do anitquotation, you can also do ''${out}, but that usually only necessary for bash arrays (e.g. ''${qtWrapperArgs[@]})

pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/cpupower/gui.nix Outdated Show resolved Hide resolved
pkgs/top-level/all-packages.nix Outdated Show resolved Hide resolved
@unode unode force-pushed the unode:cpupower-gui branch from 0ca6657 to b083eaa Oct 15, 2020
config = mkIf cfg.enable {
environment.systemPackages = [ config.boot.kernelPackages.cpupower-gui ];
services.dbus.packages = [ config.boot.kernelPackages.cpupower-gui ];
systemd.packages = [ config.boot.kernelPackages.cpupower-gui ];

This comment has been minimized.

@jonringer

jonringer Oct 16, 2020
Contributor

I don't know if this is right, I think we should convert the systemd unit files into their respective services:

cat ./results//linuxPackages.cpupower-gui/share/systemd/user/cpupower-gui-user.service
[Unit]
Description=Apply cpupower-gui config at user login

[Service]
Type=oneshot
ExecStart=/usr/bin/cpupower-gui --apply-config

[Install]
WantedBy=graphical-session.target
$ cat ./results/linuxPackages.cpupower-gui/lib/systemd/system/cpupower-gui-helper.service
[Unit]
Description=cpupower-gui system helper

[Service]
Type=dbus
BusName=org.rnd2.cpupower_gui.helper
ExecStart=/nix/store/babjq2zphc87j70qc97mqvb234lqhldx-cpupower-gui-0.9.1/lib/cpupower-gui/cpupower-gui-helper

This comment has been minimized.

@unode

unode Oct 16, 2020
Author Member

Oops. The first one definitely looks wrong. That ExecStart clearly won't work. Will fix.
The second is working here and gets triggered when the application is executed.

By converting, do you mean to explicitly define them as part of the derivation?
My understanding is that if they are installed in the systemd folder, they will be picked up by systemd when installed. This is how the dbus systemd service is being executed. Though I agree that for the first, given it's a oneshot service, we could have a flag to enable it.

This comment has been minimized.

@vagnum08

vagnum08 Oct 16, 2020

What is the problem with the first service? This a user service and it can be enabled with systemctl --user enable cpupower-gui-user.service.

It is oneshot as when you apply the settings, you don't need to re-apply. Unless something changed the settings, but then you would have to competing things trying to change the settings.

For the second service, it is used for automatic dbus activation.
D-bus will start the helper when someone tries to connect to them.
By having the SystemdService key in org.rnd2.cpupower_gui.helper.service dbus will use systemd to manage the binary.

[D-BUS Service]
Name=org.rnd2.cpupower_gui.helper
Exec=@pkglibdir@/cpupower-gui-helper
User=root
SystemdService=cpupower-gui-helper.service

This comment has been minimized.

@unode

unode Oct 16, 2020
Author Member

@vagnum08 in NixOS there is no /usr/bin/cpupower-bin so the ExecStart path needs to modified to point to the correct location.

This comment has been minimized.

@vagnum08

vagnum08 Oct 16, 2020

Does it become something like /nix/store/babjq2zphc87j70qc97mqvb234lqhldx-cpupower-gui-0.9.1/bin.

If I change the service upstream to

[Unit]
Description=Apply cpupower-gui config at user login

[Service]
Type=oneshot
ExecStart=@pkgbindir@/cpupower-gui --apply-config

[Install]
WantedBy=graphical-session.target

where @pkgbindir@ is the path prefix/bindir using meson's prefix/bindir variables. Will it work?

This comment has been minimized.

@jonringer

jonringer Oct 16, 2020
Contributor

nixos modules is able to define systemd units as nix code.

User unit example:

System unit example:

systemd.services.pulseaudio = {

This comment has been minimized.

@jonringer

jonringer Oct 16, 2020
Contributor

The systemd unit files should be converted over to using nix, which is nix-aware.

The systemd unit files should then be cleaned up from the package.

It's also common to expose a "package" option, which can be used to have a different version of the executed program. Example:

package = mkOption {

This comment has been minimized.

@vagnum08

vagnum08 Oct 16, 2020

Thanks for the explanation.

@unode unode force-pushed the unode:cpupower-gui branch from b083eaa to ff20ade Oct 17, 2020
@unode unode force-pushed the unode:cpupower-gui branch from ff20ade to 785a275 Nov 14, 2020
@unode unode changed the title linuxPackages.cpupower-gui: init at 0.9.1 linuxPackages.cpupower-gui: init at 1.0.0 Nov 14, 2020
@unode unode force-pushed the unode:cpupower-gui branch from 785a275 to 18fe44c Nov 14, 2020
@unode
Copy link
Member Author

@unode unode commented Nov 14, 2020

Hi all, finally had the time to pick this up again but hit a bit a dependency bump in the road.

I've updated the derivation and included the systemd definitions.
The recipe now targets cpupower-gui 1.0.0 which requires a new dependency (libhandy).
In both 20.03 and 20.09 libhandy seems to be too old (0.13) giving:

Traceback (most recent call last):
  File "/nix/store/mw8qwbi9f0siq2r580frqbvfy40bpnsx-cpupower-gui-1.0.0/bin/.cpupower-gui-wrapped", line 368, in <module>
    from cpupower_gui import main
  File "/nix/store/mw8qwbi9f0siq2r580frqbvfy40bpnsx-cpupower-gui-1.0.0/share/cpupower-gui/cpupower_gui/main.py", line 38, in <module>
    from .window import CpupowerGuiWindow
  File "/nix/store/mw8qwbi9f0siq2r580frqbvfy40bpnsx-cpupower-gui-1.0.0/share/cpupower-gui/cpupower_gui/window.py", line 23, in <module>
    gi.require_version("Handy", "1")
  File "/nix/store/ffq6gk1jj2zq5xwn7yl4pvp69f0zzx9i-python3.7-pygobject-3.34.0/lib/python3.7/site-packages/gi/__init__.py", line 133, in require_version
    (namespace, version))
ValueError: Namespace Handy not available for version 1

The master branch of nixpkgs includes a newer libhandy (version 1.0.0) but backporting wasn't a simple copy so I didn't pursue that direction.
Building cpupower-gui and executing from master seems to work fine.

Due to the extra dependency, I'm still running cpupower-gui 0.9.1 which works fine in nixos-20.03 (my current) and I assume will also in nixos-20.09.

With that I'm thinking that it might make sense to push cpupower-gui 0.9.1 first to allow inclusion in nixos-20.09 and push 1.0.0 in a separate PR. Thoughts?

@worldofpeace
Copy link
Member

@worldofpeace worldofpeace commented Nov 14, 2020

Hi all, finally had the time to pick this up again but hit a bit a dependency bump in the road.

I've updated the derivation and included the systemd definitions.
The recipe now targets cpupower-gui 1.0.0 which requires a new dependency (libhandy).
In both 20.03 and 20.09 libhandy seems to be too old (0.13) giving:

Traceback (most recent call last):
  File "/nix/store/mw8qwbi9f0siq2r580frqbvfy40bpnsx-cpupower-gui-1.0.0/bin/.cpupower-gui-wrapped", line 368, in <module>
    from cpupower_gui import main
  File "/nix/store/mw8qwbi9f0siq2r580frqbvfy40bpnsx-cpupower-gui-1.0.0/share/cpupower-gui/cpupower_gui/main.py", line 38, in <module>
    from .window import CpupowerGuiWindow
  File "/nix/store/mw8qwbi9f0siq2r580frqbvfy40bpnsx-cpupower-gui-1.0.0/share/cpupower-gui/cpupower_gui/window.py", line 23, in <module>
    gi.require_version("Handy", "1")
  File "/nix/store/ffq6gk1jj2zq5xwn7yl4pvp69f0zzx9i-python3.7-pygobject-3.34.0/lib/python3.7/site-packages/gi/__init__.py", line 133, in require_version
    (namespace, version))
ValueError: Namespace Handy not available for version 1

The master branch of nixpkgs includes a newer libhandy (version 1.0.0) but backporting wasn't a simple copy so I didn't pursue that direction.
Building cpupower-gui and executing from master seems to work fine.

Due to the extra dependency, I'm still running cpupower-gui 0.9.1 which works fine in nixos-20.03 (my current) and I assume will also in nixos-20.09.

With that I'm thinking that it might make sense to push cpupower-gui 0.9.1 first to allow inclusion in nixos-20.09 and push 1.0.0 in a separate PR. Thoughts?

We could just add a libhandy package to 20.09 like libhandy_1. And in the backport PR you can add that package, cherry-pick this, and then make another commit to make it use libhandy_1 so it builds.

@unode
Copy link
Member Author

@unode unode commented Nov 22, 2020

@worldofpeace would it make sense to backport libhandy following the same naming scheme used in nixpkgs/master?
Making libhandy version 1 and libhandy_0 the current one.
Seems like all of the work of identifying which packages still need version 0.x and which are already 1.x compatible has already been done in master, some of your authorship :)

In nixos-20.03 libhandy also depended on glade which wasn't available so I didn't pursue this. In nixos-20.09 glade is available. Backporting the latest https://github.com/NixOS/nixpkgs/blob/master/pkgs/development/libraries/libhandy/default.nix works.
I'm happy to do this pull request while fixing also the reference for related/depending packages.

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

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.