-
-
Notifications
You must be signed in to change notification settings - Fork 14.7k
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
networkmanager: make packages configurable #300751
Conversation
What's the problem with |
Ideally I'd get rid of both, I didn't have enough time yesterday. The main issue is honestly that it makes |
Finished the code cleanup. Let's see if the tests pass... |
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.
Could you please split the cleanups, formatting and the actual change into separate commits, this is hard to review.
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.
I can try, I've never done that before.
listToAttrs | ||
; | ||
|
||
inherit (lib) |
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.
Most of these occur just a few times, it would be cleaner to just use lib.
prefix inline. Same for the builtins
.
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.
Just for the ones that don't get used often, or for every invocation of a function in lib
? If it's the former, then what's the threshold?
connectionPath = id: | ||
"/run/NetworkManager/system-connections/${id}.nmconnection"; | ||
|
||
inputPath = ini.generate (escapeShellArg profile.n) profile.v; |
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.
Error here, should be lib.ini
.
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.
caught it, just haven't been back at my computer for a bit
There's another PR that also implements similar changes #316824 |
That PR looks like what I would like to see from this one. As I don't have the time to go through this PR at the moment, I'll probably close it. |
Description of changes
Added configuration options for packages used by the NetworkManager module. Also some general code cleanup.
Allows overriding the version of e.g. ModemManager used by the system without rebuilding everything that depends on it.
I used this to try the experimental XMM7360 fork of ModemManager.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.