linuxPackages.librem-ec-acpi: init #352070
Conversation
9970087 to
18aa9c0
Compare
|
@ofborg build linuxKernel.packages.linux_6_6.librem-ec-acpi |
|
LGTM. Would be good to squash the commits. |
636b07a to
8a1c00b
Compare
Done |
There was a problem hiding this comment.
Picked a random PR to review. Overall looks good, although I think there's a couple of easy-to-fix changes needed to get this over the line.
New maintainer
- No GPG key, or GPG key is used to sign the commit(s)
- GitHub account name and ID number match
New package
- package path fits guidelines
- package name fits guidelines
- package version fits guidelines
- package builds on x86_64-linux
- executables tested on x86_64-linux (kernel module so nothing to test)
-
meta.descriptionis set and fits guidelines -
meta.licensefits upstream license (see review comment) -
meta.platformsis set -
meta.maintainersis set -
meta.mainProgramis set, if applicable. - build time only dependencies are declared in
nativeBuildInputs - source is fetched using the appropriate function
- the list of
phasesis not overridden - when a phase (like
installPhase) is overridden it starts withrunHook preInstalland ends withrunHook postInstall. - patches have a comment describing either the upstream URL or a reason why the patch wasn't upstreamed (see review comment)
- patches that are remotely available are fetched rather than vendored
| installFlags = [ "DEPMOD=true" ]; | ||
| enableParallelBuilding = true; | ||
|
|
||
| patches = [ ./Makefile.patch ]; |
There was a problem hiding this comment.
This should either have a comment describing either the upstream URL, or a reason why the patch wasn't upstreamed.
|
|
||
| nativeBuildInputs = kernel.moduleBuildDependencies; | ||
|
|
||
| buildFlags = [ "all" ]; |
There was a problem hiding this comment.
Nit: this seems unnecessary, as all is the default build target already.
| owner = "nicole.faerber"; | ||
| repo = "librem-ec-acpi-dkms"; | ||
| rev = "cec8c0e8bf1532c9c605f60bb02a2ef0f98e5d77"; | ||
| sha256 = "gPb1/4UPbwjJiqtP27KKjiIKxIYEl2nlQACUAlxVEFU="; |
There was a problem hiding this comment.
Nit: per https://nixos.org/manual/nixpkgs/stable/#sec-pkgs-fetchers-fetchurl-inputs it's generally preferred to provide this as a hash attribute.
| domain = "source.puri.sm"; | ||
| owner = "nicole.faerber"; | ||
| repo = "librem-ec-acpi-dkms"; | ||
| rev = "cec8c0e8bf1532c9c605f60bb02a2ef0f98e5d77"; |
There was a problem hiding this comment.
Nit: this is the commit hash for the tag, but it'd be clearer to just reference the tag name in my opinion. That'd probably mean you'd want something like
let
version = "0.9.2";
in
stdenv.mkDerivation {
version = "${version}-${kernel.version}";
src = fetchFromGitLab {
rev = "v${version}";
...
};
...
}
|
d0a91d6 to
4a490ef
Compare
|
Thanks for the review! I've made some changes to address your feedback. Please have another look. |
4a490ef to
f385b97
Compare
|
Changes look good, although I've not yet tried rebuilding since the failed builds last time around... |
|
...huh, Nonetheless, I am still getting build failures when I try to build |
I noticed there's an upstream MR which addresses a build failure on Linux 6.10.3. Is this the error you see? |
|
Yes, that looks like the problem! |
|
In that case the linked patch should be included here. |
| maintainers = [ maintainers.sjamaan ]; | ||
| license = lib.licenses.gpl2Only; | ||
| description = "Kernel module for the Purism Librem EC ACPI DKMS"; | ||
| platforms = platforms.linux; |
There was a problem hiding this comment.
| platforms = platforms.linux; | |
| platforms = [ "x86_64-linux" ]; |
The hardware is only made for x86
|
Please also squash the last three commits so you just have:
Other than that it LGTM. Will merge once you are done :) |
Users of NixOS on the Librem 14 laptop from Purism will want to include
this out-of-tree kernel module in their system configuration
boot.extraModulePackages = with config.boot.kernelPackages;
[ librem-ec-acpi ];
cf54e3e to
7350882
Compare
|
Sorry, was sick for a while and didn't have the energy to pick this back up. Should be good now |
This packages the librem-ec-acpi-dkms kernel module.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.