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.

Sign up for GitHub

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

Jump to bottom

nixos/networkmanager: split modemmanager into a separate module #316824

Merged
merged 4 commits into from
Dec 20, 2024
Merged

nixos/networkmanager: split modemmanager into a separate module #316824

merged 4 commits into from
Dec 20, 2024

Conversation

uninsane
Copy link
Contributor

@uninsane uninsane commented Jun 3, 2024

Description of changes

this does not change any default behavior: it's just refactoring for easier development and use.

  • separate modemmanager bits into modemmanager.nix.
  • give the two modules their own enable option, with networkmanager enabling modemmanager by default.
    neither one of these is dependent on the other -- they're just commonly used together. now our nixos modules reflect that.
  • give each module its own package option.
    networkmanager and modemmanager somehow end up as inputs to a lot of other packages, so now we can iterate the service-specific parts without mass-rebuilding (for example, the possibility to run NetworkManager as its own user instead of root may require patching the actual package).
  • introduce a networking.networkmanager.enableDefaultPlugins option, default true, to give an easier way to deploy without the plugins. several of the plugins are costly to build (requiring e.g. webkitgtk for captive portals) or don't even build for all platforms (especially cross-platform builds).

Things done

  • deployed to a x86_64 ethernet-only box, an x86_64 ethernet + wifi box, an x86_64 wifi-only box, and an aarch64 wifi + cellular modem box.
  • Built on platform(s)
    • x86_64-linux (and pkgsCross.aarch64-multiplatform)
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • 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/)
  • 24.11 Release Notes (or backporting 23.11 and 24.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.

Add a 👍 reaction to pull requests you find important.

@uninsane uninsane requested a review from Janik-Haag as a code owner June 3, 2024 07:04
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Jun 3, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Jun 3, 2024
@uninsane uninsane mentioned this pull request Jun 3, 2024
Merged
13 tasks
@jtojnar jtojnar self-requested a review June 3, 2024 15:20
@Janik-Haag
Copy link
Member

This is a reasonable change, but I quit nixpkgs, and thus won't be reviewing this.

@Janik-Haag Janik-Haag removed their request for review July 7, 2024 13:10
@uninsane uninsane force-pushed the pr-networkmanager-refactor branch from 21fae89 to de129ff Compare July 7, 2024 18:35
@uninsane
Copy link
Contributor Author

uninsane commented Jul 7, 2024

i've updated meta.maintainers to just be lib.teams.freedesktop.members. i'm willing to take on formal maintainership at least for modemmanager, or for networkmanager itself in a more limited role (i.e. testing) -- if the FDO team would want that.

@Enzime Enzime mentioned this pull request Aug 27, 2024
Closed
13 tasks
Enzime
Enzime approved these changes Sep 6, 2024
View reviewed changes
Copy link
Member

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

LGTM

@Enzime Enzime added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Sep 6, 2024
@0xf09f95b4 0xf09f95b4 mentioned this pull request Oct 21, 2024
Draft
13 tasks
misuzu
misuzu reviewed Dec 14, 2024
View reviewed changes
Copy link
Contributor

@misuzu misuzu left a comment

Choose a reason for hiding this comment

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

Needs a release notes entry and nixfmt, LGTM otherwise

@jtojnar
Copy link
Member

jtojnar commented Dec 17, 2024

Sorry, forgot about this.

  • Separate ModemManager module sounds reasonable – the NM module should never have gotten so big in the first place.
  • The addition of Freedesktop maintainers are fine with me, though I will want to move the declarative NM config stuff out since I have no capacity to maintain that. But that can be done separately.
  • package options should be acceptable in this case to reduce rebuilds, though not as critical as in the past now that NM is no longer a mass rebuild trigger through libproxy/webkitgtk.
  • Still not sure about enableDefaultPlugins option design, might be better to split it into separate PR so that we can merge this one.

@uninsane
nixos/networkmanager: split ModemManager bits into own module
0deadd5 
this should not result in any observable change by default, the
motivation is to make working on either one of these components in
isolation of the other a bit easier.
@uninsane
nixos/networkmanager: format with nixfmt
5a04fc7
@uninsane uninsane force-pushed the pr-networkmanager-refactor branch from de129ff to b72d61f Compare December 19, 2024 22:45
@github-actions github-actions bot added 8.has: documentation This PR adds or changes documentation 8.has: changelog labels Dec 19, 2024
@uninsane
Copy link
Contributor Author

Needs a release notes entry and nixfmt, LGTM otherwise

done and done. nixos-manual-build CI is showing red, but i don't understand the error and the manual builds fine for me locally with the same command (nix-build nixos/release.nix -A manual.x86_64-linux).

Still not sure about enableDefaultPlugins option design, might be better to split it into separate PR so that we can merge this one.

removed the enableDefaultPlugins patch for the time being. if someone else has interest in that part i'll let them resurrect it (feel free to tag me on that), otherwise seems like it's probably not worth pushing for.

@misuzu
Copy link
Contributor

misuzu commented Dec 20, 2024

nixos-manual-build CI is showing red

Rerunning the action fixed the manual issue, but nixfmt check didn't pass

@uninsane
nixos/modemmanager: add a package option
9d4d21b 
this is helpful for testing module changes or making downstream patches
in a way which doesn't force large rebuilds as an overlay would.
@uninsane
nixos/networkmanager: add a package option
efc3208 
this is helpful for testing module changes or making downstream patches
in a way which doesn't force large rebuilds as an overlay would.
@uninsane uninsane force-pushed the pr-networkmanager-refactor branch from b72d61f to efc3208 Compare December 20, 2024 10:04
@uninsane
Copy link
Contributor Author

formatted; CI shows green

@misuzu misuzu merged commit a01b0bf into NixOS:master Dec 20, 2024
24 of 25 checks passed
@misuzu misuzu mentioned this pull request Dec 20, 2024
Open
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Enzime Enzime Enzime approved these changes

@misuzu misuzu misuzu approved these changes

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@uninsane @Janik-Haag @jtojnar @misuzu @Enzime