nixos/grub: light grub by default, 150MB closure size reduction #42793

infinisil · 2018-06-29T23:27:09Z

Since the default grub2 package has ZFS enabled and this was the one
that got used for NixOS and it's not being overridden, every NixOS user
gets the ZFS dependencies, even when they're not using ZFS at all.

This PR effectively removes 150MB from the system closure on a default minimal setup. Bringing the closure size for an empty configuration.nix from 1049MB to 903MB (tested with nix path-info -S of a nix-build nixos -A config.system.build.toplevel).

Can it break anything?

This switches grub2 for grub2_minimal for NixOS. Since the latter is being defined like this

nixpkgs/pkgs/top-level/all-packages.nix

Lines 2833 to 2835 in ad112ca

    
           grub2_light = grub2.override { 
        
             zfsSupport = false; 
        
           };

and it's being overridden with zfsSupport = true when a ZFS filesystem is being used

nixpkgs/nixos/modules/system/boot/loader/grub/grub.nix

Line 12 in ad112ca

else if cfg.zfsSupport then pkgs.grub2.override { zfsSupport = true; }

(grub2_light for this PR), this can only break systems who relied on ZFS support using NixOS' non-fixed grub derivation propagated through here

nixpkgs/nixos/modules/system/boot/loader/grub/grub.nix

Lines 561 to 567 in ad112ca

    
           system.build.grub = grub; 
        
           # Common attribute for boot loaders so only one of them can be 
        
           # set at once. 
        
           system.boot.loader.id = "grub"; 
        
           environment.systemPackages = optional (grub != null) grub;

Ping @aszlig @symphorien

Things done

I ran the NixOS test installer.zfsroot successfully. Also tried to run installer.simpleUefiGrub, but it failed for some unrelated networking issue.

Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
Built on platform(s)
- NixOS
Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
Determined the impact on package closure size (by running nix path-info -S before and after)
Fits CONTRIBUTING.md.

Since the default grub2 package has ZFS enabled and this was the one that got used for NixOS and it's not being overridden, every NixOS user gets the ZFS dependencies, even when they're not using ZFS at all.

aszlig · 2018-06-30T00:22:58Z

@infinisil: The reason the installer tests now fail is because it's still referring to the previous grub derivation. It's probably a good idea to use config.system.build.grub instead there.

infinisil · 2018-06-30T00:31:14Z

@aszlig Well spotted, I'll change that. Which test didn't succeed for you though? The networking issue I mentioned probably doesn't have anything to do with this

aszlig · 2018-06-30T00:37:12Z

@infinisil: I didn't run any tests, but it seems you did:

I ran the NixOS test installer.zfsroot successfully. Also tried to run installer.simpleUefiGrub, but it failed for some unrelated networking issue.

We don't have networking access within the test VMs, so with the change to default to grub2_light we now no longer simply add pkgs.grub2 and pkgs.grub2_efi to the closure, because the grub NixOS module overrides it like this (simplified): grubReal = pkgs.grub2_light.override { zfsSupport = ...; }; grubEfi = grubReal.override { efiSupport = ...; }; So if we'd simply use [ pkgs.grub2_light pkgs.grub2_efi ] we would end up with the wrong derivation in EFI tests because pkgs.grub2_efi is actually an override of grub2 *with* ZFS but the actual installer test would use grub2 *without* ZFS and thus will try to rebuild grub. Tested against all the installer NixOS tests and all of them still succeed except the grub1 test (which already failed prior to the previous change). Signed-off-by: aszlig <aszlig@nix.build>

aszlig · 2018-06-30T03:57:01Z

@infinisil: Test fix added.

infinisil · 2018-06-30T09:20:34Z

Ah I see now, thanks a lot @aszlig

grahamc · 2018-07-07T21:51:35Z

nixos/modules/system/boot/loader/grub/grub.nix

@@ -9,12 +9,12 @@ let
  efi = config.boot.loader.efi;

  realGrub = if cfg.version == 1 then pkgs.grub
-    else if cfg.zfsSupport then pkgs.grub2.override { zfsSupport = true; }
+    else if cfg.zfsSupport then pkgs.grub2_light.override { zfsSupport = true; }


seems we should just be using regular grub in this case?

especially since:

2826 grub2_light = grub2.override { 2827 zfsSupport = false; 2828 };

nixpkgs/pkgs/top-level/all-packages.nix

Lines 2818 to 2828 in 129f94f

grub2 = grub2_full;

grub2_full = callPackage ../tools/misc/grub/2.0x.nix { };

grub2_efi = grub2.override {

efiSupport = true;

};

grub2_light = grub2.override {

zfsSupport = false;

};

The default grub has ZFS support enabled

Oh I get what you mean, overriding light with zfs = true is the same as the normal one, gotcha

Yes, and that is what we want here:

+ else if cfg.zfsSupport then pkgs.grub2_light.override { zfsSupport = true; }

Ideally the grub2_light variant should be the one used by the NixOS module, so that only the relevant features are enabled, because permuting all the features that might be available through the module as package definitions in all-packages.nix would be even more overkill.

Besides (and unrelated) these grub2.overrides should really be self.grub2.override so they can be overridden via overlays.

grahamc · 2018-07-07T21:53:26Z

nixos/tests/installer.nix

+            system.extraDependencies = let
+              # This is because the EFI version of grub overrides the base
+              # version of GRUB 2, so we can't just use pkgs.grub2_efi here.
+              grub2 = pkgs.grub2_light.override { zfsSupport = grubUseZfs; };


