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

Merged
merged 1 commit into from Sep 21, 2021
Merged

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 commented Oct 11, 2020

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

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
nixos/modules/services/desktops/cpupower-gui.nix Outdated Show resolved Hide resolved
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.

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 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/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 cpupower-gui branch 4 times, most recently from 4b1c563 to 2ff45e7 Compare October 12, 2020 02:07
@unode
Copy link
Member Author

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 cpupower-gui branch 2 times, most recently from 52efad2 to 84e627b Compare October 12, 2020 03:23
@unode unode force-pushed the cpupower-gui branch 2 times, most recently from ea3db02 to d75e7e8 Compare October 12, 2020 03:38
@worldofpeace
Copy link
Contributor

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

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

unode commented Oct 15, 2020

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

@unode unode changed the title linuxPackages.cpupower-gui: init at 0.9.0 linuxPackages.cpupower-gui: init at 0.9.1 Oct 15, 2020
@unode
Copy link
Member Author

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
Contributor

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 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.

@unode unode mentioned this pull request Dec 11, 2020
10 tasks
@unode
Copy link
Member Author

unode commented Dec 11, 2020

Hi everyone. Trying to push this forward there's now: #106680 which should allow us to have version cpupower-gui 1.0.0 in the tree.

@stale
Copy link

stale bot commented Jul 20, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 20, 2021
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 23, 2021
@unode
Copy link
Member Author

unode commented Jul 23, 2021

Is there any doc where I can read what might be wrong with the description field that is causing the manual build test to fail?
Was there perhaps a change affecting this attribute? It didn't seem to fail before.

@unode
Copy link
Member Author

unode commented Jul 28, 2021

Ok, I was using mkEnableOption and providing description in the wrong location.
Hopefully now fixed and ready for a final review.

@unode
Copy link
Member Author

unode commented Sep 20, 2021

This has been ready for merge for a while, can someone review and do the final step?

@Artturin
Copy link
Member

Artturin commented Sep 20, 2021

Have you tested the module?
https://nixos.wiki/wiki/Nixpkgs/Reviewing_changes#Modules

@unode
Copy link
Member Author

unode commented Sep 21, 2021

Yes, I've been using it since July without issues.

Ah sorry, no I'm not familiar with those modules.
From the documentation it's also not clear, at the moment, how it relates with this PR.

@Artturin Artturin merged commit 3b2440a into NixOS:master Sep 21, 2021
@Artturin
Copy link
Member

Ah sorry, no I'm not familiar with those modules.
From the documentation it's also not clear, at the moment, how it relates with this PR.

sent it so you could test the module if you hadn't tested it yet

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

6 participants