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

tuxedo-keyboard: fix compilation for kernel 6.10 and 6.11 #336633

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

Keksgesicht
Copy link

@Keksgesicht Keksgesicht commented Aug 22, 2024

Description of changes

It seems like the tuxedo-keyboard has moved to GitLab a little time ago. Thus, I changed the source git the new GitLab URL and updated the version number. This also fixes a bug when compiling with Kernel 6.10.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@K900
Copy link
Contributor

K900 commented Aug 25, 2024

Please change the commit message format to fit the guidelines in CONTRIBUTING.md

@K900
Copy link
Contributor

K900 commented Aug 25, 2024

Also, it may be worth renaming the package to fit its presumably now extended scope?

@Keksgesicht
Copy link
Author

I also renamed the related config options to the new scope of the tuxedo driver. I hope this is ok.

@Keksgesicht
Copy link
Author

There are already commit in the new upstream repo to fix compile errors with kernel 6.11. Thus, I will most likely provide a follow up pull request the coming days to update this driver to version 4.6.3.

@Keksgesicht Keksgesicht changed the title fix compilation of tuxedo-keyboard for kernel 6.10 tuxedo-keyboard: fix compilation for kernel 6.10 Sep 4, 2024
@@ -488,7 +488,7 @@ in {

rust-out-of-tree-module = if lib.versionAtLeast kernel.version "6.7" then callPackage ../os-specific/linux/rust-out-of-tree-module { } else null;

tuxedo-keyboard = if lib.versionAtLeast kernel.version "4.14" then callPackage ../os-specific/linux/tuxedo-keyboard { } else null;
tuxedo-driver = if lib.versionAtLeast kernel.version "4.14" then callPackage ../os-specific/linux/tuxedo-driver { } else null;
Copy link
Contributor

Choose a reason for hiding this comment

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

As you renamed the package here, could you add an alias to block at line 608, in-case someone didn't use the module.
Something like this:

tuxedo-keyboard = self.tuxedo-drivers;

Copy link
Contributor

@MinerSebas MinerSebas left a comment

Choose a reason for hiding this comment

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

Still works on my Pulse Gen 1.

@Keksgesicht
Copy link
Author

Now it also runs with kernel 6.11 on my Aura 15 Gen1.

@Keksgesicht Keksgesicht changed the title tuxedo-keyboard: fix compilation for kernel 6.10 tuxedo-keyboard: fix compilation for kernel 6.10 and 6.11 Sep 21, 2024
@Keksgesicht
Copy link
Author

@blanky0230 What do you think? As you are the maintainer could you review my changes please so it's clear to committers in what state this is?

@h7x4 h7x4 mentioned this pull request Sep 28, 2024
13 tasks
@Keksgesicht
Copy link
Author

Hate to nitpick catching up with reviews this late but: Since the manufacturer's repo is named "tuxedo-drivers" (plural)

Not sure, why I kept ingoring the "s". I pushed an updated version.


Can be used with the "hardware.tuxedo-drivers" NixOS module.
'';
maintainers = [ lib.maintainers.blanky0230 ];
Copy link
Member

Choose a reason for hiding this comment

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

@Keksgesicht @CutestNekoAqua If you like, join yourselves in as maintainers. Certainly better to have a bus-factor > 1 for this package I suppose.
(I'll soon have a suitable device again too).

Copy link
Member

Choose a reason for hiding this comment

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

@Keksgesicht As per #343483 ->

maintainers = [ lib.maintainers.aprl ];

Also getting @xaverdh into the loop. So they know we're trying to tighten up all the various loose ends.

Copy link
Author

Choose a reason for hiding this comment

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

I am not sure, whether I should add other people. But as they clearly want to develop this further, I could add them in the commit too.

Copy link
Contributor

Choose a reason for hiding this comment

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

feel free to add me!

@Keksgesicht
Copy link
Author

I'd love for @CutestNekoAqua to take over the project. @CutestNekoAqua Maybe you can combine this PR here with your most relevant / urgent updates from #293017 and seize the occasion?

The most important differences to my PR are the build commands for the kernel module and this is an area I have not enough knowledge. Thus, I would leave my PR in the current state and leave any further changes to @CutestNekoAqua.

@blanky0230
Copy link
Member

blanky0230 commented Sep 28, 2024

@CutestNekoAqua Please have a look when you have some time to spare. Also in regards to setting the list of maintainers: Your call. Didn't mean to exclude you from the decision at: #336633 (comment)

@CutestNekoAqua
Copy link
Contributor

I'd love for @CutestNekoAqua to take over the project. @CutestNekoAqua Maybe you can combine this PR here with your most relevant / urgent updates from #293017 and seize the occasion?

The most important differences to my PR are the build commands for the kernel module and this is an area I have not enough knowledge. Thus, I would leave my PR in the current state and leave any further changes to @CutestNekoAqua.

Sure!

Copy link
Contributor

@xaverdh xaverdh left a comment

Choose a reason for hiding this comment

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

I would prefer INSTALL_MOD_PATH over copying after the fact, but this approach works as well.
Running this on my system now, LGTM

Copy link
Contributor

@dasj19 dasj19 left a comment

Choose a reason for hiding this comment

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

Changes look good and I have briefly tested in a Gnome environment.
Hotkeys and setting keyboard backlight and fan speed work well.

@blanky0230
Copy link
Member

@NixOS/nixpkgs-merge-bot merge

@nixpkgs-merge-bot
Copy link
Contributor

@blanky0230 merge not permitted (#305350):
maintainers/maintainer-list.nix is not in pkgs/by-name/
nixos/modules/hardware/tuxedo-drivers.nix is not in pkgs/by-name/
nixos/modules/hardware/tuxedo-keyboard.nix is not in pkgs/by-name/
nixos/modules/module-list.nix is not in pkgs/by-name/
nixos/modules/services/hardware/tuxedo-rs.nix is not in pkgs/by-name/
pkgs/os-specific/linux/tuxedo-drivers/default.nix is not in pkgs/by-name/
pkgs/os-specific/linux/tuxedo-keyboard/default.nix is not in pkgs/by-name/
pkgs/top-level/linux-kernels.nix is not in pkgs/by-name/

nagymathev added a commit to nagymathev/nixos that referenced this pull request Oct 5, 2024
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.

9 participants