Should probably use if grubUseZfs then pkgs.grub2 else pkgs.grub2_light, and move the comment about the EFI stuff to below this line.

@grahamc: It's already a bit obscure that way, doing if grubUseZfs then pkgs.grub2 else pkgs.grub2_light IMHO makes it worse because then you have to look at the actual definition in all-packages.nix to check which flags are enabled (which itself again uses .override).

I guess my perspective is the _light variant becomes meaningless and overriding an overridden package to disable the override seems really weird.

I agree. Ideally the grub2 package should be the main package and grub2_full should be an override of grub2_light with all that stuff enabled. In the line here pkgs.grub2.override would also work, because grub2_full is just grub2_light with ZFS support.

grahamc · 2018-07-07T21:54:39Z

nixos/tests/installer.nix

+              # curl's tarball, we see what it's trying to download
+              curl
+            ] ++ optional (bootLoader == "grub" && grubVersion == 1) pkgs.grub
+              ++ optionals (bootLoader == "grub" && grubVersion == 2) [ grub2 grub2Efi ];


It feels a bit like much of this diff is moving code just to make the formatting slightly nicer. That can make it much more difficult to read the diff, for a minor improvement.

There is git diff -w which should make it way easier to follow.

(also GitHub has an option Hide whitespace changes, which does something similar)

grahamc · 2018-07-07T21:55:50Z

Aside from some very minor changes, lgtm.

infinisil · 2018-07-08T21:30:23Z

So, should I mess around some more to try get this to look a bit better? Not sure if worth the effort even :)

matthewbauer

I am not a fan of using special cased things like this. I know they exist in all-packages.nix , but IMO they shouldn't be there.

The issue with something like this is that it will break overlays settings grub2. Previously you could make an overlay with something like:

self: super: {
  grub2 = super.grub2.override {
    zfsSupport = true;
    xenSupport = false;
    efiSupport = true;
  };
}

If you think we should use the grub2 light by default, you should make grub2 in all-packages.nix point to it. Doing it this way is just kind of confusing.

infinisil · 2019-03-26T03:31:43Z

While this would be very nice, I really don't feel like doing this now. Maybe I'll do it later, but until then I'm closing this. Somebody else feel free to do this.

nixos/grub: light grub by default

75ca70a

Since the default grub2 package has ZFS enabled and this was the one that got used for NixOS and it's not being overridden, every NixOS user gets the ZFS dependencies, even when they're not using ZFS at all.

infinisil changed the title ~~nixos/grub: light grub by default~~ nixos/grub: light grub by default, 150MB closure size reduction Jun 29, 2018

GrahamcOfBorg added 6.topic: nixos 8.has: module (update) 10.rebuild-darwin: 0 10.rebuild-linux: 0 labels Jun 29, 2018

aszlig added the 6.topic: closure-size label Jun 30, 2018

aszlig approved these changes Jun 30, 2018

View reviewed changes

grahamc reviewed Jul 7, 2018

View reviewed changes

matthewbauer requested changes Aug 2, 2018

View reviewed changes

puffnfresh approved these changes Sep 9, 2018

View reviewed changes

infinisil closed this Mar 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nixos/grub: light grub by default, 150MB closure size reduction #42793

nixos/grub: light grub by default, 150MB closure size reduction #42793

infinisil commented Jun 29, 2018 •

edited

aszlig commented Jun 30, 2018

infinisil commented Jun 30, 2018

aszlig commented Jun 30, 2018

aszlig commented Jun 30, 2018

infinisil commented Jun 30, 2018

grahamc Jul 7, 2018

grahamc Jul 7, 2018

infinisil Jul 7, 2018

infinisil Jul 7, 2018

infinisil Jul 7, 2018

grahamc Jul 7, 2018 •

edited

aszlig Jul 7, 2018

grahamc Jul 7, 2018 •

edited

aszlig Jul 7, 2018

grahamc Jul 7, 2018

aszlig Jul 7, 2018

grahamc Jul 7, 2018

aszlig Jul 7, 2018

aszlig Jul 7, 2018

grahamc commented Jul 7, 2018

infinisil commented Jul 8, 2018

matthewbauer left a comment

infinisil commented Mar 26, 2019

	system.build.grub = grub;

	# Common attribute for boot loaders so only one of them can be
	# set at once.
	system.boot.loader.id = "grub";

	environment.systemPackages = optional (grub != null) grub;

	grub2 = grub2_full;

	grub2_full = callPackage ../tools/misc/grub/2.0x.nix { };

	grub2_efi = grub2.override {
	efiSupport = true;
	};

	grub2_light = grub2.override {
	zfsSupport = false;
	};

nixos/grub: light grub by default, 150MB closure size reduction #42793

nixos/grub: light grub by default, 150MB closure size reduction #42793

Conversation

infinisil commented Jun 29, 2018 • edited

Can it break anything?

Things done

aszlig commented Jun 30, 2018

infinisil commented Jun 30, 2018

aszlig commented Jun 30, 2018

aszlig commented Jun 30, 2018

infinisil commented Jun 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc Jul 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc Jul 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grahamc commented Jul 7, 2018

infinisil commented Jul 8, 2018

matthewbauer left a comment

Choose a reason for hiding this comment

infinisil commented Mar 26, 2019

infinisil commented Jun 29, 2018 •

edited

grahamc Jul 7, 2018 •

edited

grahamc Jul 7, 2018 •

edited