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

nixos/plasma5: add kwinPackage option and kwin-lowlatency package #108987

Closed
wants to merge 3 commits into from
Closed

nixos/plasma5: add kwinPackage option and kwin-lowlatency package #108987

wants to merge 3 commits into from

Conversation

InternetUnexplorer
Copy link
Contributor

Motivation for this change

This is my attempt at #101041, since the original PR has not been updated in a while. It introduces an option services.xserver.desktopManager.plasma5.kwinPackage and a new package kwin-lowlatency.

I've done some basic testing to make sure it works and have tried to make sure it adheres to style/naming conventions used across Nixpkgs and NixOS. However, I'm still new to contributing, and so any feedback (including nitpicks) would be appreciated.

Things to consider
  • Is it worth having an option for this? kwin-lowlatency is fairly popular, but recent changes to KWin might make it obsolete soon (or not, I have no idea). Other KWin forks (e.g. KWinFT) also exist, but I don't know how popular they are.
  • Is this a good name for the option? Other ideas include kwin.package or even just kwin, but kwinPackage seemed the most obvious to me.
  • Is there a good way to ensure that kwin-lowlatency is updated when regular kwin (and the rest of Plasma) is? I thought maybe it could be marked as broken if its version doesn't match the Plasma version, but I couldn't find a nice way to get the Plasma version.
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.

@InternetUnexplorer
Copy link
Contributor Author

Just realized o should probably be oldAttrs. I thought it would be a quick edit but I probably should've made another commit as I evidently messed up the PR a bit. Very sorry for the noise.

@InternetUnexplorer
Copy link
Contributor Author

One day I'll figure out how to use Git properly; in the meantime, sorry to those who were needlessly requested for review.

@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 108987 run on x86_64-linux 1

2 packages marked as broken and skipped:
  • libsForQt512.kwin-lowlatency
  • libsForQt514.kwin-lowlatency
1 package blacklisted:
  • tests.nixos-functions.nixos-test
2 packages failed to build and are new build failures:
  • libsForQt5.kwin-lowlatency: log was empty
  • (libsForQt515.kwin-lowlatency): log was empty

@FRidh
Copy link
Member

FRidh commented Jan 11, 2021

kwin-lowlatency is fairly popular, but recent changes to KWin might make it obsolete soon (or not, I have no idea).

Never heard of it before, but then again, I don't look for it either so I am not doubting it is not used.

Given how simple it is to apply the patch I think that is exactly what users that are interested in should do; override the attribute and apply the patch. No need for the module option. I think this is too much of a niche.

@InternetUnexplorer
Copy link
Contributor Author

Fair enough, thanks for the feedback.

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

3 participants