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

Jump to bottom

redshift-plasma-applet: add patch for compatibility with redshift 1.12 update #101471

Merged
merged 1 commit into from Oct 23, 2021
Merged

redshift-plasma-applet: add patch for compatibility with redshift 1.12 update #101471

merged 1 commit into from Oct 23, 2021

Conversation

aquarial
Copy link
Contributor

@aquarial aquarial commented Oct 23, 2020 •
edited

No version update. Latest source fixes a very annoying
issue where the widget cannot be turned down lower
#83250

Patch every redshift to ${redshift}/bin/redshift and
Patch killall to pkill so it actually kills the right process.

The redshift binary hides behind a python wrapper called
.redshift-wrapp (run ps e | grep redshift to find it),
but killall doesn't fix this. Using pkill kills it correctly

Is using procps here appropriate?

Motivation for this change

This issue: #83250

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.

@aquarial
Copy link
Contributor Author

@benley

Does this patch look right?

Can you start borg for the new src remote? I am not a trusted user

doronbehar
doronbehar reviewed Oct 23, 2020
View reviewed changes
pkgs/applications/misc/redshift-plasma-applet/default.nix Outdated
repo = "plasma-applet-redshift-control";
rev = "v${version}";
sha256 = "122nnbafa596rxdxlfshxk45lzch8c9342bzj7kzrsjkjg0xr9pq";
src = builtins.fetchGit {
Copy link
Contributor

Choose a reason for hiding this comment

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

Use fetchgit. That's why you have an ofborg error.

@ofborg ofborg bot requested review from benley and zraexy October 23, 2020 19:50
worldofpeace
worldofpeace reviewed Oct 23, 2020
View reviewed changes
pkgs/applications/misc/redshift-plasma-applet/default.nix Outdated
Comment on lines 3 to 14
let version = "1.0.18"; in

stdenv.mkDerivation {
pname = "redshift-plasma-applet";
inherit version;

src = fetchFromGitHub {
owner = "kotelnik";
repo = "plasma-applet-redshift-control";
rev = "v${version}";
sha256 = "122nnbafa596rxdxlfshxk45lzch8c9342bzj7kzrsjkjg0xr9pq";
src = fetchgit {
url = "https://invent.kde.org/plasma/plasma-redshift-control.git";
rev = "8b889f48988ede33ab28a7215e70ab2bdf1d0235";
sha256 = "1i8rhn4jdkhcfcia866d171j4251f9yn9a3577kli8v2wmfpcpn6";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

changing the source but not the version is incorrect. Generally, we shouldn't use a non released version of something when there are official releases. I do see from kotelnik/** it was moved under the KDE umbrella, but still no new release. Please see the manual on unstable and YYYY-MM-DD versioning

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The most recent release is broken with the most recent version of redshift binary. #83250

This specific commit fixes it: https://invent.kde.org/plasma/plasma-redshift-control/-/commit/898c3a4cfc6c317915f1e664078d8606497c4049 so I thought it would be better to update the package src.

Would it be better to directly substituteInPlace the fix now, and then remove the quick fix on next release?

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 actually just patch the source instead of updating

patches = [
  (fetchpatch {
    url = "https://invent.kde.org/plasma/plasma-redshift-control/-/commit/898c3a4cfc6c317915f1e664078d8606497c4049.patch";
   sha256 = ...
  })
];

see the nixpkgs manual on fetchpatch

@aquarial
Copy link
Contributor Author

Tested this new suggested patched version works as a local override.

@aquarial aquarial changed the title redshift: 1.0.18 -> latest src redshift-plasma-applet: add patch for compatibility with redshift 1.12 update Oct 25, 2020
@SuperSandro2000
Copy link
Member

Please quash the commits together.

@SuperSandro2000
Copy link
Member

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

1 package built:
  • redshift-plasma-applet

@rmcgibbo
Copy link
Contributor

rmcgibbo commented Jan 5, 2021

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

1 package built:
  • redshift-plasma-applet

@SuperSandro2000
Copy link
Member

This is a semi-automatic executed nixpkgs-review which does not build all packages (e.g. lumo, tensorflow or pytorch)
If you find some bugs or got suggestions for further things to search or run please reach out to SuperSandro2000 on IRC.

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

1 package built:
  • redshift-plasma-applet

@aquarial
redshift-plasma-applet: patch to work with redshift 1.12 update
d274ef1 
From patch commit message:

> Redshift version >= 1.12 requires the -P option to clear the
> existing gamma ramps for one-shot mode. Without it the screen
> gets darker and darker until it is impossible to see anything.

Apply this fix since a new version of the applet has not been
released.
@ofborg ofborg bot removed the request for review from worldofpeace June 10, 2021 06:59
@aquarial
Copy link
Contributor Author

Confirmed this is still an issue after running sudo nix-channel --update && sudo nixos rebuild --switch

Rebased the pullrequest onto master. Still no upstream src version release so still have to inline the patch.

@aquarial
Copy link
Contributor Author

Tested again with nix-channel = nixos https://nixos.org/channels/nixos-20.03. Still an issue

@aquarial
Copy link
Contributor Author

/marvin opt-in
/status needs_reviewer

@marvin-mk2
Copy link

marvin-mk2 bot commented Oct 19, 2021

Hi! I'm an experimental bot. My goal is to guide this PR through its stages, hopefully ending with a merge. You can read up on the usage here.

@marvin-mk2 marvin-mk2 bot requested a review from timokau October 20, 2021 18:33
timokau
timokau approved these changes Oct 23, 2021
View reviewed changes
Copy link
Member

@timokau timokau left a comment

Choose a reason for hiding this comment

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

I think the numeric version comparison in the patch is technically wrong (1.100 < 1.12), but it's unlikely that this will be a problem in practice. If anyone has the necessary accounts and experience with KDE contributions it might be a somewhat low hanging fruit to improve this.

This should be good to go for now. It seems like this issue has already existed for quite a while. Thank you!

@timokau timokau merged commit ea05036 into NixOS:master Oct 23, 2021
@timokau timokau mentioned this pull request Oct 23, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@doronbehar doronbehar doronbehar left review comments

@worldofpeace worldofpeace worldofpeace left review comments

@timokau timokau timokau approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 approved these changes

@benley benley Awaiting requested review from benley

@zraexy zraexy Awaiting requested review from zraexy

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 awaiting_reviewer marvin
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@aquarial @SuperSandro2000 @rmcgibbo @timokau @doronbehar @worldofpeace