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

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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

@aquarial
Copy link

@aquarial aquarial commented Oct 23, 2020

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
Author

@aquarial aquarial commented Oct 23, 2020

@benley

Does this patch look right?

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

repo = "plasma-applet-redshift-control";
rev = "v${version}";
sha256 = "122nnbafa596rxdxlfshxk45lzch8c9342bzj7kzrsjkjg0xr9pq";
src = builtins.fetchGit {
Copy link
Contributor

@doronbehar doronbehar Oct 23, 2020

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

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

@worldofpeace worldofpeace Oct 23, 2020

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
Author

@aquarial aquarial Oct 23, 2020

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

@worldofpeace worldofpeace Oct 23, 2020

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
Author

@aquarial aquarial commented Oct 24, 2020

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

@SuperSandro2000 SuperSandro2000 commented Nov 26, 2020

Please quash the commits together.

@SuperSandro2000
Copy link
Member

@SuperSandro2000 SuperSandro2000 commented Nov 26, 2020

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

1 package built:
  • redshift-plasma-applet

@rmcgibbo
Copy link
Contributor

@rmcgibbo 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

@SuperSandro2000 SuperSandro2000 commented Jan 10, 2021

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

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 Jun 10, 2021
@aquarial
Copy link
Author

@aquarial aquarial commented Jun 10, 2021

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.

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

5 participants