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: make basePackages configurable, rename packages… #140666

Closed
wants to merge 1 commit into from

Conversation

Lassulus
Copy link
Member

@Lassulus Lassulus commented Oct 5, 2021

… to extraPackages

Motivation for this change

alternate implementation of #84433

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 via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all packages 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/)
  • 21.11 Release Notes (or backporting 21.05 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
  • Fits CONTRIBUTING.md.

type = types.listOf types.package;
default = with pkgs; [
modemmanager
networkmanager
Copy link
Member

Choose a reason for hiding this comment

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

I think networkmanager should just always be included? Does it make sense to enable the module without including the base package?

Copy link
Member Author

Choose a reason for hiding this comment

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

maybe someone wants to override the package? but yeah, maybe we should do it some other way, but this was the least complex way I could think about

'';
};

extraPackages = mkOption {
Copy link
Member

Choose a reason for hiding this comment

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

Wait, now I got to this point and I'm a bit confused, why are all those plugins in the base instead of here?

Copy link
Member Author

@Lassulus Lassulus Oct 6, 2021

Choose a reason for hiding this comment

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

just for backwards compatibility. so people don't get missing packages if they don't change anything. the old packages option was really an extraPackages option and if we set a value here, it will override the default list.

@jtojnar
Copy link
Contributor

jtojnar commented Oct 6, 2021

The packages are also included in environment.etc unconditionally so this will not be enough. We will need the passthru solution as described on the other PR.

@jtojnar
Copy link
Contributor

jtojnar commented Oct 6, 2021

Also with that, there should not really be a need to distinguish the base and extra packages.

@Lassulus
Copy link
Member Author

Lassulus commented Oct 6, 2021

If I get this correctly, the passthru stuff should be added to each networkmanager package? I can try to do this also in this PR.
I did the split between base/extra-packages only because of backwards compatiblity. maybe a solution with stateVersion and a single packages option would be nicer?

@roberth
Copy link
Member

roberth commented Oct 6, 2021

Shouldn't this be an attrset? That way you can override specific packages.

@jtojnar
Copy link
Contributor

jtojnar commented Oct 6, 2021

If I get this correctly, the passthru stuff should be added to each networkmanager package?

Yes, I would imagine something like

passthru = {
  networkManagerVpnPlugInName = "nm-strongswan-service";
};

And the module would link those files.

Shouldn't this be an attrset? That way you can override specific packages.

That would be nice but I have not seen any standard for this to emerge so far. I think @infinisil was considering one?

@Lassulus
Copy link
Member Author

Lassulus commented Oct 6, 2021

Alright, I hope I did the right thing with the passthrus. Marked is as draft for now, until some (maybe me?) can test it on an actual system.

Also I'm not sure if the split in basePackages + extraPackges is fine. Maybe we can write something in the release notes or add the packages depending on desktop environment so we have less complex code in the module.

@Lassulus Lassulus marked this pull request as ready for review January 11, 2022 22:13
@jtojnar jtojnar force-pushed the networkmanager branch 2 times, most recently from 076144f to d2aa259 Compare March 17, 2022 03:22
@jtojnar
Copy link
Contributor

jtojnar commented Mar 17, 2022

I have rebased this in #164531 and slightly changed.

@infinisil
Copy link
Member

Shouldn't this be an attrset? That way you can override specific packages.

That would be nice but I have not seen any standard for this to emerge so far. I think @infinisil was considering one?

There's no established standard, but I think this is a good case where making this an attribute set is very beneficial. This way users can not only override plugins, but also remove them (by setting the attribute to null).

@infinisil
Copy link
Member

Though I haven't looked into the backwards compatibility aspect of this, since there already is the existing packages option of type list

@roberth
Copy link
Member

roberth commented Mar 18, 2022

A coercedTo type can host the deprecated and the new type in the same option.

@Lassulus
Copy link
Member Author

closing this in favor of #164531 which is a cleaner solution IMHO

@Lassulus Lassulus closed this Mar 18, 2022
@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.

None yet

6 participants