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

nixos/networkmanager: Allow overriding installed plug-ins #164531

Merged
merged 1 commit into from Apr 10, 2022

Conversation

jtojnar
Copy link
Contributor

@jtojnar jtojnar commented Mar 17, 2022

Description of changes

Now, one can just use networking.networkmanager.plugins = lib.mkForce []; if they want to get rid of the plug-ins.

Alternative implementation of #140666
Fixes: #137338

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 22.05 Release Notes (or backporting 21.11 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
    • (Release notes changes) Ran nixos/doc/manual/md-to-db.sh to update generated release notes
  • Fits CONTRIBUTING.md.

'';
};
in
types.listOf networkManagerPluginPackage;
default = [ ];
Copy link
Contributor

Choose a reason for hiding this comment

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

it might be helpful to add a defaultText listing the plugins added by default. if the docs say Default: [ ] we'd assume that no plugins would be loaded, which is not what's happening.

Copy link
Contributor Author

@jtojnar jtojnar Mar 17, 2022

Choose a reason for hiding this comment

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

I added a note to the description (defaultText IMO would not be not right since we do not set the default value but rather the regular value).

Copy link
Member

Choose a reason for hiding this comment

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

This whole thing looks really off to me. Why have an option with no default if it effectively has one? If there is a default value, then shouldn't it be set at the option declaration location and not somewhere else?

Copy link
Contributor Author

@jtojnar jtojnar Mar 17, 2022

Choose a reason for hiding this comment

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

If you use default, and user sets networking.networkmanager.plugins, the default will not be used. Whereas this way, it will be merged. And people will be able to use lib.mkForce to replace the pseudo-default value.

Now, one can just use `networking.networkmanager.plugins = lib.mkForce [];`
if they want to get rid of the plug-ins.

Co-authored-by: lassulus <lassulus@lassul.us>
Copy link
Member

@Lassulus Lassulus left a comment

Choose a reason for hiding this comment

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

looks cooler than my implementation

@jtojnar jtojnar mentioned this pull request Apr 3, 2022
@Lassulus Lassulus merged commit adc7fbb into NixOS:master Apr 10, 2022
@jtojnar jtojnar deleted the networkmanager branch April 10, 2022 10:36
@colemickens
Copy link
Member

Does this still work? I put this in my config and on rebuild I can still see openconnect and gtk3 trying to build.

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/networkmanager-plugins-installed-by-default/39682/8

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.

Make NetworkManager plugins optional
6 participants