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

Jump to bottom

nixos/gnome3: don't enable modules for excludePackages #98510

Merged
merged 1 commit into from Oct 6, 2020
Merged

nixos/gnome3: don't enable modules for excludePackages #98510

merged 1 commit into from Oct 6, 2020

Conversation

mvnetbiz
Copy link
Contributor

Motivation for this change

#24667
some packages disabled through environment.gnome3.excludePackages end up enabled anyways by their program module.
example:

services.xserver.desktopManager.gnome3.enable = true;
environment.gnome3.excludePackages = [ pkgs.gnome3.sushi ];

$ which sushi
/run/current-system/sw/bin/sushi
Things done

Enable program module by default only if it's corresponding program is not listed in "excludedPackages"

  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs 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/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot added 6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 8.has: module (update) labels Sep 23, 2020
@mvnetbiz
Copy link
Contributor Author

These are confusing options since for these specific programs using environment.gnome3.excludePackages doesn't do anything, and if this solution is accepted, then there are two separate ways to configure the same thing. Perhaps a better way would be in each relevant program module (ex. nixos/modules/services/desktops/gnome3/sushi.nix) to have an assertion that the package is not in excludePackages?

@mvnetbiz
Copy link
Contributor Author

I think it actually belongs in gnome3 module since that is what enables it, and the program modules should probably be independent of gnome3, and some people might use them separately. I don't think anyone is relying on the silent ignoring of excludePackages that will get left without a program they had been using, so I think it belongs here.

@worldofpeace
Copy link
Contributor

@mvnetbiz This is nice!!! I was wondering of a way to do this, because it would make everything a lot nicer.
I did document the old behavior in #86288, and I wasn't happy that excludePackages wouldn't work for modules.

@worldofpeace worldofpeace requested a review from a team October 1, 2020 20:06
@worldofpeace worldofpeace added this to the 20.09 milestone Oct 1, 2020
worldofpeace
worldofpeace suggested changes Oct 1, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

Can you make your PR title the same as the commit message?

@worldofpeace
Copy link
Contributor

Maybe we should have integration testing for excludePackages. I wouldn't require it in this PR because our tests for gnome aren't that great.

@mvnetbiz mvnetbiz changed the title nixos/gnome3: don't enable modules for excludePackages gnome3: don't enable modules for excludePackages Oct 1, 2020
@worldofpeace worldofpeace changed the title gnome3: don't enable modules for excludePackages nixos/gnome3: don't enable modules for excludePackages Oct 2, 2020
@worldofpeace
Copy link
Contributor

Sorry about that, it seems what I said could easily been interpreted the other way around. Module changes should have nixos/.

worldofpeace
worldofpeace approved these changes Oct 2, 2020
View reviewed changes
Copy link
Contributor

@worldofpeace worldofpeace left a comment

Choose a reason for hiding this comment

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

I tested in a gnome vm with

environment.gnome3.excludePackages = [
  pkgs.gnome3.geary
];

and verified that geary wasn't included in the installed system.

@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Oct 2, 2020

Oh that makes sense. I was confused by GitHub UI I couldn't tell what the suggested review actually was. It doesn't explicitly show in the files/review tab when there is commit message amendment I think.

@worldofpeace worldofpeace merged commit 89281dd into NixOS:master Oct 6, 2020
@worldofpeace
Copy link
Contributor

worldofpeace commented Oct 6, 2020 •
edited

backported in 3c4e4be
Thanks a lot for this, it feels elegant in that the user doesn't have to care about the silly fact that not everything can be installed with environment.systemPackages

@mvnetbiz
Copy link
Contributor Author

mvnetbiz commented Oct 6, 2020

Thanks! I didn't want to overcomplicate it, maybe there will be a more general approach to this kind of situation at some point.

@mvnetbiz mvnetbiz deleted the gnome3-excludepackages branch October 6, 2020 05:40
@mvnetbiz mvnetbiz mentioned this pull request Oct 26, 2020
Closed
@jtojnar jtojnar mentioned this pull request Dec 1, 2020
Merged
10 tasks
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/debloating-gnome/11975/2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@worldofpeace worldofpeace worldofpeace approved these changes

Assignees
No one assigned
Labels
6.topic: GNOME GNOME desktop environment and its underlying platform 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10
Projects
None yet
Milestone
20.09
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@mvnetbiz @worldofpeace @nixos-discourse