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

linuxPackages.system76, system76-acpi, system76-io: init at 1.0.x #96514

Merged
merged 3 commits into from Sep 15, 2020

Conversation

khumba
Copy link
Contributor

@khumba khumba commented Aug 28, 2020

Motivation for this change

This adds System76's kernel modules to Nixpkgs. These modules provide support for hardware in System76 computers, enabling multimedia keys and keyboard backlight keys. This is split off from a PR of mine in the nixos-hardware repo here, to move the drivers into Nixpkgs itself.

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/)
    • I've been running these patches on 20.03 for ~5 months and they work fine on my darp6.
  • Determined the impact on package closure size (by running nix path-info -S before and after)
    • system76-dkms: 88K
    • system76-acpi-dkms: 32K
    • system76-io-dkms: 44K
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@khumba
Copy link
Contributor Author

khumba commented Aug 28, 2020

Sorry to ping you again @stites, but we would like to add the System76 drivers to Nixpkgs instead. Can I ask for your okay on relicensing from BSD-3 to MIT please?

@Mic92
Copy link
Member

Mic92 commented Aug 28, 2020

Sorry to ping you again @stites, but we would like to add the System76 drivers to Nixpkgs instead. Can I ask for your okay on relicensing from BSD-3 to MIT please?

BSD-3 and MIT is compatible. We only need MIT/BSD-3 for nix expressions the patches itself can have a different license.

@khumba
Copy link
Contributor Author

khumba commented Sep 4, 2020

Okay, I've applied all of your suggestions, thanks for the feedback. I did some other clean-up and edits for consistency with other module expressions too:

  • Removed unused fetchpatch argument, seemingly unnecessary dontPatchELF = true, and the unnecessary kernel variable.
  • Added the kernel version to the package name after its version.

BSD-3 and MIT is compatible. We only need MIT/BSD-3 for nix expressions the patches itself can have a different license.

Yes, just talking about the expressions. I wasn't sure if BSD-3 work could be included in MIT work, since that requires removing the advertising clause.

@khumba khumba marked this pull request as ready for review September 4, 2020 02:04
@Mic92
Copy link
Member

Mic92 commented Sep 4, 2020

According to nix-review some older kernels don't built with this kernel module:

6 packages marked as broken and skipped:
linuxPackages_hardkernel_4_14.system76 linuxPackages_hardkernel_4_14.system76-acpi linuxPackages_hardkernel_4_14.system76-io linuxPackages_hardkernel_latest.system76 linuxPa
ckages_hardkernel_latest.system76-acpi linuxPackages_hardkernel_latest.system76-io

4 packages failed to build:
linuxPackages_4_4.system76 linuxPackages_4_4.system76-acpi linuxPackages_4_9.system76 linuxPackages_4_9.system76-acpi

47 packages built:
linuxPackages-libre.system76 linuxPackages-libre.system76-acpi linuxPackages-libre.system76-io linuxPackages.system76 linuxPackages.system76-acpi linuxPackages.system76-io l
inuxPackages_4_14.system76 linuxPackages_4_14.system76-acpi linuxPackages_4_14.system76-io linuxPackages_4_19.system76 linuxPackages_4_19.system76-acpi linuxPackages_4_19.sy
stem76-io linuxPackages_4_4.system76-io linuxPackages_4_9.system76-io linuxPackages_5_7.system76 linuxPackages_5_7.system76-acpi linuxPackages_5_7.system76-io linuxPackages_
5_8.system76 linuxPackages_5_8.system76-acpi linuxPackages_5_8.system76-io linuxPackages_hardened.system76 linuxPackages_hardened.system76-acpi linuxPackages_hardened.system
76-io linuxPackages_latest-libre.system76 linuxPackages_latest-libre.system76-acpi linuxPackages_latest-libre.system76-io linuxPackages_latest_hardened.system76 linuxPackage
s_latest_hardened.system76-acpi linuxPackages_latest_hardened.system76-io linuxPackages_latest_xen_dom0.system76 linuxPackages_latest_xen_dom0.system76-acpi linuxPackages_la
test_xen_dom0.system76-io linuxPackages_latest_xen_dom0_hardened.system76 linuxPackages_latest_xen_dom0_hardened.system76-acpi linuxPackages_latest_xen_dom0_hardened.system7
6-io linuxPackages_testing_bcachefs.system76 linuxPackages_testing_bcachefs.system76-acpi linuxPackages_testing_bcachefs.system76-io linuxPackages_xen_dom0.system76 linuxPac
kages_xen_dom0.system76-acpi linuxPackages_xen_dom0.system76-io linuxPackages_xen_dom0_hardened.system76 linuxPackages_xen_dom0_hardened.system76-acpi linuxPackages_xen_dom0
_hardened.system76-io linuxPackages_zen.system76 linuxPackages_zen.system76-acpi linuxPackages_zen.system76-io

I would add something like to each affected package

broken = versionOlder kernel.version "4.9" || stdenv.isAarch64;

(assuming that this driver does not make sense on aarch64 anyway).

@khumba
Copy link
Contributor Author

khumba commented Sep 5, 2020

According to nix-review some older kernels don't built with this kernel module:

...

4 packages failed to build:
linuxPackages_4_4.system76 linuxPackages_4_4.system76-acpi linuxPackages_4_9.system76 linuxPackages_4_9.system76-acpi

...

I would add something like to each affected package

broken = versionOlder kernel.version "4.9" || stdenv.isAarch64;

(assuming that this driver does not make sense on aarch64 anyway).

Good point. I've blacklisted them for versions below 4.14, since that's the lowest version in Nixpkgs that builds. After remembering to remove allowBroken = true; from my ~/.config/nixpkgs/config.nix, nixpkgs-review is still trying to build these targets, although I can confirm that evaluating e.g. linuxPackages_4_4.system76.meta.broken in nix repl shows true. I'm not sure why I don't see "N packages marked as broken and skipped:" myself.

Is the aarch64 check necessary? I've only listed i686 and x86_64 as supported platforms.

@Mic92
Copy link
Member

Mic92 commented Sep 6, 2020

According to nix-review some older kernels don't built with this kernel module:

...

4 packages failed to build:
linuxPackages_4_4.system76 linuxPackages_4_4.system76-acpi linuxPackages_4_9.system76 linuxPackages_4_9.system76-acpi

...

I would add something like to each affected package

broken = versionOlder kernel.version "4.9" || stdenv.isAarch64;

(assuming that this driver does not make sense on aarch64 anyway).

Good point. I've blacklisted them for versions below 4.14, since that's the lowest version in Nixpkgs that builds. After remembering to remove allowBroken = true; from my ~/.config/nixpkgs/config.nix, nixpkgs-review is still trying to build these targets, although I can confirm that evaluating e.g. linuxPackages_4_4.system76.meta.broken in nix repl shows true. I'm not sure why I don't see "N packages marked as broken and skipped:" myself.

Is the aarch64 check necessary? I've only listed i686 and x86_64 as supported platforms.

This is a bug in nix-review that was fixed recently.

@Mic92
Copy link
Member

Mic92 commented Sep 6, 2020

@GrahamcOfBorg eval

@khumba khumba changed the title system76-dkms, system76-acpi-dkms, system76-io-dkms: init at 1.0.x linuxPackages.system76, system76-acpi, system76-io: init at 1.0.x Sep 7, 2020
@Mic92 Mic92 merged commit c407eb8 into NixOS:master Sep 15, 2020
@shlevy
Copy link
Member

shlevy commented Sep 22, 2020

@khumba Do you know how to tell if your system76 device needs these modules? Do they come with udev rules or do you need to explicitly load them?

@Mic92
Copy link
Member

Mic92 commented Sep 23, 2020

@shlevy They just need to be added as kernel modules to NixOS. This nixos-hardware module does it: NixOS/nixos-hardware#186

@khumba khumba deleted the system76 branch January 3, 2021 15:13
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