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

openrazer: init at 2.6.0 #64552

Merged
merged 5 commits into from
Sep 3, 2019
Merged

Conversation

evanjs
Copy link
Member

@evanjs evanjs commented Jul 10, 2019

Motivation for this change

Picking up on #47009
It would be nice to be able to configure Razer stuff from NixOS, etc

Things done
  • Kernel drivers for talking to Razer products (keyboards, mice, etc.)
  • Daemon for talking to the kernel drivers.
  • Python library for talking to the daemon.
  • NixOS module hardware.openrazer which adds the kernel drivers and
    daemon to a system.
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • 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 nix-review --run "nix-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.

One question, though:
As this is in place of another PR that went silent, should I edit maintainers and just add myself? Not sure what the proper route is.

@ofborg ofborg bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` labels Jul 10, 2019
@alexarice
Copy link
Contributor

Whatever the situation with the maintainer is, the addition to maintainers.nix should probably be a separate commit. OfBorg is saying there is some error in the meta though I couldn't see it straight away

Copy link
Member

@FRidh FRidh left a comment

Choose a reason for hiding this comment

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

Comments made in the other PR need to be addressed.

@evanjs
Copy link
Member Author

evanjs commented Jul 10, 2019

Comments made in the other PR need to be addressed.

Oops! Must've resolved the wrong commit.

To confirm: Do I need to rewrite all openrazer expressions in all-packages and python-packages to be under pkgs/development/python-modules/*, or just those in python-packages?

nixos/modules/hardware/openrazer.nix Outdated Show resolved Hide resolved
nixos/modules/hardware/openrazer.nix Outdated Show resolved Hide resolved
nixos/modules/hardware/openrazer.nix Outdated Show resolved Hide resolved
nixos/modules/hardware/openrazer.nix Outdated Show resolved Hide resolved
pkgs/development/python-modules/openrazer/daemon.nix Outdated Show resolved Hide resolved
pkgs/top-level/python-packages.nix Show resolved Hide resolved
pkgs/development/python-modules/openrazer/daemon.nix Outdated Show resolved Hide resolved
'';

postBuild = ''
DESTDIR="$out" PREFIX="" make manpages
Copy link
Member

Choose a reason for hiding this comment

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

Both this and above make command make more sense in buildPhase, because it's really the thing that builds it, not some pre/post action

Copy link
Member Author

@evanjs evanjs Jul 21, 2019

Choose a reason for hiding this comment

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

Between the rejected patch and postBuild, I was unable to get this working within the normal buildPhase. Any other thoughts on what might work?

pkgs/development/python-modules/openrazer/daemon.nix Outdated Show resolved Hide resolved
@evanjs
Copy link
Member Author

evanjs commented Jul 18, 2019

Pretty much all the aforementioned changes have been made, thanks!

The one blocker I have right now is the kernel module, and how to access the src submodule if part of this is being moved to python-modules (per @FRidh)

That, and I'm still sort of new to modules and not sure how to deal with kernelPackages.
e.g. error: attribute 'openrazer' missing

context: boot.extraModulePackages = [ config.boot.kernelPackages.openrazer ];

Though I'm assuming this is related to the split between python-modules and os-specific/linux.

@infinisil
Copy link
Member

Forgot to push? Because I can't see any changes :)

@evanjs
Copy link
Member Author

evanjs commented Jul 18, 2019

Forgot to push? Because I can't see any changes :)

Yeah, I thought about that after I posted 😀
I discovered linuxPackagesFor, anyway, and I was able to configure and rebuild with openrazer enabled.

Going to look it over once more and push for further review.

@evanjs
Copy link
Member Author

evanjs commented Jul 27, 2019

Comments made in the other PR need to be addressed.

@FRidh Still a little lost on how this needs to be split between os-specific and python-modules; Please let me know if I need to change anything else related to the paths.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin and removed 10.rebuild-darwin: 1-10 labels Aug 28, 2019
Copy link
Member

@infinisil infinisil 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 changes look mostly good, but now the commit history should be cleaned up: an initial commit for adding the maintainer (which is okay with being added I hope), then a separate commit for each package, and a final one for the NixOS module

maintainers/maintainer-list.nix Show resolved Hide resolved
@evanjs
Copy link
Member Author

evanjs commented Aug 29, 2019

I think the changes look mostly good, but now the commit history should be cleaned up: an initial commit for adding the maintainer (which is okay with being added I hope), then a separate commit for each package, and a final one for the NixOS module

Otherwise, I can just add myself, and add them if they ask in the future 🤷‍♂

@z3ntu
Copy link

z3ntu commented Aug 29, 2019

Maybe it would be beneficial to update this PR to v2.6.0?

@evanjs
Copy link
Member Author

evanjs commented Aug 30, 2019

Maybe it would be beneficial to update this PR to v2.6.0?

Without any changes outside of removing the patch that was merged since 2.3.1, 2.6.0 seems to work fine without any other changes.

@infinisil @FRidh Is bumping like this during PRs okay?

@infinisil
Copy link
Member

Sounds fine to me, assuming you'll test the new version as well. I'd be fine without a bump too though, can always be updated later, and any package is better than none.

@evanjs
Copy link
Member Author

evanjs commented Aug 30, 2019

Sounds fine to me, assuming you'll test the new version as well. I'd be fine without a bump too though, can always be updated later, and any package is better than none.

Sounds good. Bumped and rebased using the commit order @infinisil requested and building once more to make sure it still runs. Building on master is fun 😴

@evanjs evanjs changed the title openrazer: init at 2.3.1 openrazer: init at 2.6.0 Aug 30, 2019
@evanjs
Copy link
Member Author

evanjs commented Aug 30, 2019

Okay, everything has been tested once more and verified running on 2.6.0, systemctl status, modinfo razerkbd, etc.
Please let me know if there is anything else you guys can think of.
Thanks again for the reviews!

@disassembler
Copy link
Member

This looks good to me. @infinisil @FRidh any additional requests or can this be merged?

Copy link
Member

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

LGTM

@infinisil infinisil merged commit ce944b4 into NixOS:master Sep 3, 2019
@evanjs evanjs deleted the feature/openrazer-2.3.1 branch September 3, 2019 17:23
@alkeryn
Copy link
Member

alkeryn commented Jan 3, 2023

hey guys, sorry for the bump but do you know how i could use an overlay to override the package's src ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 8.has: module (update) This PR changes an existing module in `nixos/` 8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 11-100
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants