The changes are not tested yet. I am trying to create a test based on installer.nix

This is why I have labeled this WIP. Do tell if it is wrong.

@samueldr Please remove the WIP label from the PR. I have added a test and verified it. This PR is ready for review.

@samueldr @srhb Could you please review the PR and/or suggest reviewers ?

• Thank you

Hi, thanks for the contribution!

First, a friendly note, you did nothing wrong in asking! Though it's not unusual to have a PR aging, like a fine wine, for a couple of days once it is opened. More than a few days without activity then can become a worry. Some of the contributors (all?) work in spurts. I personally had this PR in my sights since the moment I saw it (which is why I labelled it); fixing a long-term regression is great!

Now, to the actual review! I'll be leaving other comments along the way, but I have a first task, an easy one, that needs your collaboration: The whole nixpkgs project is expected to be indented using spaces, two-per-indent-level for .nix files. nixos/tests/installer-child.nix has mixed indents, with a section using tabs.

Expect more comments as now I'm actually reviewing the thing 😄.

There may be an issue with the configuration names.

Given this option in the root configuration: boot.loader.grub.configurationName = "HI THERE ♥";, the default configuration is named Default (instead of the expected HI THERE ♥); the child configuration are named as expected. Though, now that I am re-reading, this may come from the fact that the default configuration (already) was always named "NixOS - Default" and this is not really relevant; though the behaviour is surprising.

{ config, pkgs, lib, ... }:

{
  imports = [ ./hardware-configuration.nix ];

  # Use the systemd-boot EFI boot loader.
  boot.loader.systemd-boot.enable = false;
  boot.loader.grub = {
    efiSupport = true;
    efiInstallAsRemovable = true;
  };

  users.users.root.initialHashedPassword = "";
  users.users.alice =
  { isNormalUser = true;
    home = "/home/alice";
    description = "Alice Foobar";
    extraGroups = [ "wheel" "networkmanager" ];
    initialHashedPassword = "";
  };
  services.mingetty.autologinUser = "root";
  system.stateVersion = "18.09";

  boot.loader.grub.configurationName = "HI THERE ♥";
  nesting.clone = [
    {
      boot.loader.grub.configurationName = lib.mkForce "Other";

      environment.etc = {
        "gitconfig".text = "
        [core]
        gitproxy = none for work.com
        ";
      };
    }
    {
      #boot.loader.grub.configurationName = lib.mkForce "Otter";
    }
    {
      #boot.loader.grub.configurationName = lib.mkForce "Utter nonsense";
    }

    # Verifying ordering
    {
      #boot.loader.grub.configurationName = lib.mkForce "1";
    }
    {
      #boot.loader.grub.configurationName = lib.mkForce "3";
    }
    {
      #boot.loader.grub.configurationName = lib.mkForce "2";
    }
  ];

  virtualisation.useEFIBoot = true;
}

This is somewhat confusing since the configuration in your example is using the name "Default", making this seem like an issue. Let's make a note that this needs to be fixed in another PR, in another time, otherwise it's going to feel weird for the end-users of child configurations. Also keeping this now irrelevant comment up to document this for other testers :).

@samueldr Thanks for the review. I apologize if I was pushy. My only intent was to get some comments over the weekend so that I could address them. Once the work week starts, it becomes difficult to take time out.

I have fixed the indenting in installer-child.nix. Please let me know if there are other changes to be done.

Don't worry, I understand why, this was mainly to assuage fears of your PR being forgotten.

I was stopped in the middle of reviewing yesterday, I'm finishing up my points soon.

As with the previous implementation, and the last existing implementation, this will not create entries for previous generations; only the entries for the current generations will be made available in the menu,

It is also noted in this 2009 comment.

Hi, I think the nixos/tests/installer-child.nix file was inadvertently replaced with an older revision without the actual tests verifying the child config.

Here's the diff from the previous revision in case you lost it.

Here's the diff from current state to previous state for that file.

I will continue reviewing the installer child test as it was; when it was actually testing the feature.

The installer-child.nix file is mostly a copy of the installer.nix file. This seems a bit wasteful, and will not be able to get value from updates to the installer.nix tests.

It should be possible to integrate the child configurations tests directly into installer.nix; using the appropriate hooks to add the additional test commands. Though, I must think you already explored the possibility: were there anything stopping you from integrating? What needs to be done to integrate them better?

I agree that including these tests in installer.nix would be a better approach.

On my system, test for one configuration takes roughly 45 mins. Most the time in the test is spent in the nixos-install step. Due to the turn around time, I opted for a minimal test case rather than integrating the test into installer.nix. I am running nixos in a virtualbox guest on a Windows 10 host. I dont know if this configuration is contributing to the large test runtime.

Furthermore, I would like to see an additional test for the child configurations using EFI grub; in theory there shouldn't be any differences, but building both tests ensures it will also work on EFI.

I can add this.

On my system, test for one configuration takes roughly 45 mins. Most the time in the test is spent in the nixos-install step. Due to the turn around time, I opted for a minimal test case rather than integrating the test into installer.nix. I am running nixos in a virtualbox guest on a Windows 10 host. I dont know if this configuration is contributing to the large test runtime.

Yes it is! VirtualBox does not support nested virtualization, so you're getting pure emulation for the tests! (You're a patient individual!)

If the issue is about running the other tests, when running the tests for installer.nix you are not forced to run them all. Using nix-build on the test file, you can give an attribute name using -A to select one test to run.

.../nixpkgs $ nix-build nixos/tests/installer.nix -A testNameHere

Looks good to me

@samueldr What about you?

rebased.

@samueldr pinging for review

@samueldr pinging for review.

@GrahamcOfBorg test installer.simpleUefiGrub

Here it fails with:

ub.drv'...
starting VDE switch for network 1
running the VM test script
error: syntax error at (eval 21) line 115, near "() "
syntax error at (eval 21) line 148, near ";
}"
(0.00 seconds)
syntax error at (eval 21) line 115, near "() "
syntax error at (eval 21) line 148, near ";
}"
vde_switch: EOF on stdin, cleaning up and exiting
cleaning up
(0.00 seconds)
builder for '/nix/store/qb2j9m2gikch879ipfrgbgixlgyvr5l5-vm-test-run-installer-simpleUefiGrub.drv' failed with exit code 2

The syntax errors happening in the newly added branch in the tests. The clone tests do work, though.

Making the whole block of code conditional in nix, instead of in perl, fixes the issue. (Though, reformat the added block accordingly too.)

diff --git a/nixos/tests/installer.nix b/nixos/tests/installer.nix
index 3218475c38a..93479134712 100644
--- a/nixos/tests/installer.nix
+++ b/nixos/tests/installer.nix
@@ -188,8 +188,8 @@ let
       $machine->waitForUnit("network.target");
       $machine->shutdown;

+      ${optionalString testCloneConfig ''
       # Tests for validating clone configuration entries in grub menu
-      if (${toString testCloneConfig}) {
         # Reboot Machine
         $machine = createMachine({ ${hdFlags} qemuFlags => "${qemuFlags}", name => "clone-default-config" });
         ${preBootCommands}
@@ -222,7 +222,7 @@ let
         $machine->succeed("test -e /etc/gitconfig");

         $machine->shutdown;
-      }
+      ''}

     '';

(And to test, just in case, nix-build nixos/release.nix -A tests.installer.simpleUefiGrubClone.x86_64-linux and nix-build nixos/release.nix -A tests.installer.simpleUefiGrub.x86_64-linux)

@samueldr Thanks. I will make the changes.

@GrahamcOfBorg test simpleUefiGrub
@GrahamcOfBorg test simpleUefiGrubClone
@GrahamcOfBorg test simple
@GrahamcOfBorg test simpleClone

@GrahamcOfBorg test simpleUefiGrub
@GrahamcOfBorg test simpleUefiGrubClone
@GrahamcOfBorg test simple
@GrahamcOfBorg test simpleClone

@vmandela need to have builder privileges to use this

@worldofpeace Thanks for launching the build. I did not know of the builder privileges.

@vmandela You can get privileges by adding yourself to here https://github.com/NixOS/ofborg/blob/released/config.extra-known-users.json and creating a PR.

@GrahamcOfBorg test installer.simpleUefiGrub
@GrahamcOfBorg test installer.simpleUefiGrubClone
@GrahamcOfBorg test installer.simple
@GrahamcOfBorg test installer.simpleClone

Thank you for the contribution, and you patience, @vmandela.

@samueldr @infinisil Thanks for the reviews. Glad to have this merged.