Skip to content

libnm: init, libproxy: use libnm instead of full networkmanager#241762

Closed
SuperSandro2000 wants to merge 2 commits intoNixOS:stagingfrom
SuperSandro2000:libnm
Closed

libnm: init, libproxy: use libnm instead of full networkmanager#241762
SuperSandro2000 wants to merge 2 commits intoNixOS:stagingfrom
SuperSandro2000:libnm

Conversation

@SuperSandro2000
Copy link
Member

Description of changes

@jtojnar do you have any comments about this?

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/)
  • 23.11 Release Notes (or backporting 23.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.

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Jul 5, 2023
@ofborg ofborg bot requested review from amaxine, domenkozar, jtojnar and obadz July 5, 2023 20:57
@ofborg ofborg bot added 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. labels Jul 5, 2023
@jtojnar
Copy link
Member

jtojnar commented Jul 5, 2023

I think this makes sense in theory but for Nix, we probably care about build closure, so it will likely need more work.

Do you have numbers on the runtime closure reduction?

Why are you using separate flag for plug-ins?

@SuperSandro2000
Copy link
Member Author

I think this makes sense in theory but for Nix, we probably care about build closure, so it will likely need more work.

this is only about runtime closure.

Do you have numbers on the runtime closure reduction?

 ➜ nix path-info -Sh ./result*
/nix/store/0qv89sww54rylfbfpr61q5hwqrl80hay-networkmanager-1.42.6        468.6M
/nix/store/5qaad13xxnx7md81mmqfx4ib2q8gqsy9-networkmanager-1.42.6        203.3M
/nix/store/dahjw7k8qbvwi2b073wk1ni5vh38qpi6-networkmanager-1.42.6-man    201.1K
/nix/store/rmiadcs6syal240da2rzn6fhwfwbxkc4-networkmanager-1.42.6-man    201.1K

Why are you using separate flag for plug-ins?

🤷🏼 removed it again

Copy link
Member

Choose a reason for hiding this comment

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

Should not this be negated?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh, right, derp

@jtojnar
Copy link
Member

jtojnar commented Jul 17, 2023

I think this makes sense in theory but for Nix, we probably care about build closure, so it will likely need more work.

this is only about runtime closure.

Right. But I am questioning whether this change, which adds a lot of complexity to the expression, has any use case as is. Because I believe that Nix cares about build time closure of its dependencies. Please correct me if I am mistaken.

@ofborg ofborg bot requested a review from jtojnar July 17, 2023 22:16
@SuperSandro2000
Copy link
Member Author

Because I believe that Nix cares about build time closure of its dependencies. Please correct me if I am mistaken.

Also runtime closure, see NixOS/nix#8648 . We don't need to add 200MB of extra dependencies to nix which aren't really needed and also other distros do this split.

@jtojnar
Copy link
Member

jtojnar commented Jul 21, 2023

Well, runtime closure improvements are useless if you cannot use the dependency due to build closure size.

@SuperSandro2000
Copy link
Member Author

I am not sure which parts you exactly mean. Can you give an example?

As far as I understood the linked issue this should fix the mentioned problem. If I didn't understood it correctly please point me at the detail I missed.

@jtojnar
Copy link
Member

jtojnar commented Jul 22, 2023

The issue just talks about closure size. I understand that to refer to build closure.

Comment on lines +152 to +160
# for nmtui
newt
readline
# for plugins
bluez5
dnsmasq
modemmanager
readline
newt
libsoup
jansson
dbus # used to get directory paths with pkg-config during configuration
mobile-broadband-provider-info
modemmanager
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved most plugin dependencies out here, so build and runtime closure size should both be significant smaller now.

@jtojnar
Copy link
Member

jtojnar commented Oct 29, 2023

Maybe we should instead bump libproxy – the latest version does not appear to use libnm.

@SuperSandro2000
Copy link
Member Author

did that in #272353

@amaxine amaxine mentioned this pull request Dec 5, 2023
13 tasks
@bryango
Copy link
Member

bryango commented Jan 27, 2024

The networkmanager closure is just terrifyingly large, depending on openconnect, systemd, dnsmasq among other things. For people like me that uses nix as a package manager (not NixOS), these are all dead weight 😭

And it seems that the libproxy bump turns out to be non-trivial... Can we merge this first before that is sorted out? 🥺 Because other apps that depend on networkmanager can also benefit from switching to libnm.

@jtojnar
Copy link
Member

jtojnar commented Jan 27, 2024

Hopefully, libproxy will not take much longer.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 19, 2024
@amaxine amaxine removed their request for review April 27, 2024 17:55
@stale stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Apr 27, 2024
@jtojnar
Copy link
Member

jtojnar commented May 2, 2024

No longer needed with #272353

@jtojnar jtojnar closed this May 2, 2024
@SuperSandro2000 SuperSandro2000 deleted the libnm branch May 3, 2024 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

8.has: package (new) This PR adds a new package 10.rebuild-darwin: 11-100 This PR causes between 11 and 100 packages to rebuild on Darwin. 10.rebuild-linux: 501-1000 This PR causes many rebuilds on Linux and should normally target the staging branches. 10.rebuild-linux: 501+ This PR causes many rebuilds on Linux and should normally target the staging branches.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants