-
-
Notifications
You must be signed in to change notification settings - Fork 17.8k
linuxPackages.morse-driver: init at 1.16.4 #458358
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
Conversation
alyssais
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch does not look like something we should be applying for Nixpkgs for a new package. It should go upstream.
d69c22c to
85b080e
Compare
85b080e to
06197c8
Compare
alyssais
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Latter two commits should be squashed into the first one per CONTRIBUTING.md.
| (fetchpatch { | ||
| url = "https://github.com/MorseMicro/morse_driver/commit/366772a6801baa37171afbf4ecec103d58f97de5.patch"; | ||
| sha256 = "sha256-DqpPYBeeQXkXvmqvC7ltAwrFRO/rtNv+PZYd3I01H4M="; | ||
| }) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Per pkgs/README.md, unmerged GitHub PRs shouldn't be fetched with fetchpatch. Include the patch file in the package directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for the review, updated.
06197c8 to
2d91531
Compare
|
Okay, the package looks fine, but the patch is quite big, and the fact that they haven't managed to update to support Linux 6.12 a year after it was released does not fill me with confidence for the future maintainability of this package, so I'll wait for a second opinion on whether this is something we feel we can maintain in Nixpkgs. |
Thanks @alyssais for the review comments. Right now I am keeping a downstream derivation and hoping to fetch from upstream Nixpkgs soon. As per Morsemicro, they will get this merged in upcoming release updates. @Ma27 @NeQuissimus and others any feedback, much appreciated. |
|
|
I checked the situation and it seems they work on upstream support and cannot accept this patch for some compliance reason (missing CLA process). Other nix community folks also failed to build this: https://community.morsemicro.com/t/fail-to-compile-morse-driver-for-rv1106-kernel/932/6 So hopefully this mean we can limit this kernel module at some point to old linux kernel versions? Or just drop it at this point? I am willing to accept this patch, if we get also future fixes from @govindsi . |
|
Reviewed the patch and I have one open question for it: https://github.com/MorseMicro/morse_driver/pull/7/files#r2585255598 |
Add the morse Wi-Fi halow driver package version 1.16.4. Signed-off-by: Govind Singh <govind.sk85@gmail.com>
Signed-off-by: Govind Singh <govind.sk85@gmail.com>
e23b970 to
94fc1eb
Compare
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The patch is now very small and given the new very constraint version range, if we actually fall behind updating this, it will just fall out of the nixpkgs automatically because no kernel would support this anymore. I think this is the best outcome as it just creates 4 builds in total (2 per architecture)
The patch has been proposed upstream - but upstream doesn't accept patches at the moment. If things go to plan we will longterm see an new upstream kernel module for new kernels instead.
Add the morse Wi-Fi driver package version 1.16.4.
Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.