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

auto-cpufreq: 1.9.9 -> 2.2.0 #258684

Closed
wants to merge 10 commits into from
Closed

Conversation

shadeyg56
Copy link

@shadeyg56 shadeyg56 commented Oct 2, 2023

Description of changes

Closes #258208
Changelog
This update introduces a GTK gui, so several new build inputs were added to make this work, as well as copying the desktop entry file and polkit policy to the Nix store
Also adds small subtituteInPlace patches and fixed the prevent-update.patch file to work with this new update.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 23.11 Release Notes (or backporting 23.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested a review from Technical27 October 2, 2023 23:52
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Oct 2, 2023
@MikaelFangel
Copy link
Contributor

Closes #258208

Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

Running auto-cpufreq-gtk in a nix-shell results in an error:

Traceback (most recent call last):
  File "/nix/store/m6c995hay2sa1wh0yjsp3c6mmp7w0r2j-python3.10-auto-cpufreq-2.0.0/bin/..auto-cpufreq-gtk-wrapped-wrapped", line 15, in <module>
    win = ToolWindow()
  File "/nix/store/m6c995hay2sa1wh0yjsp3c6mmp7w0r2j-python3.10-auto-cpufreq-2.0.0/lib/python3.10/site-packages/auto_cpufreq/gui/app.py", line 30, in __init__
    self.load_css()
  File "/nix/store/m6c995hay2sa1wh0yjsp3c6mmp7w0r2j-python3.10-auto-cpufreq-2.0.0/lib/python3.10/site-packages/auto_cpufreq/gui/app.py", line 92, in load_css
    self.gtk_provider.load_from_file(Gio.File.new_for_path(CSS_FILE))
gi.repository.GLib.GError: gtk-css-provider-error-quark: <broken file>:1:0Failed to import: Error opening file /usr/local/share/auto-cpufreq/scripts/style.css: No such file or directory (2)

Comment on lines 37 to 40
postPatch = ''
substituteInPlace auto_cpufreq/core.py --replace '/opt/auto-cpufreq/override.pickle' /var/run/override.pickle
substituteInPlace scripts/org.auto-cpufreq.pkexec.policy --replace "/opt/auto-cpufreq/venv/bin/auto-cpufreq" $out/bin/auto-cpufreq
'';
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
postPatch = ''
substituteInPlace auto_cpufreq/core.py --replace '/opt/auto-cpufreq/override.pickle' /var/run/override.pickle
substituteInPlace scripts/org.auto-cpufreq.pkexec.policy --replace "/opt/auto-cpufreq/venv/bin/auto-cpufreq" $out/bin/auto-cpufreq
'';
postPatch = ''
substituteInPlace auto_cpufreq/core.py --replace '/opt/auto-cpufreq/override.pickle' /var/run/override.pickle
substituteInPlace auto_cpufreq/gui/app.py --replace '/usr/local/share/auto-cpufreq/images/icon.png' $out/share/auto-cpufreq/images/icon.png
substituteInPlace auto_cpufreq/gui/app.py --replace '/usr/local/share/auto-cpufreq/scripts/style.css' $out/share/auto-cpufreq/scripts/style.css
substituteInPlace scripts/org.auto-cpufreq.pkexec.policy --replace "/opt/auto-cpufreq/venv/bin/auto-cpufreq" $out/bin/auto-cpufreq
'';

Copy link
Contributor

Choose a reason for hiding this comment

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

You can now use --replace-fail in case of unforeseen upstream changes.

Comment on lines +50 to +52
# desktop icon
mkdir -p $out/share/applications
mkdir $out/share/pixmaps

This comment was marked as resolved.

@FliegendeWurst FliegendeWurst added the 12. first-time contribution This PR is the author's first one; please be gentle! label Oct 18, 2023
@shadeyg56
Copy link
Author

Sorry about that. We actually identified the same issue on our repo and somebody ended up fixing it, but I forgot to include the same fix here. I just took it from the file I linked. You don't need to copy the image again since it is already copied into $out/share/pixmaps

@shadeyg56
Copy link
Author

is there any way we can get the attention of someone who can merge this?

@FliegendeWurst FliegendeWurst added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 14, 2023
@FliegendeWurst
Copy link
Member

FliegendeWurst commented Nov 14, 2023

is there any way we can get the attention of someone who can merge this?

Yes, for example I just added the approvals label (forgot that after my last review...)

Other than that, the best place to ask for a merge would be one of the channels on Matrix.

@FliegendeWurst FliegendeWurst added the 11.by: upstream-developer This issue or PR was created by the developer of packaged software label Nov 14, 2023
@MikaelFangel MikaelFangel mentioned this pull request Nov 24, 2023
13 tasks
@nyabinary
Copy link
Contributor

nyabinary commented Nov 25, 2023

Need to update this to 2.1.0 and also squash this into one commit with this message auto-cpufreq: 1.9.9 -> 2.1.0

EDIT: Also a way to disable AdnanHodzic/auto-cpufreq#587 would be very nice as well.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hi !

Thanks for your first contribution! Welcome to the Nix family.

I've left some comments, let me know if this is clear enough.

pkgs/tools/system/auto-cpufreq/default.nix Outdated Show resolved Hide resolved
Comment on lines 30 to 32
# patch to prevent update
./prevent-update.patch

Copy link
Contributor

Choose a reason for hiding this comment

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

This change was not necessary since the patch was already there, please revert.

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that there is no need to move this patch before ./prevent-install-and-copy.patch.
Please leave it where it was initially.
This PR should only contain the changes relevant for the update, nothing else.

@shadeyg56 shadeyg56 changed the title auto-cpufreq: 1.9.9 -> 2.0.0 auto-cpufreq: 1.9.9 -> 2.1.0 Dec 17, 2023
@shadeyg56
Copy link
Author

Sorry for my absence. I have been quite busy with uni finals and whatnot and just got around to this.

Bumped to version 2.1.0 and fixed the patches to work with it.

@drupol I passed each dependency from pkgs, but didn't understand your second comment about the patch. All I did was move it up. I can't remember why I originally did that but I think it might be because of the order the patches needed to be applied in. I don't know, I'm not great at all the patch stuff haha.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

No problem for the delay, thanks for letting us know.

I added a comment for the patch, hope it's ok.

Comment on lines 30 to 32
# patch to prevent update
./prevent-update.patch

Copy link
Contributor

Choose a reason for hiding this comment

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

What I mean is that there is no need to move this patch before ./prevent-install-and-copy.patch.
Please leave it where it was initially.
This PR should only contain the changes relevant for the update, nothing else.


# polkit policy
mkdir -p $out/share/polkit-1/actions
cp scripts/org.auto-cpufreq.pkexec.policy $out/share/polkit-1/actions
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need a specific NixOS module for this ? I guess just enabling auto-cpufreq (in environment.packages) is not enough?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, auto-cpufreq usually runs as a daemon/background process, so just installing the package should only provide the binaries etc. I like that it's a service.

Also, the autocpufreq-service clashes with gnome3's power management (even with the service, sadly), so automatically running the service after installing the package would maybe also not be super nice.

Copy link
Contributor

@drupol drupol Dec 18, 2023

Choose a reason for hiding this comment

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

My question was merely how to make sure that the polkit policy is correctly used.

As an example using custom udev rules, is projecteur, as you can see, it provides an udev rule and services.udev.packages need to be enabled.

Do we need to do something similar with auto-cpufreq ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, sorry, then I got the question wrong, can't help in this case 🙈

@LamprosPitsillos
Copy link
Contributor

Is there a reason sometimes ${src}/scripts is used and sometimes scripts/
The later is preferable imo

@deviantsemicolon
Copy link
Contributor

auto-cpufreq 2.2.0 is out. Does a new request need to be opened?

@nyabinary
Copy link
Contributor

auto-cpufreq 2.2.0 is out. Does a new request need to be opened?

I think someone should probably open a new pr seeing as this one is a bit stale.

@drupol
Copy link
Contributor

drupol commented Feb 16, 2024

auto-cpufreq 2.2.0 is out. Does a new request need to be opened?

No, you can update this one.

Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Hello, I made further comments, let me know if you plan continue working on this PR. Close it otherwise.

pkgs/tools/system/auto-cpufreq/default.nix Outdated Show resolved Hide resolved
pname = "auto-cpufreq";
version = "1.9.9";
version = "2.1.0";

src = fetchFromGitHub {
owner = "AdnanHodzic";
repo = pname;
Copy link
Contributor

Choose a reason for hiding this comment

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

Please don't use this.

repo = pname; is nice for DRY but creates a binding that goes too far.

See further information about this here: nix-community/nixpkgs-lint#21

pkgs/tools/system/auto-cpufreq/default.nix Outdated Show resolved Hide resolved
pkgs/tools/system/auto-cpufreq/default.nix Outdated Show resolved Hide resolved
@shadeyg56 shadeyg56 changed the title auto-cpufreq: 1.9.9 -> 2.1.0 auto-cpufreq: 1.9.9 -> 2.2.0 Feb 16, 2024
@shadeyg56
Copy link
Author

Made requested changes and once again bumped versions.
Apologies again for this having been on the back burner

@shadeyg56 shadeyg56 requested a review from drupol February 23, 2024 23:57
Copy link
Contributor

@drupol drupol left a comment

Choose a reason for hiding this comment

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

Looks like the build is broken now.

@shadeyg56
Copy link
Author

I see. It builds the package on my system, but I assume it can't be installed because in this Nix package, we have patched out the requests package which is expected in the projects pyproject.toml file.

@drupol
Copy link
Contributor

drupol commented Mar 4, 2024

I see. It builds the package on my system, but I assume it can't be installed because in this Nix package, we have patched out the requests package which is expected in the projects pyproject.toml file.

If I understand correctly, it doesn't build on the CI, but it builds on your machine, right ? How can such a thing could be possible?

@wegank wegank removed the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Mar 8, 2024
@marrobHD
Copy link
Contributor

marrobHD commented Apr 3, 2024

I can can confirm this does not build.
It's due modification of the requirements.txt in the prevent-update.patch I created in PR #253595.
As we patch out the part which requires the python module requests, we also have to remove it in the requirements.txt.


Result of nixpkgs-review pr 258684 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
2 packages failed to build:
  • auto-cpufreq
  • auto-cpufreq.dist

@drupol drupol marked this pull request as draft April 3, 2024 12:14
@isabelroses isabelroses mentioned this pull request May 28, 2024
13 tasks
@JohnRTitor
Copy link
Contributor

JohnRTitor commented Jun 1, 2024

Closed per #315383

@JohnRTitor JohnRTitor closed this Jun 1, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: upstream-developer This issue or PR was created by the developer of packaged software 12. first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update request: auto-cpufreq 1.9.9 → 2.3.0